[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] intel_iommu: make sure its init before PCI dev

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH] intel_iommu: make sure its init before PCI dev
Date: Thu, 23 Feb 2017 10:10:57 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1

On 02/23/2017 05:06 AM, Peter Xu wrote:
On Wed, Feb 22, 2017 at 10:30:47AM -0700, Alex Williamson wrote:
On Wed, 22 Feb 2017 13:49:25 +0800
Peter Xu <address@hidden> wrote:

Intel vIOMMU devices are created with "-device" parameter, while here
actually we need to make sure this device will be created before some
other PCI devices (like vfio-pci devices) so that we know iommu_fn will
be setup correctly before realizations of those PCI devices.

Here we do explicit check to make sure intel-iommu device will be inited
before all the rest of the PCI devices. This is done by checking against
the devices dangled under current root PCIe bus and we should see
nothing there besides integrated ICH9 ones.


Commit b86eacb8 (hw/pci: delay bus_master_enable_region initialization)
creates the IOMMU memory region at machine_done time so the
devices creation order wouldn't matter.

I don't think we use the iommu_fn before machine_done.
What have I missed?


If the user violated this rule, we abort the program.

Maybe one day we will be able to manage the ordering of device
initialization, and then we can grant VT-d devices a higher init
priority. But before that, let's have this explicit check to make sure
of it.

Signed-off-by: Peter Xu <address@hidden>
 hw/i386/intel_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 22d8226..db74124 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -31,6 +31,7 @@
 #include "hw/i386/apic-msidef.h"
 #include "hw/boards.h"
 #include "hw/i386/x86-iommu.h"
+#include "hw/i386/ich9.h"
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
 #include "hw/i386/apic_internal.h"
@@ -2560,6 +2561,41 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error 
     return true;

+static bool vtd_has_inited_pci_devices(PCIBus *bus, Error **errp)
+    int i;
+    uint8_t func;
+    /* We check against root bus */
+    assert(bus && pci_bus_is_root(bus));
+    /*
+     * We need to make sure vIOMMU device is created before other PCI
+     * devices other than the integrated ICH9 ones, so that they can
+     * get correct iommu_fn setup even during its realize(). Some
+     * devices (e.g., vfio-pci) will need a correct iommu_fn to work.
+     */
+    for (i = 1; i < PCI_FUNC_MAX * PCI_SLOT_MAX; i++) {
+        /* Skip the checking against ICH9 integrated devices */
+        if (PCI_SLOT(i) == ICH9_LPC_DEV) {
+            func = PCI_FUNC(i);
+            if (func == ICH9_LPC_FUNC ||
+                func == ICH9_SATA1_FUNC ||
+                func == ICH9_SMB_FUNC) {
+                continue;
+            }
+        }

Whitelisting specific devfns seems pretty sketchy and fragile.  Can we
even assume we're on a Q35 chipset?  I don't see that vtd_realize()
takes any particular precautions not to allow initialization on 440fx,
or whatever generic chipset we come up with next that may not have
these specific devices.

Yes. IIUC VT-d now can only work with Q35, right? Cc Marcel and Paolo
in case I misunderstood it.

If so, maybe here we should check against q35 in vtd realization. How
about something like:

    if (!object_property_find((Object *)pcms, "q35", NULL)) {
        error_setg(errp, "Currently Intel vIOMMU only support Q35 platform. "
                   "Please specify \"-M q35\" to enable it.");


It would probably be a better idea to use
object_dynamic_cast() if you want to whitelist specific devices.
Perhaps this could even be used to validate the chipset as well.

Now Jintack reported another issue, that we may have two default
devices there if not specifying "-nodefaults", and that two devices
will always be the first ones to be inited.

How about here we just explicitly check against vfio-pci devices, so
we just make sure vfio-pci devices will be put after intel-iommu?
Since actually vfio-pci devices are the only ones that we know we need
to be inited explicitly after the VT-d device.




+        if (bus->devices[i]) {
+            error_setg(errp, "Please init intel-iommu before "
+                       "other PCI devices");
+            return true;
+        }
+    }
+    return false;
 static void vtd_realize(DeviceState *dev, Error **errp)
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
@@ -2567,6 +2603,10 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
     X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);

+    if (vtd_has_inited_pci_devices(bus, errp)) {
+        return;
+    }
     x86_iommu->type = TYPE_INTEL;

-- peterx

reply via email to

[Prev in Thread] Current Thread [Next in Thread]