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

[Pankaj Gupta <pagupta@redhat.com>]

> On Fri, Feb 24, 2017 at 01:35:10AM -0500, Pankaj Gupta wrote:
> > 
> > > 
> > > On Fri, Feb 24, 2017 at 12:07:33AM -0500, Pankaj Gupta wrote:
> > > > Hello Peter,
> > > 
> > > Hi, Pankaj!
> > > 
> > > > 
> > > > 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.
> > > 
> > > Yes this is my intention to emphasize that we should possibly put
> > > intel-iommu even before all the PCI devices. As mentioned above (and
> > > also in the commit message), although vfio-pci is the only one that is
> > > affected by this, we should probably put intel-iommu even before all
> > > the rest of PCI devices. E.g., in the future we can have new devices
> > > that need this dependency as well (that vt-d being inited before that
> > > device), which is still legal imho.
> > 
> > yes, something like this can help "intel-iommu should be specified as first
> > device"?
> 
> I can switch this description if you prefer this one. :) Not sure
> whether this can be touched up during merge if this patch is good for
> 2.9, or I'll just repost with yours if there is further comments.

Sure.
> 
> > Or we can just check for "intel-iommu" as first device at the start in
> > place of
> > checking for "vfio-pci". This will also help us to check for all other
> > devices.
> 
> Actually your suggestion is just what I did in version 1. The problem
> is that, it's not easy to let vt-d really be the first device to be
> inited... Please check pc_q35_init(), within that we have:
> 
>     pc_vga_init(isa_bus, host_bus);
>     pc_nic_init(isa_bus, host_bus);
> 
> These are integrated devices along with ICH9. Such devices are
> possibly pci devices as well, but they are created much earlier than
> vt-d device, since that's still during machine init.

o.k. 

Thanks,
Pankaj
> 
> Thanks,
> 
> -- peterx
>