Re: [PATCH] hw/arm/boot: Use NUMA node ID in memory node name

[Andrew Jones]

On Tue, Jun 01, 2021 at 03:30:04PM +0800, Gavin Shan wrote:
> We possibly populate empty nodes where memory isn't included and might
> be hot added at late time. The FDT memory nodes can't be created due
> to conflicts on their names if multiple empty nodes are specified.
> For example, the VM fails to start with the following error messages.
> 
>   /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64          \
>   -accel kvm -machine virt,gic-version=host                        \
>   -cpu host -smp 4,sockets=2,cores=2,threads=1 -m 1024M,maxmem=64G \
>   -object memory-backend-ram,id=mem0,size=512M                     \
>   -object memory-backend-ram,id=mem1,size=512M                     \
>   -numa node,nodeid=0,cpus=0-1,memdev=mem0                         \
>   -numa node,nodeid=1,cpus=2-3,memdev=mem1                         \
>   -numa node,nodeid=2                                              \
>   -numa node,nodeid=3                                              \
>     :
>   -device virtio-balloon-pci,id=balloon0,free-page-reporting=yes
> 
>   qemu-system-aarch64: FDT: Failed to create subnode /memory@80000000: \
>                        FDT_ERR_EXISTS
> 
> This fixes the issue by using NUMA node ID or zero in the memory node
> name to avoid the conflicting memory node names. With this applied, the
> VM can boot successfully with above command lines.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/boot.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index d7b059225e..3169bdf595 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -432,7 +432,12 @@ static int fdt_add_memory_node(void *fdt, uint32_t 
> acells, hwaddr mem_base,
>      char *nodename;
>      int ret;
>  
> -    nodename = g_strdup_printf("/memory@%" PRIx64, mem_base);
> +    if (numa_node_id >= 0) {
> +        nodename = g_strdup_printf("/memory@%d", numa_node_id);
> +    } else {
> +        nodename = g_strdup("/memory@0");
> +    }
> +
>      qemu_fdt_add_subnode(fdt, nodename);
>      qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
>      ret = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", acells, 
> mem_base,
> -- 
> 2.23.0
>

Hi Gavin,

Is it conventional to use the unit-address like this? If so, can you point
out where that's documented? If it's not conventional, then we shouldn't
do it. And then I'm not sure what we should do in this case. Here's a
couple links I found, but they don't really help...

https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#sect-node-names
https://devicetree-specification.readthedocs.io/en/latest/chapter3-devicenodes.html#memory-node

Thanks,
drew