diff mbox series

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

Message ID 20220927100347.176606-8-jean-philippe@linaro.org
State Superseded
Headers show
Series hw/arm/virt: Fix dt-schema warnings | expand

Commit Message

Jean-Philippe Brucker Sept. 27, 2022, 10:03 a.m. UTC
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(-)

Comments

Peter Maydell Sept. 27, 2022, 11:46 a.m. UTC | #1
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
Eric Auger Sept. 27, 2022, 2:35 p.m. UTC | #2
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
>
Jean-Philippe Brucker Oct. 21, 2022, 2:33 p.m. UTC | #3
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/
Peter Maydell Oct. 24, 2022, 10:44 a.m. UTC | #4
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
diff mbox series

Patch

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",