Re: [PATCH v7 01/11] KVM: pfncache: add a map helper function

[Paul Durrant]

On 31/10/2023 23:20, Sean Christopherson wrote:
> On Mon, Oct 02, 2023, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> > Reviewed-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
> > ---
> > Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> > Cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > ---
> >   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