[Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings

[Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings

[Eric Auger]

When running dtc on the guest /proc/device-tree, we get the
following warnings: "Warning (unit_address_vs_reg): Node <name>
has a reg or ranges property, but no unit name", with name:
/intc, /intc/its, /intc/v2m, /memory.

This series removes those warnings by adding the unit address to
the corresponding node names.

Best Regards

Eric

Eric Auger (2):
  hw/arm/virt: Silence dtc /intc warnings
  hw/arm/virt: Silence dtc /memory warning

 hw/arm/boot.c | 20 ++++++++++++------
 hw/arm/virt.c | 65 ++++++++++++++++++++++++++++++++++-------------------------
 2 files changed, 51 insertions(+), 34 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/2] hw/arm/virt: Silence dtc /intc warnings

[Eric Auger]

When running dtc on the guest /proc/device-tree we get the
following warnings: "Warning (unit_address_vs_reg): Node <name>
has a reg or ranges property, but no unit name", with name:
/intc, /intc/its, /intc/v2m.

Nodes should have a name in the form <name>[@<unit-address>] where
unit-address is the primary address used to access the device, listed
in the node's reg property. This fix seems to make dtc happy.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---
---
 hw/arm/virt.c | 59 +++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 11b9f59..b1e9a6b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -368,66 +368,81 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
 
 static void fdt_add_its_gic_node(VirtMachineState *vms)
 {
+    char *nodename;
+
     vms->msi_phandle = qemu_fdt_alloc_phandle(vms->fdt);
-    qemu_fdt_add_subnode(vms->fdt, "/intc/its");
-    qemu_fdt_setprop_string(vms->fdt, "/intc/its", "compatible",
+    nodename = g_strdup_printf("/intc/its@%" PRIx64,
+                               vms->memmap[VIRT_GIC_ITS].base);
+    qemu_fdt_add_subnode(vms->fdt, nodename);
+    qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
                             "arm,gic-v3-its");
-    qemu_fdt_setprop(vms->fdt, "/intc/its", "msi-controller", NULL, 0);
-    qemu_fdt_setprop_sized_cells(vms->fdt, "/intc/its", "reg",
+    qemu_fdt_setprop(vms->fdt, nodename, "msi-controller", NULL, 0);
+    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
                                  2, vms->memmap[VIRT_GIC_ITS].base,
                                  2, vms->memmap[VIRT_GIC_ITS].size);
-    qemu_fdt_setprop_cell(vms->fdt, "/intc/its", "phandle", vms->msi_phandle);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", vms->msi_phandle);
+    g_free(nodename);
 }
 
 static void fdt_add_v2m_gic_node(VirtMachineState *vms)
 {
+    char *nodename;
+
+    nodename = g_strdup_printf("/intc/v2m@%" PRIx64,
+                               vms->memmap[VIRT_GIC_V2M].base);
     vms->msi_phandle = qemu_fdt_alloc_phandle(vms->fdt);
-    qemu_fdt_add_subnode(vms->fdt, "/intc/v2m");
-    qemu_fdt_setprop_string(vms->fdt, "/intc/v2m", "compatible",
+    qemu_fdt_add_subnode(vms->fdt, nodename);
+    qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
                             "arm,gic-v2m-frame");
-    qemu_fdt_setprop(vms->fdt, "/intc/v2m", "msi-controller", NULL, 0);
-    qemu_fdt_setprop_sized_cells(vms->fdt, "/intc/v2m", "reg",
+    qemu_fdt_setprop(vms->fdt, nodename, "msi-controller", NULL, 0);
+    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
                                  2, vms->memmap[VIRT_GIC_V2M].base,
                                  2, vms->memmap[VIRT_GIC_V2M].size);
-    qemu_fdt_setprop_cell(vms->fdt, "/intc/v2m", "phandle", vms->msi_phandle);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", vms->msi_phandle);
+    g_free(nodename);
 }
 
 static void fdt_add_gic_node(VirtMachineState *vms)
 {
+    char *nodename;
+
     vms->gic_phandle = qemu_fdt_alloc_phandle(vms->fdt);
     qemu_fdt_setprop_cell(vms->fdt, "/", "interrupt-parent", vms->gic_phandle);
 
-    qemu_fdt_add_subnode(vms->fdt, "/intc");
-    qemu_fdt_setprop_cell(vms->fdt, "/intc", "#interrupt-cells", 3);
-    qemu_fdt_setprop(vms->fdt, "/intc", "interrupt-controller", NULL, 0);
-    qemu_fdt_setprop_cell(vms->fdt, "/intc", "#address-cells", 0x2);
-    qemu_fdt_setprop_cell(vms->fdt, "/intc", "#size-cells", 0x2);
-    qemu_fdt_setprop(vms->fdt, "/intc", "ranges", NULL, 0);
+    nodename = g_strdup_printf("/intc@%" PRIx64,
+                               vms->memmap[VIRT_GIC_DIST].base);
+    qemu_fdt_add_subnode(vms->fdt, nodename);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "#interrupt-cells", 3);
+    qemu_fdt_setprop(vms->fdt, nodename, "interrupt-controller", NULL, 0);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 0x2);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 0x2);
+    qemu_fdt_setprop(vms->fdt, nodename, "ranges", NULL, 0);
     if (vms->gic_version == 3) {
-        qemu_fdt_setprop_string(vms->fdt, "/intc", "compatible",
+        qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
                                 "arm,gic-v3");
-        qemu_fdt_setprop_sized_cells(vms->fdt, "/intc", "reg",
+        qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
                                      2, vms->memmap[VIRT_GIC_DIST].base,
                                      2, vms->memmap[VIRT_GIC_DIST].size,
                                      2, vms->memmap[VIRT_GIC_REDIST].base,
                                      2, vms->memmap[VIRT_GIC_REDIST].size);
         if (vms->virt) {
-            qemu_fdt_setprop_cells(vms->fdt, "/intc", "interrupts",
+            qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
                                    GIC_FDT_IRQ_TYPE_PPI, ARCH_GICV3_MAINT_IRQ,
                                    GIC_FDT_IRQ_FLAGS_LEVEL_HI);
         }
     } else {
         /* 'cortex-a15-gic' means 'GIC v2' */
-        qemu_fdt_setprop_string(vms->fdt, "/intc", "compatible",
+        qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
                                 "arm,cortex-a15-gic");
-        qemu_fdt_setprop_sized_cells(vms->fdt, "/intc", "reg",
+        qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
                                       2, vms->memmap[VIRT_GIC_DIST].base,
                                       2, vms->memmap[VIRT_GIC_DIST].size,
                                       2, vms->memmap[VIRT_GIC_CPU].base,
                                       2, vms->memmap[VIRT_GIC_CPU].size);
     }
 
