Re: [PATCH] hw/virtio/virtio-iommu-pci: Enforce the device is plugged on the root bus
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 > --- > 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. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] hw/virtio/virtio-iommu-pci: Enforce the device is plugged on the root bus
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
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(_dev->pci_dev);
> DeviceState *vdev = DEVICE(>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
Tested-by: Jean-Philippe Brucker
> }
> }
> +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(_dev->pci_dev)),
> - _abort);
> + OBJECT(pbus), _abort);
> +
> virtio_pci_force_virtio_1(vpci_dev);
> qdev_realize(vdev, BUS(_dev->bus), errp);
> }
> --
> 2.31.1
>
[PATCH] hw/virtio/virtio-iommu-pci: Enforce the device is plugged on the root bus
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
---
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(_dev->pci_dev);
DeviceState *vdev = DEVICE(>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(_dev->pci_dev)),
- _abort);
+ OBJECT(pbus), _abort);
+
virtio_pci_force_virtio_1(vpci_dev);
qdev_realize(vdev, BUS(_dev->bus), errp);
}
--
2.31.1
