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 <[email protected]>
> ---
>
> 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?
