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

[Alex Williamson]

On Thu, 28 Oct 2021 23:36:33 +0800
Peter Xu <peterx@redhat.com> wrote:

> 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;

Maybe this is where the naming of the previous attempt along these lines
didn't work.  There's no "consolidation" requirement and the IOMMU is
only related in that it is a driver that provides new address spaces
for devices.  This is why I thought we might be able to make it
automatic if pci_device_iommu_address_space() records that the address
space for a device has been consumed.

What we're trying to describe is that the address space object for a
device must be fixed at the time the device is realized.  Along those
lines, maybe a better name is something like "required_fixed_as_obj".

> +
>      /* 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.

Better.  Like the class layering proposal, a downside is that the
driver needs to be aware that it's imposing this requirement to be able
to mark it in the class init function rather than some automatic means,
like an "as_object_consumed" flag set automatically on the device
structure via accessors like pci_device_iommu_address_space().  Thanks,

Alex