| Message ID | 20231002095740.1472907-2-paul@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: xen: update shared_info and vcpu_info handling | expand |
On Mon, Oct 02, 2023, Paul Durrant wrote: > From: Paul Durrant <pdurrant@amazon.com> Please make the changelog standalone, i.e. don't rely on the shortlog to provide context. Yeah, it can be silly and repetive sometimes, particularly when viewing git commits where the shortlog+changelog are bundled fairly close together, but when viewing patches in a mail client, e.g. when I'm doing initial review, the shortlog is in the subject which may be far away or even completely hidden (as is the case as I'm typing this). I could have sworn I included this in Documentation/process/maintainer-kvm-x86.rst, but I'm not finding it. > We have an unmap helper but mapping is open-coded. Arguably this is fine Pronouns. > because mapping is done in only one place, hva_to_pfn_retry(), but adding > the helper does make that function more readable. > > No functional change intended. > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > Reviewed-by: David Woodhouse <dwmw@amazon.co.uk> > --- > Cc: Sean Christopherson <seanjc@google.com> > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Paolo Bonzini <pbonzini@redhat.com> > --- > virt/kvm/pfncache.c | 43 +++++++++++++++++++++++++------------------ > 1 file changed, 25 insertions(+), 18 deletions(-) > > diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c > index 2d6aba677830..0f36acdf577f 100644 > --- a/virt/kvm/pfncache.c > +++ b/virt/kvm/pfncache.c > @@ -96,17 +96,28 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len) > } > EXPORT_SYMBOL_GPL(kvm_gpc_check); > > -static void gpc_unmap_khva(kvm_pfn_t pfn, void *khva) > +static void *gpc_map(kvm_pfn_t pfn) > +{ > + if (pfn_valid(pfn)) > + return kmap(pfn_to_page(pfn)); > +#ifdef CONFIG_HAS_IOMEM > + else There's no need for the "else", the happy path is terminal. > + return memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB); > +#endif This needs a return for CONFIG_HAS_IOMEM=n. I haven't tried to compile, but I'm guessing s390 won't be happy. This? static void *gpc_map(kvm_pfn_t pfn) { if (pfn_valid(pfn)) return kmap(pfn_to_page(pfn)); #ifdef CONFIG_HAS_IOMEM return memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB); #else return NULL; #endif } > +} > + > +static void gpc_unmap(kvm_pfn_t pfn, void *khva) > { > /* Unmap the old pfn/page if it was mapped before. */ > - if (!is_error_noslot_pfn(pfn) && khva) { > - if (pfn_valid(pfn)) > - kunmap(pfn_to_page(pfn)); > + if (is_error_noslot_pfn(pfn) || !khva) > + return; > + > + if (pfn_valid(pfn)) > + kunmap(pfn_to_page(pfn)); > #ifdef CONFIG_HAS_IOMEM > - else > - memunmap(khva); > + else > + memunmap(khva); > #endif I don't mind the refactoring, but it needs to be at least mentioned in the changelog. And if we're going to bother, it probably makes sense to add a WARN in the CONFIG_HAS_IOMEM=n path, e.g. /* Unmap the old pfn/page if it was mapped before. */ if (is_error_noslot_pfn(pfn) || !khva) return; if (pfn_valid(pfn)) kunmap(pfn_to_page(pfn)); else #ifdef CONFIG_HAS_IOMEM memunmap(khva); #else WARN_ON_ONCE(1); #endif
On 31/10/2023 23:20, Sean Christopherson wrote: > On Mon, Oct 02, 2023, Paul Durrant wrote: >> From: Paul Durrant <pdurrant@amazon.com> > > Please make the changelog standalone, i.e. don't rely on the shortlog to provide > context. Yeah, it can be silly and repetive sometimes, particularly when viewing > git commits where the shortlog+changelog are bundled fairly close together, but > when viewing patches in a mail client, e.g. when I'm doing initial review, the > shortlog is in the subject which may be far away or even completely hidden (as is > the case as I'm typing this). > > I could have sworn I included this in Documentation/process/maintainer-kvm-x86.rst, > but I'm not finding it. > OK, I'll add some more text. >> We have an unmap helper but mapping is open-coded. Arguably this is fine > > Pronouns. > Sorry... didn't realize that was an issue. >> because mapping is done in only one place, hva_to_pfn_retry(), but adding >> the helper does make that function more readable. >> >> No functional change intended. >> >> Signed-off-by: Paul Durrant <pdurrant@amazon.com> >> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk> >> --- >> Cc: Sean Christopherson <seanjc@google.com> >> Cc: David Woodhouse <dwmw2@infradead.org> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> --- >> virt/kvm/pfncache.c | 43 +++++++++++++++++++++++++------------------ >> 1 file changed, 25 insertions(+), 18 deletions(-) >> >> diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c >> index 2d6aba677830..0f36acdf577f 100644 >> --- a/virt/kvm/pfncache.c >> +++ b/virt/kvm/pfncache.c >> @@ -96,17 +96,28 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len) >> } >> EXPORT_SYMBOL_GPL(kvm_gpc_check); >> >> -static void gpc_unmap_khva(kvm_pfn_t pfn, void *khva) >> +static void *gpc_map(kvm_pfn_t pfn) >> +{ >> + if (pfn_valid(pfn)) >> + return kmap(pfn_to_page(pfn)); >> +#ifdef CONFIG_HAS_IOMEM >> + else > > There's no need for the "else", the happy path is terminal. > >> + return memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB); >> +#endif > > This needs a return for CONFIG_HAS_IOMEM=n. I haven't tried to compile, but I'm > guessing s390 won't be happy. > Oops, yes, of course. > This? > > static void *gpc_map(kvm_pfn_t pfn) > { > if (pfn_valid(pfn)) > return kmap(pfn_to_page(pfn)); > > #ifdef CONFIG_HAS_IOMEM > return memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB); > #else > return NULL; > #endif > } > Looks good. Thanks, Paul >> +} >> + >> +static void gpc_unmap(kvm_pfn_t pfn, void *khva) >> { >> /* Unmap the old pfn/page if it was mapped before. */ >> - if (!is_error_noslot_pfn(pfn) && khva) { >> - if (pfn_valid(pfn)) >> - kunmap(pfn_to_page(pfn)); >> + if (is_error_noslot_pfn(pfn) || !khva) >> + return; >> + >> + if (pfn_valid(pfn)) >> + kunmap(pfn_to_page(pfn)); >> #ifdef CONFIG_HAS_IOMEM >> - else >> - memunmap(khva); >> + else >> + memunmap(khva); >> #endif > > I don't mind the refactoring, but it needs to be at least mentioned in the > changelog. And if we're going to bother, it probably makes sense to add a WARN > in the CONFIG_HAS_IOMEM=n path, e.g. > > /* Unmap the old pfn/page if it was mapped before. */ > if (is_error_noslot_pfn(pfn) || !khva) > return; > > if (pfn_valid(pfn)) > kunmap(pfn_to_page(pfn)); > else > #ifdef CONFIG_HAS_IOMEM > memunmap(khva); > #else > WARN_ON_ONCE(1); > #endif >
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index 2d6aba677830..0f36acdf577f 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -96,17 +96,28 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len) } EXPORT_SYMBOL_GPL(kvm_gpc_check); -static void gpc_unmap_khva(kvm_pfn_t pfn, void *khva) +static void *gpc_map(kvm_pfn_t pfn) +{ + if (pfn_valid(pfn)) + return kmap(pfn_to_page(pfn)); +#ifdef CONFIG_HAS_IOMEM + else + return memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB); +#endif +} + +static void gpc_unmap(kvm_pfn_t pfn, void *khva) { /* Unmap the old pfn/page if it was mapped before. */ - if (!is_error_noslot_pfn(pfn) && khva) { - if (pfn_valid(pfn)) - kunmap(pfn_to_page(pfn)); + if (is_error_noslot_pfn(pfn) || !khva) + return; + + if (pfn_valid(pfn)) + kunmap(pfn_to_page(pfn)); #ifdef CONFIG_HAS_IOMEM - else - memunmap(khva); + else + memunmap(khva); #endif - } } static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_seq) @@ -175,7 +186,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) * the existing mapping and didn't create a new one. */ if (new_khva != old_khva) - gpc_unmap_khva(new_pfn, new_khva); + gpc_unmap(new_pfn, new_khva); kvm_release_pfn_clean(new_pfn); @@ -193,15 +204,11 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) * too must be done outside of gpc->lock! */ if (gpc->usage & KVM_HOST_USES_PFN) { - if (new_pfn == gpc->pfn) { + if (new_pfn == gpc->pfn) new_khva = old_khva; - } else if (pfn_valid(new_pfn)) { - new_khva = kmap(pfn_to_page(new_pfn)); -#ifdef CONFIG_HAS_IOMEM - } else { - new_khva = memremap(pfn_to_hpa(new_pfn), PAGE_SIZE, MEMREMAP_WB); -#endif - } + else + new_khva = gpc_map(new_pfn); + if (!new_khva) { kvm_release_pfn_clean(new_pfn); goto out_error; @@ -326,7 +333,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, mutex_unlock(&gpc->refresh_lock); if (unmap_old) - gpc_unmap_khva(old_pfn, old_khva); + gpc_unmap(old_pfn, old_khva); return ret; } @@ -412,7 +419,7 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc) list_del(&gpc->list); spin_unlock(&kvm->gpc_lock); - gpc_unmap_khva(old_pfn, old_khva); + gpc_unmap(old_pfn, old_khva); } } EXPORT_SYMBOL_GPL(kvm_gpc_deactivate);