-    qemu_fdt_setprop_cell(vms->fdt, "/intc", "phandle", vms->gic_phandle);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", vms->gic_phandle);
+    g_free(nodename);
 }
 
 static void fdt_add_pmu_nodes(const VirtMachineState *vms)
-- 
2.5.5

[Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning

[Eric Auger]

When running dtc on the guest /proc/device-tree we get the
following warning: Warning (unit_address_vs_reg): Node /memory
has a reg or ranges property, but no unit name".

Let's fix that by adding the unit address to the node name. We also
don't create the /memory node anymore in create_fdt(). We directly
create it in load_dtb. /chosen still needs to be created in create_fdt
as the uart needs it. In case the user provided his own dtb, either
the bank is added to the existing /memory node or if this latter is
not found we create a new separate memory node.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/boot.c | 20 ++++++++++++++------
 hw/arm/virt.c |  6 ------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 1e2be20..2054670 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -593,24 +593,32 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
             g_free(nodename);
         }
     } else {
+        char *nodename = g_strdup("/memory");
         Error *err = NULL;
 
-        rc = fdt_path_offset(fdt, "/memory");
+        /* If there is an existing /memory node (user provided dtb), we add the
+         * new bank into it, otherwise we create a /memory@addr node
+         */
+        rc = fdt_path_offset(fdt, nodename);
         if (rc < 0) {
-            qemu_fdt_add_subnode(fdt, "/memory");
+            g_free(nodename);
+            nodename = g_strdup_printf("/memory@%" PRIx64, binfo->loader_start);
+
+            qemu_fdt_add_subnode(fdt, nodename);
         }
 
-        if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) {
-            qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory");
+        if (!qemu_fdt_getprop(fdt, nodename, "device_type", NULL, &err)) {
+            qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
         }
 
-        rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
+        rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
                                           acells, binfo->loader_start,
                                           scells, binfo->ram_size);
         if (rc < 0) {
-            fprintf(stderr, "couldn't set /memory/reg\n");
+            fprintf(stderr, "couldn't set %s reg\n", nodename);
             goto fail;
         }
