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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxx>
> > > ---
> > >
> > > 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)?
Cheers,

--
Julien Grall