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

[Auger Eric <eric.auger@redhat.com>]

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
>