Re: [XEN v2 02/11] xen/arm: Use the correct format specifier

[Julien Grall <julien@xxxxxxx>]

On 20/01/2023 16:03, Michal Orzel wrote:
> Hi Julien,
Hi Michal,

> On 20/01/2023 16:09, Julien Grall wrote:
> > On 20/01/2023 14:40, Michal Orzel wrote:
> > > Hello,
> > Hi,
> >
> > > On 20/01/2023 10:32, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 19/01/2023 22:54, Stefano Stabellini wrote:
> > > > > On Tue, 17 Jan 2023, Ayan Kumar Halder wrote:
> > > > > > 1. One should use 'PRIpaddr' to display 'paddr_t' variables.
> > > > > > 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>
> > > > > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > >
> > > > I have committed the patch.
> > > The CI test jobs (static-mem) failed on this patch:
> > > https://gitlab.com/xen-project/xen/-/pipelines/752911309
> > Thanks for the report.
> >
> > > I took a look at it and this is because in the test script we
> > > try to find a node whose unit-address does not have leading zeroes.
> > > However, after this patch, we switched to PRIpaddr which is defined as 
> > > 016lx/016llx and
> > > we end up creating nodes like:
> > >
> > > memory@0000000050000000
> > >
> > > instead of:
> > >
> > > memory@60000000
> > >
> > > We could modify the script,
> > TBH, I think it was a mistake for the script to rely on how Xen describe
> > the memory banks in the Device-Tree.
> >
> > For instance, from my understanding, it would be valid for Xen to create
> > a single node for all the banks or even omitting the unit-address if
> > there is only one bank.
> >
> > > but do we really want to create nodes
> > > with leading zeroes? The dt spec does not mention it, although [1]
> > > specifies that the Linux convention is not to have leading zeroes.
> > Reading through the spec in [2], it suggested the current naming is
> > fine. That said the example match the Linux convention (I guess that's
> > not surprising...).
> >
> > I am open to remove the leading. However, I think the CI also needs to
> > be updated (see above why).
> Yes, the CI needs to be updated as well.
Can either you or Ayan look at it?

> I'm in favor of removing leading zeroes because this is what Xen uses in all
> the other places when creating nodes (or copying them from the host dtb) 
> including xl
> when creating dtb for domUs. Having a mismatch may be confusing and having a 
> unit-address
> with leading zeroes looks unusual.
I decided to revert the patch mainly because it will be easier to review 
the fix if it is folded in this patch.
I would consider to create a wrapper on top of fdt_begin_node() that 
will take the name of the node and the unit. Something like:
/* XXX: Explain why the wrapper is needed */
static void domain_fdt_begin_node(void *fdt, const char *name, uint64_t 
unit)
{
   char buf[X];

   snprintf(buf, sizeof(buf), "%s@%"PRIx64, ...);
   /* XXX check the return of snprintf() */


   return fdt_begin_node(buf);
}

X would be a value that is large enough to accommodate the existing name.

The reason I don't suggest a new PRI* is because I can't think of a name 
that is short and descriptive enough to understand the different with 
the existing PRIpaddr.
Cheers,

--
Julien Grall