| Message ID | 20230123134451.47185-2-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Pre-requisite patches for supporting 32 bit physical address | expand |
On Mon, 23 Jan 2023, Ayan Kumar Halder wrote: > 1. One should use 'PRIpaddr' to display 'paddr_t' variables. However, > while creating nodes in fdt, the address (if present in the node name) > should be represented using 'PRIx64'. This is to be in conformance > with the following rule present in https://elinux.org/Device_Tree_Linux > > . node names > "unit-address does not have leading zeros" > > As 'PRIpaddr' introduces leading zeros, we cannot use it. > > So, we have introduced a wrapper ie domain_fdt_begin_node() which will > represent physical address using 'PRIx64'. > > 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. > > v2 - 1. Use PRIx64 for appending addresses to fdt node names. This fixes the CI failure. > > xen/arch/arm/domain_build.c | 45 +++++++++++++++++-------------------- > xen/arch/arm/gic-v2.c | 6 ++--- > xen/arch/arm/mm.c | 2 +- The changes to mm.c and gic-v2.c look OK and I'd ack them already. One question on the changes to domain_build.c below. > 3 files changed, 25 insertions(+), 28 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index f35f4d2456..97c2395f9a 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1288,6 +1288,20 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo, > return res; > } > > +static int __init domain_fdt_begin_node(void *fdt, const char *name, > + uint64_t unit) > +{ > + /* > + * The size of the buffer to hold the longest possible string ie > + * interrupt-controller@ + a 64-bit number + \0 > + */ > + char buf[38]; > + > + /* ePAPR 3.4 */ > + snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit); > + return fdt_begin_node(fdt, buf); > +} > + > static int __init make_memory_node(const struct domain *d, > void *fdt, > int addrcells, int sizecells, > @@ -1296,8 +1310,6 @@ static int __init make_memory_node(const struct domain *d, > unsigned int i; > int res, reg_size = addrcells + sizecells; > int nr_cells = 0; > - /* Placeholder for memory@ + a 64-bit number + \0 */ > - char buf[24]; > __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; > __be32 *cells; > > @@ -1314,9 +1326,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); > - res = fdt_begin_node(fdt, buf); > + res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start); Basically this "hides" the paddr_t->uint64_t cast because it happens implicitly when passing mem->bank[i].start as an argument to domain_fdt_begin_node. To be honest, I don't know if it is necessary. Also a normal cast would be fine: snprintf(buf, sizeof(buf), "memory@%"PRIx64, (uint64_t)mem->bank[i].start); res = fdt_begin_node(fdt, buf); Julien, what do you prefer?
Hi Stefano, On 23/01/2023 21:19, Stefano Stabellini wrote: > On Mon, 23 Jan 2023, Ayan Kumar Halder wrote: >> 1. One should use 'PRIpaddr' to display 'paddr_t' variables. However, >> while creating nodes in fdt, the address (if present in the node name) >> should be represented using 'PRIx64'. This is to be in conformance >> with the following rule present in https://elinux.org/Device_Tree_Linux >> >> . node names >> "unit-address does not have leading zeros" >> >> As 'PRIpaddr' introduces leading zeros, we cannot use it. >> >> So, we have introduced a wrapper ie domain_fdt_begin_node() which will >> represent physical address using 'PRIx64'. >> >> 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. >> >> v2 - 1. Use PRIx64 for appending addresses to fdt node names. This fixes the CI failure. >> >> xen/arch/arm/domain_build.c | 45 +++++++++++++++++-------------------- >> xen/arch/arm/gic-v2.c | 6 ++--- >> xen/arch/arm/mm.c | 2 +- > > The changes to mm.c and gic-v2.c look OK and I'd ack them already. One > question on the changes to domain_build.c below. > > >> 3 files changed, 25 insertions(+), 28 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index f35f4d2456..97c2395f9a 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -1288,6 +1288,20 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo, >> return res; >> } >> >> +static int __init domain_fdt_begin_node(void *fdt, const char *name, >> + uint64_t unit) >> +{ >> + /* >> + * The size of the buffer to hold the longest possible string ie >> + * interrupt-controller@ + a 64-bit number + \0 >> + */ >> + char buf[38]; >> + >> + /* ePAPR 3.4 */ >> + snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit); The return wants to be checked. >> + return fdt_begin_node(fdt, buf); >> +} >> + >> static int __init make_memory_node(const struct domain *d, >> void *fdt, >> int addrcells, int sizecells, >> @@ -1296,8 +1310,6 @@ static int __init make_memory_node(const struct domain *d, >> unsigned int i; >> int res, reg_size = addrcells + sizecells; >> int nr_cells = 0; >> - /* Placeholder for memory@ + a 64-bit number + \0 */ >> - char buf[24]; >> __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; >> __be32 *cells; >> >> @@ -1314,9 +1326,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); >> - res = fdt_begin_node(fdt, buf); >> + res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start); > > Basically this "hides" the paddr_t->uint64_t cast because it happens > implicitly when passing mem->bank[i].start as an argument to > domain_fdt_begin_node. > > To be honest, I don't know if it is necessary. Also a normal cast would > be fine: > > snprintf(buf, sizeof(buf), "memory@%"PRIx64, (uint64_t)mem->bank[i].start); > res = fdt_begin_node(fdt, buf); The problem with the open-coding version is you would need to explain the cast everywhere (I disliked unexplained one). I don't particular mind 'hidden cast' but I think we need to explain on top of domain_fdt_begin_node() why it is necessary. > > Julien, what do you prefer? Definitely the function because that's what I suggested (see the rationale above). Cheers,
On Mon, 23 Jan 2023, Julien Grall wrote: > Hi Stefano, > > On 23/01/2023 21:19, Stefano Stabellini wrote: > > On Mon, 23 Jan 2023, Ayan Kumar Halder wrote: > > > 1. One should use 'PRIpaddr' to display 'paddr_t' variables. However, > > > while creating nodes in fdt, the address (if present in the node name) > > > should be represented using 'PRIx64'. This is to be in conformance > > > with the following rule present in https://elinux.org/Device_Tree_Linux > > > > > > . node names > > > "unit-address does not have leading zeros" > > > > > > As 'PRIpaddr' introduces leading zeros, we cannot use it. > > > > > > So, we have introduced a wrapper ie domain_fdt_begin_node() which will > > > represent physical address using 'PRIx64'. > > > > > > 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. > > > > > > v2 - 1. Use PRIx64 for appending addresses to fdt node names. This fixes > > > the CI failure. > > > > > > xen/arch/arm/domain_build.c | 45 +++++++++++++++++-------------------- > > > xen/arch/arm/gic-v2.c | 6 ++--- > > > xen/arch/arm/mm.c | 2 +- > > > > The changes to mm.c and gic-v2.c look OK and I'd ack them already. One > > question on the changes to domain_build.c below. > > > > > > > 3 files changed, 25 insertions(+), 28 deletions(-) > > > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > > index f35f4d2456..97c2395f9a 100644 > > > --- a/xen/arch/arm/domain_build.c > > > +++ b/xen/arch/arm/domain_build.c > > > @@ -1288,6 +1288,20 @@ static int __init fdt_property_interrupts(const > > > struct kernel_info *kinfo, > > > return res; > > > } > > > +static int __init domain_fdt_begin_node(void *fdt, const char *name, > > > + uint64_t unit) > > > +{ > > > + /* > > > + * The size of the buffer to hold the longest possible string ie > > > + * interrupt-controller@ + a 64-bit number + \0 > > > + */ > > > + char buf[38]; > > > + > > > + /* ePAPR 3.4 */ > > > + snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit); > > The return wants to be checked. > > > > + return fdt_begin_node(fdt, buf); > > > +} > > > + > > > static int __init make_memory_node(const struct domain *d, > > > void *fdt, > > > int addrcells, int sizecells, > > > @@ -1296,8 +1310,6 @@ static int __init make_memory_node(const struct > > > domain *d, > > > unsigned int i; > > > int res, reg_size = addrcells + sizecells; > > > int nr_cells = 0; > > > - /* Placeholder for memory@ + a 64-bit number + \0 */ > > > - char buf[24]; > > > __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; > > > __be32 *cells; > > > @@ -1314,9 +1326,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); > > > - res = fdt_begin_node(fdt, buf); > > > + res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start); > > > > Basically this "hides" the paddr_t->uint64_t cast because it happens > > implicitly when passing mem->bank[i].start as an argument to > > domain_fdt_begin_node. > > > > To be honest, I don't know if it is necessary. Also a normal cast would > > be fine: > > > > snprintf(buf, sizeof(buf), "memory@%"PRIx64, > > (uint64_t)mem->bank[i].start); > > res = fdt_begin_node(fdt, buf); > The problem with the open-coding version is you would need to explain the cast > everywhere (I disliked unexplained one). > > I don't particular mind 'hidden cast' but I think we need to explain on top of > domain_fdt_begin_node() why it is necessary. > > > > > Julien, what do you prefer? > > Definitely the function because that's what I suggested (see the rationale > above). OK, no worries
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index f35f4d2456..97c2395f9a 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1288,6 +1288,20 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo, return res; } +static int __init domain_fdt_begin_node(void *fdt, const char *name, + uint64_t unit) +{ + /* + * The size of the buffer to hold the longest possible string ie + * interrupt-controller@ + a 64-bit number + \0 + */ + char buf[38]; + + /* ePAPR 3.4 */ + snprintf(buf, sizeof(buf), "%s@%"PRIx64, name, unit); + return fdt_begin_node(fdt, buf); +} + static int __init make_memory_node(const struct domain *d, void *fdt, int addrcells, int sizecells, @@ -1296,8 +1310,6 @@ static int __init make_memory_node(const struct domain *d, unsigned int i; int res, reg_size = addrcells + sizecells; int nr_cells = 0; - /* Placeholder for memory@ + a 64-bit number + \0 */ - char buf[24]; __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; __be32 *cells; @@ -1314,9 +1326,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); - res = fdt_begin_node(fdt, buf); + res = domain_fdt_begin_node(fdt, "memory", mem->bank[i].start); if ( res ) return res; @@ -1375,16 +1385,13 @@ static int __init make_shm_memory_node(const struct domain *d, { uint64_t start = mem->bank[i].start; uint64_t size = mem->bank[i].size; - /* Placeholder for xen-shmem@ + a 64-bit number + \0 */ - char buf[27]; const char compat[] = "xen,shared-memory-v1"; /* Worst case addrcells + sizecells */ __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; __be32 *cells; unsigned int len = (addrcells + sizecells) * sizeof(__be32); - snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start); - res = fdt_begin_node(fdt, buf); + res = domain_fdt_begin_node(fdt, "xen-shmem", mem->bank[i].start); if ( res ) return res; @@ -2716,12 +2723,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; __be32 *cells; const struct domain *d = kinfo->d; - /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */ - char buf[38]; - snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, - vgic_dist_base(&d->arch.vgic)); - res = fdt_begin_node(fdt, buf); + res = domain_fdt_begin_node(fdt, "interrupt-controller", + vgic_dist_base(&d->arch.vgic)); if ( res ) return res; @@ -2771,14 +2775,10 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo) int res = 0; __be32 *reg, *cells; const struct domain *d = kinfo->d; - /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */ - char buf[38]; unsigned int i, len = 0; - snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, - vgic_dist_base(&d->arch.vgic)); - - res = fdt_begin_node(fdt, buf); + res = domain_fdt_begin_node(fdt, "interrupt-controller", + vgic_dist_base(&d->arch.vgic)); if ( res ) return res; @@ -2858,11 +2858,8 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; __be32 *cells; struct domain *d = kinfo->d; - /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */ - char buf[27]; - snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr); - res = fdt_begin_node(fdt, buf); + res = domain_fdt_begin_node(fdt, "sbsa-uart", d->arch.vpl011.base_addr); 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. However, while creating nodes in fdt, the address (if present in the node name) should be represented using 'PRIx64'. This is to be in conformance with the following rule present in https://elinux.org/Device_Tree_Linux . node names "unit-address does not have leading zeros" As 'PRIpaddr' introduces leading zeros, we cannot use it. So, we have introduced a wrapper ie domain_fdt_begin_node() which will represent physical address using 'PRIx64'. 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. v2 - 1. Use PRIx64 for appending addresses to fdt node names. This fixes the CI failure. xen/arch/arm/domain_build.c | 45 +++++++++++++++++-------------------- xen/arch/arm/gic-v2.c | 6 ++--- xen/arch/arm/mm.c | 2 +- 3 files changed, 25 insertions(+), 28 deletions(-)