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

[Peter Xu]

On Thu, Oct 28, 2021 at 08:52:42AM -0600, Alex Williamson wrote:
> > +void x86_iommu_pre_plug(X86IOMMUState *iommu, Error **errp)
> > +{
> > +    bool ambiguous = false;
> > +    Object *object;
> > +
> > +    object = object_resolve_path_type("", TYPE_VFIO_PCI, &ambiguous);
> > +    if (object || ambiguous) {
> > +        /* There're one or more vfio-pci devices detected */
> > +        error_setg(errp, "Please specify all the vfio-pci devices to be 
> > after "
> > +                   "the vIOMMU device");
> > +    }
> 
> I still really don't buy the argument that vfio-pci is the only driver
> that does "this thing", therefore we can just look for vfio-pci devices
> by name rather than try to generically detect devices that have this
> dependency.  That seems short sighted.
> 
> I've already suggested that pci-core could record on the PCIDevice
> structure if the device address space has been accessed.  We could also
> do something like create a TYPE_PCI_AS_DEVICE class derived from
> TYPE_PCI_DEVICE and any PCI drivers that make use of the device address
> space before machine-init-done would be of this class.  That could even
> be enforced by pci_device_iommu_address_space() and would allow the
> same sort of object resolution as used here.  Thanks,

Sorry Alex, I didn't receive any follow up so I thought you were fine with it.

I was always fine with either way, though I think another parent class would be
an overkill just for this.  Would you think below acceptable?

---8<---
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5cdf1d4298..2156b5d3ed 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3266,6 +3266,7 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, 
void *data)
     pdc->exit = vfio_exitfn;
     pdc->config_read = vfio_pci_read_config;
     pdc->config_write = vfio_pci_write_config;
+    pdc->require_consolidated_iommu_as = true;
 }

 static const TypeInfo vfio_pci_dev_info = {
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 6813f128e0..ffddc766ba 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -239,6 +239,14 @@ struct PCIDeviceClass {
      */
     bool is_bridge;

+    /*
+     * Set this to true when a pci device needs consolidated result from the
+     * pci_device_iommu_address_space() in its realize() fn.  This also means
+     * when specified in cmdline, this "-device" parameter needs to be put
+     * before the vIOMMU devices so as to make it work.
+     */
+    bool require_consolidated_iommu_as;
+
     /* rom bar */
     const char *romfile;
 };
---8<---

Then I'll need to pick the dropped patch back on pci scanning, since then I
won't be able to use object_resolve_path_type() anymore, and I'll need to check
up PCIDeviceClass instead.

Michael, Igor, others - any objections?

-- 
Peter Xu