[v8,07/15] KVM: pfncache: include page offset in uhva and use it consistently

  From: Paul Durrant <pdurrant@amazon.com>

Currently the pfncache page offset is sometimes determined using the gpa
and sometimes the khva, whilst the uhva is always page-aligned. After a
subsequent patch is applied the gpa will not always be valid so adjust
the code to include the page offset in the uhva and use it consistently
as the source of truth.

Also, where a page-aligned address is required, use PAGE_ALIGN_DOWN()
for clarity.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>

v8:
 - New in this version.
---
 virt/kvm/pfncache.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)
  

On Tue, 2023-11-21 at 18:02 +0000, Paul Durrant wrote:
> @@ -242,8 +242,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
>         }
>  
>         old_pfn = gpc->pfn;
> -       old_khva = gpc->khva - offset_in_page(gpc->khva);
> -       old_uhva = gpc->uhva;
> +       old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva);
>  
>         /* If the userspace HVA is invalid, refresh that first */
>         if (gpc->gpa != gpa || gpc->generation != slots->generation ||
> @@ -259,13 +258,25 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
>                         ret = -EFAULT;
>                         goto out;
>                 }


There's a subtle behaviour change here, isn't there? I'd *really* like
you do say 'No functional change intended' where that is true, and then
the absence of that sentence in this one would be meaningful.

You are now calling hva_to_pfn_retry() even when the uhva page hasn't
changed. Which is harmless and probably not important, but IIUC fixable
by the addition of:

 +              if (gpc->uhva != PAGE_ALIGN_DOWN(old_uhva)) 
