| Message ID | 20221012163448.121368-1-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/virtio/virtio-iommu-pci: Enforce the device is plugged on the root bus | expand |
On Wed, Oct 12, 2022 at 12:34:48PM -0400, Eric Auger wrote: > In theory the virtio-iommu-pci could be plugged anywhere in the PCIe > topology and as long as the dt/acpi info are properly built this should > work. However at the moment we fail to do that because the > virtio-iommu-pci BDF is not computed at plug time and in that case > vms->virtio_iommu_bdf gets an incorrect value. > > For instance if the virtio-iommu-pci is plugged onto a pcie root port > and the virtio-iommu protects a virtio-block-pci device the guest does > not boot. > > So let's do not pretend we do support this case and fail the initialize() > if we detect the virtio-iommu-pci is plugged anywhere else than on the > root bus. Anyway this ability is not needed. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> I agree with the reasoning. It's not supported, difficult to support and not needed, so let's make the error more obvious. If I remember correctly, the problem with supporting an IOMMU behind a bridge is that static topology descriptions like DT and ACPI identify devices using bus numbers, which QEMU would need to allocate. > --- > hw/virtio/virtio-iommu-pci.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c > index 79ea8334f0..7ef2f9dcdb 100644 > --- a/hw/virtio/virtio-iommu-pci.c > +++ b/hw/virtio/virtio-iommu-pci.c > @@ -17,6 +17,7 @@ > #include "hw/qdev-properties-system.h" > #include "qapi/error.h" > #include "hw/boards.h" > +#include "hw/pci/pci_bus.h" > #include "qom/object.h" > > typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; > @@ -44,6 +45,7 @@ static Property virtio_iommu_pci_properties[] = { > static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > { > VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); > + PCIBus *pbus = pci_get_bus(&vpci_dev->pci_dev); > DeviceState *vdev = DEVICE(&dev->vdev); > VirtIOIOMMU *s = VIRTIO_IOMMU(vdev); > > @@ -57,11 +59,17 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > s->reserved_regions[i].type != VIRTIO_IOMMU_RESV_MEM_T_MSI) { > error_setg(errp, "reserved region %d has an invalid type", i); > error_append_hint(errp, "Valid values are 0 and 1\n"); > + return; Should this be a separate fix? Anyway, for both changes Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org> Tested-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > } > } > + if (!pci_bus_is_root(pbus)) { > + error_setg(errp, "virtio-iommu-pci must be plugged on the root bus"); > + return; > + } > + > object_property_set_link(OBJECT(dev), "primary-bus", > - OBJECT(pci_get_bus(&vpci_dev->pci_dev)), > - &error_abort); > + OBJECT(pbus), &error_abort); > + > virtio_pci_force_virtio_1(vpci_dev); > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > } > -- > 2.31.1 >
On Wed, Oct 12, 2022 at 12:34:48PM -0400, Eric Auger wrote: > In theory the virtio-iommu-pci could be plugged anywhere in the PCIe > topology and as long as the dt/acpi info are properly built this should > work. However at the moment we fail to do that because the > virtio-iommu-pci BDF is not computed at plug time and in that case > vms->virtio_iommu_bdf gets an incorrect value. > > For instance if the virtio-iommu-pci is plugged onto a pcie root port > and the virtio-iommu protects a virtio-block-pci device the guest does > not boot. > > So let's do not pretend we do support this case and fail the initialize() > if we detect the virtio-iommu-pci is plugged anywhere else than on the > root bus. Anyway this ability is not needed. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > hw/virtio/virtio-iommu-pci.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) FYI libvirt will already reject any attempts to place the device anywhere but directly on pcie.0, so from our point of view merging this patch is perfectly fine.
diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c index 79ea8334f0..7ef2f9dcdb 100644 --- a/hw/virtio/virtio-iommu-pci.c +++ b/hw/virtio/virtio-iommu-pci.c @@ -17,6 +17,7 @@ #include "hw/qdev-properties-system.h" #include "qapi/error.h" #include "hw/boards.h" +#include "hw/pci/pci_bus.h" #include "qom/object.h" typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; @@ -44,6 +45,7 @@ static Property virtio_iommu_pci_properties[] = { static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) { VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); + PCIBus *pbus = pci_get_bus(&vpci_dev->pci_dev); DeviceState *vdev = DEVICE(&dev->vdev); VirtIOIOMMU *s = VIRTIO_IOMMU(vdev); @@ -57,11 +59,17 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) s->reserved_regions[i].type != VIRTIO_IOMMU_RESV_MEM_T_MSI) { error_setg(errp, "reserved region %d has an invalid type", i); error_append_hint(errp, "Valid values are 0 and 1\n"); + return; } } + if (!pci_bus_is_root(pbus)) { + error_setg(errp, "virtio-iommu-pci must be plugged on the root bus"); + return; + } + object_property_set_link(OBJECT(dev), "primary-bus", - OBJECT(pci_get_bus(&vpci_dev->pci_dev)), - &error_abort); + OBJECT(pbus), &error_abort); + virtio_pci_force_virtio_1(vpci_dev); qdev_realize(vdev, BUS(&vpci_dev->bus), errp); }
In theory the virtio-iommu-pci could be plugged anywhere in the PCIe topology and as long as the dt/acpi info are properly built this should work. However at the moment we fail to do that because the virtio-iommu-pci BDF is not computed at plug time and in that case vms->virtio_iommu_bdf gets an incorrect value. For instance if the virtio-iommu-pci is plugged onto a pcie root port and the virtio-iommu protects a virtio-block-pci device the guest does not boot. So let's do not pretend we do support this case and fail the initialize() if we detect the virtio-iommu-pci is plugged anywhere else than on the root bus. Anyway this ability is not needed. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- hw/virtio/virtio-iommu-pci.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)