Re: [XEN v3 1/3] xen/arm: Use the correct format specifier

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 23 Jan 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/01/2023 21:19, Stefano Stabellini wrote:
> > On Mon, 23 Jan 2023, Ayan Kumar Halder wrote:
> > > 1. One should use 'PRIpaddr' to display 'paddr_t' variables. However,
> > > while creating nodes in fdt, the address (if present in the node name)
> > > should be represented using 'PRIx64'. This is to be in conformance
> > > with the following rule present in https://elinux.org/Device_Tree_Linux
> > > 
> > > . node names
> > > "unit-address does not have leading zeros"
> > > 
> > > As 'PRIpaddr' introduces leading zeros, we cannot use it.
> > > 
> > > So, we have introduced a wrapper ie domain_fdt_begin_node() which will
> > > represent physical address using 'PRIx64'.
> > > 
> > > 2. One should use 'PRIx64' to display 'u64' in hex format. The current
> > > use of 'PRIpaddr' for printing PTE is buggy as this is not a physical
> > > address.
> > > 
> > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > > ---
> > > 
> > > Changes from -
> > > 
> > > v1 - 1. Moved the patch earlier.
> > > 2. Moved a part of change from "[XEN v1 8/9] xen/arm: Other adaptations
> > > required to support 32bit paddr"
> > > into this patch.
> > > 
> > > v2 - 1. Use PRIx64 for appending addresses to fdt node names. This fixes
> > > the CI failure.
> > > 
> > >   xen/arch/arm/domain_build.c | 45 +++++++++++++++++--------------------
> > >   xen/arch/arm/gic-v2.c       |  6 ++---
> > >   xen/arch/arm/mm.c           |  2 +-
> > 
> > The changes to mm.c and gic-v2.c look OK and I'd ack them already. One
> > question on the changes to domain_build.c below.
> > 
> > 
> > >   3 files changed, 25 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index f35f4d2456..97c2395f9a 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -1288,6 +1288,20 @@ static int __init fdt_property_interrupts(const
> > > struct kernel_info *kinfo,
> > >       return res;
> > >   }
> > >   +static int __init domain_fdt_begin_node(void *fdt, const char *name,
> > > +                                        uint64_t unit)
> > > +{
> > > +    /*
> > > +     * The size of the buffer to hold the longest possible string ie
> > > +     * interrupt-controller@ + a 64-bit number + \0
> > > +     */
> > > +    char buf[38];
> > > +
> > > +    /* ePAPR 3.4 */
> > > +    snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit);
> 
> The return wants to be checked.
> 
> > > +    return fdt_begin_node(fdt, buf);
> > > +}
> > > +
> > >   static int __init make_memory_node(const struct domain *d,
> > >                                      void *fdt,
> > >                                      int addrcells, int sizecells,
> > > @@ -1296,8 +1310,6 @@ static int __init make_memory_node(const struct
> > > domain *d,
> > >       unsigned int i;
> > >       int res, reg_size = addrcells + sizecells;
> > >       int nr_cells = 0;
> > > -    /* Placeholder for memory@ + a 64-bit number + \0 */
> > > -    char buf[24];
> > >       __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
> > >       __be32 *cells;
> > >   @@ -1314,9 +1326,7 @@ static int __init make_memory_node(const struct
> > > domain *d,
> > >         dt_dprintk("Create memory node\n");
> > >   -    /* ePAPR 3.4 */
> > > -    snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start);
> > > -    res = fdt_begin_node(fdt, buf);
> > > +    res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start);
> > 
> > Basically this "hides" the paddr_t->uint64_t cast because it happens
> > implicitly when passing mem->bank[i].start as an argument to
> > domain_fdt_begin_node.
> > 
> > To be honest, I don't know if it is necessary. Also a normal cast would
> > be fine:
> > 
> >      snprintf(buf, sizeof(buf), "memory@%"PRIx64,
> > (uint64_t)mem->bank[i].start);
> >      res = fdt_begin_node(fdt, buf);
> The problem with the open-coding version is you would need to explain the cast
> everywhere (I disliked unexplained one).
> 
> I don't particular mind 'hidden cast' but I think we need to explain on top of
> domain_fdt_begin_node() why it is necessary.
> 
> > 
> > Julien, what do you prefer?
> 
> Definitely the function because that's what I suggested (see the rationale
> above).

OK, no worries