* [PATCH v3] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps
@ 2023-10-19 11:54 Cédric Le Goater
2023-10-19 12:33 ` Eric Auger
0 siblings, 1 reply; 4+ messages in thread
From: Cédric Le Goater @ 2023-10-19 11:54 UTC (permalink / raw)
To: qemu-devel
Cc: Joao Martins, Liu Yi L, Kevin Tian, Jacob Pan, Peter Xu,
Eric Auger, Yi Sun, David Gibson, Michael S. Tsirkin,
Peter Maydell, Paolo Bonzini, Jason Wang, Andrey Smirnov,
Helge Deller, Hervé Poussineau, Mark Cave-Ayland,
BALATON Zoltan, Elena Ufimtseva, Jagannathan Raman,
Matthew Rosato, Eric Farman, Halil Pasic, Christian Borntraeger,
Thomas Huth, Philippe Mathieu-Daudé, Cédric Le Goater
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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
[ clg: - refreshed on latest QEMU
- included hw/remote/iommu.c
- documentation update
- asserts in pci_setup_iommu()
- removed checks on iommu_bus->iommu_ops->get_address_space ]
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/
Changes in v3 :
- New Documentation
- Create a pci.rst for the kernel-doc comments of
include/hw/pci/pci.h
- asserts in pci_setup_iommu()
- removed checks on iommu_bus->iommu_ops->get_address_space
Changes in v2 :
- PCIIOMMUOps definition cleanups
docs/devel/index-api.rst | 1 +
docs/devel/pci.rst | 8 ++++++++
include/hw/pci/pci.h | 39 +++++++++++++++++++++++++++++++++++++--
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 +++++-
21 files changed, 141 insertions(+), 25 deletions(-)
create mode 100644 docs/devel/pci.rst
diff --git a/docs/devel/index-api.rst b/docs/devel/index-api.rst
index 539ad29c215e803568b1d7b0ba6766c0cc9d2b8b..fe01b2b488d0b61376560011a660389603e4ce8e 100644
--- a/docs/devel/index-api.rst
+++ b/docs/devel/index-api.rst
@@ -11,6 +11,7 @@ generated from in-code annotations to function prototypes.
loads-stores
memory
modules
+ pci
qom-api
qdev-api
ui
diff --git a/docs/devel/pci.rst b/docs/devel/pci.rst
new file mode 100644
index 0000000000000000000000000000000000000000..68739334f3c936cc28b8fceac6dbebbfe68e78a2
--- /dev/null
+++ b/docs/devel/pci.rst
@@ -0,0 +1,8 @@
+=============
+PCI subsystem
+=============
+
+API Reference
+-------------
+
+.. kernel-doc:: include/hw/pci/pci.h
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b70a0b95ff5ae367ed7f98483ec8d1d1b6274530..0a4bcd13f39fde60326a548a3cd6637ec2287475 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -366,10 +366,45 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range);
void pci_device_deassert_intx(PCIDevice *dev);
-typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
+
+/**
+ * struct PCIIOMMUOps: callbacks structure for specific IOMMU handlers
+ * of a PCIBus
+ *
+ * Allows to modify the behavior of some IOMMU operations of the PCI
+ * framework for a set of devices on a PCI bus.
+ */
+typedef struct PCIIOMMUOps {
+ /**
+ * @get_address_space: get the address space for a set of devices
+ * on a PCI bus.
+ *
+ * Returns a pointer to AddressSpace, otherwise NULL.
+ *
+ * Optional method: if this method is not set, then the default
+ * address_space is used.
+ *
+ * @bus: the #PCIBus being accessed.
+ *
+ * @opaque: the data passed to pci_setup_iommu().
+ *
+ * @devfn: device and function number
+ */
+ AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
+} PCIIOMMUOps;
AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
-void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
+
+/**
+ * pci_setup_iommu: Initialize specific IOMMU handlers for a PCIBus
+ *
+ * Let PCI host bridges define specific operations.
+ *
+ * @bus: the #PCIBus being updated.
+ * @ops: the #PCIIOMMUOps
+ * @opaque: passed to callbacks of the @ops structure.
+ */
+void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque);
pcibus_t pci_bar_address(PCIDevice *d,
int reg, uint8_t type, pcibus_t size);
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 56531759578ffe2c6eccc92ebae4acabf47e390d..226131254621f257eefb45356489c125381e6a87 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -33,7 +33,7 @@ enum PCIBusFlags {
struct PCIBus {
BusState qbus;
enum PCIBusFlags flags;
- PCIIOMMUFunc iommu_fn;
+ const PCIIOMMUOps *iommu_ops;
void *iommu_opaque;
uint8_t devfn_min;
uint32_t slot_reserved_mask;
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 49a80550c54d4ed9ee40f9959cbf2c83240d8e39..e8711ae16a3e16c733405034be5e65688ef4d3df 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -738,6 +738,10 @@ static AddressSpace *typhoon_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
return &s->pchip.iommu_as;
}
+static const PCIIOMMUOps typhoon_iommu_ops = {
+ .get_address_space = typhoon_pci_dma_iommu,
+};
+
static void typhoon_set_irq(void *opaque, int irq, int level)
{
TyphoonState *s = opaque;
@@ -897,7 +901,7 @@ PCIBus *typhoon_init(MemoryRegion *ram, qemu_irq *p_isa_irq,
"iommu-typhoon", UINT64_MAX);
address_space_init(&s->pchip.iommu_as, MEMORY_REGION(&s->pchip.iommu),
"pchip0-pci");
- pci_setup_iommu(b, typhoon_pci_dma_iommu, s);
+ pci_setup_iommu(b, &typhoon_iommu_ops, s);
/* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB. */
memory_region_init_io(&s->pchip.reg_iack, OBJECT(s), &alpha_pci_iack_ops,
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index f35ae9aa22cb1a62cc16c2534cb3c290862b0799..9a8ac45431abb80fc5e9f60104248cb356509088 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -605,6 +605,10 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
return &sdev->as;
}
+static const PCIIOMMUOps smmu_ops = {
+ .get_address_space = smmu_find_add_as,
+};
+
IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid)
{
uint8_t bus_n, devfn;
@@ -661,7 +665,7 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
if (s->primary_bus) {
- pci_setup_iommu(s->primary_bus, smmu_find_add_as, s);
+ pci_setup_iommu(s->primary_bus, &smmu_ops, s);
} else {
error_setg(errp, "SMMU is not attached to any PCI bus!");
}
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 8d0f2f99ddbf49c3df834b6d40d2b23cafbb81ea..94f0f8a7f22130e5684926a8790182376723e58e 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1450,6 +1450,10 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
return &iommu_as[devfn]->as;
}
+static const PCIIOMMUOps amdvi_iommu_ops = {
+ .get_address_space = amdvi_host_dma_iommu,
+};
+
static const MemoryRegionOps mmio_mem_ops = {
.read = amdvi_mmio_read,
.write = amdvi_mmio_write,
@@ -1582,7 +1586,7 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
- pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
+ pci_setup_iommu(bus, &amdvi_iommu_ops, s);
amdvi_init(s);
}
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 2c832ab68b3774dbec6eee4ffe7c13e7a1bf7bcc..87b07a390482160e6db630a2c2c4f6f522e0dc72 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4034,6 +4034,10 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
return &vtd_as->as;
}
+static PCIIOMMUOps vtd_iommu_ops = {
+ .get_address_space = vtd_host_dma_iommu,
+};
+
static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
{
X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
@@ -4157,7 +4161,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
g_free, g_free);
vtd_init(s);
sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
- pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
+ pci_setup_iommu(bus, &vtd_iommu_ops, dev);
/* Pseudo address space under root PCI bus. */
x86ms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 6f5442f108874f6217fa122ac08c1ce27ad20cb9..f477f97847d753341f341281ed2abed2ce242574 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -663,6 +663,10 @@ static AddressSpace *designware_pcie_host_set_iommu(PCIBus *bus, void *opaque,
return &s->pci.address_space;
}
+static const PCIIOMMUOps designware_iommu_ops = {
+ .get_address_space = designware_pcie_host_set_iommu,
+};
+
static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
{
PCIHostState *pci = PCI_HOST_BRIDGE(dev);
@@ -705,7 +709,7 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
address_space_init(&s->pci.address_space,
&s->pci.address_space_root,
"pcie-bus-address-space");
- pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s);
+ pci_setup_iommu(pci->bus, &designware_iommu_ops, s);
qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
}
diff --git a/hw/pci-host/dino.c b/hw/pci-host/dino.c
index 82503229faf8a44b3974b2256b641ce18a77692b..5b0947a16c9ec4abf8668bc4563b1cc181f154f3 100644
--- a/hw/pci-host/dino.c
+++ b/hw/pci-host/dino.c
@@ -354,6 +354,10 @@ static AddressSpace *dino_pcihost_set_iommu(PCIBus *bus, void *opaque,
return &s->bm_as;
}
+static const PCIIOMMUOps dino_iommu_ops = {
+ .get_address_space = dino_pcihost_set_iommu,
+};
+
/*
* Dino interrupts are connected as shown on Page 78, Table 23
* (Little-endian bit numbers)
@@ -481,7 +485,7 @@ static void dino_pcihost_init(Object *obj)
g_free(name);
}
- pci_setup_iommu(phb->bus, dino_pcihost_set_iommu, s);
+ pci_setup_iommu(phb->bus, &dino_iommu_ops, s);
sysbus_init_mmio(sbd, &s->this_mem);
diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index c5e58f4086aef34fa0b5307c10b261c0c6e3ac34..2a74dbe45f59b207b57b3b8af8856ac5cf04d114 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -968,6 +968,10 @@ static AddressSpace *pnv_phb3_dma_iommu(PCIBus *bus, void *opaque, int devfn)
return &ds->dma_as;
}
+static PCIIOMMUOps pnv_phb3_iommu_ops = {
+ .get_address_space = pnv_phb3_dma_iommu,
+};
+
static void pnv_phb3_instance_init(Object *obj)
{
PnvPHB3 *phb = PNV_PHB3(obj);
@@ -1012,7 +1016,7 @@ void pnv_phb3_bus_init(DeviceState *dev, PnvPHB3 *phb)
object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
&error_abort);
- pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
+ pci_setup_iommu(pci->bus, &pnv_phb3_iommu_ops, phb);
}
static void pnv_phb3_realize(DeviceState *dev, Error **errp)
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 29cb11a5d94c6a72a8111dd95346be906a7bc4fe..37c7afc18c9b17b25df8356907f0d87d7d621ac2 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1518,6 +1518,10 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
&phb->phb_regs_mr);
}
+static PCIIOMMUOps pnv_phb4_iommu_ops = {
+ .get_address_space = pnv_phb4_dma_iommu,
+};
+
static void pnv_phb4_instance_init(Object *obj)
{
PnvPHB4 *phb = PNV_PHB4(obj);
@@ -1557,7 +1561,7 @@ void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb)
object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
&error_abort);
- pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
+ pci_setup_iommu(pci->bus, &pnv_phb4_iommu_ops, phb);
pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
}
diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index 38814247f2a5b7c1a6b55e18673f3fe8c348fbee..453a4e6ed3b02ed448c3fad71bd45eb73a5dbbb3 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -435,6 +435,10 @@ static AddressSpace *e500_pcihost_set_iommu(PCIBus *bus, void *opaque,
return &s->bm_as;
}
+static const PCIIOMMUOps ppce500_iommu_ops = {
+ .get_address_space = e500_pcihost_set_iommu,
+};
+
static void e500_pcihost_realize(DeviceState *dev, Error **errp)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
@@ -469,7 +473,7 @@ static void e500_pcihost_realize(DeviceState *dev, Error **errp)
memory_region_init(&s->bm, OBJECT(s), "bm-e500", UINT64_MAX);
memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
address_space_init(&s->bm_as, &s->bm, "pci-bm");
- pci_setup_iommu(b, e500_pcihost_set_iommu, s);
+ pci_setup_iommu(b, &ppce500_iommu_ops, s);
pci_create_simple(b, 0, "e500-host-bridge");
diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 9a11ac4b2b610646790ed2016f73a16bce4fa42f..86c3a4908712988637484967e88155afb1413742 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -223,6 +223,10 @@ static AddressSpace *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
return &s->bm_as;
}
+static const PCIIOMMUOps raven_iommu_ops = {
+ .get_address_space = raven_pcihost_set_iommu,
+};
+
static void raven_change_gpio(void *opaque, int n, int level)
{
PREPPCIState *s = opaque;
@@ -320,7 +324,7 @@ static void raven_pcihost_initfn(Object *obj)
memory_region_add_subregion(&s->bm, 0 , &s->bm_pci_memory_alias);
memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
address_space_init(&s->bm_as, &s->bm, "raven-bm");
- pci_setup_iommu(&s->pci_bus, raven_pcihost_set_iommu, s);
+ pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
h->bus = &s->pci_bus;
diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index dcb2e230b67274d4b87c86eb884e052c26031c27..d0851b48b02237d41f64f6cce6f0aa4740ca0d87 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -112,6 +112,10 @@ static AddressSpace *sabre_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
return &is->iommu_as;
}
+static const PCIIOMMUOps sabre_iommu_ops = {
+ .get_address_space = sabre_pci_dma_iommu,
+};
+
static void sabre_config_write(void *opaque, hwaddr addr,
uint64_t val, unsigned size)
{
@@ -384,7 +388,7 @@ static void sabre_realize(DeviceState *dev, Error **errp)
/* IOMMU */
memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1);
- pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
+ pci_setup_iommu(phb->bus, &sabre_iommu_ops, s->iommu);
/* APB secondary busses */
pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), TYPE_SIMBA_PCI_BRIDGE);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b0d21bf43a9da8e624507dad868f0aa775fb2ad6..553a91d53c24249bb28e336bce7ee3790ddbfbc9 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2685,7 +2685,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
PCIBus *iommu_bus = bus;
uint8_t devfn = dev->devfn;
- while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) {
+ while (iommu_bus && !iommu_bus->iommu_ops && iommu_bus->parent_dev) {
PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
/*
@@ -2724,15 +2724,23 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
iommu_bus = parent_bus;
}
- if (!pci_bus_bypass_iommu(bus) && iommu_bus && iommu_bus->iommu_fn) {
- return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn);
+ if (!pci_bus_bypass_iommu(bus) && iommu_bus->iommu_ops) {
+ return iommu_bus->iommu_ops->get_address_space(bus,
+ iommu_bus->iommu_opaque, devfn);
}
return &address_space_memory;
}
-void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
+void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
{
- bus->iommu_fn = fn;
+ /*
+ * If called, pci_setup_iommu() should provide a minimum set of
+ * useful callbacks for the bus.
+ */
+ assert(ops);
+ assert(ops->get_address_space);
+
+ bus->iommu_ops = ops;
bus->iommu_opaque = opaque;
}
diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
index 672090de9478e9c8e9ba8151b72bcaf93495d223..df4ee374d04bea092c4f81098f82514c12c0519c 100644
--- a/hw/ppc/ppc440_pcix.c
+++ b/hw/ppc/ppc440_pcix.c
@@ -449,6 +449,10 @@ static AddressSpace *ppc440_pcix_set_iommu(PCIBus *b, void *opaque, int devfn)
return &s->bm_as;
}
+static const PCIIOMMUOps ppc440_iommu_ops = {
+ .get_address_space = ppc440_pcix_set_iommu,
+};
+
/*
* Some guests on sam460ex write all kinds of garbage here such as
* missing enable bit and low bits set and still expect this to work
@@ -503,7 +507,7 @@ static void ppc440_pcix_realize(DeviceState *dev, Error **errp)
memory_region_init(&s->bm, OBJECT(s), "bm-ppc440-pcix", UINT64_MAX);
memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
address_space_init(&s->bm_as, &s->bm, "pci-bm");
- pci_setup_iommu(h->bus, ppc440_pcix_set_iommu, s);
+ pci_setup_iommu(h->bus, &ppc440_iommu_ops, s);
memory_region_init(&s->container, OBJECT(s), "pci-container", PCI_ALL_SIZE);
memory_region_init_io(&h->conf_mem, OBJECT(s), &ppc440_pcix_host_conf_ops,
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 370c5a90f2184bbe75de9a3b950096960c67189e..a27024e45a4188c5b21e7d41ecf3d337bbbb47f0 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -780,6 +780,10 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
return &phb->iommu_as;
}
+static const PCIIOMMUOps spapr_iommu_ops = {
+ .get_address_space = spapr_pci_dma_iommu,
+};
+
static char *spapr_phb_vfio_get_loc_code(SpaprPhbState *sphb, PCIDevice *pdev)
{
g_autofree char *path = NULL;
@@ -1978,7 +1982,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
memory_region_add_subregion(&sphb->iommu_root, SPAPR_PCI_MSI_WINDOW,
&sphb->msiwindow);
- pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
+ pci_setup_iommu(bus, &spapr_iommu_ops, sphb);
pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
diff --git a/hw/remote/iommu.c b/hw/remote/iommu.c
index 1391dd712ceda0bb5b524651362350471f31f93a..7c56aad0fc14a3a411303dcf3baacef5edfd3d1e 100644
--- a/hw/remote/iommu.c
+++ b/hw/remote/iommu.c
@@ -100,6 +100,10 @@ static void remote_iommu_finalize(Object *obj)
iommu->elem_by_devfn = NULL;
}
+static const PCIIOMMUOps remote_iommu_ops = {
+ .get_address_space = remote_iommu_find_add_as,
+};
+
void remote_iommu_setup(PCIBus *pci_bus)
{
RemoteIommu *iommu = NULL;
@@ -108,7 +112,7 @@ void remote_iommu_setup(PCIBus *pci_bus)
iommu = REMOTE_IOMMU(object_new(TYPE_REMOTE_IOMMU));
- pci_setup_iommu(pci_bus, remote_iommu_find_add_as, iommu);
+ pci_setup_iommu(pci_bus, &remote_iommu_ops, iommu);
object_property_add_child(OBJECT(pci_bus), "remote-iommu", OBJECT(iommu));
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 2ca36f9f3be11866b6c9cb83a61da01d93a4b7c0..347580ebacfe1dd832063b667aa593097ba6926d 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -652,6 +652,10 @@ static AddressSpace *s390_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
return &iommu->as;
}
+static const PCIIOMMUOps s390_iommu_ops = {
+ .get_address_space = s390_pci_dma_iommu,
+};
+
static uint8_t set_ind_atomic(uint64_t ind_loc, uint8_t to_be_set)
{
uint8_t expected, actual;
@@ -839,7 +843,7 @@ static void s390_pcihost_realize(DeviceState *dev, Error **errp)
b = pci_register_root_bus(dev, NULL, s390_pci_set_irq, s390_pci_map_irq,
NULL, get_system_memory(), get_system_io(), 0,
64, TYPE_PCI_BUS);
- pci_setup_iommu(b, s390_pci_dma_iommu, s);
+ pci_setup_iommu(b, &s390_iommu_ops, s);
bus = BUS(b);
qbus_set_hotplug_handler(bus, OBJECT(dev));
@@ -1058,7 +1062,7 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
pdev = PCI_DEVICE(dev);
pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq);
- pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s);
+ pci_setup_iommu(&pb->sec_bus, &s390_iommu_ops, s);
qbus_set_hotplug_handler(BUS(&pb->sec_bus), OBJECT(s));
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index be51635895ce88cb69ec987a5c7d3fa33a703c9e..4c62378f210a63dcd49e596a56802101bb14f0af 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -444,6 +444,10 @@ static AddressSpace *virtio_iommu_find_add_as(PCIBus *bus, void *opaque,
return &sdev->as;
}
+static const PCIIOMMUOps virtio_iommu_ops = {
+ .get_address_space = virtio_iommu_find_add_as,
+};
+
static int virtio_iommu_attach(VirtIOIOMMU *s,
struct virtio_iommu_req_attach *req)
{
@@ -1206,7 +1210,7 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
s->as_by_busptr = g_hash_table_new_full(NULL, NULL, NULL, g_free);
if (s->primary_bus) {
- pci_setup_iommu(s->primary_bus, virtio_iommu_find_add_as, s);
+ pci_setup_iommu(s->primary_bus, &virtio_iommu_ops, s);
} else {
error_setg(errp, "VIRTIO-IOMMU is not attached to any PCI bus!");
}
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps
2023-10-19 11:54 [PATCH v3] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps Cédric Le Goater
@ 2023-10-19 12:33 ` Eric Auger
2023-10-19 14:02 ` Cédric Le Goater
0 siblings, 1 reply; 4+ messages in thread
From: Eric Auger @ 2023-10-19 12:33 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Joao Martins, Liu Yi L, Kevin Tian, Jacob Pan, Peter Xu, Yi Sun,
David Gibson, Michael S. Tsirkin, Peter Maydell, Paolo Bonzini,
Jason Wang, Andrey Smirnov, Helge Deller, Hervé Poussineau,
Mark Cave-Ayland, BALATON Zoltan, Elena Ufimtseva,
Jagannathan Raman, Matthew Rosato, Eric Farman, Halil Pasic,
Christian Borntraeger, Thomas Huth, Philippe Mathieu-Daudé
Hi Cédric,
On 10/19/23 13:54, 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>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> [ clg: - refreshed on latest QEMU
> - included hw/remote/iommu.c
> - documentation update
> - asserts in pci_setup_iommu()
> - removed checks on iommu_bus->iommu_ops->get_address_space ]
> 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/
>
> Changes in v3 :
>
> - New Documentation
> - Create a pci.rst for the kernel-doc comments of
> include/hw/pci/pci.h
> - asserts in pci_setup_iommu()
> - removed checks on iommu_bus->iommu_ops->get_address_space
>
> Changes in v2 :
>
> - PCIIOMMUOps definition cleanups
>
> docs/devel/index-api.rst | 1 +
> docs/devel/pci.rst | 8 ++++++++
> include/hw/pci/pci.h | 39 +++++++++++++++++++++++++++++++++++++--
> 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 +++++-
> 21 files changed, 141 insertions(+), 25 deletions(-)
> create mode 100644 docs/devel/pci.rst
>
> diff --git a/docs/devel/index-api.rst b/docs/devel/index-api.rst
> index 539ad29c215e803568b1d7b0ba6766c0cc9d2b8b..fe01b2b488d0b61376560011a660389603e4ce8e 100644
> --- a/docs/devel/index-api.rst
> +++ b/docs/devel/index-api.rst
> @@ -11,6 +11,7 @@ generated from in-code annotations to function prototypes.
> loads-stores
> memory
> modules
> + pci
> qom-api
> qdev-api
> ui
> diff --git a/docs/devel/pci.rst b/docs/devel/pci.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..68739334f3c936cc28b8fceac6dbebbfe68e78a2
> --- /dev/null
> +++ b/docs/devel/pci.rst
> @@ -0,0 +1,8 @@
> +=============
> +PCI subsystem
> +=============
> +
> +API Reference
> +-------------
> +
> +.. kernel-doc:: include/hw/pci/pci.h
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index b70a0b95ff5ae367ed7f98483ec8d1d1b6274530..0a4bcd13f39fde60326a548a3cd6637ec2287475 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -366,10 +366,45 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range);
>
> void pci_device_deassert_intx(PCIDevice *dev);
>
> -typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
> +
> +/**
> + * struct PCIIOMMUOps: callbacks structure for specific IOMMU handlers
> + * of a PCIBus
> + *
> + * Allows to modify the behavior of some IOMMU operations of the PCI
> + * framework for a set of devices on a PCI bus.
> + */
> +typedef struct PCIIOMMUOps {
> + /**
> + * @get_address_space: get the address space for a set of devices
> + * on a PCI bus.
> + *
> + * Returns a pointer to AddressSpace, otherwise NULL.
> + *
> + * Optional method: if this method is not set, then the default
> + * address_space is used.
isn't it a mandated method? You assert on
pci_setup_iommu() if it isn't set, right?
Eric
> + *
> + * @bus: the #PCIBus being accessed.
> + *
> + * @opaque: the data passed to pci_setup_iommu().
> + *
> + * @devfn: device and function number
> + */
> + AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
> +} PCIIOMMUOps;
>
> AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
> +
> +/**
> + * pci_setup_iommu: Initialize specific IOMMU handlers for a PCIBus
> + *
> + * Let PCI host bridges define specific operations.
> + *
> + * @bus: the #PCIBus being updated.
> + * @ops: the #PCIIOMMUOps
> + * @opaque: passed to callbacks of the @ops structure.
> + */
> +void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque);
>
> pcibus_t pci_bar_address(PCIDevice *d,
> int reg, uint8_t type, pcibus_t size);
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 56531759578ffe2c6eccc92ebae4acabf47e390d..226131254621f257eefb45356489c125381e6a87 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -33,7 +33,7 @@ enum PCIBusFlags {
> struct PCIBus {
> BusState qbus;
> enum PCIBusFlags flags;
> - PCIIOMMUFunc iommu_fn;
> + const PCIIOMMUOps *iommu_ops;
> void *iommu_opaque;
> uint8_t devfn_min;
> uint32_t slot_reserved_mask;
> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
> index 49a80550c54d4ed9ee40f9959cbf2c83240d8e39..e8711ae16a3e16c733405034be5e65688ef4d3df 100644
> --- a/hw/alpha/typhoon.c
> +++ b/hw/alpha/typhoon.c
> @@ -738,6 +738,10 @@ static AddressSpace *typhoon_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> return &s->pchip.iommu_as;
> }
>
> +static const PCIIOMMUOps typhoon_iommu_ops = {
> + .get_address_space = typhoon_pci_dma_iommu,
> +};
> +
> static void typhoon_set_irq(void *opaque, int irq, int level)
> {
> TyphoonState *s = opaque;
> @@ -897,7 +901,7 @@ PCIBus *typhoon_init(MemoryRegion *ram, qemu_irq *p_isa_irq,
> "iommu-typhoon", UINT64_MAX);
> address_space_init(&s->pchip.iommu_as, MEMORY_REGION(&s->pchip.iommu),
> "pchip0-pci");
> - pci_setup_iommu(b, typhoon_pci_dma_iommu, s);
> + pci_setup_iommu(b, &typhoon_iommu_ops, s);
>
> /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB. */
> memory_region_init_io(&s->pchip.reg_iack, OBJECT(s), &alpha_pci_iack_ops,
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index f35ae9aa22cb1a62cc16c2534cb3c290862b0799..9a8ac45431abb80fc5e9f60104248cb356509088 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -605,6 +605,10 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
> return &sdev->as;
> }
>
> +static const PCIIOMMUOps smmu_ops = {
> + .get_address_space = smmu_find_add_as,
> +};
> +
> IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid)
> {
> uint8_t bus_n, devfn;
> @@ -661,7 +665,7 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
> s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
>
> if (s->primary_bus) {
> - pci_setup_iommu(s->primary_bus, smmu_find_add_as, s);
> + pci_setup_iommu(s->primary_bus, &smmu_ops, s);
> } else {
> error_setg(errp, "SMMU is not attached to any PCI bus!");
> }
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 8d0f2f99ddbf49c3df834b6d40d2b23cafbb81ea..94f0f8a7f22130e5684926a8790182376723e58e 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1450,6 +1450,10 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> return &iommu_as[devfn]->as;
> }
>
> +static const PCIIOMMUOps amdvi_iommu_ops = {
> + .get_address_space = amdvi_host_dma_iommu,
> +};
> +
> static const MemoryRegionOps mmio_mem_ops = {
> .read = amdvi_mmio_read,
> .write = amdvi_mmio_write,
> @@ -1582,7 +1586,7 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>
> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
> - pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
> + pci_setup_iommu(bus, &amdvi_iommu_ops, s);
> amdvi_init(s);
> }
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 2c832ab68b3774dbec6eee4ffe7c13e7a1bf7bcc..87b07a390482160e6db630a2c2c4f6f522e0dc72 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4034,6 +4034,10 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> return &vtd_as->as;
> }
>
> +static PCIIOMMUOps vtd_iommu_ops = {
> + .get_address_space = vtd_host_dma_iommu,
> +};
> +
> static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> {
> X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
> @@ -4157,7 +4161,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
> g_free, g_free);
> vtd_init(s);
> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
> - pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
> + pci_setup_iommu(bus, &vtd_iommu_ops, dev);
> /* Pseudo address space under root PCI bus. */
> x86ms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
> qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index 6f5442f108874f6217fa122ac08c1ce27ad20cb9..f477f97847d753341f341281ed2abed2ce242574 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -663,6 +663,10 @@ static AddressSpace *designware_pcie_host_set_iommu(PCIBus *bus, void *opaque,
> return &s->pci.address_space;
> }
>
> +static const PCIIOMMUOps designware_iommu_ops = {
> + .get_address_space = designware_pcie_host_set_iommu,
> +};
> +
> static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
> {
> PCIHostState *pci = PCI_HOST_BRIDGE(dev);
> @@ -705,7 +709,7 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
> address_space_init(&s->pci.address_space,
> &s->pci.address_space_root,
> "pcie-bus-address-space");
> - pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s);
> + pci_setup_iommu(pci->bus, &designware_iommu_ops, s);
>
> qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
> }
> diff --git a/hw/pci-host/dino.c b/hw/pci-host/dino.c
> index 82503229faf8a44b3974b2256b641ce18a77692b..5b0947a16c9ec4abf8668bc4563b1cc181f154f3 100644
> --- a/hw/pci-host/dino.c
> +++ b/hw/pci-host/dino.c
> @@ -354,6 +354,10 @@ static AddressSpace *dino_pcihost_set_iommu(PCIBus *bus, void *opaque,
> return &s->bm_as;
> }
>
> +static const PCIIOMMUOps dino_iommu_ops = {
> + .get_address_space = dino_pcihost_set_iommu,
> +};
> +
> /*
> * Dino interrupts are connected as shown on Page 78, Table 23
> * (Little-endian bit numbers)
> @@ -481,7 +485,7 @@ static void dino_pcihost_init(Object *obj)
> g_free(name);
> }
>
> - pci_setup_iommu(phb->bus, dino_pcihost_set_iommu, s);
> + pci_setup_iommu(phb->bus, &dino_iommu_ops, s);
>
> sysbus_init_mmio(sbd, &s->this_mem);
>
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index c5e58f4086aef34fa0b5307c10b261c0c6e3ac34..2a74dbe45f59b207b57b3b8af8856ac5cf04d114 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -968,6 +968,10 @@ static AddressSpace *pnv_phb3_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> return &ds->dma_as;
> }
>
> +static PCIIOMMUOps pnv_phb3_iommu_ops = {
> + .get_address_space = pnv_phb3_dma_iommu,
> +};
> +
> static void pnv_phb3_instance_init(Object *obj)
> {
> PnvPHB3 *phb = PNV_PHB3(obj);
> @@ -1012,7 +1016,7 @@ void pnv_phb3_bus_init(DeviceState *dev, PnvPHB3 *phb)
> object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
> &error_abort);
>
> - pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
> + pci_setup_iommu(pci->bus, &pnv_phb3_iommu_ops, phb);
> }
>
> static void pnv_phb3_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 29cb11a5d94c6a72a8111dd95346be906a7bc4fe..37c7afc18c9b17b25df8356907f0d87d7d621ac2 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1518,6 +1518,10 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
> &phb->phb_regs_mr);
> }
>
> +static PCIIOMMUOps pnv_phb4_iommu_ops = {
> + .get_address_space = pnv_phb4_dma_iommu,
> +};
> +
> static void pnv_phb4_instance_init(Object *obj)
> {
> PnvPHB4 *phb = PNV_PHB4(obj);
> @@ -1557,7 +1561,7 @@ void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb)
> object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
> &error_abort);
>
> - pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
> + pci_setup_iommu(pci->bus, &pnv_phb4_iommu_ops, phb);
> pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> }
>
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index 38814247f2a5b7c1a6b55e18673f3fe8c348fbee..453a4e6ed3b02ed448c3fad71bd45eb73a5dbbb3 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -435,6 +435,10 @@ static AddressSpace *e500_pcihost_set_iommu(PCIBus *bus, void *opaque,
> return &s->bm_as;
> }
>
> +static const PCIIOMMUOps ppce500_iommu_ops = {
> + .get_address_space = e500_pcihost_set_iommu,
> +};
> +
> static void e500_pcihost_realize(DeviceState *dev, Error **errp)
> {
> SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> @@ -469,7 +473,7 @@ static void e500_pcihost_realize(DeviceState *dev, Error **errp)
> memory_region_init(&s->bm, OBJECT(s), "bm-e500", UINT64_MAX);
> memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
> address_space_init(&s->bm_as, &s->bm, "pci-bm");
> - pci_setup_iommu(b, e500_pcihost_set_iommu, s);
> + pci_setup_iommu(b, &ppce500_iommu_ops, s);
>
> pci_create_simple(b, 0, "e500-host-bridge");
>
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index 9a11ac4b2b610646790ed2016f73a16bce4fa42f..86c3a4908712988637484967e88155afb1413742 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -223,6 +223,10 @@ static AddressSpace *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
> return &s->bm_as;
> }
>
> +static const PCIIOMMUOps raven_iommu_ops = {
> + .get_address_space = raven_pcihost_set_iommu,
> +};
> +
> static void raven_change_gpio(void *opaque, int n, int level)
> {
> PREPPCIState *s = opaque;
> @@ -320,7 +324,7 @@ static void raven_pcihost_initfn(Object *obj)
> memory_region_add_subregion(&s->bm, 0 , &s->bm_pci_memory_alias);
> memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
> address_space_init(&s->bm_as, &s->bm, "raven-bm");
> - pci_setup_iommu(&s->pci_bus, raven_pcihost_set_iommu, s);
> + pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
>
> h->bus = &s->pci_bus;
>
> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> index dcb2e230b67274d4b87c86eb884e052c26031c27..d0851b48b02237d41f64f6cce6f0aa4740ca0d87 100644
> --- a/hw/pci-host/sabre.c
> +++ b/hw/pci-host/sabre.c
> @@ -112,6 +112,10 @@ static AddressSpace *sabre_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> return &is->iommu_as;
> }
>
> +static const PCIIOMMUOps sabre_iommu_ops = {
> + .get_address_space = sabre_pci_dma_iommu,
> +};
> +
> static void sabre_config_write(void *opaque, hwaddr addr,
> uint64_t val, unsigned size)
> {
> @@ -384,7 +388,7 @@ static void sabre_realize(DeviceState *dev, Error **errp)
> /* IOMMU */
> memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
> sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1);
> - pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
> + pci_setup_iommu(phb->bus, &sabre_iommu_ops, s->iommu);
>
> /* APB secondary busses */
> pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), TYPE_SIMBA_PCI_BRIDGE);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b0d21bf43a9da8e624507dad868f0aa775fb2ad6..553a91d53c24249bb28e336bce7ee3790ddbfbc9 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2685,7 +2685,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> PCIBus *iommu_bus = bus;
> uint8_t devfn = dev->devfn;
>
> - while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) {
> + while (iommu_bus && !iommu_bus->iommu_ops && iommu_bus->parent_dev) {
> PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
>
> /*
> @@ -2724,15 +2724,23 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>
> iommu_bus = parent_bus;
> }
> - if (!pci_bus_bypass_iommu(bus) && iommu_bus && iommu_bus->iommu_fn) {
> - return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn);
> + if (!pci_bus_bypass_iommu(bus) && iommu_bus->iommu_ops) {
> + return iommu_bus->iommu_ops->get_address_space(bus,
> + iommu_bus->iommu_opaque, devfn);
> }
> return &address_space_memory;
> }
>
> -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
> +void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
> {
> - bus->iommu_fn = fn;
> + /*
> + * If called, pci_setup_iommu() should provide a minimum set of
> + * useful callbacks for the bus.
> + */
> + assert(ops);
> + assert(ops->get_address_space);
> +
> + bus->iommu_ops = ops;
> bus->iommu_opaque = opaque;
> }
>
> diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
> index 672090de9478e9c8e9ba8151b72bcaf93495d223..df4ee374d04bea092c4f81098f82514c12c0519c 100644
> --- a/hw/ppc/ppc440_pcix.c
> +++ b/hw/ppc/ppc440_pcix.c
> @@ -449,6 +449,10 @@ static AddressSpace *ppc440_pcix_set_iommu(PCIBus *b, void *opaque, int devfn)
> return &s->bm_as;
> }
>
> +static const PCIIOMMUOps ppc440_iommu_ops = {
> + .get_address_space = ppc440_pcix_set_iommu,
> +};
> +
> /*
> * Some guests on sam460ex write all kinds of garbage here such as
> * missing enable bit and low bits set and still expect this to work
> @@ -503,7 +507,7 @@ static void ppc440_pcix_realize(DeviceState *dev, Error **errp)
> memory_region_init(&s->bm, OBJECT(s), "bm-ppc440-pcix", UINT64_MAX);
> memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
> address_space_init(&s->bm_as, &s->bm, "pci-bm");
> - pci_setup_iommu(h->bus, ppc440_pcix_set_iommu, s);
> + pci_setup_iommu(h->bus, &ppc440_iommu_ops, s);
>
> memory_region_init(&s->container, OBJECT(s), "pci-container", PCI_ALL_SIZE);
> memory_region_init_io(&h->conf_mem, OBJECT(s), &ppc440_pcix_host_conf_ops,
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 370c5a90f2184bbe75de9a3b950096960c67189e..a27024e45a4188c5b21e7d41ecf3d337bbbb47f0 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -780,6 +780,10 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> return &phb->iommu_as;
> }
>
> +static const PCIIOMMUOps spapr_iommu_ops = {
> + .get_address_space = spapr_pci_dma_iommu,
> +};
> +
> static char *spapr_phb_vfio_get_loc_code(SpaprPhbState *sphb, PCIDevice *pdev)
> {
> g_autofree char *path = NULL;
> @@ -1978,7 +1982,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> memory_region_add_subregion(&sphb->iommu_root, SPAPR_PCI_MSI_WINDOW,
> &sphb->msiwindow);
>
> - pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
> + pci_setup_iommu(bus, &spapr_iommu_ops, sphb);
>
> pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
>
> diff --git a/hw/remote/iommu.c b/hw/remote/iommu.c
> index 1391dd712ceda0bb5b524651362350471f31f93a..7c56aad0fc14a3a411303dcf3baacef5edfd3d1e 100644
> --- a/hw/remote/iommu.c
> +++ b/hw/remote/iommu.c
> @@ -100,6 +100,10 @@ static void remote_iommu_finalize(Object *obj)
> iommu->elem_by_devfn = NULL;
> }
>
> +static const PCIIOMMUOps remote_iommu_ops = {
> + .get_address_space = remote_iommu_find_add_as,
> +};
> +
> void remote_iommu_setup(PCIBus *pci_bus)
> {
> RemoteIommu *iommu = NULL;
> @@ -108,7 +112,7 @@ void remote_iommu_setup(PCIBus *pci_bus)
>
> iommu = REMOTE_IOMMU(object_new(TYPE_REMOTE_IOMMU));
>
> - pci_setup_iommu(pci_bus, remote_iommu_find_add_as, iommu);
> + pci_setup_iommu(pci_bus, &remote_iommu_ops, iommu);
>
> object_property_add_child(OBJECT(pci_bus), "remote-iommu", OBJECT(iommu));
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 2ca36f9f3be11866b6c9cb83a61da01d93a4b7c0..347580ebacfe1dd832063b667aa593097ba6926d 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -652,6 +652,10 @@ static AddressSpace *s390_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> return &iommu->as;
> }
>
> +static const PCIIOMMUOps s390_iommu_ops = {
> + .get_address_space = s390_pci_dma_iommu,
> +};
> +
> static uint8_t set_ind_atomic(uint64_t ind_loc, uint8_t to_be_set)
> {
> uint8_t expected, actual;
> @@ -839,7 +843,7 @@ static void s390_pcihost_realize(DeviceState *dev, Error **errp)
> b = pci_register_root_bus(dev, NULL, s390_pci_set_irq, s390_pci_map_irq,
> NULL, get_system_memory(), get_system_io(), 0,
> 64, TYPE_PCI_BUS);
> - pci_setup_iommu(b, s390_pci_dma_iommu, s);
> + pci_setup_iommu(b, &s390_iommu_ops, s);
>
> bus = BUS(b);
> qbus_set_hotplug_handler(bus, OBJECT(dev));
> @@ -1058,7 +1062,7 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>
> pdev = PCI_DEVICE(dev);
> pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq);
> - pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s);
> + pci_setup_iommu(&pb->sec_bus, &s390_iommu_ops, s);
>
> qbus_set_hotplug_handler(BUS(&pb->sec_bus), OBJECT(s));
>
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index be51635895ce88cb69ec987a5c7d3fa33a703c9e..4c62378f210a63dcd49e596a56802101bb14f0af 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -444,6 +444,10 @@ static AddressSpace *virtio_iommu_find_add_as(PCIBus *bus, void *opaque,
> return &sdev->as;
> }
>
> +static const PCIIOMMUOps virtio_iommu_ops = {
> + .get_address_space = virtio_iommu_find_add_as,
> +};
> +
> static int virtio_iommu_attach(VirtIOIOMMU *s,
> struct virtio_iommu_req_attach *req)
> {
> @@ -1206,7 +1210,7 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
> s->as_by_busptr = g_hash_table_new_full(NULL, NULL, NULL, g_free);
>
> if (s->primary_bus) {
> - pci_setup_iommu(s->primary_bus, virtio_iommu_find_add_as, s);
> + pci_setup_iommu(s->primary_bus, &virtio_iommu_ops, s);
> } else {
> error_setg(errp, "VIRTIO-IOMMU is not attached to any PCI bus!");
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps
2023-10-19 12:33 ` Eric Auger
@ 2023-10-19 14:02 ` Cédric Le Goater
2023-10-19 14:59 ` Eric Auger
0 siblings, 1 reply; 4+ messages in thread
From: Cédric Le Goater @ 2023-10-19 14:02 UTC (permalink / raw)
To: eric.auger, qemu-devel
Cc: Joao Martins, Liu Yi L, Kevin Tian, Jacob Pan, Peter Xu, Yi Sun,
David Gibson, Michael S. Tsirkin, Peter Maydell, Paolo Bonzini,
Jason Wang, Andrey Smirnov, Helge Deller, Hervé Poussineau,
Mark Cave-Ayland, BALATON Zoltan, Elena Ufimtseva,
Jagannathan Raman, Matthew Rosato, Eric Farman, Halil Pasic,
Christian Borntraeger, Thomas Huth, Philippe Mathieu-Daudé
On 10/19/23 14:33, Eric Auger wrote:
> Hi Cédric,
>
> On 10/19/23 13:54, 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>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> [ clg: - refreshed on latest QEMU
>> - included hw/remote/iommu.c
>> - documentation update
>> - asserts in pci_setup_iommu()
>> - removed checks on iommu_bus->iommu_ops->get_address_space ]
>> 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/
>>
>> Changes in v3 :
>>
>> - New Documentation
>> - Create a pci.rst for the kernel-doc comments of
>> include/hw/pci/pci.h
>> - asserts in pci_setup_iommu()
>> - removed checks on iommu_bus->iommu_ops->get_address_space
>>
>> Changes in v2 :
>>
>> - PCIIOMMUOps definition cleanups
>>
>> docs/devel/index-api.rst | 1 +
>> docs/devel/pci.rst | 8 ++++++++
>> include/hw/pci/pci.h | 39 +++++++++++++++++++++++++++++++++++++--
>> 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 +++++-
>> 21 files changed, 141 insertions(+), 25 deletions(-)
>> create mode 100644 docs/devel/pci.rst
>>
>> diff --git a/docs/devel/index-api.rst b/docs/devel/index-api.rst
>> index 539ad29c215e803568b1d7b0ba6766c0cc9d2b8b..fe01b2b488d0b61376560011a660389603e4ce8e 100644
>> --- a/docs/devel/index-api.rst
>> +++ b/docs/devel/index-api.rst
>> @@ -11,6 +11,7 @@ generated from in-code annotations to function prototypes.
>> loads-stores
>> memory
>> modules
>> + pci
>> qom-api
>> qdev-api
>> ui
>> diff --git a/docs/devel/pci.rst b/docs/devel/pci.rst
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..68739334f3c936cc28b8fceac6dbebbfe68e78a2
>> --- /dev/null
>> +++ b/docs/devel/pci.rst
>> @@ -0,0 +1,8 @@
>> +=============
>> +PCI subsystem
>> +=============
>> +
>> +API Reference
>> +-------------
>> +
>> +.. kernel-doc:: include/hw/pci/pci.h
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index b70a0b95ff5ae367ed7f98483ec8d1d1b6274530..0a4bcd13f39fde60326a548a3cd6637ec2287475 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -366,10 +366,45 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range);
>>
>> void pci_device_deassert_intx(PCIDevice *dev);
>>
>> -typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
>> +
>> +/**
>> + * struct PCIIOMMUOps: callbacks structure for specific IOMMU handlers
>> + * of a PCIBus
>> + *
>> + * Allows to modify the behavior of some IOMMU operations of the PCI
>> + * framework for a set of devices on a PCI bus.
>> + */
>> +typedef struct PCIIOMMUOps {
>> + /**
>> + * @get_address_space: get the address space for a set of devices
>> + * on a PCI bus.
>> + *
>> + * Returns a pointer to AddressSpace, otherwise NULL.
>> + *
>> + * Optional method: if this method is not set, then the default
>> + * address_space is used.
> isn't it a mandated method? You assert on
>
> pci_setup_iommu() if it isn't set, right?
I made it mandatory in pci_setup_iommu() indeed and now, PCIIOMMUOps
can be NULL but if not NULL, the .get_address_space() handler should
be defined.
I will fix the comment.
Thanks,
C.
> Eric
>
>> + *
>> + * @bus: the #PCIBus being accessed.
>> + *
>> + * @opaque: the data passed to pci_setup_iommu().
>> + *
>> + * @devfn: device and function number
>> + */
>> + AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
>> +} PCIIOMMUOps;
>>
>> AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
>> -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
>> +
>> +/**
>> + * pci_setup_iommu: Initialize specific IOMMU handlers for a PCIBus
>> + *
>> + * Let PCI host bridges define specific operations.
>> + *
>> + * @bus: the #PCIBus being updated.
>> + * @ops: the #PCIIOMMUOps
>> + * @opaque: passed to callbacks of the @ops structure.
>> + */
>> +void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque);
>>
>> pcibus_t pci_bar_address(PCIDevice *d,
>> int reg, uint8_t type, pcibus_t size);
>> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
>> index 56531759578ffe2c6eccc92ebae4acabf47e390d..226131254621f257eefb45356489c125381e6a87 100644
>> --- a/include/hw/pci/pci_bus.h
>> +++ b/include/hw/pci/pci_bus.h
>> @@ -33,7 +33,7 @@ enum PCIBusFlags {
>> struct PCIBus {
>> BusState qbus;
>> enum PCIBusFlags flags;
>> - PCIIOMMUFunc iommu_fn;
>> + const PCIIOMMUOps *iommu_ops;
>> void *iommu_opaque;
>> uint8_t devfn_min;
>> uint32_t slot_reserved_mask;
>> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
>> index 49a80550c54d4ed9ee40f9959cbf2c83240d8e39..e8711ae16a3e16c733405034be5e65688ef4d3df 100644
>> --- a/hw/alpha/typhoon.c
>> +++ b/hw/alpha/typhoon.c
>> @@ -738,6 +738,10 @@ static AddressSpace *typhoon_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> return &s->pchip.iommu_as;
>> }
>>
>> +static const PCIIOMMUOps typhoon_iommu_ops = {
>> + .get_address_space = typhoon_pci_dma_iommu,
>> +};
>> +
>> static void typhoon_set_irq(void *opaque, int irq, int level)
>> {
>> TyphoonState *s = opaque;
>> @@ -897,7 +901,7 @@ PCIBus *typhoon_init(MemoryRegion *ram, qemu_irq *p_isa_irq,
>> "iommu-typhoon", UINT64_MAX);
>> address_space_init(&s->pchip.iommu_as, MEMORY_REGION(&s->pchip.iommu),
>> "pchip0-pci");
>> - pci_setup_iommu(b, typhoon_pci_dma_iommu, s);
>> + pci_setup_iommu(b, &typhoon_iommu_ops, s);
>>
>> /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB. */
>> memory_region_init_io(&s->pchip.reg_iack, OBJECT(s), &alpha_pci_iack_ops,
>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>> index f35ae9aa22cb1a62cc16c2534cb3c290862b0799..9a8ac45431abb80fc5e9f60104248cb356509088 100644
>> --- a/hw/arm/smmu-common.c
>> +++ b/hw/arm/smmu-common.c
>> @@ -605,6 +605,10 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
>> return &sdev->as;
>> }
>>
>> +static const PCIIOMMUOps smmu_ops = {
>> + .get_address_space = smmu_find_add_as,
>> +};
>> +
>> IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid)
>> {
>> uint8_t bus_n, devfn;
>> @@ -661,7 +665,7 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
>> s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
>>
>> if (s->primary_bus) {
>> - pci_setup_iommu(s->primary_bus, smmu_find_add_as, s);
>> + pci_setup_iommu(s->primary_bus, &smmu_ops, s);
>> } else {
>> error_setg(errp, "SMMU is not attached to any PCI bus!");
>> }
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 8d0f2f99ddbf49c3df834b6d40d2b23cafbb81ea..94f0f8a7f22130e5684926a8790182376723e58e 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -1450,6 +1450,10 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> return &iommu_as[devfn]->as;
>> }
>>
>> +static const PCIIOMMUOps amdvi_iommu_ops = {
>> + .get_address_space = amdvi_host_dma_iommu,
>> +};
>> +
>> static const MemoryRegionOps mmio_mem_ops = {
>> .read = amdvi_mmio_read,
>> .write = amdvi_mmio_write,
>> @@ -1582,7 +1586,7 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>>
>> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
>> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
>> - pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
>> + pci_setup_iommu(bus, &amdvi_iommu_ops, s);
>> amdvi_init(s);
>> }
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 2c832ab68b3774dbec6eee4ffe7c13e7a1bf7bcc..87b07a390482160e6db630a2c2c4f6f522e0dc72 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -4034,6 +4034,10 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> return &vtd_as->as;
>> }
>>
>> +static PCIIOMMUOps vtd_iommu_ops = {
>> + .get_address_space = vtd_host_dma_iommu,
>> +};
>> +
>> static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>> {
>> X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>> @@ -4157,7 +4161,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>> g_free, g_free);
>> vtd_init(s);
>> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
>> - pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
>> + pci_setup_iommu(bus, &vtd_iommu_ops, dev);
>> /* Pseudo address space under root PCI bus. */
>> x86ms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
>> qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);
>> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
>> index 6f5442f108874f6217fa122ac08c1ce27ad20cb9..f477f97847d753341f341281ed2abed2ce242574 100644
>> --- a/hw/pci-host/designware.c
>> +++ b/hw/pci-host/designware.c
>> @@ -663,6 +663,10 @@ static AddressSpace *designware_pcie_host_set_iommu(PCIBus *bus, void *opaque,
>> return &s->pci.address_space;
>> }
>>
>> +static const PCIIOMMUOps designware_iommu_ops = {
>> + .get_address_space = designware_pcie_host_set_iommu,
>> +};
>> +
>> static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
>> {
>> PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>> @@ -705,7 +709,7 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
>> address_space_init(&s->pci.address_space,
>> &s->pci.address_space_root,
>> "pcie-bus-address-space");
>> - pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s);
>> + pci_setup_iommu(pci->bus, &designware_iommu_ops, s);
>>
>> qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
>> }
>> diff --git a/hw/pci-host/dino.c b/hw/pci-host/dino.c
>> index 82503229faf8a44b3974b2256b641ce18a77692b..5b0947a16c9ec4abf8668bc4563b1cc181f154f3 100644
>> --- a/hw/pci-host/dino.c
>> +++ b/hw/pci-host/dino.c
>> @@ -354,6 +354,10 @@ static AddressSpace *dino_pcihost_set_iommu(PCIBus *bus, void *opaque,
>> return &s->bm_as;
>> }
>>
>> +static const PCIIOMMUOps dino_iommu_ops = {
>> + .get_address_space = dino_pcihost_set_iommu,
>> +};
>> +
>> /*
>> * Dino interrupts are connected as shown on Page 78, Table 23
>> * (Little-endian bit numbers)
>> @@ -481,7 +485,7 @@ static void dino_pcihost_init(Object *obj)
>> g_free(name);
>> }
>>
>> - pci_setup_iommu(phb->bus, dino_pcihost_set_iommu, s);
>> + pci_setup_iommu(phb->bus, &dino_iommu_ops, s);
>>
>> sysbus_init_mmio(sbd, &s->this_mem);
>>
>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>> index c5e58f4086aef34fa0b5307c10b261c0c6e3ac34..2a74dbe45f59b207b57b3b8af8856ac5cf04d114 100644
>> --- a/hw/pci-host/pnv_phb3.c
>> +++ b/hw/pci-host/pnv_phb3.c
>> @@ -968,6 +968,10 @@ static AddressSpace *pnv_phb3_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> return &ds->dma_as;
>> }
>>
>> +static PCIIOMMUOps pnv_phb3_iommu_ops = {
>> + .get_address_space = pnv_phb3_dma_iommu,
>> +};
>> +
>> static void pnv_phb3_instance_init(Object *obj)
>> {
>> PnvPHB3 *phb = PNV_PHB3(obj);
>> @@ -1012,7 +1016,7 @@ void pnv_phb3_bus_init(DeviceState *dev, PnvPHB3 *phb)
>> object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
>> &error_abort);
>>
>> - pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
>> + pci_setup_iommu(pci->bus, &pnv_phb3_iommu_ops, phb);
>> }
>>
>> static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index 29cb11a5d94c6a72a8111dd95346be906a7bc4fe..37c7afc18c9b17b25df8356907f0d87d7d621ac2 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1518,6 +1518,10 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>> &phb->phb_regs_mr);
>> }
>>
>> +static PCIIOMMUOps pnv_phb4_iommu_ops = {
>> + .get_address_space = pnv_phb4_dma_iommu,
>> +};
>> +
>> static void pnv_phb4_instance_init(Object *obj)
>> {
>> PnvPHB4 *phb = PNV_PHB4(obj);
>> @@ -1557,7 +1561,7 @@ void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb)
>> object_property_set_int(OBJECT(pci->bus), "chip-id", phb->chip_id,
>> &error_abort);
>>
>> - pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
>> + pci_setup_iommu(pci->bus, &pnv_phb4_iommu_ops, phb);
>> pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>> }
>>
>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
>> index 38814247f2a5b7c1a6b55e18673f3fe8c348fbee..453a4e6ed3b02ed448c3fad71bd45eb73a5dbbb3 100644
>> --- a/hw/pci-host/ppce500.c
>> +++ b/hw/pci-host/ppce500.c
>> @@ -435,6 +435,10 @@ static AddressSpace *e500_pcihost_set_iommu(PCIBus *bus, void *opaque,
>> return &s->bm_as;
>> }
>>
>> +static const PCIIOMMUOps ppce500_iommu_ops = {
>> + .get_address_space = e500_pcihost_set_iommu,
>> +};
>> +
>> static void e500_pcihost_realize(DeviceState *dev, Error **errp)
>> {
>> SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> @@ -469,7 +473,7 @@ static void e500_pcihost_realize(DeviceState *dev, Error **errp)
>> memory_region_init(&s->bm, OBJECT(s), "bm-e500", UINT64_MAX);
>> memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
>> address_space_init(&s->bm_as, &s->bm, "pci-bm");
>> - pci_setup_iommu(b, e500_pcihost_set_iommu, s);
>> + pci_setup_iommu(b, &ppce500_iommu_ops, s);
>>
>> pci_create_simple(b, 0, "e500-host-bridge");
>>
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index 9a11ac4b2b610646790ed2016f73a16bce4fa42f..86c3a4908712988637484967e88155afb1413742 100644
>> --- a/hw/pci-host/raven.c
>> +++ b/hw/pci-host/raven.c
>> @@ -223,6 +223,10 @@ static AddressSpace *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
>> return &s->bm_as;
>> }
>>
>> +static const PCIIOMMUOps raven_iommu_ops = {
>> + .get_address_space = raven_pcihost_set_iommu,
>> +};
>> +
>> static void raven_change_gpio(void *opaque, int n, int level)
>> {
>> PREPPCIState *s = opaque;
>> @@ -320,7 +324,7 @@ static void raven_pcihost_initfn(Object *obj)
>> memory_region_add_subregion(&s->bm, 0 , &s->bm_pci_memory_alias);
>> memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
>> address_space_init(&s->bm_as, &s->bm, "raven-bm");
>> - pci_setup_iommu(&s->pci_bus, raven_pcihost_set_iommu, s);
>> + pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
>>
>> h->bus = &s->pci_bus;
>>
>> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
>> index dcb2e230b67274d4b87c86eb884e052c26031c27..d0851b48b02237d41f64f6cce6f0aa4740ca0d87 100644
>> --- a/hw/pci-host/sabre.c
>> +++ b/hw/pci-host/sabre.c
>> @@ -112,6 +112,10 @@ static AddressSpace *sabre_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> return &is->iommu_as;
>> }
>>
>> +static const PCIIOMMUOps sabre_iommu_ops = {
>> + .get_address_space = sabre_pci_dma_iommu,
>> +};
>> +
>> static void sabre_config_write(void *opaque, hwaddr addr,
>> uint64_t val, unsigned size)
>> {
>> @@ -384,7 +388,7 @@ static void sabre_realize(DeviceState *dev, Error **errp)
>> /* IOMMU */
>> memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
>> sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1);
>> - pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
>> + pci_setup_iommu(phb->bus, &sabre_iommu_ops, s->iommu);
>>
>> /* APB secondary busses */
>> pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), TYPE_SIMBA_PCI_BRIDGE);
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index b0d21bf43a9da8e624507dad868f0aa775fb2ad6..553a91d53c24249bb28e336bce7ee3790ddbfbc9 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2685,7 +2685,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>> PCIBus *iommu_bus = bus;
>> uint8_t devfn = dev->devfn;
>>
>> - while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) {
>> + while (iommu_bus && !iommu_bus->iommu_ops && iommu_bus->parent_dev) {
>> PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
>>
>> /*
>> @@ -2724,15 +2724,23 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>>
>> iommu_bus = parent_bus;
>> }
>> - if (!pci_bus_bypass_iommu(bus) && iommu_bus && iommu_bus->iommu_fn) {
>> - return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn);
>> + if (!pci_bus_bypass_iommu(bus) && iommu_bus->iommu_ops) {
>> + return iommu_bus->iommu_ops->get_address_space(bus,
>> + iommu_bus->iommu_opaque, devfn);
>> }
>> return &address_space_memory;
>> }
>>
>> -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
>> +void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
>> {
>> - bus->iommu_fn = fn;
>> + /*
>> + * If called, pci_setup_iommu() should provide a minimum set of
>> + * useful callbacks for the bus.
>> + */
>> + assert(ops);
>> + assert(ops->get_address_space);
>> +
>> + bus->iommu_ops = ops;
>> bus->iommu_opaque = opaque;
>> }
>>
>> diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
>> index 672090de9478e9c8e9ba8151b72bcaf93495d223..df4ee374d04bea092c4f81098f82514c12c0519c 100644
>> --- a/hw/ppc/ppc440_pcix.c
>> +++ b/hw/ppc/ppc440_pcix.c
>> @@ -449,6 +449,10 @@ static AddressSpace *ppc440_pcix_set_iommu(PCIBus *b, void *opaque, int devfn)
>> return &s->bm_as;
>> }
>>
>> +static const PCIIOMMUOps ppc440_iommu_ops = {
>> + .get_address_space = ppc440_pcix_set_iommu,
>> +};
>> +
>> /*
>> * Some guests on sam460ex write all kinds of garbage here such as
>> * missing enable bit and low bits set and still expect this to work
>> @@ -503,7 +507,7 @@ static void ppc440_pcix_realize(DeviceState *dev, Error **errp)
>> memory_region_init(&s->bm, OBJECT(s), "bm-ppc440-pcix", UINT64_MAX);
>> memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
>> address_space_init(&s->bm_as, &s->bm, "pci-bm");
>> - pci_setup_iommu(h->bus, ppc440_pcix_set_iommu, s);
>> + pci_setup_iommu(h->bus, &ppc440_iommu_ops, s);
>>
>> memory_region_init(&s->container, OBJECT(s), "pci-container", PCI_ALL_SIZE);
>> memory_region_init_io(&h->conf_mem, OBJECT(s), &ppc440_pcix_host_conf_ops,
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 370c5a90f2184bbe75de9a3b950096960c67189e..a27024e45a4188c5b21e7d41ecf3d337bbbb47f0 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -780,6 +780,10 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> return &phb->iommu_as;
>> }
>>
>> +static const PCIIOMMUOps spapr_iommu_ops = {
>> + .get_address_space = spapr_pci_dma_iommu,
>> +};
>> +
>> static char *spapr_phb_vfio_get_loc_code(SpaprPhbState *sphb, PCIDevice *pdev)
>> {
>> g_autofree char *path = NULL;
>> @@ -1978,7 +1982,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>> memory_region_add_subregion(&sphb->iommu_root, SPAPR_PCI_MSI_WINDOW,
>> &sphb->msiwindow);
>>
>> - pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
>> + pci_setup_iommu(bus, &spapr_iommu_ops, sphb);
>>
>> pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
>>
>> diff --git a/hw/remote/iommu.c b/hw/remote/iommu.c
>> index 1391dd712ceda0bb5b524651362350471f31f93a..7c56aad0fc14a3a411303dcf3baacef5edfd3d1e 100644
>> --- a/hw/remote/iommu.c
>> +++ b/hw/remote/iommu.c
>> @@ -100,6 +100,10 @@ static void remote_iommu_finalize(Object *obj)
>> iommu->elem_by_devfn = NULL;
>> }
>>
>> +static const PCIIOMMUOps remote_iommu_ops = {
>> + .get_address_space = remote_iommu_find_add_as,
>> +};
>> +
>> void remote_iommu_setup(PCIBus *pci_bus)
>> {
>> RemoteIommu *iommu = NULL;
>> @@ -108,7 +112,7 @@ void remote_iommu_setup(PCIBus *pci_bus)
>>
>> iommu = REMOTE_IOMMU(object_new(TYPE_REMOTE_IOMMU));
>>
>> - pci_setup_iommu(pci_bus, remote_iommu_find_add_as, iommu);
>> + pci_setup_iommu(pci_bus, &remote_iommu_ops, iommu);
>>
>> object_property_add_child(OBJECT(pci_bus), "remote-iommu", OBJECT(iommu));
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 2ca36f9f3be11866b6c9cb83a61da01d93a4b7c0..347580ebacfe1dd832063b667aa593097ba6926d 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -652,6 +652,10 @@ static AddressSpace *s390_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> return &iommu->as;
>> }
>>
>> +static const PCIIOMMUOps s390_iommu_ops = {
>> + .get_address_space = s390_pci_dma_iommu,
>> +};
>> +
>> static uint8_t set_ind_atomic(uint64_t ind_loc, uint8_t to_be_set)
>> {
>> uint8_t expected, actual;
>> @@ -839,7 +843,7 @@ static void s390_pcihost_realize(DeviceState *dev, Error **errp)
>> b = pci_register_root_bus(dev, NULL, s390_pci_set_irq, s390_pci_map_irq,
>> NULL, get_system_memory(), get_system_io(), 0,
>> 64, TYPE_PCI_BUS);
>> - pci_setup_iommu(b, s390_pci_dma_iommu, s);
>> + pci_setup_iommu(b, &s390_iommu_ops, s);
>>
>> bus = BUS(b);
>> qbus_set_hotplug_handler(bus, OBJECT(dev));
>> @@ -1058,7 +1062,7 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>
>> pdev = PCI_DEVICE(dev);
>> pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq);
>> - pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s);
>> + pci_setup_iommu(&pb->sec_bus, &s390_iommu_ops, s);
>>
>> qbus_set_hotplug_handler(BUS(&pb->sec_bus), OBJECT(s));
>>
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index be51635895ce88cb69ec987a5c7d3fa33a703c9e..4c62378f210a63dcd49e596a56802101bb14f0af 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -444,6 +444,10 @@ static AddressSpace *virtio_iommu_find_add_as(PCIBus *bus, void *opaque,
>> return &sdev->as;
>> }
>>
>> +static const PCIIOMMUOps virtio_iommu_ops = {
>> + .get_address_space = virtio_iommu_find_add_as,
>> +};
>> +
>> static int virtio_iommu_attach(VirtIOIOMMU *s,
>> struct virtio_iommu_req_attach *req)
>> {
>> @@ -1206,7 +1210,7 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
>> s->as_by_busptr = g_hash_table_new_full(NULL, NULL, NULL, g_free);
>>
>> if (s->primary_bus) {
>> - pci_setup_iommu(s->primary_bus, virtio_iommu_find_add_as, s);
>> + pci_setup_iommu(s->primary_bus, &virtio_iommu_ops, s);
>> } else {
>> error_setg(errp, "VIRTIO-IOMMU is not attached to any PCI bus!");
>> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps
2023-10-19 14:02 ` Cédric Le Goater
@ 2023-10-19 14:59 ` Eric Auger
0 siblings, 0 replies; 4+ messages in thread
From: Eric Auger @ 2023-10-19 14:59 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Joao Martins, Liu Yi L, Kevin Tian, Jacob Pan, Peter Xu, Yi Sun,
David Gibson, Michael S. Tsirkin, Peter Maydell, Paolo Bonzini,
Jason Wang, Andrey Smirnov, Helge Deller, Hervé Poussineau,
Mark Cave-Ayland, BALATON Zoltan, Elena Ufimtseva,
Jagannathan Raman, Matthew Rosato, Eric Farman, Halil Pasic,
Christian Borntraeger, Thomas Huth, Philippe Mathieu-Daudé
Hi Cédric,
On 10/19/23 16:02, Cédric Le Goater wrote:
> On 10/19/23 14:33, Eric Auger wrote:
>> Hi Cédric,
>>
>> On 10/19/23 13:54, 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>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> [ clg: - refreshed on latest QEMU
>>> - included hw/remote/iommu.c
>>> - documentation update
>>> - asserts in pci_setup_iommu()
>>> - removed checks on iommu_bus->iommu_ops->get_address_space ]
>>> 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/
>>>
>>> Changes in v3 :
>>>
>>> - New Documentation
>>> - Create a pci.rst for the kernel-doc comments of
>>> include/hw/pci/pci.h
>>> - asserts in pci_setup_iommu()
>>> - removed checks on iommu_bus->iommu_ops->get_address_space
>>>
>>> Changes in v2 :
>>>
>>> - PCIIOMMUOps definition cleanups
>>> docs/devel/index-api.rst | 1 +
>>> docs/devel/pci.rst | 8 ++++++++
>>> include/hw/pci/pci.h | 39 +++++++++++++++++++++++++++++++++++++--
>>> 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 +++++-
>>> 21 files changed, 141 insertions(+), 25 deletions(-)
>>> create mode 100644 docs/devel/pci.rst
>>>
>>> diff --git a/docs/devel/index-api.rst b/docs/devel/index-api.rst
>>> index
>>> 539ad29c215e803568b1d7b0ba6766c0cc9d2b8b..fe01b2b488d0b61376560011a660389603e4ce8e
>>> 100644
>>> --- a/docs/devel/index-api.rst
>>> +++ b/docs/devel/index-api.rst
>>> @@ -11,6 +11,7 @@ generated from in-code annotations to function
>>> prototypes.
>>> loads-stores
>>> memory
>>> modules
>>> + pci
>>> qom-api
>>> qdev-api
>>> ui
>>> diff --git a/docs/devel/pci.rst b/docs/devel/pci.rst
>>> new file mode 100644
>>> index
>>> 0000000000000000000000000000000000000000..68739334f3c936cc28b8fceac6dbebbfe68e78a2
>>> --- /dev/null
>>> +++ b/docs/devel/pci.rst
>>> @@ -0,0 +1,8 @@
>>> +=============
>>> +PCI subsystem
>>> +=============
>>> +
>>> +API Reference
>>> +-------------
>>> +
>>> +.. kernel-doc:: include/hw/pci/pci.h
>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>> index
>>> b70a0b95ff5ae367ed7f98483ec8d1d1b6274530..0a4bcd13f39fde60326a548a3cd6637ec2287475
>>> 100644
>>> --- a/include/hw/pci/pci.h
>>> +++ b/include/hw/pci/pci.h
>>> @@ -366,10 +366,45 @@ void pci_bus_get_w64_range(PCIBus *bus, Range
>>> *range);
>>> void pci_device_deassert_intx(PCIDevice *dev);
>>> -typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
>>> +
>>> +/**
>>> + * struct PCIIOMMUOps: callbacks structure for specific IOMMU handlers
>>> + * of a PCIBus
>>> + *
>>> + * Allows to modify the behavior of some IOMMU operations of the PCI
>>> + * framework for a set of devices on a PCI bus.
>>> + */
>>> +typedef struct PCIIOMMUOps {
>>> + /**
>>> + * @get_address_space: get the address space for a set of devices
>>> + * on a PCI bus.
>>> + *
>>> + * Returns a pointer to AddressSpace, otherwise NULL.
>>> + *
>>> + * Optional method: if this method is not set, then the default
>>> + * address_space is used.
>> isn't it a mandated method? You assert on
>>
>> pci_setup_iommu() if it isn't set, right?
>
> I made it mandatory in pci_setup_iommu() indeed and now, PCIIOMMUOps
> can be NULL but if not NULL, the .get_address_space() handler should
> be defined.
>
> I will fix the comment.
OK. with that fixed, feel free to add my
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
>
> Thanks,
>
> C.
>
>
>> Eric
>>
>>> + *
>>> + * @bus: the #PCIBus being accessed.
>>> + *
>>> + * @opaque: the data passed to pci_setup_iommu().
>>> + *
>>> + * @devfn: device and function number
>>> + */
>>> + AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque,
>>> int devfn);
>>> +} PCIIOMMUOps;
>>> AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
>>> -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
>>> +
>>> +/**
>>> + * pci_setup_iommu: Initialize specific IOMMU handlers for a PCIBus
>>> + *
>>> + * Let PCI host bridges define specific operations.
>>> + *
>>> + * @bus: the #PCIBus being updated.
>>> + * @ops: the #PCIIOMMUOps
>>> + * @opaque: passed to callbacks of the @ops structure.
>>> + */
>>> +void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void
>>> *opaque);
>>> pcibus_t pci_bar_address(PCIDevice *d,
>>> int reg, uint8_t type, pcibus_t size);
>>> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
>>> index
>>> 56531759578ffe2c6eccc92ebae4acabf47e390d..226131254621f257eefb45356489c125381e6a87
>>> 100644
>>> --- a/include/hw/pci/pci_bus.h
>>> +++ b/include/hw/pci/pci_bus.h
>>> @@ -33,7 +33,7 @@ enum PCIBusFlags {
>>> struct PCIBus {
>>> BusState qbus;
>>> enum PCIBusFlags flags;
>>> - PCIIOMMUFunc iommu_fn;
>>> + const PCIIOMMUOps *iommu_ops;
>>> void *iommu_opaque;
>>> uint8_t devfn_min;
>>> uint32_t slot_reserved_mask;
>>> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
>>> index
>>> 49a80550c54d4ed9ee40f9959cbf2c83240d8e39..e8711ae16a3e16c733405034be5e65688ef4d3df
>>> 100644
>>> --- a/hw/alpha/typhoon.c
>>> +++ b/hw/alpha/typhoon.c
>>> @@ -738,6 +738,10 @@ static AddressSpace
>>> *typhoon_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>> return &s->pchip.iommu_as;
>>> }
>>> +static const PCIIOMMUOps typhoon_iommu_ops = {
>>> + .get_address_space = typhoon_pci_dma_iommu,
>>> +};
>>> +
>>> static void typhoon_set_irq(void *opaque, int irq, int level)
>>> {
>>> TyphoonState *s = opaque;
>>> @@ -897,7 +901,7 @@ PCIBus *typhoon_init(MemoryRegion *ram, qemu_irq
>>> *p_isa_irq,
>>> "iommu-typhoon", UINT64_MAX);
>>> address_space_init(&s->pchip.iommu_as,
>>> MEMORY_REGION(&s->pchip.iommu),
>>> "pchip0-pci");
>>> - pci_setup_iommu(b, typhoon_pci_dma_iommu, s);
>>> + pci_setup_iommu(b, &typhoon_iommu_ops, s);
>>> /* Pchip0 PCI special/interrupt acknowledge,
>>> 0x801.F800.0000, 64MB. */
>>> memory_region_init_io(&s->pchip.reg_iack, OBJECT(s),
>>> &alpha_pci_iack_ops,
>>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>>> index
>>> f35ae9aa22cb1a62cc16c2534cb3c290862b0799..9a8ac45431abb80fc5e9f60104248cb356509088
>>> 100644
>>> --- a/hw/arm/smmu-common.c
>>> +++ b/hw/arm/smmu-common.c
>>> @@ -605,6 +605,10 @@ static AddressSpace *smmu_find_add_as(PCIBus
>>> *bus, void *opaque, int devfn)
>>> return &sdev->as;
>>> }
>>> +static const PCIIOMMUOps smmu_ops = {
>>> + .get_address_space = smmu_find_add_as,
>>> +};
>>> +
>>> IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid)
>>> {
>>> uint8_t bus_n, devfn;
>>> @@ -661,7 +665,7 @@ static void smmu_base_realize(DeviceState *dev,
>>> Error **errp)
>>> s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
>>> if (s->primary_bus) {
>>> - pci_setup_iommu(s->primary_bus, smmu_find_add_as, s);
>>> + pci_setup_iommu(s->primary_bus, &smmu_ops, s);
>>> } else {
>>> error_setg(errp, "SMMU is not attached to any PCI bus!");
>>> }
>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>> index
>>> 8d0f2f99ddbf49c3df834b6d40d2b23cafbb81ea..94f0f8a7f22130e5684926a8790182376723e58e
>>> 100644
>>> --- a/hw/i386/amd_iommu.c
>>> +++ b/hw/i386/amd_iommu.c
>>> @@ -1450,6 +1450,10 @@ static AddressSpace
>>> *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>> return &iommu_as[devfn]->as;
>>> }
>>> +static const PCIIOMMUOps amdvi_iommu_ops = {
>>> + .get_address_space = amdvi_host_dma_iommu,
>>> +};
>>> +
>>> static const MemoryRegionOps mmio_mem_ops = {
>>> .read = amdvi_mmio_read,
>>> .write = amdvi_mmio_write,
>>> @@ -1582,7 +1586,7 @@ static void amdvi_sysbus_realize(DeviceState
>>> *dev, Error **errp)
>>> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
>>> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
>>> - pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
>>> + pci_setup_iommu(bus, &amdvi_iommu_ops, s);
>>> amdvi_init(s);
>>> }
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index
>>> 2c832ab68b3774dbec6eee4ffe7c13e7a1bf7bcc..87b07a390482160e6db630a2c2c4f6f522e0dc72
>>> 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -4034,6 +4034,10 @@ static AddressSpace
>>> *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>> return &vtd_as->as;
>>> }
>>> +static PCIIOMMUOps vtd_iommu_ops = {
>>> + .get_address_space = vtd_host_dma_iommu,
>>> +};
>>> +
>>> static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>>> {
>>> X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>>> @@ -4157,7 +4161,7 @@ static void vtd_realize(DeviceState *dev,
>>> Error **errp)
>>> g_free, g_free);
>>> vtd_init(s);
>>> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0,
>>> Q35_HOST_BRIDGE_IOMMU_ADDR);
>>> - pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
>>> + pci_setup_iommu(bus, &vtd_iommu_ops, dev);
>>> /* Pseudo address space under root PCI bus. */
>>> x86ms->ioapic_as = vtd_host_dma_iommu(bus, s,
>>> Q35_PSEUDO_DEVFN_IOAPIC);
>>> qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);
>>> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
>>> index
>>> 6f5442f108874f6217fa122ac08c1ce27ad20cb9..f477f97847d753341f341281ed2abed2ce242574
>>> 100644
>>> --- a/hw/pci-host/designware.c
>>> +++ b/hw/pci-host/designware.c
>>> @@ -663,6 +663,10 @@ static AddressSpace
>>> *designware_pcie_host_set_iommu(PCIBus *bus, void *opaque,
>>> return &s->pci.address_space;
>>> }
>>> +static const PCIIOMMUOps designware_iommu_ops = {
>>> + .get_address_space = designware_pcie_host_set_iommu,
>>> +};
>>> +
>>> static void designware_pcie_host_realize(DeviceState *dev, Error
>>> **errp)
>>> {
>>> PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>>> @@ -705,7 +709,7 @@ static void
>>> designware_pcie_host_realize(DeviceState *dev, Error **errp)
>>> address_space_init(&s->pci.address_space,
>>> &s->pci.address_space_root,
>>> "pcie-bus-address-space");
>>> - pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s);
>>> + pci_setup_iommu(pci->bus, &designware_iommu_ops, s);
>>> qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal);
>>> }
>>> diff --git a/hw/pci-host/dino.c b/hw/pci-host/dino.c
>>> index
>>> 82503229faf8a44b3974b2256b641ce18a77692b..5b0947a16c9ec4abf8668bc4563b1cc181f154f3
>>> 100644
>>> --- a/hw/pci-host/dino.c
>>> +++ b/hw/pci-host/dino.c
>>> @@ -354,6 +354,10 @@ static AddressSpace
>>> *dino_pcihost_set_iommu(PCIBus *bus, void *opaque,
>>> return &s->bm_as;
>>> }
>>> +static const PCIIOMMUOps dino_iommu_ops = {
>>> + .get_address_space = dino_pcihost_set_iommu,
>>> +};
>>> +
>>> /*
>>> * Dino interrupts are connected as shown on Page 78, Table 23
>>> * (Little-endian bit numbers)
>>> @@ -481,7 +485,7 @@ static void dino_pcihost_init(Object *obj)
>>> g_free(name);
>>> }
>>> - pci_setup_iommu(phb->bus, dino_pcihost_set_iommu, s);
>>> + pci_setup_iommu(phb->bus, &dino_iommu_ops, s);
>>> sysbus_init_mmio(sbd, &s->this_mem);
>>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>>> index
>>> c5e58f4086aef34fa0b5307c10b261c0c6e3ac34..2a74dbe45f59b207b57b3b8af8856ac5cf04d114
>>> 100644
>>> --- a/hw/pci-host/pnv_phb3.c
>>> +++ b/hw/pci-host/pnv_phb3.c
>>> @@ -968,6 +968,10 @@ static AddressSpace *pnv_phb3_dma_iommu(PCIBus
>>> *bus, void *opaque, int devfn)
>>> return &ds->dma_as;
>>> }
>>> +static PCIIOMMUOps pnv_phb3_iommu_ops = {
>>> + .get_address_space = pnv_phb3_dma_iommu,
>>> +};
>>> +
>>> static void pnv_phb3_instance_init(Object *obj)
>>> {
>>> PnvPHB3 *phb = PNV_PHB3(obj);
>>> @@ -1012,7 +1016,7 @@ void pnv_phb3_bus_init(DeviceState *dev,
>>> PnvPHB3 *phb)
>>> object_property_set_int(OBJECT(pci->bus), "chip-id",
>>> phb->chip_id,
>>> &error_abort);
>>> - pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
>>> + pci_setup_iommu(pci->bus, &pnv_phb3_iommu_ops, phb);
>>> }
>>> static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>>> index
>>> 29cb11a5d94c6a72a8111dd95346be906a7bc4fe..37c7afc18c9b17b25df8356907f0d87d7d621ac2
>>> 100644
>>> --- a/hw/pci-host/pnv_phb4.c
>>> +++ b/hw/pci-host/pnv_phb4.c
>>> @@ -1518,6 +1518,10 @@ static void pnv_phb4_xscom_realize(PnvPHB4 *phb)
>>> &phb->phb_regs_mr);
>>> }
>>> +static PCIIOMMUOps pnv_phb4_iommu_ops = {
>>> + .get_address_space = pnv_phb4_dma_iommu,
>>> +};
>>> +
>>> static void pnv_phb4_instance_init(Object *obj)
>>> {
>>> PnvPHB4 *phb = PNV_PHB4(obj);
>>> @@ -1557,7 +1561,7 @@ void pnv_phb4_bus_init(DeviceState *dev,
>>> PnvPHB4 *phb)
>>> object_property_set_int(OBJECT(pci->bus), "chip-id",
>>> phb->chip_id,
>>> &error_abort);
>>> - pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
>>> + pci_setup_iommu(pci->bus, &pnv_phb4_iommu_ops, phb);
>>> pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>>> }
>>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
>>> index
>>> 38814247f2a5b7c1a6b55e18673f3fe8c348fbee..453a4e6ed3b02ed448c3fad71bd45eb73a5dbbb3
>>> 100644
>>> --- a/hw/pci-host/ppce500.c
>>> +++ b/hw/pci-host/ppce500.c
>>> @@ -435,6 +435,10 @@ static AddressSpace
>>> *e500_pcihost_set_iommu(PCIBus *bus, void *opaque,
>>> return &s->bm_as;
>>> }
>>> +static const PCIIOMMUOps ppce500_iommu_ops = {
>>> + .get_address_space = e500_pcihost_set_iommu,
>>> +};
>>> +
>>> static void e500_pcihost_realize(DeviceState *dev, Error **errp)
>>> {
>>> SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>> @@ -469,7 +473,7 @@ static void e500_pcihost_realize(DeviceState
>>> *dev, Error **errp)
>>> memory_region_init(&s->bm, OBJECT(s), "bm-e500", UINT64_MAX);
>>> memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
>>> address_space_init(&s->bm_as, &s->bm, "pci-bm");
>>> - pci_setup_iommu(b, e500_pcihost_set_iommu, s);
>>> + pci_setup_iommu(b, &ppce500_iommu_ops, s);
>>> pci_create_simple(b, 0, "e500-host-bridge");
>>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>>> index
>>> 9a11ac4b2b610646790ed2016f73a16bce4fa42f..86c3a4908712988637484967e88155afb1413742
>>> 100644
>>> --- a/hw/pci-host/raven.c
>>> +++ b/hw/pci-host/raven.c
>>> @@ -223,6 +223,10 @@ static AddressSpace
>>> *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
>>> return &s->bm_as;
>>> }
>>> +static const PCIIOMMUOps raven_iommu_ops = {
>>> + .get_address_space = raven_pcihost_set_iommu,
>>> +};
>>> +
>>> static void raven_change_gpio(void *opaque, int n, int level)
>>> {
>>> PREPPCIState *s = opaque;
>>> @@ -320,7 +324,7 @@ static void raven_pcihost_initfn(Object *obj)
>>> memory_region_add_subregion(&s->bm, 0 ,
>>> &s->bm_pci_memory_alias);
>>> memory_region_add_subregion(&s->bm, 0x80000000,
>>> &s->bm_ram_alias);
>>> address_space_init(&s->bm_as, &s->bm, "raven-bm");
>>> - pci_setup_iommu(&s->pci_bus, raven_pcihost_set_iommu, s);
>>> + pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
>>> h->bus = &s->pci_bus;
>>> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
>>> index
>>> dcb2e230b67274d4b87c86eb884e052c26031c27..d0851b48b02237d41f64f6cce6f0aa4740ca0d87
>>> 100644
>>> --- a/hw/pci-host/sabre.c
>>> +++ b/hw/pci-host/sabre.c
>>> @@ -112,6 +112,10 @@ static AddressSpace *sabre_pci_dma_iommu(PCIBus
>>> *bus, void *opaque, int devfn)
>>> return &is->iommu_as;
>>> }
>>> +static const PCIIOMMUOps sabre_iommu_ops = {
>>> + .get_address_space = sabre_pci_dma_iommu,
>>> +};
>>> +
>>> static void sabre_config_write(void *opaque, hwaddr addr,
>>> uint64_t val, unsigned size)
>>> {
>>> @@ -384,7 +388,7 @@ static void sabre_realize(DeviceState *dev,
>>> Error **errp)
>>> /* IOMMU */
>>> memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
>>>
>>> sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1);
>>> - pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
>>> + pci_setup_iommu(phb->bus, &sabre_iommu_ops, s->iommu);
>>> /* APB secondary busses */
>>> pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0),
>>> TYPE_SIMBA_PCI_BRIDGE);
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index
>>> b0d21bf43a9da8e624507dad868f0aa775fb2ad6..553a91d53c24249bb28e336bce7ee3790ddbfbc9
>>> 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -2685,7 +2685,7 @@ AddressSpace
>>> *pci_device_iommu_address_space(PCIDevice *dev)
>>> PCIBus *iommu_bus = bus;
>>> uint8_t devfn = dev->devfn;
>>> - while (iommu_bus && !iommu_bus->iommu_fn &&
>>> iommu_bus->parent_dev) {
>>> + while (iommu_bus && !iommu_bus->iommu_ops &&
>>> iommu_bus->parent_dev) {
>>> PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
>>> /*
>>> @@ -2724,15 +2724,23 @@ AddressSpace
>>> *pci_device_iommu_address_space(PCIDevice *dev)
>>> iommu_bus = parent_bus;
>>> }
>>> - if (!pci_bus_bypass_iommu(bus) && iommu_bus &&
>>> iommu_bus->iommu_fn) {
>>> - return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque,
>>> devfn);
>>> + if (!pci_bus_bypass_iommu(bus) && iommu_bus->iommu_ops) {
>>> + return iommu_bus->iommu_ops->get_address_space(bus,
>>> + iommu_bus->iommu_opaque, devfn);
>>> }
>>> return &address_space_memory;
>>> }
>>> -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
>>> +void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void
>>> *opaque)
>>> {
>>> - bus->iommu_fn = fn;
>>> + /*
>>> + * If called, pci_setup_iommu() should provide a minimum set of
>>> + * useful callbacks for the bus.
>>> + */
>>> + assert(ops);
>>> + assert(ops->get_address_space);
>>> +
>>> + bus->iommu_ops = ops;
>>> bus->iommu_opaque = opaque;
>>> }
>>> diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
>>> index
>>> 672090de9478e9c8e9ba8151b72bcaf93495d223..df4ee374d04bea092c4f81098f82514c12c0519c
>>> 100644
>>> --- a/hw/ppc/ppc440_pcix.c
>>> +++ b/hw/ppc/ppc440_pcix.c
>>> @@ -449,6 +449,10 @@ static AddressSpace
>>> *ppc440_pcix_set_iommu(PCIBus *b, void *opaque, int devfn)
>>> return &s->bm_as;
>>> }
>>> +static const PCIIOMMUOps ppc440_iommu_ops = {
>>> + .get_address_space = ppc440_pcix_set_iommu,
>>> +};
>>> +
>>> /*
>>> * Some guests on sam460ex write all kinds of garbage here such as
>>> * missing enable bit and low bits set and still expect this to work
>>> @@ -503,7 +507,7 @@ static void ppc440_pcix_realize(DeviceState
>>> *dev, Error **errp)
>>> memory_region_init(&s->bm, OBJECT(s), "bm-ppc440-pcix",
>>> UINT64_MAX);
>>> memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
>>> address_space_init(&s->bm_as, &s->bm, "pci-bm");
>>> - pci_setup_iommu(h->bus, ppc440_pcix_set_iommu, s);
>>> + pci_setup_iommu(h->bus, &ppc440_iommu_ops, s);
>>> memory_region_init(&s->container, OBJECT(s),
>>> "pci-container", PCI_ALL_SIZE);
>>> memory_region_init_io(&h->conf_mem, OBJECT(s),
>>> &ppc440_pcix_host_conf_ops,
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index
>>> 370c5a90f2184bbe75de9a3b950096960c67189e..a27024e45a4188c5b21e7d41ecf3d337bbbb47f0
>>> 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -780,6 +780,10 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus
>>> *bus, void *opaque, int devfn)
>>> return &phb->iommu_as;
>>> }
>>> +static const PCIIOMMUOps spapr_iommu_ops = {
>>> + .get_address_space = spapr_pci_dma_iommu,
>>> +};
>>> +
>>> static char *spapr_phb_vfio_get_loc_code(SpaprPhbState *sphb,
>>> PCIDevice *pdev)
>>> {
>>> g_autofree char *path = NULL;
>>> @@ -1978,7 +1982,7 @@ static void spapr_phb_realize(DeviceState
>>> *dev, Error **errp)
>>> memory_region_add_subregion(&sphb->iommu_root,
>>> SPAPR_PCI_MSI_WINDOW,
>>> &sphb->msiwindow);
>>> - pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
>>> + pci_setup_iommu(bus, &spapr_iommu_ops, sphb);
>>> pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
>>> diff --git a/hw/remote/iommu.c b/hw/remote/iommu.c
>>> index
>>> 1391dd712ceda0bb5b524651362350471f31f93a..7c56aad0fc14a3a411303dcf3baacef5edfd3d1e
>>> 100644
>>> --- a/hw/remote/iommu.c
>>> +++ b/hw/remote/iommu.c
>>> @@ -100,6 +100,10 @@ static void remote_iommu_finalize(Object *obj)
>>> iommu->elem_by_devfn = NULL;
>>> }
>>> +static const PCIIOMMUOps remote_iommu_ops = {
>>> + .get_address_space = remote_iommu_find_add_as,
>>> +};
>>> +
>>> void remote_iommu_setup(PCIBus *pci_bus)
>>> {
>>> RemoteIommu *iommu = NULL;
>>> @@ -108,7 +112,7 @@ void remote_iommu_setup(PCIBus *pci_bus)
>>> iommu = REMOTE_IOMMU(object_new(TYPE_REMOTE_IOMMU));
>>> - pci_setup_iommu(pci_bus, remote_iommu_find_add_as, iommu);
>>> + pci_setup_iommu(pci_bus, &remote_iommu_ops, iommu);
>>> object_property_add_child(OBJECT(pci_bus), "remote-iommu",
>>> OBJECT(iommu));
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index
>>> 2ca36f9f3be11866b6c9cb83a61da01d93a4b7c0..347580ebacfe1dd832063b667aa593097ba6926d
>>> 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -652,6 +652,10 @@ static AddressSpace *s390_pci_dma_iommu(PCIBus
>>> *bus, void *opaque, int devfn)
>>> return &iommu->as;
>>> }
>>> +static const PCIIOMMUOps s390_iommu_ops = {
>>> + .get_address_space = s390_pci_dma_iommu,
>>> +};
>>> +
>>> static uint8_t set_ind_atomic(uint64_t ind_loc, uint8_t to_be_set)
>>> {
>>> uint8_t expected, actual;
>>> @@ -839,7 +843,7 @@ static void s390_pcihost_realize(DeviceState
>>> *dev, Error **errp)
>>> b = pci_register_root_bus(dev, NULL, s390_pci_set_irq,
>>> s390_pci_map_irq,
>>> NULL, get_system_memory(),
>>> get_system_io(), 0,
>>> 64, TYPE_PCI_BUS);
>>> - pci_setup_iommu(b, s390_pci_dma_iommu, s);
>>> + pci_setup_iommu(b, &s390_iommu_ops, s);
>>> bus = BUS(b);
>>> qbus_set_hotplug_handler(bus, OBJECT(dev));
>>> @@ -1058,7 +1062,7 @@ static void s390_pcihost_plug(HotplugHandler
>>> *hotplug_dev, DeviceState *dev,
>>> pdev = PCI_DEVICE(dev);
>>> pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq);
>>> - pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s);
>>> + pci_setup_iommu(&pb->sec_bus, &s390_iommu_ops, s);
>>> qbus_set_hotplug_handler(BUS(&pb->sec_bus), OBJECT(s));
>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>> index
>>> be51635895ce88cb69ec987a5c7d3fa33a703c9e..4c62378f210a63dcd49e596a56802101bb14f0af
>>> 100644
>>> --- a/hw/virtio/virtio-iommu.c
>>> +++ b/hw/virtio/virtio-iommu.c
>>> @@ -444,6 +444,10 @@ static AddressSpace
>>> *virtio_iommu_find_add_as(PCIBus *bus, void *opaque,
>>> return &sdev->as;
>>> }
>>> +static const PCIIOMMUOps virtio_iommu_ops = {
>>> + .get_address_space = virtio_iommu_find_add_as,
>>> +};
>>> +
>>> static int virtio_iommu_attach(VirtIOIOMMU *s,
>>> struct virtio_iommu_req_attach *req)
>>> {
>>> @@ -1206,7 +1210,7 @@ static void
>>> virtio_iommu_device_realize(DeviceState *dev, Error **errp)
>>> s->as_by_busptr = g_hash_table_new_full(NULL, NULL, NULL,
>>> g_free);
>>> if (s->primary_bus) {
>>> - pci_setup_iommu(s->primary_bus, virtio_iommu_find_add_as, s);
>>> + pci_setup_iommu(s->primary_bus, &virtio_iommu_ops, s);
>>> } else {
>>> error_setg(errp, "VIRTIO-IOMMU is not attached to any PCI
>>> bus!");
>>> }
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-10-19 14:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-19 11:54 [PATCH v3] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps Cédric Le Goater
2023-10-19 12:33 ` Eric Auger
2023-10-19 14:02 ` Cédric Le Goater
2023-10-19 14:59 ` Eric Auger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).