Re: [PATCH v2 5/5] pc/q35: Add pre-plug hook for x86-iommu

[Peter Xu]

On Thu, Oct 28, 2021 at 09:17:35AM +0200, David Hildenbrand wrote:
> On 28.10.21 06:31, Peter Xu wrote:
> > Add a pre-plug hook for x86-iommu, so that we can detect vfio-pci devices
> > before realizing the vIOMMU device.
> > 
> > When the guest contains both the x86 vIOMMU and vfio-pci devices, the user
> > needs to specify the x86 vIOMMU before the vfio-pci devices.  The reason is,
> > vfio_realize() calls pci_device_iommu_address_space() to fetch the correct 
> > dma
> > address space for the device, while that API can only work right after the
> > vIOMMU device initialized first.
> > 
> > For example, the iommu_fn() that is used in 
> > pci_device_iommu_address_space() is
> > only setup in realize() of the vIOMMU devices.
> > 
> > For a long time we have had libvirt making sure that the ordering is 
> > correct,
> > however from qemu side we never fail a guest from booting even if the 
> > ordering
> > is specified wrongly.  When the order is wrong, the guest will encounter
> > misterious error when operating on the vfio-pci device because in QEMU we'll
> > still assume the vfio-pci devices are put into the default DMA domain 
> > (which is
> > normally the direct GPA mapping), so e.g. the DMAs will never go right.
> > 
> > This patch fails the guest from booting when we detected such errornous 
> > cmdline
> > specified, then the guest at least won't encounter weird device behavior 
> > after
> > booted.  The error message will also help the user to know how to fix the 
> > issue.
> > 
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> I think that's a big improvement. I ran into this issue myself and found
> the documentation at https://wiki.qemu.org/Features/VT-d at one time
> ("Meanwhile, the intel-iommu device must be specified as the first
> device in the parameter list (before all the rest of the devices). ").
> 
> So feel free to add my
> 
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks, will do.

> > @@ -172,4 +172,12 @@ void x86_iommu_iec_notify_all(X86IOMMUState *iommu, 
> > bool global,
> >   * @out: Output MSI message
> >   */
> >  void x86_iommu_irq_to_msi_message(X86IOMMUIrq *irq, MSIMessage *out);
> > +
> > +/**
> > + * x86_iommu_pre_plug: called before plugging the iommu device
> > + * @X86IOMMUState: the pointer to x86 iommu state
> > + * @errp: the double pointer to Error, set if we want to fail the plug
> > + */
> 
> I'd drop that documentation because it's essentially just how any other
> pre_plug handlers works. But maybe it's just me that knows how the whole
> hotplug machinery works, so ...

Yes the documentation is not very helpful because it shouldn't be called
randomly but only in the machine pre-plug hook of x86.  It's just trying to not
be the 1st one exported function in the header that does not have a comment.

Thanks,

-- 
Peter Xu