Re: [XEN v2 05/11] xen/arm: Use paddr_t instead of u64 for address/size

[Ayan Kumar Halder <ayankuma@amd.com>]

Hi Julien,

On 07/02/2023 09:03, Julien Grall wrote:
>
>
> On 06/02/2023 19:21, Ayan Kumar Halder wrote:
>> Hi Stefano,
>>
>> On 19/01/2023 23:24, Stefano Stabellini wrote:
>>> On Tue, 17 Jan 2023, Ayan Kumar Halder wrote:
>>>> One should now be able to use 'paddr_t' to represent address and size.
>>>> Consequently, one should use 'PRIpaddr' as a format specifier for 
>>>> paddr_t.
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>> ---
>>>>
>>>> Changes from -
>>>>
>>>> v1 - 1. Rebased the patch.
>>>>
>>>>   xen/arch/arm/domain_build.c        |  9 +++++----
>>>>   xen/arch/arm/gic-v3.c              |  2 +-
>>>>   xen/arch/arm/platforms/exynos5.c   | 26 +++++++++++++-------------
>>>>   xen/drivers/char/exynos4210-uart.c |  2 +-
>>>>   xen/drivers/char/ns16550.c         |  8 ++++----
>>>>   xen/drivers/char/omap-uart.c       |  2 +-
>>>>   xen/drivers/char/pl011.c           |  4 ++--
>>>>   xen/drivers/char/scif-uart.c       |  2 +-
>>>>   xen/drivers/passthrough/arm/smmu.c |  6 +++---
>>>>   9 files changed, 31 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index 72b9afbb4c..cf8ae37a14 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -1666,7 +1666,7 @@ static int __init find_memory_holes(const 
>>>> struct kernel_info *kinfo,
>>>>       dt_for_each_device_node( dt_host, np )
>>>>       {
>>>>           unsigned int naddr;
>>>> -        u64 addr, size;
>>>> +        paddr_t addr, size;
>>>>           naddr = dt_number_of_address(np);
>>>> @@ -2445,7 +2445,7 @@ static int __init handle_device(struct domain 
>>>> *d, struct dt_device_node *dev,
>>>>       unsigned int naddr;
>>>>       unsigned int i;
>>>>       int res;
>>>> -    u64 addr, size;
>>>> +    paddr_t addr, size;
>>>>       bool own_device = !dt_device_for_passthrough(dev);
>>>>       /*
>>>>        * We want to avoid mapping the MMIO in dom0 for the 
>>>> following cases:
>>>> @@ -2941,9 +2941,10 @@ static int __init 
>>>> handle_passthrough_prop(struct kernel_info *kinfo,
>>>>           if ( res )
>>>>           {
>>>>               printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>>> -                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
>>>> +                   " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
>>>>                      kinfo->d->domain_id,
>>>> -                   mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 
>>>> 1);
>>>> +                   (paddr_t) (mstart & PAGE_MASK),
>>>> +                   (paddr_t) (PAGE_ALIGN(mstart + size) - 1));
>>> Why do you need the casts here? mstart is already defined as paddr_t
>>
>> Actually, this is because the PAGE_MASK is defined as 'long'. See 
>> xen/include/xen/page-size.h :-
>>
>> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
>> #define PAGE_MASK           (~(PAGE_SIZE-1))
>>
>> So, the resultant type inferred is 'long unsigned int'. Thus, we need 
>> to add an explicit cast.
>
> Hmmm... I am a bit confused with this statement. If the resultant type 
> inferred is actually 'long unsigned int', then why was the current 
> specifier worked before ('long unsigned int' is 32-bit on Arm32)?

Before this change, mstart was of type paddr_t (ie u64 ie unsigned long 
long on Arm_32). When 'unsigned long long' was operated with 'long' (ie 
PAGE_SIZE), then the resultant type is 'unsigned long long'. The rule 
from 'C Standard 2011' 
(https://web.cs.dal.ca/~vlado/pl/C_Standard_2011-n1570.pdf) , page 53 
(Section 6.3.1.8 Usual arithmetic conversions) is :-

"Otherwise, if the operand that has unsigned integer type has rank 
greater or equal to the rank of the type of the other operand, then the 
operand with signed integer type is converted to the type of the operand 
with unsigned integer type."

And from 6.3.1.1

"The rank of long long int shall be greater than the rank of long int, 
which shall be greater than the rank of int, which shall be greater than 
the rank of short int, which shall be greater than the rank of signed char.

The rank of any unsigned integer type shall equal the rank of the 
corresponding signed integer type, if any."

And using 'PRIx64' to print 'unsigned long long' will not require any cast.

Now with the change,
mstart is of paddr_t (ie 'unsigned int'). 'unsigned int' operated with 
'long', then  the result is 'unsigned long int'. As the rank of 
'unsigned long int' > 'int', thus it cannot be printed using PRIx32 
(integer format specifier) without an explicit cast.

Please let me know if this makes sense.

-Ayan
>
> Cheers,
>