Re: [XEN v2 03/11] xen/arm: domain_build: Replace use of paddr_t in find_domU_holes()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 19/01/2023 23:02, Stefano Stabellini wrote:
> On Tue, 17 Jan 2023, Ayan Kumar Halder wrote:
> > bankbase, banksize and bankend are used to hold values of type 'unsigned
> > long long'. This can be represented as 'uint64_t' instead of 'paddr_t'.
> > This will ensure consistency with allocate_static_memory() (where we use
> > 'uint64_t' for rambase and ramsize).
> >
> > In future, paddr_t can be used for 'uin32_t' as well to represent 32bit
> > physical addresses.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> I saw that Julien commented about using unsigned long long. To be
> honest, given that we typically use explicitly-sized integers (which is
> a good thing) 
From the CODING_STYLE:

"Fixed width types should only be used when a fixed width quantity is
meant (which for example may be a value read from or to be written to a
register)."

This is also how we used it in the Arm port so far.

> and unsigned long long is always uint64_t on all
> architectures, I can see the benefits of using uint64_t here.
From my understanding of the C spec, the only requirement is that 
"unsigned long long" can fit a 2^64 - 1. So it may technically be bigger 
than 64-bit.
> At the same time, I can see that the reason for change the type here is
> that we are dealing with ULL values, so it would make sense to use
> unsigned long long. >
> I cannot see any big problem/advantages either way, so I am OK with
> this patch. (Julien, if you prefer unsigned long long I am fine with
> that also.)
I don't mind too much here.

> Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
>
>
> > ---
> >
> > Changes from -
> >
> > v1 - Modified the title/commit message from "[XEN v1 6/9] xen/arm: Use 'u64' to 
> > represent 'unsigned long long"
> > and moved it earlier.
> >
> >   xen/arch/arm/domain_build.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 33a5945a2d..f904f12408 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1726,9 +1726,9 @@ static int __init find_domU_holes(const struct 
> > kernel_info *kinfo,
> >                                     struct meminfo *ext_regions)
> >   {
> >       unsigned int i;
> > -    paddr_t bankend;
> > -    const paddr_t bankbase[] = GUEST_RAM_BANK_BASES;
> > -    const paddr_t banksize[] = GUEST_RAM_BANK_SIZES;
> > +    uint64_t bankend;
> > +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> > +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> >       int res = -ENOENT;
> >   
> >       for ( i = 0; i < GUEST_RAM_BANKS; i++ )
> > --
> > 2.17.1
Cheers,

--
Julien Grall