Re: [PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one

From: Julien Grall <julien.grall@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>, Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	xen-devel@lists.xen.org, Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one
Date: Thu, 23 Jun 2016 11:43:40 +0100	[thread overview]
Message-ID: <576BBD5C.5030404@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.10.1606231114270.2575@sstabellini-ThinkPad-X260>

Hi Stefano,

On 23/06/16 11:20, Stefano Stabellini wrote:
> On Tue, 21 Jun 2016, Julien Grall wrote:
>> The x86 version of the function xenmem_add_to_physmap_one contains
>> variable name gpfn and gfn which make the code very confusing.
>> I have left unchanged for now.
>>
>> Also, rename gpfn to gfn in the ARM version as the latter is the correct
>> acronym for a guest physical frame.
>>
>> Finally, remove the trailing whitespace around the changes.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Tim Deegan <tim@xen.org>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>>
>>      Changes in v3:
>>          - Add Jan's acked-by for non-ARM bits
>> ---
>>   xen/arch/arm/mm.c    | 10 +++++-----
>>   xen/arch/x86/mm.c    | 15 +++++++--------
>>   xen/common/memory.c  |  6 +++---
>>   xen/include/xen/mm.h |  2 +-
>>   4 files changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 5ab9b75..6882d54 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1046,7 +1046,7 @@ int xenmem_add_to_physmap_one(
>>       unsigned int space,
>>       union xen_add_to_physmap_batch_extra extra,
>>       unsigned long idx,
>> -    xen_pfn_t gpfn)
>> +    gfn_t gfn)
>
> I think there is a possible loss of information here: xen_pfn_t is
> always uint64_t on both ARM and ARM64, while gfn_t is unsigned long with
> its current definition. Or am I missing something?
>
> In fact, given that ARM supports LPAE, shouldn't gfn by defined as
> xen_pfn_t in terms of storage size (TYPE_SAFE(xen_pfn_t, gfn)) ?

With LPAE, ARM32 supports up to 40-bit PA so the frame will be encoded 
on 28-bit. So unsigned long is perfectly fine here.

Note that the truncation already occurs in subsequent caller because we 
are using unsigned long for the P2M parameters.

>
>
>>   {
>>       unsigned long mfn = 0;
>>       int rc;
>> @@ -1081,8 +1081,8 @@ int xenmem_add_to_physmap_one(
>>               else
>>                   return -EINVAL;
>>           }
>> -
>> -        d->arch.grant_table_gpfn[idx] = gpfn;
>> +
>> +        d->arch.grant_table_gpfn[idx] = gfn_x(gfn);
>
> Similarly grant_table_gpfn is an array of xen_pfn_t (uint64_t), while
> gfn_x unboxes to the storage size of gfn (unsigned long).

Patch #5 will switch grant_table_gpfn to an array of gfn_t.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel