Re: [Xen-devel] [PATCH v3 13/14] xen/mm: Convert {s, g}et_gpfn_from_mfn() to use typesafe MFN

From: Julien Grall <julien.grall@arm.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, WeiLiu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v3 13/14] xen/mm: Convert {s, g}et_gpfn_from_mfn() to use typesafe MFN
Date: Wed, 5 Jun 2019 12:03:52 +0100	[thread overview]
Message-ID: <8270ed07-74af-aeb2-40c9-3d218d09d51d@arm.com> (raw)
In-Reply-To: <5CF7A1090200007800235782@prv1-mh.provo.novell.com>

Hi Jan,

Sorry, I seem to have missed the comments on this patch while respinning the series.

I will look at it and address them in v4.

Cheers,

On 05/06/2019 12:01, Jan Beulich wrote:
>>>> On 03.06.19 at 18:03, <julien.grall@arm.com> wrote:
>> --- a/xen/arch/x86/x86_64/traps.c
>> +++ b/xen/arch/x86/x86_64/traps.c
>> @@ -184,7 +184,8 @@ void vcpu_show_registers(const struct vcpu *v)
>>   
>>   void show_page_walk(unsigned long addr)
>>   {
>> -    unsigned long pfn, mfn = read_cr3() >> PAGE_SHIFT;
>> +    unsigned long pfn;
>> +    mfn_t mfn = maddr_to_mfn(read_cr3());
> 
> Quoting my v1 comment:
> 
> "I realize you simply take what has been there and transform it, but
>   maddr_to_mfn() (other than shifting by PAGE_SHIFT) is not truly
>   applicable here: What the CR3 register holds is not a physical address,
>   both the low twelve bits as well as the high twelve ones have different
>   meaning. The shift is correct currently because the high ones are
>   (right now) zero on reads. Please consider AND-ing with
>   X86_CR3_ADDR_MASK (or keeping the shift)."
> 
> FOAD by "please consider" I don't mean "leave it as is will be fine as
> well", but to do one of the two possible changes, with a preference
> towards the AND-ing, as that's the ultimately correct approach.
> 
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1416,7 +1416,7 @@ static void free_heap_pages(
>>   
>>           /* This page is not a guest frame any more. */
>>           page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */
>> -        set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY);
>> +        set_pfn_from_mfn(mfn_add(mfn, + i), INVALID_M2P_ENTRY);
> 
> Quoting my v1 comment again: "Stray leftover + ?"
> 
>> --- a/xen/include/asm-x86/mm.h
>> +++ b/xen/include/asm-x86/mm.h
>> @@ -492,22 +492,26 @@ extern struct domain *dom_xen, *dom_io, *dom_cow;	/* for vmcoreinfo */
>>    */
>>   extern bool machine_to_phys_mapping_valid;
>>   
>> -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
>> +static inline void set_pfn_from_mfn(mfn_t mfn, unsigned long pfn)
>>   {
>> -    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
>> +    const unsigned long mfn_ = mfn_x(mfn);
>> +    struct domain *d = page_get_owner(mfn_to_page(mfn));
> 
> const? (Or is this a place where I've similarly asked on an earlier
> patch already?)
> 
> Btw, the cheaper (in terms of code churn) change here would seem to
> be to name the function parameter mfn_, and the local variable mfn.
> That'll also reduce the number of uses of the unfortunate trailing-
> underscore-name.
> 
> Jan
> 
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel