* [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings @ 2018-06-09 14:23 Eric Auger 2018-06-09 14:23 ` [Qemu-devel] [PATCH 1/2] hw/arm/virt: Silence dtc /intc warnings Eric Auger ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Eric Auger @ 2018-06-09 14:23 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH 1/2] hw/arm/virt: Silence dtc /intc warnings 2018-06-09 14:23 [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings Eric Auger @ 2018-06-09 14:23 ` Eric Auger 2018-06-15 12:23 ` Peter Maydell 2018-06-09 14:23 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning Eric Auger 2018-06-15 12:22 ` [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings Peter Maydell 2 siblings, 1 reply; 10+ messages in thread From: Eric Auger @ 2018-06-09 14:23 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Silence dtc /intc warnings 2018-06-09 14:23 ` [Qemu-devel] [PATCH 1/2] hw/arm/virt: Silence dtc /intc warnings Eric Auger @ 2018-06-15 12:23 ` Peter Maydell 0 siblings, 0 replies; 10+ messages in thread From: Peter Maydell @ 2018-06-15 12:23 UTC (permalink / raw) To: Eric Auger; +Cc: Eric Auger, QEMU Developers, qemu-arm 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning 2018-06-09 14:23 [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings Eric Auger 2018-06-09 14:23 ` [Qemu-devel] [PATCH 1/2] hw/arm/virt: Silence dtc /intc warnings Eric Auger @ 2018-06-09 14:23 ` Eric Auger 2018-06-09 14:37 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé 2018-06-15 12:26 ` [Qemu-devel] " Peter Maydell 2018-06-15 12:22 ` [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings Peter Maydell 2 siblings, 2 replies; 10+ messages in thread From: Eric Auger @ 2018-06-09 14:23 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning 2018-06-09 14:23 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning Eric Auger @ 2018-06-09 14:37 ` Philippe Mathieu-Daudé 2018-06-15 12:26 ` [Qemu-devel] " Peter Maydell 1 sibling, 0 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2018-06-09 14:37 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning 2018-06-09 14:23 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning Eric Auger 2018-06-09 14:37 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé @ 2018-06-15 12:26 ` Peter Maydell 2018-06-15 13:16 ` Peter Maydell 2018-06-15 15:13 ` Auger Eric 1 sibling, 2 replies; 10+ messages in thread From: Peter Maydell @ 2018-06-15 12:26 UTC (permalink / raw) To: Eric Auger; +Cc: Eric Auger, QEMU Developers, qemu-arm 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning 2018-06-15 12:26 ` [Qemu-devel] " Peter Maydell @ 2018-06-15 13:16 ` Peter Maydell 2018-06-15 15:04 ` Auger Eric 2018-06-15 15:13 ` Auger Eric 1 sibling, 1 reply; 10+ messages in thread From: Peter Maydell @ 2018-06-15 13:16 UTC (permalink / raw) To: Eric Auger; +Cc: Eric Auger, QEMU Developers, qemu-arm 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning 2018-06-15 13:16 ` Peter Maydell @ 2018-06-15 15:04 ` Auger Eric 0 siblings, 0 replies; 10+ messages in thread From: Auger Eric @ 2018-06-15 15:04 UTC (permalink / raw) To: Peter Maydell; +Cc: Eric Auger, QEMU Developers, qemu-arm 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning 2018-06-15 12:26 ` [Qemu-devel] " Peter Maydell 2018-06-15 13:16 ` Peter Maydell @ 2018-06-15 15:13 ` Auger Eric 1 sibling, 0 replies; 10+ messages in thread From: Auger Eric @ 2018-06-15 15:13 UTC (permalink / raw) To: Peter Maydell; +Cc: Eric Auger, QEMU Developers, qemu-arm 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings 2018-06-09 14:23 [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings Eric Auger 2018-06-09 14:23 ` [Qemu-devel] [PATCH 1/2] hw/arm/virt: Silence dtc /intc warnings Eric Auger 2018-06-09 14:23 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning Eric Auger @ 2018-06-15 12:22 ` Peter Maydell 2 siblings, 0 replies; 10+ messages in thread From: Peter Maydell @ 2018-06-15 12:22 UTC (permalink / raw) To: Eric Auger; +Cc: Eric Auger, QEMU Developers, qemu-arm 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-06-15 15:14 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-09 14:23 [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings Eric Auger 2018-06-09 14:23 ` [Qemu-devel] [PATCH 1/2] hw/arm/virt: Silence dtc /intc warnings Eric Auger 2018-06-15 12:23 ` Peter Maydell 2018-06-09 14:23 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Silence dtc /memory warning Eric Auger 2018-06-09 14:37 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé 2018-06-15 12:26 ` [Qemu-devel] " Peter Maydell 2018-06-15 13:16 ` Peter Maydell 2018-06-15 15:04 ` Auger Eric 2018-06-15 15:13 ` Auger Eric 2018-06-15 12:22 ` [Qemu-devel] [PATCH 0/2] ARM virt: Silence dtc warnings Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.