Re: [PATCH] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps

[Philippe Mathieu-Daudé]

Hi Cédric, Liu, Joao,

On 13/10/23 16:56, Cédric Le Goater wrote:
> From: Liu Yi L <yi.l.liu@intel.com>
>
> This patch modifies pci_setup_iommu() to set PCIIOMMUOps
> instead of setting PCIIOMMUFunc. PCIIOMMUFunc is used to
> get an address space for a PCI device in vendor specific
> way. The PCIIOMMUOps still offers this functionality. But
> using PCIIOMMUOps leaves space to add more iommu related
> vendor specific operations.
>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Yi Sun <yi.y.sun@linux.intel.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: "Hervé Poussineau" <hpoussin@reactos.org>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Cc: Jagannathan Raman <jag.raman@oracle.com>
> Cc: Matthew Rosato <mjrosato@linux.ibm.com>
> Cc: Eric Farman <farman@linux.ibm.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> [ clg: - refreshed on latest QEMU
>         - included hw/remote/iommu.c   ]
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
>
>   Hello,
>
>   Initially sent by Yi Liu as part of series "intel_iommu: expose
>   Shared Virtual Addressing to VMs" [1], this patch would also simplify
>   the changes Joao wants to introduce in "vfio: VFIO migration support
>   with vIOMMU" [2].
>
>   Has anyone objections ?
>
>   Thanks,
>
>   C.
>
>   [1] 
> https://lore.kernel.org/qemu-devel/20210302203827.437645-5-yi.l.liu@intel.com/
>   [2] 
> https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/
>
>
>
>   include/hw/pci/pci.h     |  8 ++++++--
>   include/hw/pci/pci_bus.h |  2 +-
>   hw/alpha/typhoon.c       |  6 +++++-
>   hw/arm/smmu-common.c     |  6 +++++-
>   hw/i386/amd_iommu.c      |  6 +++++-
>   hw/i386/intel_iommu.c    |  6 +++++-
>   hw/pci-host/designware.c |  6 +++++-
>   hw/pci-host/dino.c       |  6 +++++-
>   hw/pci-host/pnv_phb3.c   |  6 +++++-
>   hw/pci-host/pnv_phb4.c   |  6 +++++-
>   hw/pci-host/ppce500.c    |  6 +++++-
>   hw/pci-host/raven.c      |  6 +++++-
>   hw/pci-host/sabre.c      |  6 +++++-
>   hw/pci/pci.c             | 18 +++++++++++++-----
>   hw/ppc/ppc440_pcix.c     |  6 +++++-
>   hw/ppc/spapr_pci.c       |  6 +++++-
>   hw/remote/iommu.c        |  6 +++++-
>   hw/s390x/s390-pci-bus.c  |  8 ++++++--
>   hw/virtio/virtio-iommu.c |  6 +++++-
>   19 files changed, 101 insertions(+), 25 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 
> b70a0b95ff5ae367ed7f98483ec8d1d1b6274530..486e54174b1755995328f2352fd4571d01e107dc
>  100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -366,10 +366,14 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range);
>   
>   void pci_device_deassert_intx(PCIDevice *dev);
>   
> -typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
> +typedef struct PCIIOMMUOps PCIIOMMUOps;

Preferably:

typedef ...

> +struct PCIIOMMUOps {

       /* documentation ... */

> +    AddressSpace * (*get_address_space)(PCIBus *bus,
> +                                void *opaque, int32_t devfn);
> +};

... PCIIOMMUOps;

Should this be PciIommuOps?

Do we need 'int32_t' for devfn or 'int' is enough?

Would "lookup_address_space" be clearer?

>   AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);

Since the prototype is modified, we can take the opportunity to
document it :)

> +void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *iommu_ops, void *opaque);

Otherwise the change makes sense.

Regards,

Phil.