+        g_free(nodename);
     }
 
     rc = fdt_path_offset(fdt, "/chosen");
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b1e9a6b..46c5c3f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -203,13 +203,7 @@ static void create_fdt(VirtMachineState *vms)
     qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
     qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
 
-    /*
-     * /chosen and /memory nodes must exist for load_dtb
-     * to fill in necessary properties later
-     */
     qemu_fdt_add_subnode(fdt, "/chosen");
-    qemu_fdt_add_subnode(fdt, "/memory");
-    qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory");
 
     /* Clock node, for the benefit of the UART. The kernel device tree
      * binding documentation claims the PL011 node clock properties are
-- 
2.5.5

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning

[Philippe Mathieu-Daudé]

Hi Eric,

On 06/09/2018 11:23 AM, Eric Auger wrote:
> When running dtc on the guest /proc/device-tree we get the
> following warning: Warning (unit_address_vs_reg): Node /memory
> has a reg or ranges property, but no unit name".
> 
> Let's fix that by adding the unit address to the node name. We also
> don't create the /memory node anymore in create_fdt(). We directly
> create it in load_dtb. /chosen still needs to be created in create_fdt
> as the uart needs it. In case the user provided his own dtb, either
> the bank is added to the existing /memory node or if this latter is
> not found we create a new separate memory node.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/boot.c | 20 ++++++++++++++------
>  hw/arm/virt.c |  6 ------
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 1e2be20..2054670 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -593,24 +593,32 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>              g_free(nodename);
>          }
>      } else {
> +        char *nodename = g_strdup("/memory");
>          Error *err = NULL;
>  
> -        rc = fdt_path_offset(fdt, "/memory");
> +        /* If there is an existing /memory node (user provided dtb), we add the
> +         * new bank into it, otherwise we create a /memory@addr node
> +         */
> +        rc = fdt_path_offset(fdt, nodename);
>          if (rc < 0) {
> -            qemu_fdt_add_subnode(fdt, "/memory");
> +            g_free(nodename);
> +            nodename = g_strdup_printf("/memory@%" PRIx64, binfo->loader_start);
> +
> +            qemu_fdt_add_subnode(fdt, nodename);
>          }
>  
> -        if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) {
> -            qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory");
> +        if (!qemu_fdt_getprop(fdt, nodename, "device_type", NULL, &err)) {
> +            qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
>          }
>  
> -        rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
> +        rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
>                                            acells, binfo->loader_start,
>                                            scells, binfo->ram_size);
>          if (rc < 0) {
> -            fprintf(stderr, "couldn't set /memory/reg\n");
> +            fprintf(stderr, "couldn't set %s reg\n", nodename);
>              goto fail;
>          }
> +        g_free(nodename);
>      }

This part looks good.

>  
>      rc = fdt_path_offset(fdt, "/chosen");
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b1e9a6b..46c5c3f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -203,13 +203,7 @@ static void create_fdt(VirtMachineState *vms)
>      qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
>      qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
>  
> -    /*
> -     * /chosen and /memory nodes must exist for load_dtb
> -     * to fill in necessary properties later
> -     */
>      qemu_fdt_add_subnode(fdt, "/chosen");

Here I think you should keep the comment, but update it (info from your
commit message).

