[PATCH v2 0/8] hw/arm/virt: Fix dt-schema warnings

[PATCH v2 0/8] hw/arm/virt: Fix dt-schema warnings

[Jean-Philippe Brucker]

Fix some warnings thrown by dt-validate for the aarch64 virt devicetree.

Since v1 [1]:
* Submitted more DT bindings changes where appropriate [2]. All of them
  applied for Linux v6.1 so I dropped the related QEMU changes.
* Grouped all node name changes into patch 8
* Improved commit messages

I'm testing by running various virt machine configurations with -M
dumpdtb=qemu.dtb, then running dt-validate [3] and dtc:

  dt-validate -s linux/Documentation/devicetree/bindings/ qemu.dtb
  dtc -O dts qemu.dtb -o qemu.dts

[1] https://lore.kernel.org/all/20220824155113.286730-1-jean-philippe@linaro.org/
[2] SMMU interrupt order https://lore.kernel.org/all/20220916133145.1910549-1-jean-philippe@linaro.org/
    arch-timer compatible https://lore.kernel.org/all/20220922161149.371565-1-jean-philippe@linaro.org/
    virtio-iommu https://lore.kernel.org/all/20220923074435.420531-1-jean-philippe@linaro.org/
[3] https://github.com/devicetree-org/dt-schema

Jean-Philippe Brucker (8):
  hw/arm/virt: Fix devicetree warning about the root node
  hw/arm/virt: Fix devicetree warning about the GIC node
  hw/arm/virt: Use "msi-map" devicetree property for PCI
  hw/arm/virt: Fix devicetree warning about the gpio-key node
  hw/arm/virt: Fix devicetree warnings about the GPIO node
  hw/arm/virt: Fix devicetree warning about the SMMU node
  hw/arm/virt: Fix devicetree warnings about the virtio-iommu node
  hw/arm/virt: Fix devicetree warnings about node names

 hw/arm/virt.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

-- 
2.37.3

[PATCH v2 1/8] hw/arm/virt: Fix devicetree warning about the root node

[Jean-Philippe Brucker]

The devicetree specification requires a 'model' property in the root
node. Fix the corresponding dt-validate warning:

  /: 'model' is a required property
  From schema: dtschema/schemas/root-node.yaml

Use the same name for model as for compatible. The specification
recommends that 'compatible' follows the format 'manufacturer,model' and
'model' follows the format 'manufacturer,model-number'. Since our
'compatible' doesn't observe this, 'model' doesn't really need to
either.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1a6480fd2a..83ab1a37fb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -252,6 +252,7 @@ static void create_fdt(VirtMachineState *vms)
     qemu_fdt_setprop_string(fdt, "/", "compatible", "linux,dummy-virt");
     qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
     qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
+    qemu_fdt_setprop_string(fdt, "/", "model", "linux,dummy-virt");
 
     /* /chosen must exist for load_dtb to fill in necessary properties later */
     qemu_fdt_add_subnode(fdt, "/chosen");
-- 
2.37.3

[PATCH v2 2/8] hw/arm/virt: Fix devicetree warning about the GIC node

[Jean-Philippe Brucker]

The GICv3 bindings requires a #msi-cells property for the ITS node. Fix
the corresponding dt-validate warning:

  interrupt-controller@8000000: msi-controller@8080000: '#msi-cells' is a required property
  From schema: linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 83ab1a37fb..ed6f1ac41e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -487,6 +487,7 @@ static void fdt_add_its_gic_node(VirtMachineState *vms)
     qemu_fdt_setprop_string(ms->fdt, nodename, "compatible",
                             "arm,gic-v3-its");
     qemu_fdt_setprop(ms->fdt, nodename, "msi-controller", NULL, 0);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#msi-cells", 1);
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, vms->memmap[VIRT_GIC_ITS].base,
                                  2, vms->memmap[VIRT_GIC_ITS].size);
-- 
2.37.3

[PATCH v2 3/8] hw/arm/virt: Use "msi-map" devicetree property for PCI

[Jean-Philippe Brucker]

The "msi-parent" property can be used on the PCI node when MSIs do not
contain sideband data (device IDs) [1]. In QEMU, MSI transactions
contain the requester ID, so the PCI node should use the "msi-map"
property instead of "msi-parent". In our case the property describes an
identity map between requester ID and sideband data.

This fixes a warning when passing the DTB generated by QEMU to dtc,
following a recent change to the GICv3 node:

  Warning (msi_parent_property): /pcie@10000000:msi-parent: property size (4) too small for cell size 1

[1] linux/Documentation/devicetree/bindings/pci/pci-msi.txt

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ed6f1ac41e..8605f5058a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1489,8 +1489,8 @@ static void create_pcie(VirtMachineState *vms)
     qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
 
     if (vms->msi_phandle) {
-        qemu_fdt_setprop_cells(ms->fdt, nodename, "msi-parent",
-                               vms->msi_phandle);
+        qemu_fdt_setprop_cells(ms->fdt, nodename, "msi-map",
+                               0, vms->msi_phandle, 0, 0x10000);
     }
 
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
-- 
2.37.3

[PATCH v2 4/8] hw/arm/virt: Fix devicetree warning about the gpio-key node

[Jean-Philippe Brucker]

