[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps
|
From: |
Cédric Le Goater |
|
Subject: |
Re: [PATCH] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps |
|
Date: |
Mon, 16 Oct 2023 11:33:33 +0200 |
|
User-agent: |
Mozilla Thunderbird |
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.