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

  From: Paul Durrant <pdurrant@amazon.com>

We have an unmap helper but mapping is open-coded. Arguably this is fine
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(-)
  

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
>