> -    qemu_fdt_add_subnode(fdt, "/memory");
> -    qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory");
>  
>      /* Clock node, for the benefit of the UART. The kernel device tree
>       * binding documentation claims the PL011 node clock properties are
> 

Re: [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings

[Peter Maydell]

On 9 June 2018 at 15:23, Eric Auger <eric.auger@redhat.com> wrote:
> When running dtc on the guest /proc/device-tree, we get the
> following warnings: "Warning (unit_address_vs_reg): Node <name>
> has a reg or ranges property, but no unit name", with name:
> /intc, /intc/its, /intc/v2m, /memory.
>
> This series removes those warnings by adding the unit address to
> the corresponding node names.

Thanks for this patchset -- Marc Z reported this ages ago
but I never got round to looking at it.

Patch 1 looks fine, but patch 2 changing the "/memory" node
handling I'm less happy with. Currently we're consistent: we
assume that the memory node must always be called "/memory",
and we look for an existing node that way and create our own
with that name. If in fact memory nodes might not be named
"/memory" then we need to look for any preexisting "/memory@*" node,
not just "/memory", don't we?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Silence dtc /intc warnings

[Peter Maydell]

On 9 June 2018 at 15:23, Eric Auger <eric.auger@redhat.com> wrote:
> When running dtc on the guest /proc/device-tree we get the
> following warnings: "Warning (unit_address_vs_reg): Node <name>
> has a reg or ranges property, but no unit name", with name:
> /intc, /intc/its, /intc/v2m.
>
> Nodes should have a name in the form <name>[@<unit-address>] where
> unit-address is the primary address used to access the device, listed
> in the node's reg property. This fix seems to make dtc happy.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

I don't understand why this requirement is in the dt spec, but
whatever...

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning

[Peter Maydell]

On 9 June 2018 at 15:23, Eric Auger <eric.auger@redhat.com> wrote:
> When running dtc on the guest /proc/device-tree we get the
> following warning: Warning (unit_address_vs_reg): Node /memory
> has a reg or ranges property, but no unit name".
>
> Let's fix that by adding the unit address to the node name. We also
> don't create the /memory node anymore in create_fdt(). We directly
> create it in load_dtb. /chosen still needs to be created in create_fdt
> as the uart needs it. In case the user provided his own dtb, either
> the bank is added to the existing /memory node or if this latter is
> not found we create a new separate memory node.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/boot.c | 20 ++++++++++++++------
>  hw/arm/virt.c |  6 ------
>  2 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 1e2be20..2054670 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -593,24 +593,32 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>              g_free(nodename);
>          }
>      } else {

I think you need also to change the "if" half of this if..else,
which deals with NUMA, because it assumes a "/memory" node was
created by the virt.c code and now it will not be.

> +        char *nodename = g_strdup("/memory");
>          Error *err = NULL;
>
> -        rc = fdt_path_offset(fdt, "/memory");
> +        /* If there is an existing /memory node (user provided dtb), we add the
> +         * new bank into it, otherwise we create a /memory@addr node
> +         */
> +        rc = fdt_path_offset(fdt, nodename);

If we create /memory@addr nodes then we should also look for them in
an input dtb, I think. Otherwise we'll break the usecase where the
user asks QEMU to dump the generated dtb to a file, edits it and
then passes it back to a subsequent QEMU invocation, because we won't
find the memory node we created.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning

[Peter Maydell]