The node name of the gpio-key devicetree node should be "key-poweroff":

  gpio-keys: 'poweroff' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+'
  From schema: linux/Documentation/devicetree/bindings/input/gpio-keys.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8605f5058a..6805c57530 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -932,12 +932,12 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
     qemu_fdt_add_subnode(fdt, "/gpio-keys");
     qemu_fdt_setprop_string(fdt, "/gpio-keys", "compatible", "gpio-keys");
 
-    qemu_fdt_add_subnode(fdt, "/gpio-keys/poweroff");
-    qemu_fdt_setprop_string(fdt, "/gpio-keys/poweroff",
+    qemu_fdt_add_subnode(fdt, "/gpio-keys/key-poweroff");
+    qemu_fdt_setprop_string(fdt, "/gpio-keys/key-poweroff",
                             "label", "GPIO Key Poweroff");
-    qemu_fdt_setprop_cell(fdt, "/gpio-keys/poweroff", "linux,code",
+    qemu_fdt_setprop_cell(fdt, "/gpio-keys/key-poweroff", "linux,code",
                           KEY_POWER);
-    qemu_fdt_setprop_cells(fdt, "/gpio-keys/poweroff",
+    qemu_fdt_setprop_cells(fdt, "/gpio-keys/key-poweroff",
                            "gpios", phandle, 3, 0);
 }
 
-- 
2.37.3

[PATCH v2 5/8] hw/arm/virt: Fix devicetree warnings about the GPIO node

[Jean-Philippe Brucker]

Since the pl061 device can be used as interrupt controller, its node
should contain "interrupt-controller" and "#interrupt-cells" properties.
Fix the corresponding dt-validate warnings:

  pl061@9030000: 'interrupt-controller' is a required property
  From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
  pl061@9030000: '#interrupt-cells' is a required property
  From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 6805c57530..10ce66c722 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1012,6 +1012,8 @@ static void create_gpio_devices(const VirtMachineState *vms, int gpio,
     qemu_fdt_setprop_cell(ms->fdt, nodename, "clocks", vms->clock_phandle);
     qemu_fdt_setprop_string(ms->fdt, nodename, "clock-names", "apb_pclk");
     qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle", phandle);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#interrupt-cells", 2);
+    qemu_fdt_setprop(ms->fdt, nodename, "interrupt-controller", NULL, 0);
 
     if (gpio != VIRT_GPIO) {
         /* Mark as not usable by the normal world */
-- 
2.37.3

[PATCH v2 6/8] hw/arm/virt: Fix devicetree warning about the SMMU node

[Jean-Philippe Brucker]

The SMMUv3 node isn't expected to have clock properties. Fix the
corresponding dt-validate warning:

  smmuv3@9050000: 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
  From schema: linux/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 10ce66c722..2de16f6324 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1362,8 +1362,6 @@ static void create_smmu(const VirtMachineState *vms,
     qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
                      sizeof(irq_names));
 
-    qemu_fdt_setprop_cell(ms->fdt, node, "clocks", vms->clock_phandle);
-    qemu_fdt_setprop_string(ms->fdt, node, "clock-names", "apb_pclk");
     qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
 
     qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
-- 
2.37.3

[PATCH v2 7/8] hw/arm/virt: Fix devicetree warnings about the virtio-iommu node

[Jean-Philippe Brucker]

The "PCI Bus Binding to: IEEE Std 1275-1994" defines the compatible
string for a PCIe bus or endpoint as "pci<vendorid>,<deviceid>" or
similar. Since the initial binding for PCI virtio-iommu didn't follow
this rule, it was modified to accept both strings and ensure backward
compatibility. Also, the unit-name for the node should be
"device,function".

Fix corresponding dt-validate and dtc warnings:

  pcie@10000000: virtio_iommu@16:compatible: ['virtio,pci-iommu'] does not contain items matching the given schema
  pcie@10000000: Unevaluated properties are not allowed (... 'virtio_iommu@16' were unexpected)
  From schema: linux/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
  virtio_iommu@16: compatible: 'oneOf' conditional failed, one must be fixed:
        ['virtio,pci-iommu'] is too short
        'pci1af4,1057' was expected
  From schema: dtschema/schemas/pci/pci-bus.yaml

  Warning (pci_device_reg): /pcie@10000000/virtio_iommu@16: PCI unit address format error, expected "2,0"

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2de16f6324..5e16d54bbb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1372,14 +1372,15 @@ static void create_smmu(const VirtMachineState *vms,
 
 static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
 {
-    const char compat[] = "virtio,pci-iommu";
+    const char compat[] = "virtio,pci-iommu\0pci1af4,1057";
     uint16_t bdf = vms->virtio_iommu_bdf;
     MachineState *ms = MACHINE(vms);
     char *node;
 
     vms->iommu_phandle = qemu_fdt_alloc_phandle(ms->fdt);
 
-    node = g_strdup_printf("%s/virtio_iommu@%d", vms->pciehb_nodename, bdf);
+    node = g_strdup_printf("%s/virtio_iommu@%x,%x", vms->pciehb_nodename,
+                           PCI_SLOT(bdf), PCI_FUNC(bdf));
     qemu_fdt_add_subnode(ms->fdt, node);
     qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
     qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg",
-- 
2.37.3

[PATCH v2 8/8] hw/arm/virt: Fix devicetree warnings about node names

[Jean-Philippe Brucker]

The devicetree specification requires that nodes use a generic name
where appropriate. Fix the corresponding dt-validate warnings:

  pl061@9030000: $nodename:0: 'pl061@9030000' does not match '^gpio@[0-9a-f]+$'
  From schema: linux/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
  pl031@9010000: $nodename:0: 'pl031@9010000' does not match '^rtc(@.*|-[0-9a-f])*$'
  From schema: linux/Documentation/devicetree/bindings/rtc/arm,pl031.yaml
  pl011@9000000: $nodename:0: 'pl011@9000000' does not match '^serial(@.*)?$'
  From schema: linux/Documentation/devicetree/bindings/serial/pl011.yaml
  intc@8000000: $nodename:0: 'intc@8000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
  From schema: linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
  intc@8000000: 'its@8080000' does not match any of the regexes: '^(msi-controller|gic-its|interrupt-controller)@[0-9a-f]+$', '^gic-its@', '^interrupt-controller@[0-9a-f]+$', 'pinctrl-[0-9]+'
  From schema: linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
  smmuv3@9050000: $nodename:0: 'smmuv3@9050000' does not match '^iommu@[0-9a-f]*'
  From schema: linux/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 5e16d54bbb..c1e384e06f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -481,7 +481,7 @@ static void fdt_add_its_gic_node(VirtMachineState *vms)
     MachineState *ms = MACHINE(vms);
 
     vms->msi_phandle = qemu_fdt_alloc_phandle(ms->fdt);
-    nodename = g_strdup_printf("/intc/its@%" PRIx64,
+    nodename = g_strdup_printf("/interrupt-controller/msi-controller@%" PRIx64,
                                vms->memmap[VIRT_GIC_ITS].base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop_string(ms->fdt, nodename, "compatible",
@@ -500,7 +500,7 @@ static void fdt_add_v2m_gic_node(VirtMachineState *vms)
     MachineState *ms = MACHINE(vms);
     char *nodename;
 
-    nodename = g_strdup_printf("/intc/v2m@%" PRIx64,
+    nodename = g_strdup_printf("/interrupt-controller/v2m@%" PRIx64,
                                vms->memmap[VIRT_GIC_V2M].base);
     vms->msi_phandle = qemu_fdt_alloc_phandle(ms->fdt);
     qemu_fdt_add_subnode(ms->fdt, nodename);
@@ -522,7 +522,7 @@ static void fdt_add_gic_node(VirtMachineState *vms)
     vms->gic_phandle = qemu_fdt_alloc_phandle(ms->fdt);
     qemu_fdt_setprop_cell(ms->fdt, "/", "interrupt-parent", vms->gic_phandle);
 
-    nodename = g_strdup_printf("/intc@%" PRIx64,
+    nodename = g_strdup_printf("/interrupt-controller@%" PRIx64,
                                vms->memmap[VIRT_GIC_DIST].base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop_cell(ms->fdt, nodename, "#interrupt-cells", 3);
@@ -857,7 +857,7 @@ static void create_uart(const VirtMachineState *vms, int uart,
                                 sysbus_mmio_get_region(s, 0));
     sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
 
-    nodename = g_strdup_printf("/pl011@%" PRIx64, base);
+    nodename = g_strdup_printf("/serial@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     /* Note that we can't use setprop_string because of the embedded NUL */
     qemu_fdt_setprop(ms->fdt, nodename, "compatible",
@@ -897,7 +897,7 @@ static void create_rtc(const VirtMachineState *vms)
 
     sysbus_create_simple("pl031", base, qdev_get_gpio_in(vms->gic, irq));
 
-    nodename = g_strdup_printf("/pl031@%" PRIx64, base);
+    nodename = g_strdup_printf("/rtc@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop(ms->fdt, nodename, "compatible", compat, sizeof(compat));
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
@@ -999,7 +999,7 @@ static void create_gpio_devices(const VirtMachineState *vms, int gpio,
     sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
 
     uint32_t phandle = qemu_fdt_alloc_phandle(ms->fdt);
-    nodename = g_strdup_printf("/pl061@%" PRIx64, base);
+    nodename = g_strdup_printf("/gpio@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, nodename);
     qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, base, 2, size);
@@ -1348,7 +1348,7 @@ static void create_smmu(const VirtMachineState *vms,
                            qdev_get_gpio_in(vms->gic, irq + i));
     }
 
-    node = g_strdup_printf("/smmuv3@%" PRIx64, base);
+    node = g_strdup_printf("/iommu@%" PRIx64, base);
     qemu_fdt_add_subnode(ms->fdt, node);
     qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
     qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
@@ -1653,7 +1653,7 @@ void virt_machine_done(Notifier *notifier, void *data)
      * while qemu takes charge of the qom stuff.
      */
     if (info->dtb_filename == NULL) {
-        platform_bus_add_all_fdt_nodes(ms->fdt, "/intc",
+        platform_bus_add_all_fdt_nodes(ms->fdt, "/interrupt-controller",
                                        vms->memmap[VIRT_PLATFORM_BUS].base,
                                        vms->memmap[VIRT_PLATFORM_BUS].size,
                                        vms->irqmap[VIRT_PLATFORM_BUS]);
-- 
2.37.3

Re: [PATCH v2 5/8] hw/arm/virt: Fix devicetree warnings about the GPIO node

[Peter Maydell]

On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> Since the pl061 device can be used as interrupt controller, its node
> should contain "interrupt-controller" and "#interrupt-cells" properties.

It *can* be, but this PL061 is *not* an interrupt controller.
I don't see any reason why we should claim so in the DT.

thanks
-- PMM

Re: [PATCH v2 8/8] hw/arm/virt: Fix devicetree warnings about node names

[Peter Maydell]

On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> The devicetree specification requires that nodes use a generic name
> where appropriate. Fix the corresponding dt-validate warnings:

Either:
(1) guests are looking for devices in the DT by node name. In that
case we can't change the node names without breaking them
Or:
(2) guest look for nodes by compatibility, in which case why
do we care what the exact format of the node name is?

thanks
-- PMM

Re: [PATCH v2 1/8] hw/arm/virt: Fix devicetree warning about the root node

[Peter Maydell]

On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> The devicetree specification requires a 'model' property in the root
> node. Fix the corresponding dt-validate warning:
>
>   /: 'model' is a required property
>   From schema: dtschema/schemas/root-node.yaml
>
> Use the same name for model as for compatible. The specification
> recommends that 'compatible' follows the format 'manufacturer,model' and
> 'model' follows the format 'manufacturer,model-number'. Since our
> 'compatible' doesn't observe this, 'model' doesn't really need to
> either.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---

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

thanks
-- PMM

Re: [PATCH v2 2/8] hw/arm/virt: Fix devicetree warning about the GIC node

[Peter Maydell]

On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> The GICv3 bindings requires a #msi-cells property for the ITS node. Fix
> the corresponding dt-validate warning:
>
>   interrupt-controller@8000000: msi-controller@8080000: '#msi-cells' is a required property
>   From schema: linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---

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

thanks
-- PMM

Re: [PATCH v2 3/8] hw/arm/virt: Use "msi-map" devicetree property for PCI

[Peter Maydell]

On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> The "msi-parent" property can be used on the PCI node when MSIs do not
> contain sideband data (device IDs) [1]. In QEMU, MSI transactions
> contain the requester ID, so the PCI node should use the "msi-map"
> property instead of "msi-parent". In our case the property describes an
> identity map between requester ID and sideband data.
>
> This fixes a warning when passing the DTB generated by QEMU to dtc,
> following a recent change to the GICv3 node:
>
>   Warning (msi_parent_property): /pcie@10000000:msi-parent: property size (4) too small for cell size 1
>
> [1] linux/Documentation/devicetree/bindings/pci/pci-msi.txt
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

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

thanks
-- PMM

Re: [PATCH v2 6/8] hw/arm/virt: Fix devicetree warning about the SMMU node

[Peter Maydell]

On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> The SMMUv3 node isn't expected to have clock properties. Fix the
> corresponding dt-validate warning:
>
>   smmuv3@9050000: 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
>   From schema: linux/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/arm/virt.c | 2 --
>  1 file changed, 2 deletions(-)
>

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

thanks
-- PMM

Re: [PATCH v2 7/8] hw/arm/virt: Fix devicetree warnings about the virtio-iommu node

[Peter Maydell]

On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> The "PCI Bus Binding to: IEEE Std 1275-1994" defines the compatible
> string for a PCIe bus or endpoint as "pci<vendorid>,<deviceid>" or
> similar. Since the initial binding for PCI virtio-iommu didn't follow
> this rule, it was modified to accept both strings and ensure backward
> compatibility. Also, the unit-name for the node should be
> "device,function".
>
> Fix corresponding dt-validate and dtc warnings:
>
>   pcie@10000000: virtio_iommu@16:compatible: ['virtio,pci-iommu'] does not contain items matching the given schema
>   pcie@10000000: Unevaluated properties are not allowed (... 'virtio_iommu@16' were unexpected)
>   From schema: linux/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
>   virtio_iommu@16: compatible: 'oneOf' conditional failed, one must be fixed:
>         ['virtio,pci-iommu'] is too short
>         'pci1af4,1057' was expected
>   From schema: dtschema/schemas/pci/pci-bus.yaml
>
>   Warning (pci_device_reg): /pcie@10000000/virtio_iommu@16: PCI unit address format error, expected "2,0"
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/arm/virt.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 2de16f6324..5e16d54bbb 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1372,14 +1372,15 @@ static void create_smmu(const VirtMachineState *vms,
>
>  static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
>  {
> -    const char compat[] = "virtio,pci-iommu";
> +    const char compat[] = "virtio,pci-iommu\0pci1af4,1057";
>      uint16_t bdf = vms->virtio_iommu_bdf;

PCI_DEVICE_ID_VIRTIO_IOMMU is listed in include/hw/pci/pci.h
as 0x1014, so where does 1057 come from? (This is a hex value,
right?)

docs/specs/pci-ids.txt doesn't list either 1014 or 1057, so
I guess we forgot to update that...

thanks
-- PMM

Re: [PATCH v2 4/8] hw/arm/virt: Fix devicetree warning about the gpio-key node

[Peter Maydell]

On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
> The node name of the gpio-key devicetree node should be "key-poweroff":
>
>   gpio-keys: 'poweroff' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+'
>   From schema: linux/Documentation/devicetree/bindings/input/gpio-keys.yaml
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

This restriction only went into the DT documentation in July
(kernel commit 5eb5652250).

Please don't retrospectively make perfectly valid working DTs
non-valid. I don't see any reason to change QEMU here.

More generally, the set of things you might want the
validator to warn about for a fresh new human-written DTB
doesn't necessarily correspond to the set of things you want
to enforce for a pre-existing code-generated DTB. For the
former it makes much more sense to impose "coding style"
and "naming convention" type rules.

thanks
-- PMM

Re: [PATCH v2 4/8] hw/arm/virt: Fix devicetree warning about the gpio-key node

[Eric Auger]

On 9/27/22 12:03, Jean-Philippe Brucker wrote:
> The node name of the gpio-key devicetree node should be "key-poweroff":
> 
>   gpio-keys: 'poweroff' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+'
>   From schema: linux/Documentation/devicetree/bindings/input/gpio-keys.yaml
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> ---
>  hw/arm/virt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 8605f5058a..6805c57530 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -932,12 +932,12 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
>      qemu_fdt_add_subnode(fdt, "/gpio-keys");
>      qemu_fdt_setprop_string(fdt, "/gpio-keys", "compatible", "gpio-keys");
>  
> -    qemu_fdt_add_subnode(fdt, "/gpio-keys/poweroff");
> -    qemu_fdt_setprop_string(fdt, "/gpio-keys/poweroff",
> +    qemu_fdt_add_subnode(fdt, "/gpio-keys/key-poweroff");
> +    qemu_fdt_setprop_string(fdt, "/gpio-keys/key-poweroff",
>                              "label", "GPIO Key Poweroff");
> -    qemu_fdt_setprop_cell(fdt, "/gpio-keys/poweroff", "linux,code",
> +    qemu_fdt_setprop_cell(fdt, "/gpio-keys/key-poweroff", "linux,code",
>                            KEY_POWER);
> -    qemu_fdt_setprop_cells(fdt, "/gpio-keys/poweroff",
> +    qemu_fdt_setprop_cells(fdt, "/gpio-keys/key-poweroff",
>                             "gpios", phandle, 3, 0);
>  }
>  

Re: [PATCH v2 3/8] hw/arm/virt: Use "msi-map" devicetree property for PCI

[Eric Auger]

Hi Jean,

On 9/27/22 12:03, Jean-Philippe Brucker wrote:
> The "msi-parent" property can be used on the PCI node when MSIs do not
> contain sideband data (device IDs) [1]. In QEMU, MSI transactions
> contain the requester ID, so the PCI node should use the "msi-map"
> property instead of "msi-parent". In our case the property describes an
> identity map between requester ID and sideband data.
> 
> This fixes a warning when passing the DTB generated by QEMU to dtc,
> following a recent change to the GICv3 node:
> 
>   Warning (msi_parent_property): /pcie@10000000:msi-parent: property size (4) too small for cell size 1
> 
> [1] linux/Documentation/devicetree/bindings/pci/pci-msi.txt
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Effectively matches example 1 in [1]

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

Eric

> ---
>  hw/arm/virt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ed6f1ac41e..8605f5058a 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1489,8 +1489,8 @@ static void create_pcie(VirtMachineState *vms)
>      qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
>  
>      if (vms->msi_phandle) {
> -        qemu_fdt_setprop_cells(ms->fdt, nodename, "msi-parent",
> -                               vms->msi_phandle);
> +        qemu_fdt_setprop_cells(ms->fdt, nodename, "msi-map",
> +                               0, vms->msi_phandle, 0, 0x10000);
>      }
>  
>      qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",

Re: [PATCH v2 1/8] hw/arm/virt: Fix devicetree warning about the root node

[Eric Auger]

Hi jean,

On 9/27/22 12:03, Jean-Philippe Brucker wrote:
> The devicetree specification requires a 'model' property in the root
> node. Fix the corresponding dt-validate warning:
> 
>   /: 'model' is a required property
>   From schema: dtschema/schemas/root-node.yaml
> 
> Use the same name for model as for compatible. The specification
> recommends that 'compatible' follows the format 'manufacturer,model' and
> 'model' follows the format 'manufacturer,model-number'. Since our> 'compatible' doesn't observe this, 'model' doesn't really need to
> either.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> ---
>  hw/arm/virt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 1a6480fd2a..83ab1a37fb 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -252,6 +252,7 @@ static void create_fdt(VirtMachineState *vms)
>      qemu_fdt_setprop_string(fdt, "/", "compatible", "linux,dummy-virt");
>      qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2);
>      qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2);
> +    qemu_fdt_setprop_string(fdt, "/", "model", "linux,dummy-virt");
>  
>      /* /chosen must exist for load_dtb to fill in necessary properties later */
>      qemu_fdt_add_subnode(fdt, "/chosen");

Re: [PATCH v2 2/8] hw/arm/virt: Fix devicetree warning about the GIC node

[Eric Auger]

Hi Jean,

On 9/27/22 12:03, Jean-Philippe Brucker wrote:
> The GICv3 bindings requires a #msi-cells property for the ITS node. Fix
> the corresponding dt-validate warning:
> 
>   interrupt-controller@8000000: msi-controller@8080000: '#msi-cells' is a required property
>   From schema: linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml

nit: you may add
linux/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
says

      "#msi-cells":
        description:
          The single msi-cell is the DeviceID of the device which will
generate
          the MSI.
        const: 1



> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Besides,
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> ---
>  hw/arm/virt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 83ab1a37fb..ed6f1ac41e 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -487,6 +487,7 @@ static void fdt_add_its_gic_node(VirtMachineState *vms)
>      qemu_fdt_setprop_string(ms->fdt, nodename, "compatible",
>                              "arm,gic-v3-its");
>      qemu_fdt_setprop(ms->fdt, nodename, "msi-controller", NULL, 0);
> +    qemu_fdt_setprop_cell(ms->fdt, nodename, "#msi-cells", 1);
>      qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
>                                   2, vms->memmap[VIRT_GIC_ITS].base,
>                                   2, vms->memmap[VIRT_GIC_ITS].size);

Re: [PATCH v2 6/8] hw/arm/virt: Fix devicetree warning about the SMMU node

[Eric Auger]

Hi Jean,

you may add: as opposed to SMMUv2, ...
> The SMMUv3 node isn't expected to have clock properties. Fix the
> corresponding dt-validate warning:
> 
>   smmuv3@9050000: 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
>   From schema: linux/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks!

Eric

> ---
>  hw/arm/virt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 10ce66c722..2de16f6324 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1362,8 +1362,6 @@ static void create_smmu(const VirtMachineState *vms,
>      qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
>                       sizeof(irq_names));
>  
> -    qemu_fdt_setprop_cell(ms->fdt, node, "clocks", vms->clock_phandle);
> -    qemu_fdt_setprop_string(ms->fdt, node, "clock-names", "apb_pclk");
>      qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
>  
>      qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);

Re: [PATCH v2 7/8] hw/arm/virt: Fix devicetree warnings about the virtio-iommu node

[Eric Auger]

Hi,

On 9/27/22 13:46, Peter Maydell wrote:
> On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
>>
>> The "PCI Bus Binding to: IEEE Std 1275-1994" defines the compatible
>> string for a PCIe bus or endpoint as "pci<vendorid>,<deviceid>" or
>> similar. Since the initial binding for PCI virtio-iommu didn't follow
>> this rule, it was modified to accept both strings and ensure backward
>> compatibility. Also, the unit-name for the node should be
>> "device,function".
>>
>> Fix corresponding dt-validate and dtc warnings:
>>
>>   pcie@10000000: virtio_iommu@16:compatible: ['virtio,pci-iommu'] does not contain items matching the given schema
>>   pcie@10000000: Unevaluated properties are not allowed (... 'virtio_iommu@16' were unexpected)
>>   From schema: linux/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
>>   virtio_iommu@16: compatible: 'oneOf' conditional failed, one must be fixed:
>>         ['virtio,pci-iommu'] is too short
>>         'pci1af4,1057' was expected
>>   From schema: dtschema/schemas/pci/pci-bus.yaml
>>
>>   Warning (pci_device_reg): /pcie@10000000/virtio_iommu@16: PCI unit address format error, expected "2,0"
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> ---
>>  hw/arm/virt.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 2de16f6324..5e16d54bbb 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1372,14 +1372,15 @@ static void create_smmu(const VirtMachineState *vms,
>>
>>  static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
>>  {
>> -    const char compat[] = "virtio,pci-iommu";
>> +    const char compat[] = "virtio,pci-iommu\0pci1af4,1057";
>>      uint16_t bdf = vms->virtio_iommu_bdf;
> 
> PCI_DEVICE_ID_VIRTIO_IOMMU is listed in include/hw/pci/pci.h
> as 0x1014, so where does 1057 come from? (This is a hex value,
> right?)
the virtio spec states:
The PCI Device ID is calculated by adding 0x1040 to the Virtio Device ID
(this IOMMU device ID is 0d23 = 0x17 for the virtio-iommu device, also
found in include/uapi/linux/virtio_ids.h) so 0x1057 above looks correct

note that in docs/specs/pci-ids.txt there are a bunch of other device
ids not documented (virtio-mem, pmem)

What I don't get anymore is the device id in qemu include/hw/pci/pci.h

Thanks

Eric
> 
> docs/specs/pci-ids.txt doesn't list either 1014 or 1057, so
> I guess we forgot to update that...
> 
> thanks
> -- PMM
> 

Re: [PATCH v2 0/8] hw/arm/virt: Fix dt-schema warnings

[Peter Maydell]

On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> Fix some warnings thrown by dt-validate for the aarch64 virt devicetree.
>
> Since v1 [1]:
> * Submitted more DT bindings changes where appropriate [2]. All of them
>   applied for Linux v6.1 so I dropped the related QEMU changes.
> * Grouped all node name changes into patch 8
> * Improved commit messages
>
> I'm testing by running various virt machine configurations with -M
> dumpdtb=qemu.dtb, then running dt-validate [3] and dtc:
>
>   dt-validate -s linux/Documentation/devicetree/bindings/ qemu.dtb
>   dtc -O dts qemu.dtb -o qemu.dts
>
> [1] https://lore.kernel.org/all/20220824155113.286730-1-jean-philippe@linaro.org/
> [2] SMMU interrupt order https://lore.kernel.org/all/20220916133145.1910549-1-jean-philippe@linaro.org/
>     arch-timer compatible https://lore.kernel.org/all/20220922161149.371565-1-jean-philippe@linaro.org/
>     virtio-iommu https://lore.kernel.org/all/20220923074435.420531-1-jean-philippe@linaro.org/
> [3] https://github.com/devicetree-org/dt-schema
>
> Jean-Philippe Brucker (8):
>   hw/arm/virt: Fix devicetree warning about the root node
>   hw/arm/virt: Fix devicetree warning about the GIC node
>   hw/arm/virt: Use "msi-map" devicetree property for PCI
>   hw/arm/virt: Fix devicetree warning about the gpio-key node
>   hw/arm/virt: Fix devicetree warnings about the GPIO node
>   hw/arm/virt: Fix devicetree warning about the SMMU node
>   hw/arm/virt: Fix devicetree warnings about the virtio-iommu node
>   hw/arm/virt: Fix devicetree warnings about node names

I have applied patches 1, 2, 3, and 6 to target-arm.next.

thanks
-- PMM

Re: [PATCH v2 8/8] hw/arm/virt: Fix devicetree warnings about node names

[Rob Herring]

On Tue, Sep 27, 2022 at 6:28 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > The devicetree specification requires that nodes use a generic name
> > where appropriate. Fix the corresponding dt-validate warnings:
>
> Either:
> (1) guests are looking for devices in the DT by node name. In that
> case we can't change the node names without breaking them

Using node names is generally wrong unless the node name to use is
defined and that's the only way to identify them (e.g. /chosen).

> Or:
> (2) guest look for nodes by compatibility, in which case why
> do we care what the exact format of the node name is?

The spec[1] has defined standard class node names going back to 2008.
That covered all the names here except for 'iommu' and those names
date back to the 1990s. Obviously, there has been no checking of them
or many things for a long time, but now we can check much more than
reviews ever could we have a huge technical debt. The main reason on
care on these is just consistency.

Rob

[1] https://elinux.org/images/c/cf/Power_ePAPR_APPROVED_v1.1.pdf

Re: [PATCH v2 4/8] hw/arm/virt: Fix devicetree warning about the gpio-key node

[Rob Herring]

On Tue, Sep 27, 2022 at 6:56 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> > The node name of the gpio-key devicetree node should be "key-poweroff":
> >
> >   gpio-keys: 'poweroff' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+'
> >   From schema: linux/Documentation/devicetree/bindings/input/gpio-keys.yaml
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>
> This restriction only went into the DT documentation in July
> (kernel commit 5eb5652250).

Fair enough.

> Please don't retrospectively make perfectly valid working DTs
> non-valid. I don't see any reason to change QEMU here.
>
> More generally, the set of things you might want the
> validator to warn about for a fresh new human-written DTB
> doesn't necessarily correspond to the set of things you want
> to enforce for a pre-existing code-generated DTB. For the
> former it makes much more sense to impose "coding style"
> and "naming convention" type rules.

I too would like to distinguish that, but haven't come up with a way
to do that in json-schema yet. The way schemas are applied
independently makes that a challenge. So far it's been low on the
priority list as any platforms with few enough warnings to get to 0
haven't been a problem to fix (in a few cases we do end up relaxing
the schemas).

On the flip side, even existing things eventually get updated for
coding style or evolving conventions. As long as we don't break ABIs,
the same should apply to DT.

Rob

Re: [PATCH v2 4/8] hw/arm/virt: Fix devicetree warning about the gpio-key node

[Peter Maydell]

On Thu, 13 Oct 2022 at 22:47, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Sep 27, 2022 at 6:56 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > Please don't retrospectively make perfectly valid working DTs
> > non-valid. I don't see any reason to change QEMU here.
> >
> > More generally, the set of things you might want the
> > validator to warn about for a fresh new human-written DTB
> > doesn't necessarily correspond to the set of things you want
> > to enforce for a pre-existing code-generated DTB. For the
> > former it makes much more sense to impose "coding style"
> > and "naming convention" type rules.
>
> I too would like to distinguish that, but haven't come up with a way
> to do that in json-schema yet. The way schemas are applied
> independently makes that a challenge. So far it's been low on the
> priority list as any platforms with few enough warnings to get to 0
> haven't been a problem to fix (in a few cases we do end up relaxing
> the schemas).
>
> On the flip side, even existing things eventually get updated for
> coding style or evolving conventions. As long as we don't break ABIs,
> the same should apply to DT.

Yeah, but from QEMU's point of view pretty much the whole DT
*is* ABI, because we have no idea what the guest will be
doing with it, whether it is looking for things in the
"correct" way or if it happens to have hard-coded a node
name or similar: and the guest could be any of a wide
variety of operating systems or custom code, and including
pretty old versions of OSes as well as the latest-and-greatest.
This is different from Linux's handwritten device trees,
which only need to work with Linux and only with the associated
Linux version, at that. So the set of things I'm happy changing
tends to be more restricted than the set of things it's worth
cleaning up in the dt sources that ship with the kernel.

thanks
-- PMM

Re: [PATCH v2 7/8] hw/arm/virt: Fix devicetree warnings about the virtio-iommu node

[Jean-Philippe Brucker]

On Tue, Sep 27, 2022 at 04:35:25PM +0200, Eric Auger wrote:
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index 2de16f6324..5e16d54bbb 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -1372,14 +1372,15 @@ static void create_smmu(const VirtMachineState *vms,
> >>
> >>  static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
> >>  {
> >> -    const char compat[] = "virtio,pci-iommu";
> >> +    const char compat[] = "virtio,pci-iommu\0pci1af4,1057";
> >>      uint16_t bdf = vms->virtio_iommu_bdf;
> > 
> > PCI_DEVICE_ID_VIRTIO_IOMMU is listed in include/hw/pci/pci.h
> > as 0x1014, so where does 1057 come from? (This is a hex value,
> > right?)
> the virtio spec states:
> The PCI Device ID is calculated by adding 0x1040 to the Virtio Device ID
> (this IOMMU device ID is 0d23 = 0x17 for the virtio-iommu device, also
> found in include/uapi/linux/virtio_ids.h) so 0x1057 above looks correct
> 
> note that in docs/specs/pci-ids.txt there are a bunch of other device
> ids not documented (virtio-mem, pmem)
> 
> What I don't get anymore is the device id in qemu include/hw/pci/pci.h

Yes 0x1057 is the right device ID, and it matches what the
virtio-iommu-pci device gets in hw/virtio/virtio-pci.c:1691.
The wrong 0x1014 value set by hw/virtio/virtio-iommu-pci.c:78 gets
overwritten since virtio-iommu is modern only. I can send a patch to
remove it.

Peter, do you mind taking this patch as well, or should I resend it?
I can't decide what to do about the other issues in this series, we may
have to live with some warnings, but this one should be OK.

Thanks,
Jean

[1] https://lore.kernel.org/qemu-devel/aec4e9d1-b70e-2e8d-6503-b3e550c6d5ea@redhat.com/

Re: [PATCH v2 7/8] hw/arm/virt: Fix devicetree warnings about the virtio-iommu node

[Peter Maydell]

On Fri, 21 Oct 2022 at 15:33, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> On Tue, Sep 27, 2022 at 04:35:25PM +0200, Eric Auger wrote:
> > >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > >> index 2de16f6324..5e16d54bbb 100644
> > >> --- a/hw/arm/virt.c
> > >> +++ b/hw/arm/virt.c
> > >> @@ -1372,14 +1372,15 @@ static void create_smmu(const VirtMachineState *vms,
> > >>
> > >>  static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
> > >>  {
> > >> -    const char compat[] = "virtio,pci-iommu";
> > >> +    const char compat[] = "virtio,pci-iommu\0pci1af4,1057";
> > >>      uint16_t bdf = vms->virtio_iommu_bdf;
> > >
> > > PCI_DEVICE_ID_VIRTIO_IOMMU is listed in include/hw/pci/pci.h
> > > as 0x1014, so where does 1057 come from? (This is a hex value,
> > > right?)
> > the virtio spec states:
> > The PCI Device ID is calculated by adding 0x1040 to the Virtio Device ID
> > (this IOMMU device ID is 0d23 = 0x17 for the virtio-iommu device, also
> > found in include/uapi/linux/virtio_ids.h) so 0x1057 above looks correct
> >
> > note that in docs/specs/pci-ids.txt there are a bunch of other device
> > ids not documented (virtio-mem, pmem)
> >
> > What I don't get anymore is the device id in qemu include/hw/pci/pci.h
>
> Yes 0x1057 is the right device ID, and it matches what the
> virtio-iommu-pci device gets in hw/virtio/virtio-pci.c:1691.
> The wrong 0x1014 value set by hw/virtio/virtio-iommu-pci.c:78 gets
> overwritten since virtio-iommu is modern only. I can send a patch to
> remove it.
>
> Peter, do you mind taking this patch as well, or should I resend it?

Sure, I've applied this one to target-arm.next.

-- PMM

Re: [PATCH v2 5/8] hw/arm/virt: Fix devicetree warnings about the GPIO node

[Rob Herring]

On Tue, Sep 27, 2022 at 6:25 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > Since the pl061 device can be used as interrupt controller, its node
> > should contain "interrupt-controller" and "#interrupt-cells" properties.
>
> It *can* be, but this PL061 is *not* an interrupt controller.
> I don't see any reason why we should claim so in the DT.

Taking another look, it is an interrupt controller. The GPIOs are
connected to the 'gpio-keys' node which is interrupt based (there's a
polled version too). That binding happens to be pretty lax and allows
the GPIO to be specified either with 'gpios' or 'interrupts' property.
The Linux PL061 driver happens to work only because it always
registers an interrupt controller regardless of having
"interrupt-controller" and "#interrupt-cells" properties or not.

Rob

Re: [PATCH v2 5/8] hw/arm/virt: Fix devicetree warnings about the GPIO node

[Peter Maydell]

On Tue, 29 Nov 2022 at 20:56, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Sep 27, 2022 at 6:25 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
> > <jean-philippe@linaro.org> wrote:
> > >
> > > Since the pl061 device can be used as interrupt controller, its node
> > > should contain "interrupt-controller" and "#interrupt-cells" properties.
> >
> > It *can* be, but this PL061 is *not* an interrupt controller.
> > I don't see any reason why we should claim so in the DT.
>
> Taking another look, it is an interrupt controller. The GPIOs are
> connected to the 'gpio-keys' node which is interrupt based (there's a
> polled version too). That binding happens to be pretty lax and allows
> the GPIO to be specified either with 'gpios' or 'interrupts' property.
> The Linux PL061 driver happens to work only because it always
> registers an interrupt controller regardless of having
> "interrupt-controller" and "#interrupt-cells" properties or not.

No, it really isn't an interrupt controller. The interrupt controller
in this system is the GIC. The PL061 is a GPIO controller. It *has*
an interrupt line, which it connects to the GIC, but that doesn't make
it an interrupt controller any more than it makes a UART an interrupt
controller. It just means you can use the GPIOs in either a polled
or an interrupt-driven mode, same as you can use a PL011 UART in
either polled or interrupt-driven mode. And the guest knows it
can do that because we've told it "this is a PL061" and that's part
of the PL061's functionality.

The gpio-keys stuff is just "here is a wire which is tracking
the state of an emulated power button". This isn't an interrupt,
it's just a wire that has some status the guest probably
cares about.

The second PL061 which this bit of dtb-generation code also
applies to happens to currently be being used purely for
output GPIOs, so calling that one an interrupt controller
makes even less sense.

thanks
-- PMM