> +               hva_change = true;
> +       } else {
> +               /*
> +                * No need to do any re-mapping if the only thing that has
> +                * changed is the page offset. Just page align it to allow the
> +                * new offset to be added in.
> +                */
> +               gpc->uhva = PAGE_ALIGN_DOWN(gpc->uhva);
>         }
>  
> +       /* Note: the offset must be correct before calling hva_to_pfn_retry() */
> +       gpc->uhva += page_offset;
> +
>         /*
>          * If the userspace HVA changed or the PFN was already invalid,
>          * drop the lock and do the HVA to PFN lookup again.
>          */
> -       if (!gpc->valid || old_uhva != gpc->uhva) {
> +       if (!gpc->valid || hva_change) {
>                 ret = hva_to_pfn_retry(gpc);
>         } else {
>                 /*
> -- 

But I don't really think it's that important if you can come up with a
coherent justification for the change and note it in the commit
message. So either way:

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
  

On Tue, Nov 21, 2023 at 06:02:15PM +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> Currently the pfncache page offset is sometimes determined using the gpa
> and sometimes the khva, whilst the uhva is always page-aligned. After a
> subsequent patch is applied the gpa will not always be valid so adjust
> the code to include the page offset in the uhva and use it consistently
> as the source of truth.
> 
> Also, where a page-aligned address is required, use PAGE_ALIGN_DOWN()
> for clarity.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> 
> v8:
>  - New in this version.
> ---
>  virt/kvm/pfncache.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
> index 0eeb034d0674..c545f6246501 100644
> --- a/virt/kvm/pfncache.c
> +++ b/virt/kvm/pfncache.c
> @@ -48,10 +48,10 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
>  	if (!gpc->active)
>  		return false;
>  
> -	if (offset_in_page(gpc->gpa) + len > PAGE_SIZE)
> +	if (gpc->generation != slots->generation || kvm_is_error_hva(gpc->uhva))
>  		return false;
>  
> -	if (gpc->generation != slots->generation || kvm_is_error_hva(gpc->uhva))
> +	if (offset_in_page(gpc->uhva) + len > PAGE_SIZE)
>  		return false;
>  
>  	if (!gpc->valid)
> @@ -119,7 +119,7 @@ static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_s
>  static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
>  {
>  	/* Note, the new page offset may be different than the old! */
> -	void *old_khva = gpc->khva - offset_in_page(gpc->khva);
> +	void *old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva);
>  	kvm_pfn_t new_pfn = KVM_PFN_ERR_FAULT;
>  	void *new_khva = NULL;
>  	unsigned long mmu_seq;
> @@ -192,7 +192,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
>  
>  	gpc->valid = true;
>  	gpc->pfn = new_pfn;
> -	gpc->khva = new_khva + offset_in_page(gpc->gpa);
> +	gpc->khva = new_khva + offset_in_page(gpc->uhva);
>  
>  	/*
>  	 * Put the reference to the _new_ pfn.  The pfn is now tracked by the
> @@ -215,8 +215,8 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
>  	struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
>  	unsigned long page_offset = offset_in_page(gpa);
>  	bool unmap_old = false;
> -	unsigned long old_uhva;
>  	kvm_pfn_t old_pfn;
> +	bool hva_change = false;
>  	void *old_khva;
>  	int ret;
>  
> @@ -242,8 +242,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
>  	}
>  
>  	old_pfn = gpc->pfn;
> -	old_khva = gpc->khva - offset_in_page(gpc->khva);
> -	old_uhva = gpc->uhva;
> +	old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva);
>  
>  	/* If the userspace HVA is invalid, refresh that first */
>  	if (gpc->gpa != gpa || gpc->generation != slots->generation ||
> @@ -259,13 +258,25 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
>  			ret = -EFAULT;
>  			goto out;
>  		}
> +
> +		hva_change = true;
> +	} else {
> +		/*
> +		 * No need to do any re-mapping if the only thing that has
> +		 * changed is the page offset. Just page align it to allow the
> +		 * new offset to be added in.

I don't understand how the uhva('s offset) could be changed when both gpa and
slot are not changed. Maybe I have no knowledge of xen, but in later
patch you said your uhva would never change...

Thanks,
Yilun

> +		 */
> +		gpc->uhva = PAGE_ALIGN_DOWN(gpc->uhva);
>  	}
>  
> +	/* Note: the offset must be correct before calling hva_to_pfn_retry() */
> +	gpc->uhva += page_offset;
> +
>  	/*
>  	 * If the userspace HVA changed or the PFN was already invalid,
>  	 * drop the lock and do the HVA to PFN lookup again.
>  	 */
> -	if (!gpc->valid || old_uhva != gpc->uhva) {
> +	if (!gpc->valid || hva_change) {
>  		ret = hva_to_pfn_retry(gpc);
>  	} else {
>  		/*
> -- 
> 2.39.2
> 
>
  

On Wed, 2023-11-22 at 16:54 +0800, Xu Yilun wrote:
> 
> > @@ -259,13 +258,25 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
> >                         ret = -EFAULT;
> >                         goto out;
> >                 }
> > +
> > +               hva_change = true;
> > +       } else {
> > +               /*
> > +                * No need to do any re-mapping if the only thing that has
> > +                * changed is the page offset. Just page align it to allow the
> > +                * new offset to be added in.
> 
> I don't understand how the uhva('s offset) could be changed when both gpa and
> slot are not changed. Maybe I have no knowledge of xen, but in later
> patch you said your uhva would never change...

It doesn't change on a normal refresh with kvm_gpc_refresh(), which is
just for revalidation after memslot changes or MMU invalidation.

But it can change if the gpc is being reinitialized with a new address
(perhaps because the guest has made another hypercall to set the
address, etc.)

That new address could happen to be in the *same* page as the previous
one. In fact the xen_shinfo_test explicitly tests that case, IIRC.

And kvm_gpc_activate() also happens to use __kvm_gpc_refresh()
internally.
  

On 21/11/2023 22:35, David Woodhouse wrote:
> On Tue, 2023-11-21 at 18:02 +0000, Paul Durrant wrote:
>> @@ -242,8 +242,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
>>          }
>>   
>>          old_pfn = gpc->pfn;
>> -       old_khva = gpc->khva - offset_in_page(gpc->khva);
>> -       old_uhva = gpc->uhva;
>> +       old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva);
>>   
>>          /* If the userspace HVA is invalid, refresh that first */
>>          if (gpc->gpa != gpa || gpc->generation != slots->generation ||
>> @@ -259,13 +258,25 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
>>                          ret = -EFAULT;
>>                          goto out;
>>                  }
> 
> 
> There's a subtle behaviour change here, isn't there? I'd *really* like
> you do say 'No functional change intended' where that is true, and then
> the absence of that sentence in this one would be meaningful.
> 
> You are now calling hva_to_pfn_retry() even when the uhva page hasn't
> changed. Which is harmless and probably not important, but IIUC fixable
> by the addition of:
> 
>   +              if (gpc->uhva != PAGE_ALIGN_DOWN(old_uhva))

True; I can keep that optimization and then I will indeed add 'no 
functional change'... Didn't seem worth it at the time, but no harm.

>> +               hva_change = true;
>> +       } else {
>> +               /*
>> +                * No need to do any re-mapping if the only thing that has
>> +                * changed is the page offset. Just page align it to allow the
>> +                * new offset to be added in.
>> +                */
>> +               gpc->uhva = PAGE_ALIGN_DOWN(gpc->uhva);
>>          }
>>   
>> +       /* Note: the offset must be correct before calling hva_to_pfn_retry() */
>> +       gpc->uhva += page_offset;
>> +
>>          /*
>>           * If the userspace HVA changed or the PFN was already invalid,
>>           * drop the lock and do the HVA to PFN lookup again.
>>           */
>> -       if (!gpc->valid || old_uhva != gpc->uhva) {
>> +       if (!gpc->valid || hva_change) {
>>                  ret = hva_to_pfn_retry(gpc);
>>          } else {
>>                  /*
>> -- 
> 
> But I don't really think it's that important if you can come up with a
> coherent justification for the change and note it in the commit
> message. So either way:
> 
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>

Thanks,

   Paul
  

On Wed, Nov 22, 2023 at 09:12:18AM +0000, David Woodhouse wrote:
> On Wed, 2023-11-22 at 16:54 +0800, Xu Yilun wrote:
> > 
> > > @@ -259,13 +258,25 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
> > >                         ret = -EFAULT;
> > >                         goto out;
> > >                 }
> > > +
> > > +               hva_change = true;
> > > +       } else {
> > > +               /*
> > > +                * No need to do any re-mapping if the only thing that has
> > > +                * changed is the page offset. Just page align it to allow the
> > > +                * new offset to be added in.
> > 
> > I don't understand how the uhva('s offset) could be changed when both gpa and
> > slot are not changed. Maybe I have no knowledge of xen, but in later
> > patch you said your uhva would never change...
> 
> It doesn't change on a normal refresh with kvm_gpc_refresh(), which is
> just for revalidation after memslot changes or MMU invalidation.
> 
> But it can change if the gpc is being reinitialized with a new address
> (perhaps because the guest has made another hypercall to set the
> address, etc.)
> 
> That new address could happen to be in the *same* page as the previous

In this case, the lower bits of new gpa should be different to gpc->gpa,
so will hit "if (gpc->gpa != gpa ...)" branch.

And I see this comment is deleted in v9, which makes sense to me.

Thanks,
Yilun

> one. In fact the xen_shinfo_test explicitly tests that case, IIRC.
> 
> And kvm_gpc_activate() also happens to use __kvm_gpc_refresh()
> internally.
  

On Wed, 2023-11-22 at 22:27 +0800, Xu Yilun wrote:
> On Wed, Nov 22, 2023 at 09:12:18AM +0000, David Woodhouse wrote:
> > On Wed, 2023-11-22 at 16:54 +0800, Xu Yilun wrote:
> > > 
> > > > @@ -259,13 +258,25 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
> > > >                         ret = -EFAULT;
> > > >                         goto out;
> > > >                 }
> > > > +
> > > > +               hva_change = true;
> > > > +       } else {
> > > > +               /*
> > > > +                * No need to do any re-mapping if the only thing that has
> > > > +                * changed is the page offset. Just page align it to allow the
> > > > +                * new offset to be added in.
> > > 
> > > I don't understand how the uhva('s offset) could be changed when both gpa and
> > > slot are not changed. Maybe I have no knowledge of xen, but in later
> > > patch you said your uhva would never change...
> > 
> > It doesn't change on a normal refresh with kvm_gpc_refresh(), which is
> > just for revalidation after memslot changes or MMU invalidation.
> > 
> > But it can change if the gpc is being reinitialized with a new address
> > (perhaps because the guest has made another hypercall to set the
> > address, etc.)
> > 
> > That new address could happen to be in the *same* page as the previous
> 
> In this case, the lower bits of new gpa should be different to gpc->gpa,
> so will hit "if (gpc->gpa != gpa ...)" branch.

I think that 'if (gpc->gpa != gpa); branch is also gratuitously
refreshing when it doesn't need to; it only needs to refresh if the
*aligned* gpas don't match.

But it was like that already, so I won't heckle Paul any further :)
  

On 22/11/2023 15:42, David Woodhouse wrote:
> On Wed, 2023-11-22 at 22:27 +0800, Xu Yilun wrote:
>> On Wed, Nov 22, 2023 at 09:12:18AM +0000, David Woodhouse wrote:
>>> On Wed, 2023-11-22 at 16:54 +0800, Xu Yilun wrote:
>>>>
>>>>> @@ -259,13 +258,25 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
>>>>>                          ret = -EFAULT;
>>>>>                          goto out;
>>>>>                  }
>>>>> +
>>>>> +               hva_change = true;
>>>>> +       } else {
>>>>> +               /*
>>>>> +                * No need to do any re-mapping if the only thing that has
>>>>> +                * changed is the page offset. Just page align it to allow the
>>>>> +                * new offset to be added in.
>>>>
>>>> I don't understand how the uhva('s offset) could be changed when both gpa and
>>>> slot are not changed. Maybe I have no knowledge of xen, but in later
>>>> patch you said your uhva would never change...
>>>
>>> It doesn't change on a normal refresh with kvm_gpc_refresh(), which is
>>> just for revalidation after memslot changes or MMU invalidation.
>>>
>>> But it can change if the gpc is being reinitialized with a new address
>>> (perhaps because the guest has made another hypercall to set the
>>> address, etc.)
>>>
>>> That new address could happen to be in the *same* page as the previous
>>
>> In this case, the lower bits of new gpa should be different to gpc->gpa,
>> so will hit "if (gpc->gpa != gpa ...)" branch.
> 
> I think that 'if (gpc->gpa != gpa); branch is also gratuitously
> refreshing when it doesn't need to; it only needs to refresh if the
> *aligned* gpas don't match.
> 

I did look at that but decided that gfn_to_hva_memslot() was 
sufficiently lightweight that it was not really worth optimising.

> But it was like that already, so I won't heckle Paul any further :)

I appreciate it! :-)

   Paul