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

[Marcel Apfelbaum]

On 03/02/2017 07:13 AM, Michael S. Tsirkin wrote:
> On Thu, Mar 02, 2017 at 11:32:18AM +0800, Peter Xu wrote:
> > 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, virtio-pci ones) so that we know iommu_fn
> > will be setup correctly before realizations of those PCI devices (it is
> > legal 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. We need to avoid that.
> >
> > This patch tries to detect this kind of misordering issue during init of
> > VT-d device, then report to guest if misordering happened. In the
> > future, we can provide something better to solve it, e.g., to support
> > device init ordering, then we can live without this patch.
> >
> > Signed-off-by: Peter Xu <address@hidden>
>
> Unfortunately with virtio it's a regression, as it used to
> work with iommu. So I'm afraid we need to look into supporting
> arbitrary order right now :(

Hi,

A fast way to do it is to initialize iommu with a new keyword, like
    -iommu intel-iommu
Or maybe use the existing "-object" somehow ?
From vl.c comments:
  "Initial object creation happens before all other
   QEMU data types are created.... "

Another idea is to add a third run on QEMU cmd line arguments,
but this would affect the whole system.

However having an ordering system beats all other ideas.

Thanks,
Marcel

> > ---
> > v3: added virtio-pci device detection since we have that requirement as
> >     well now.
> >
> > PS. this patch should be needed along with Jason's:
> >  "virtio: unbreak virtio-pci with IOMMU after caching ring translations"
> > to make sure virtio devices with vt-d are safe for 2.9.
> > ---
> >  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..1077f90 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 and virtio-pci
> > + * devices. Here, there's no easy way to detect init of virtio-pci
> > + * devices, instead we detect the virtio bus.
> > + */
> > +static bool vtd_detected_misorder_init(Error **errp)
> > +{
> > +    if (object_resolve_path_type("", "vfio-pci", NULL) ||
> > +        object_resolve_path_type("", "virtio-pci-bus", NULL)) {
> > +        error_setg(errp, "Please specify \"intel-iommu\" before "
> > +                   "all the rest of the devices.");
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  static void vtd_realize(DeviceState *dev, Error **errp)
> >  {
> >      PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>
> We'd put this somewhere central - this hack is not vtd specific.
>
>
>
> > @@ -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