Re: [Qemu-devel] [PATCH v2] intel_iommu: check misordered init when realize

[Pankaj Gupta <pagupta@redhat.com>]

Hello Peter,

This solution looks to check dependency of 'vfio-pci' over 'intel-iommu'
before 'intel-iommu' is not initialized.

Overall it looks good to me, just a small nit below.
 
> 
> Intel vIOMMU devices are created with "-device" parameter, while here
> actually we need to make sure the dmar device be created before other
> PCI devices (like vfio-pci) so that we know iommu_fn will be setup
> correctly before realizations of those PCI devices (it is sensible that
> PCI device fetch these info during its realization). Now this ordering
> yet cannot be achieved elsewhere, and devices will be created in the
> order that user specified. That might be dangerous.
> 
> Here we add one more function to detect this kind of misordering issue,
> then report to guest. Currently, the only known device that is affected
> by this VT-d defect is the vfio-pci typed devices. So for now we just
> check against it to make sure we are safe.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 22d8226..b723ece 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2560,6 +2560,24 @@ static bool vtd_decide_config(IntelIOMMUState *s,
> Error **errp)
>      return true;
>  }
>  
> +/*
> + * TODO: we should have a better way to achieve the ordering rather
> + * than this misorder check explicitly against vfio-pci. After all, no
> + * one should be blamed for this, and vfio-pci did nothing wrong.
> + */
> +static bool vtd_detected_misorder_init(Error **errp)
> +{
> +    Object *dev = object_resolve_path_type("", "vfio-pci", NULL);
> +
> +    if (dev) {
> +        error_setg(errp, "Please specify \"intel-iommu\" before all the rest

            "before all the rest" does not give much clue to user. Do you think better
            error message would help? just a thought.
> "
> +                   "of the devices.");
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  static void vtd_realize(DeviceState *dev, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> @@ -2567,6 +2585,10 @@ static void vtd_realize(DeviceState *dev, Error
> **errp)
>      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
>  
> +    if (vtd_detected_misorder_init(errp)) {
> +        return;
> +    }
> +
>      VTD_DPRINTF(GENERAL, "");
>      x86_iommu->type = TYPE_INTEL;
>  
> --
> 2.7.4
> 
> 
>