| Message ID | 20230117174358.15344-3-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for 32 bit physical address | expand |
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> > --- > > Changes from - > > v1 - 1. Moved the patch earlier. > 2. Moved a part of change from "[XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr" > into this patch. > > xen/arch/arm/domain_build.c | 10 +++++----- > xen/arch/arm/gic-v2.c | 6 +++--- > xen/arch/arm/mm.c | 2 +- > 3 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 829cea8de8..33a5945a2d 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1315,7 +1315,7 @@ static int __init make_memory_node(const struct domain *d, > dt_dprintk("Create memory node\n"); > > /* ePAPR 3.4 */ > - snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start); > + snprintf(buf, sizeof(buf), "memory@%"PRIpaddr, mem->bank[i].start); > res = fdt_begin_node(fdt, buf); > if ( res ) > return res; > @@ -1383,7 +1383,7 @@ static int __init make_shm_memory_node(const struct domain *d, > __be32 *cells; > unsigned int len = (addrcells + sizecells) * sizeof(__be32); > > - snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start); > + snprintf(buf, sizeof(buf), "xen-shmem@%"PRIpaddr, mem->bank[i].start); > res = fdt_begin_node(fdt, buf); > if ( res ) > return res; > @@ -2719,7 +2719,7 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) > /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */ > char buf[38]; > > - snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, > + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr, > vgic_dist_base(&d->arch.vgic)); > res = fdt_begin_node(fdt, buf); > if ( res ) > @@ -2775,7 +2775,7 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo) > char buf[38]; > unsigned int i, len = 0; > > - snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, > + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr, > vgic_dist_base(&d->arch.vgic)); > > res = fdt_begin_node(fdt, buf); > @@ -2861,7 +2861,7 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) > /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */ > char buf[27]; > > - snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr); > + snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIpaddr, d->arch.vpl011.base_addr); > res = fdt_begin_node(fdt, buf); > if ( res ) > return res; > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 61802839cb..5d4d298b86 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -1049,7 +1049,7 @@ static void __init gicv2_dt_init(void) > if ( csize < SZ_8K ) > { > printk(XENLOG_WARNING "GICv2: WARNING: " > - "The GICC size is too small: %#"PRIx64" expected %#x\n", > + "The GICC size is too small: %#"PRIpaddr" expected %#x\n", > csize, SZ_8K); > if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) ) > { > @@ -1280,11 +1280,11 @@ static int __init gicv2_init(void) > gicv2.map_cbase += aliased_offset; > > printk(XENLOG_WARNING > - "GICv2: Adjusting CPU interface base to %#"PRIx64"\n", > + "GICv2: Adjusting CPU interface base to %#"PRIpaddr"\n", > cbase + aliased_offset); > } else if ( csize == SZ_128K ) > printk(XENLOG_WARNING > - "GICv2: GICC size=%#"PRIx64" but not aliased\n", > + "GICv2: GICC size=%#"PRIpaddr" but not aliased\n", > csize); > > gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE); > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 0fc6f2992d..fab54618ab 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -249,7 +249,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr, > > pte = mapping[offsets[level]]; > > - printk("%s[0x%03x] = 0x%"PRIpaddr"\n", > + printk("%s[0x%03x] = 0x%"PRIx64"\n", > level_strs[level], offsets[level], pte.bits); > > if ( level == 3 || !pte.walk.valid || !pte.walk.table ) > -- > 2.17.1 >
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. Cheers,
On 20/01/2023 09:32, Julien Grall wrote: > Hi, Hi Julien, > > 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. Thanks for the reviews and commit. :) Did you miss "[XEN v2 01/11] xen/ns16550: Remove unneeded truncation check in the DT init code" ? - Ayan > > Cheers, >
Hello, 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 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, 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. [1] https://elinux.org/Device_Tree_Linux > > Cheers, > > -- > Julien Grall > ~Michal
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). Cheers, [2] https://www.devicetree.org/
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@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. 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
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,
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}\" > >> 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, >
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)
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@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) 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
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 829cea8de8..33a5945a2d 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1315,7 +1315,7 @@ static int __init make_memory_node(const struct domain *d, dt_dprintk("Create memory node\n"); /* ePAPR 3.4 */ - snprintf(buf, sizeof(buf), "memory@%"PRIx64, mem->bank[i].start); + snprintf(buf, sizeof(buf), "memory@%"PRIpaddr, mem->bank[i].start); res = fdt_begin_node(fdt, buf); if ( res ) return res; @@ -1383,7 +1383,7 @@ static int __init make_shm_memory_node(const struct domain *d, __be32 *cells; unsigned int len = (addrcells + sizecells) * sizeof(__be32); - snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start); + snprintf(buf, sizeof(buf), "xen-shmem@%"PRIpaddr, mem->bank[i].start); res = fdt_begin_node(fdt, buf); if ( res ) return res; @@ -2719,7 +2719,7 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */ char buf[38]; - snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr, vgic_dist_base(&d->arch.vgic)); res = fdt_begin_node(fdt, buf); if ( res ) @@ -2775,7 +2775,7 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo) char buf[38]; unsigned int i, len = 0; - snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIpaddr, vgic_dist_base(&d->arch.vgic)); res = fdt_begin_node(fdt, buf); @@ -2861,7 +2861,7 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */ char buf[27]; - snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr); + snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIpaddr, d->arch.vpl011.base_addr); res = fdt_begin_node(fdt, buf); if ( res ) return res; diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 61802839cb..5d4d298b86 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -1049,7 +1049,7 @@ static void __init gicv2_dt_init(void) if ( csize < SZ_8K ) { printk(XENLOG_WARNING "GICv2: WARNING: " - "The GICC size is too small: %#"PRIx64" expected %#x\n", + "The GICC size is too small: %#"PRIpaddr" expected %#x\n", csize, SZ_8K); if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) ) { @@ -1280,11 +1280,11 @@ static int __init gicv2_init(void) gicv2.map_cbase += aliased_offset; printk(XENLOG_WARNING - "GICv2: Adjusting CPU interface base to %#"PRIx64"\n", + "GICv2: Adjusting CPU interface base to %#"PRIpaddr"\n", cbase + aliased_offset); } else if ( csize == SZ_128K ) printk(XENLOG_WARNING - "GICv2: GICC size=%#"PRIx64" but not aliased\n", + "GICv2: GICC size=%#"PRIpaddr" but not aliased\n", csize); gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE); diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 0fc6f2992d..fab54618ab 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -249,7 +249,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr, pte = mapping[offsets[level]]; - printk("%s[0x%03x] = 0x%"PRIpaddr"\n", + printk("%s[0x%03x] = 0x%"PRIx64"\n", level_strs[level], offsets[level], pte.bits); if ( level == 3 || !pte.walk.valid || !pte.walk.table )
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> --- Changes from - v1 - 1. Moved the patch earlier. 2. Moved a part of change from "[XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr" into this patch. xen/arch/arm/domain_build.c | 10 +++++----- xen/arch/arm/gic-v2.c | 6 +++--- xen/arch/arm/mm.c | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-)