| Message ID | 20220927100347.176606-8-jean-philippe@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/arm/virt: Fix dt-schema warnings | expand |
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
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 >
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/
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 --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",
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(-)