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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano, Ayan, Julien

On 21/01/2023 00:01, Stefano Stabellini wrote:
> 
> 
> On Fri, 20 Jan 2023, Ayan Kumar Halder wrote:
>> 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}\"
> 
> We need to check for ${domu_base} with or without leading zeroes:
> 
> current=\$(hexdump -e '16/1 \"%02x\"' 
> /proc/device-tree/memory@*(0)${domu_base}/reg 2>/dev/null)
This check is still bound to the way Xen exposes a memory node in a device tree 
which might change
as Julien suggested. We need to have a check not relying on device-tree.
My proposal would be to use /proc/iomem which prints memory ranges in %08x 
format.
This would look like as follows:

mem_range=$(printf \"%08x-%08x\" ${domu_base} $(( ${domu_base} + ${domu_size} - 
1 )))
if grep -q \${mem_range} /proc/iomem; then
    echo ${passed}
fi

If you are ok with that, I will push a patch on Monday.

~Michal