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

[Jintack Lim]

On Wed, Feb 22, 2017 at 6:42 AM, Jintack Lim <address@hidden>
wrote:

>
>
> On Wed, Feb 22, 2017 at 12:49 AM, 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.
>>
>> If the user violated this rule, we abort the program.
>>
>
> Hi Peter,
>
> After applying this patch, qemu gave the following error and was
> terminated, but I believe I passed parameters in a right order?
>

FYI, I've applied this patch to your vtd-vfio-enablement-v7 branch.


>
> [kvm-node ~]$sudo qemu-system-x86_64 \
> > -device intel-iommu,intremap=on,eim=off,caching-mode=on \
> > -M q35,accel=kvm,kernel-irqchip=split \
> > -m 12G \
> > -drive file=/mydata/guest0.img,format=raw --nographic -cpu host \
> > -smp 4,sockets=4,cores=1,threads=1 \
> > -device vfio-pci,host=08:00.0
> qemu-system-x86_64: -device intel-iommu,intremap=on,eim=off,caching-mode=on:
> Please init intel-iommu before other PCI devices
>
>  Thanks,
> Jintack
>
>
>> 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 **errp)
>>      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;
>> +            }
>> +        }
>> +
>> +        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;
>> +    }
>> +
>>      VTD_DPRINTF(GENERAL, "");
>>      x86_iommu->type = TYPE_INTEL;
>>
>> --
>> 2.7.4
>>
>>
>>
>