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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

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.
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.

> 
> Cheers,
> 
> [2] https://www.devicetree.org/
> 
> --
> Julien Grall

~Michal