| Message ID | 20231121180223.12484-8-paul@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: xen: update shared_info and vcpu_info handling | expand |
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
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. + */ + 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 { /*