[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v3 1/3] xen/arm: Use the correct format specifier
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@xxxxxxx> > --- > > 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?
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |