[2/2] hw/arm/virt: Silence dtc /memory warning
diff mbox series

Message ID 1528554193-27270-3-git-send-email-eric.auger@redhat.com
State New
Headers show
Series
  • ARM virt: Silence dtc warnings
Related show

Commit Message

Auger Eric June 9, 2018, 2:23 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé June 9, 2018, 2:37 p.m. UTC | #1
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
>
Peter Maydell June 15, 2018, 12:26 p.m. UTC | #2
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
Peter Maydell June 15, 2018, 1:16 p.m. UTC | #3
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
Auger Eric June 15, 2018, 3:04 p.m. UTC | #4
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
>
Auger Eric June 15, 2018, 3:13 p.m. UTC | #5
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
>

Patch
diff mbox series

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