| Message ID | 1528554193-27270-3-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM virt: Silence dtc warnings | expand |
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 >
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
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
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 >
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 >
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
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(-)