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

["Cédric Le Goater" <clg@redhat.com>]

On 10/16/23 07:03, Philippe Mathieu-Daudé wrote:
> 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 {

yes.

>         /* documentation ... */
> 
>> +    AddressSpace * (*get_address_space)(PCIBus *bus,
>> +                                void *opaque, int32_t devfn);
>> +};
> 
> ... PCIIOMMUOps;
> 
> Should this be PciIommuOps?

I think this is one of the exceptions to the QEMU coding style and
the other PCI types keep a capital PCI, PCIIORegion, PCIINTxRoute,
PCIEAERErr, etc.

Let's be consistent with the existing naming scheme, PCIIOMMUOps.

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

int is enough.

> Would "lookup_address_space" be clearer?

The calling routing is pci_device_iommu_address_space(). Let's keep
get_address_space() for now.

>>   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 :)

OK. That would be the first documentation entry in pci.h. I guess it
won't do any harm but will it be collected in the documentation under
"Internal QEMU APIs" ?
  
>> +void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *iommu_ops, void *opaque);
> 
> Otherwise the change makes sense.


Thanks,

C.