Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Tue, 31 Oct 2023 16:20:50 -0700
In-Reply-To: <20231002095740.1472907-2-paul@xen.org>
Mime-Version: 1.0
References: <20231002095740.1472907-1-paul@xen.org> <20231002095740.1472907-2-paul@xen.org>
Message-ID: <ZUGL0syLTH09BbsI@google.com>
Subject: Re: [PATCH v7 01/11] KVM: pfncache: add a map helper function
From:   Sean Christopherson <seanjc@google.com>
To:     Paul Durrant <paul@xen.org>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paul Durrant <pdurrant@amazon.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        David Woodhouse <dwmw2@infradead.org>,
        Paolo Bonzini <pbonzini@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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