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

[Julien Grall <julien@xen.org>]

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@amd.com>
>>>>>
>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>
>>>>
>>>> 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