Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
From:   Paul Durrant <xadimgnik@gmail.com>
Message-ID: <d912e3ee-f2e5-4c40-99cd-214b16fe8fac@xen.org>
Date:   Thu, 2 Nov 2023 17:09:40 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Reply-To: paul@xen.org
Subject: Re: [PATCH v7 01/11] KVM: pfncache: add a map helper function
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
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>
References: <20231002095740.1472907-1-paul@xen.org>
 <20231002095740.1472907-2-paul@xen.org> <ZUGL0syLTH09BbsI@google.com>
Organization: Xen Project
In-Reply-To: <ZUGL0syLTH09BbsI@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>