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

[Stefano Stabellini <sstabellini@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3771 bytes --]

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@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?
> 
> 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)