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

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 29 Oct 2021 10:53:07 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Thu, Oct 28, 2021 at 10:11:35AM -0600, Alex Williamson wrote:
> > 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,  
> 
> Do you mean something like this?
> 
> ---8<---
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 258290f4eb..969f4c85fd 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2729,6 +2729,10 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>      PCIBus *iommu_bus = bus;
>      uint8_t devfn = dev->devfn;
> 
> +    if (!dev->address_space_consumed) {
> +        dev->address_space_consumed = true;
> +    }

Could just set it unconditionally.

> +
>      while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) {
>          PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 6813f128e0..704c9bdc6e 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -268,6 +268,13 @@ typedef struct PCIReqIDCache PCIReqIDCache;
>  struct PCIDevice {
>      DeviceState qdev;
>      bool partially_hotplugged;
> +    /*
> +     * This will be set after the 1st time the device implementation fetches
> +     * its dma address space from pci_device_iommu_address_space().  It's used
> +     * as a sanity check for platform devices like vIOMMU to detect incorrect
> +     * ordering of device realization.
> +     */
> +    bool address_space_consumed;
> 
>      /* PCI config space */
>      uint8_t *config;
> ---8<---
> 
> Then sanity check in pre-plug of vIOMMU.
> 
> The flag will be a bit more "misterious" than the previous approach, imho, as
> the name of it will be even further from the problem it's going to solve.
> However it looks at least clean on the changeset and it looks working too.

That seems like a function of how well we name and comment the
variable, right?  We are making an assumption here that if the address
space for a device is provided then that address space is no longer
interchangeable, some decision has already been made based on the
provided address space.  If we look at the callers of
pci_device_iommu_address_space(), we have:

pci_init_bus_master() - It holds true here that the purpose of
accessing the address space is to make the memory of that address space
accessible to the device, the address space cannot be transparently
swapped for another.

vfio_realize() - The case we're concerned about, potentially the
earliest use case.

virtio_pci_iommu_enabled() - AIUI, this is where virtio devices decide
how DMA flows, the address space of the device cannot be changed after
this.

kvm_arch_fixup_msi_route() - ARM KVM decides MSI routing here, the
address space is fixed after this.

Actually, maybe there's a more simple approach, could we further assume
that if the address space for *any* device relative to an IOMMU is
evaluated, then we've passed the point where an IOMMU could be added?
IOW, maybe we don't need a per device flag and a global flag would be
enough.  A function like pci_iommu_as_evaluated() could report the
state of that flag.  For convenience to the user though, tracking per
device to be able to report which devices are mis-ordered could still
be useful though.  Thanks,

Alex