On 15 June 2018 at 13:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 June 2018 at 15:23, Eric Auger <eric.auger@redhat.com> wrote:
>> When running dtc on the guest /proc/device-tree we get the
>> following warning: Warning (unit_address_vs_reg): Node /memory
>> has a reg or ranges property, but no unit name".
>>
>> Let's fix that by adding the unit address to the node name. We also
>> don't create the /memory node anymore in create_fdt(). We directly
>> create it in load_dtb. /chosen still needs to be created in create_fdt
>> as the uart needs it. In case the user provided his own dtb, either
>> the bank is added to the existing /memory node or if this latter is
>> not found we create a new separate memory node.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/boot.c | 20 ++++++++++++++------
>>  hw/arm/virt.c |  6 ------
>>  2 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 1e2be20..2054670 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -593,24 +593,32 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>              g_free(nodename);
>>          }
>>      } else {
>
> I think you need also to change the "if" half of this if..else,
> which deals with NUMA, because it assumes a "/memory" node was
> created by the virt.c code and now it will not be.

...I think this breaks the tests/numa-test part of 'make check'.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning

[Auger Eric]

Hi Peter,

On 06/15/2018 03:16 PM, Peter Maydell wrote:
> On 15 June 2018 at 13:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 9 June 2018 at 15:23, Eric Auger <eric.auger@redhat.com> wrote:
>>> When running dtc on the guest /proc/device-tree we get the
>>> following warning: Warning (unit_address_vs_reg): Node /memory
>>> has a reg or ranges property, but no unit name".
>>>
>>> Let's fix that by adding the unit address to the node name. We also
>>> don't create the /memory node anymore in create_fdt(). We directly
>>> create it in load_dtb. /chosen still needs to be created in create_fdt
>>> as the uart needs it. In case the user provided his own dtb, either
>>> the bank is added to the existing /memory node or if this latter is
>>> not found we create a new separate memory node.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  hw/arm/boot.c | 20 ++++++++++++++------
>>>  hw/arm/virt.c |  6 ------
>>>  2 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>> index 1e2be20..2054670 100644
>>> --- a/hw/arm/boot.c
>>> +++ b/hw/arm/boot.c
>>> @@ -593,24 +593,32 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>>              g_free(nodename);
>>>          }
>>>      } else {
>>
>> I think you need also to change the "if" half of this if..else,
>> which deals with NUMA, because it assumes a "/memory" node was
>> created by the virt.c code and now it will not be.
> 
> ...I think this breaks the tests/numa-test part of 'make check'.

correct!

Thanks

Eric
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning

[Auger Eric]

Hi Peter,

On 06/15/2018 02:26 PM, Peter Maydell wrote:
> On 9 June 2018 at 15:23, Eric Auger <eric.auger@redhat.com> wrote:
>> When running dtc on the guest /proc/device-tree we get the
>> following warning: Warning (unit_address_vs_reg): Node /memory
>> has a reg or ranges property, but no unit name".
>>
>> Let's fix that by adding the unit address to the node name. We also
>> don't create the /memory node anymore in create_fdt(). We directly
>> create it in load_dtb. /chosen still needs to be created in create_fdt
>> as the uart needs it. In case the user provided his own dtb, either
>> the bank is added to the existing /memory node or if this latter is
>> not found we create a new separate memory node.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/boot.c | 20 ++++++++++++++------
>>  hw/arm/virt.c |  6 ------
>>  2 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 1e2be20..2054670 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -593,24 +593,32 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>              g_free(nodename);
>>          }
>>      } else {
> 
> I think you need also to change the "if" half of this if..else,
> which deals with NUMA, because it assumes a "/memory" node was
> created by the virt.c code and now it will not be.

You're right.

Besides, what is unclear to me is the use case where the end-user
provides his own dtb. In that case, do we want to "erase" all the
/memory@addr nodes defined by the user and replace them by the nodes
below? Potentially the end user could have provided several memory
nodes, right?

Same question for non NUMA case, do we want to remove all the /memory
nodes provided by the end-user?

Thanks

Eric
> 
>> +        char *nodename = g_strdup("/memory");
>>          Error *err = NULL;
>>
>> -        rc = fdt_path_offset(fdt, "/memory");
>> +        /* If there is an existing /memory node (user provided dtb), we add the
>> +         * new bank into it, otherwise we create a /memory@addr node
>> +         */
>> +        rc = fdt_path_offset(fdt, nodename);
> 
> If we create /memory@addr nodes then we should also look for them in
> an input dtb, I think. Otherwise we'll break the usecase where the
> user asks QEMU to dump the generated dtb to a file, edits it and
> then passes it back to a subsequent QEMU invocation, because we won't
> find the memory node we created.
> 
> thanks
> -- PMM
>