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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien/Michal,

On 20/01/2023 17:49, Julien Grall wrote:
> 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?
Does this change match the expectation ?

diff --git a/automation/scripts/qemu-smoke-dom0less-arm64.sh 
b/automation/scripts/qemu-smoke-dom0less-arm64.sh
index 2b59346fdc..9f5e700f0e 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm64.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm64.sh
@@ -20,7 +20,7 @@ if [[ "${test_variant}" == "static-mem" ]]; then
     domu_size="10000000"
     passed="${test_variant} test passed"
     domU_check="
-current=\$(hexdump -e '16/1 \"%02x\"' 
/proc/device-tree/memory@${domu_base}/reg 2>/dev/null)
+current=\$(hexdump -e '16/1 \"%02x\"' 
/proc/device-tree/memory@$[0-9]*/reg 2>/dev/null)
 expected=$(printf \"%016x%016x\" 0x${domu_base} 0x${domu_size})
 if [[ \"\${expected}\" == \"\${current}\" ]]; then
        echo \"${passed}\"

> > 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.
Looks fine to me. I can incorporate the change in this existing patch.

- Ayan

> Cheers,