[PATCH RFC RESEND v2 0/6] Introduce IOMMU Option For PCI Root Bus

[PATCH RFC RESEND v2 0/6] Introduce IOMMU Option For PCI Root Bus

[Wang Xingang]

From: Xingang Wang <wangxingang5@huawei.com>

These patches add support for configure iommu on/off for pci root bus,
including primary bus and pxb root bus. At present, All root bus
will go through iommu when iommu is configured, which is not flexible.

So this add option to enable/disable iommu for primary bus and pxb
root bus.  When iommu is enabled for the root bus, devices attached to it
will go through iommu. When iommu is disabled for the root bus, devices
will not go through iommu accordingly.

The option example for iommu configuration is like the following:

primary root bus option:
arm: -machine virt iommu=smmuv3,primary_bus_iommu=false(or true)
x86: -machine q35,primary_bus_iommu=false(or true)

pxb root bus:
 -device pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,addr=0x3.0x1,iommu=false 

History:

v1 -> v2:
- rebase on top of v6.0.0-rc0
- Fix some issues
- Took into account Eric's comments, and remove the PCI_BUS_IOMMU flag,
  replace it with a property in PCIHostState.
- Add support for x86 iommu option

Xingang Wang (6):
  hw/pci/pci_host: Add iommu property for pci host
  hw/pci: Add iommu option for pci root bus
  hw/pci: Add pci_root_bus_max_bus
  hw/arm/virt-acpi-build: Add explicit idmap info in IORT table
  hw/i386/acpi-build: Add explicit scope in DMAR table
  hw/i386/acpi-build: Add iommu filter in IVRS table

 hw/arm/virt-acpi-build.c            | 103 ++++++++++++++++++++++------
 hw/arm/virt.c                       |  25 +++++++
 hw/i386/acpi-build.c                |  70 ++++++++++++++++++-
 hw/i386/pc.c                        |  19 +++++
 hw/pci-bridge/pci_expander_bridge.c |   3 +
 hw/pci-host/q35.c                   |   1 +
 hw/pci/pci.c                        |  52 +++++++++++++-
 hw/pci/pci_host.c                   |   2 +
 include/hw/arm/virt.h               |   1 +
 include/hw/i386/pc.h                |   1 +
 include/hw/pci/pci.h                |   2 +
 include/hw/pci/pci_host.h           |   1 +
 12 files changed, 254 insertions(+), 26 deletions(-)

-- 
2.19.1

[PATCH RFC RESEND v2 1/6] hw/pci/pci_host: Add iommu property for pci host

[Wang Xingang]

From: Xingang Wang <wangxingang5@huawei.com>

The pci host iommu property is useful to check whether
the iommu is enabled on the pci root bus.

Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
 hw/pci/pci.c              | 18 +++++++++++++++++-
 hw/pci/pci_host.c         |  2 ++
 include/hw/pci/pci.h      |  1 +
 include/hw/pci/pci_host.h |  1 +
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index ac9a24889c..e17aa9075f 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -417,6 +417,22 @@ const char *pci_root_bus_path(PCIDevice *dev)
     return rootbus->qbus.name;
 }
 
+bool pci_root_bus_has_iommu(PCIBus *bus)
+{
+    PCIBus *rootbus = bus;
+    PCIHostState *host_bridge;
+
+    if (!pci_bus_is_root(bus)) {
+        rootbus = pci_device_root_bus(bus->parent_dev);
+    }
+
+    host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent);
+
+    assert(host_bridge->bus == rootbus);
+
+    return host_bridge->iommu;
+}
+
 static void pci_root_bus_init(PCIBus *bus, DeviceState *parent,
                               MemoryRegion *address_space_mem,
                               MemoryRegion *address_space_io,
@@ -2716,7 +2732,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
 
         iommu_bus = parent_bus;
     }
-    if (iommu_bus && iommu_bus->iommu_fn) {
+    if (pci_root_bus_has_iommu(bus) && iommu_bus && iommu_bus->iommu_fn) {
         return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn);
     }
     return &address_space_memory;
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 8ca5fadcbd..92ce213b18 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -222,6 +222,8 @@ const VMStateDescription vmstate_pcihost = {
 static Property pci_host_properties_common[] = {
     DEFINE_PROP_BOOL("x-config-reg-migration-enabled", PCIHostState,
                      mig_enabled, true),
+    DEFINE_PROP_BOOL("pci-host-iommu-enabled", PCIHostState,
+                     iommu, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 6be4e0c460..718b5a454a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -480,6 +480,7 @@ void pci_for_each_bus(PCIBus *bus,
 
 PCIBus *pci_device_root_bus(const PCIDevice *d);
 const char *pci_root_bus_path(PCIDevice *dev);
+bool pci_root_bus_has_iommu(PCIBus *bus);
 PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
 int pci_qdev_find_device(const char *id, PCIDevice **pdev);
 void pci_bus_get_w64_range(PCIBus *bus, Range *range);
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 52e038c019..64128e3a19 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -43,6 +43,7 @@ struct PCIHostState {
     uint32_t config_reg;
     bool mig_enabled;
     PCIBus *bus;
+    bool iommu;
 
     QLIST_ENTRY(PCIHostState) next;
 };
-- 
2.19.1

[PATCH RFC RESEND v2 2/6] hw/pci: Add iommu option for pci root bus

[Wang Xingang]

From: Xingang Wang <wangxingang5@huawei.com>

This add iommu option for pci root bus, including primary bus
and pxb root bus. The option is valid only if there is a virtual
iommu device.

Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
 hw/arm/virt.c                       | 25 +++++++++++++++++++++++++
 hw/i386/pc.c                        | 19 +++++++++++++++++++
 hw/pci-bridge/pci_expander_bridge.c |  3 +++
 hw/pci-host/q35.c                   |  1 +
 include/hw/arm/virt.h               |  1 +
 include/hw/i386/pc.h                |  1 +
 6 files changed, 50 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index aa2bbd14e0..446b3b867f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1366,6 +1366,7 @@ static void create_pcie(VirtMachineState *vms)
     }
 
     pci = PCI_HOST_BRIDGE(dev);
+    pci->iommu = vms->primary_bus_iommu;
     vms->bus = pci->bus;
     if (vms->bus) {
         for (i = 0; i < nb_nics; i++) {
@@ -2319,6 +2320,20 @@ static void virt_set_iommu(Object *obj, const char *value, Error **errp)
     }
 }
 
+static bool virt_get_primary_bus_iommu(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    return vms->primary_bus_iommu;
+}
+
+static void virt_set_primary_bus_iommu(Object *obj, bool value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    vms->primary_bus_iommu = value;
+}
+
 static CpuInstanceProperties
 virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
 {
@@ -2652,6 +2667,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
                                           "Set the IOMMU type. "
                                           "Valid values are none and smmuv3");
 
+    object_class_property_add_bool(oc, "primary_bus_iommu",
+                                  virt_get_primary_bus_iommu,
+                                  virt_set_primary_bus_iommu);
+    object_class_property_set_description(oc, "primary_bus_iommu",
+                                          "Set on/off to enable/disable "
+                                          "iommu for primary bus");
+
     object_class_property_add_bool(oc, "ras", virt_get_ras,
                                    virt_set_ras);
     object_class_property_set_description(oc, "ras",
@@ -2719,6 +2741,9 @@ static void virt_instance_init(Object *obj)
     /* Default disallows iommu instantiation */
     vms->iommu = VIRT_IOMMU_NONE;
 
+    /* The primary bus is attached to iommu by default */
+    vms->primary_bus_iommu = true;
+
     /* Default disallows RAS instantiation */
     vms->ras = false;
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8a84b25a03..b64e4bb7f2 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1529,6 +1529,21 @@ static void pc_machine_set_hpet(Object *obj, bool value, Error **errp)
     pcms->hpet_enabled = value;
 }
 
+static bool pc_machine_get_primary_bus_iommu(Object *obj, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+
+    return pcms->primary_bus_iommu;
+}
+
+static void pc_machine_set_primary_bus_iommu(Object *obj, bool value,
+                                             Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+
+    pcms->primary_bus_iommu = value;
+}
+
 static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
                                             const char *name, void *opaque,
                                             Error **errp)
@@ -1628,6 +1643,7 @@ static void pc_machine_initfn(Object *obj)
 #ifdef CONFIG_HPET
     pcms->hpet_enabled = true;
 #endif
+    pcms->primary_bus_iommu = true;
 
     pc_system_flash_create(pcms);
     pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
@@ -1752,6 +1768,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     object_class_property_add_bool(oc, "hpet",
         pc_machine_get_hpet, pc_machine_set_hpet);
 
+    object_class_property_add_bool(oc, "primary_bus_iommu",
+        pc_machine_get_primary_bus_iommu, pc_machine_set_primary_bus_iommu);
+
     object_class_property_add(oc, PC_MACHINE_MAX_FW_SIZE, "size",
         pc_machine_get_max_fw_size, pc_machine_set_max_fw_size,
         NULL, NULL);
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index aedded1064..f1a0eadc03 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -57,6 +57,7 @@ struct PXBDev {
 
     uint8_t bus_nr;
     uint16_t numa_node;
+    bool iommu;
 };
 
 static PXBDev *convert_to_pxb(PCIDevice *dev)
@@ -255,6 +256,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
     bus->map_irq = pxb_map_irq_fn;
 
     PCI_HOST_BRIDGE(ds)->bus = bus;
+    PCI_HOST_BRIDGE(ds)->iommu = pxb->iommu;
 
     pxb_register_bus(dev, bus, &local_err);
     if (local_err) {
@@ -301,6 +303,7 @@ static Property pxb_dev_properties[] = {
     /* Note: 0 is not a legal PXB bus number. */
     DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
     DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED),
+    DEFINE_PROP_BOOL("iommu", PXBDev, iommu, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 2eb729dff5..3b23fd0975 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -64,6 +64,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
                                 s->mch.address_space_io,
                                 0, TYPE_PCIE_BUS);
     PC_MACHINE(qdev_get_machine())->bus = pci->bus;
+    pci->iommu = PC_MACHINE(qdev_get_machine())->primary_bus_iommu;
     qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal);
 }
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 921416f918..1fbb19710f 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -147,6 +147,7 @@ struct VirtMachineState {
     OnOffAuto acpi;
     VirtGICType gic_version;
     VirtIOMMUType iommu;
+    bool primary_bus_iommu;
     VirtMSIControllerType msi_controller;
     uint16_t virtio_iommu_bdf;
     struct arm_boot_info bootinfo;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index dcf060b791..07b08b3ac6 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -45,6 +45,7 @@ typedef struct PCMachineState {
     bool sata_enabled;
     bool pit_enabled;
     bool hpet_enabled;
+    bool primary_bus_iommu;
     uint64_t max_fw_size;
 
     /* NUMA information: */
-- 
2.19.1

[PATCH RFC RESEND v2 3/6] hw/pci: Add pci_root_bus_max_bus

[Wang Xingang]

From: Xingang Wang <wangxingang5@huawei.com>

This helps to find max bus number of a root bus.

Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
 hw/pci/pci.c         | 34 ++++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h |  1 +
 2 files changed, 35 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e17aa9075f..c7957cbf7c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -538,6 +538,40 @@ int pci_bus_num(PCIBus *s)
     return PCI_BUS_GET_CLASS(s)->bus_num(s);
 }
 
+int pci_root_bus_max_bus(PCIBus *bus)
+{
+    PCIHostState *host;
+    PCIDevice *dev;
+    int max_bus = 0;
+    int type, devfn;
+    uint8_t subordinate;
+
+    if (!pci_bus_is_root(bus)) {
+        return 0;
+    }
+
+    host = PCI_HOST_BRIDGE(BUS(bus)->parent);
+    max_bus = pci_bus_num(host->bus);
+
+    for (devfn = 0; devfn < ARRAY_SIZE(host->bus->devices); devfn++) {
+        dev = host->bus->devices[devfn];
+
+        if (!dev) {
+            continue;
+        }
+
+        type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
+        if (type == PCI_HEADER_TYPE_BRIDGE) {
+            subordinate = dev->config[PCI_SUBORDINATE_BUS];
+            if (subordinate > max_bus) {
+                max_bus = subordinate;
+            }
+        }
+    }
+
+    return max_bus;
+}
+
 int pci_bus_numa_node(PCIBus *bus)
 {
     return PCI_BUS_GET_CLASS(bus)->numa_node(bus);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 718b5a454a..e0c69534f4 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -450,6 +450,7 @@ static inline PCIBus *pci_get_bus(const PCIDevice *dev)
     return PCI_BUS(qdev_get_parent_bus(DEVICE(dev)));
 }
 int pci_bus_num(PCIBus *s);
+int pci_root_bus_max_bus(PCIBus *bus);
 static inline int pci_dev_bus_num(const PCIDevice *dev)
 {
     return pci_bus_num(pci_get_bus(dev));
-- 
2.19.1

[PATCH RFC RESEND v2 4/6] hw/arm/virt-acpi-build: Add explicit idmap info in IORT table

[Wang Xingang]

From: Xingang Wang <wangxingang5@huawei.com>

The idmap of smmuv3 and root complex covers the whole RID space for now,
this patch add explicit idmap info according to root bus number range.
This add smmuv3 idmap for certain bus which has enabled the iommu property.

Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
 hw/arm/virt-acpi-build.c | 103 ++++++++++++++++++++++++++++++---------
 1 file changed, 81 insertions(+), 22 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f5a2b2d4cb..5491036c86 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -44,6 +44,7 @@
 #include "hw/acpi/tpm.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
 #include "hw/pci-host/gpex.h"
 #include "hw/arm/virt.h"
 #include "hw/mem/nvdimm.h"
@@ -237,6 +238,41 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
     aml_append(scope, dev);
 }
 
+typedef
+struct AcpiIortMapping {
+    AcpiIortIdMapping idmap;
+    bool iommu;
+} AcpiIortMapping;
+
+/* For all PCI host bridges, walk and insert DMAR scope */
+static int
+iort_host_bridges(Object *obj, void *opaque)
+{
+    GArray *map_blob = opaque;
+    AcpiIortMapping map;
+    AcpiIortIdMapping *idmap = &map.idmap;
+    int bus_num, max_bus;
+
+    if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
+        PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
+
+        if (bus) {
+            bus_num = pci_bus_num(bus);
+            max_bus = pci_root_bus_max_bus(bus);
+
+            idmap->input_base = cpu_to_le32(bus_num << 8);
+            idmap->id_count = cpu_to_le32((max_bus - bus_num + 1) << 8);
+            idmap->output_base = cpu_to_le32(bus_num << 8);
+            idmap->flags = cpu_to_le32(0);
+
+            map.iommu = pci_root_bus_has_iommu(bus);
+            g_array_append_val(map_blob, map);
+        }
+    }
+
+    return 0;
+}
+
 static void
 build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
@@ -247,6 +283,21 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     AcpiIortSmmu3 *smmu;
     size_t node_size, iort_node_offset, iort_length, smmu_offset = 0;
     AcpiIortRC *rc;
+    int smmu_mapping_count;
+    GArray *map_blob = g_array_new(false, true, sizeof(AcpiIortMapping));
+    AcpiIortMapping *map;
+
+    /* pci_for_each_bus(vms->bus, insert_map, map_blob); */
+    object_child_foreach_recursive(object_get_root(),
+                                   iort_host_bridges, map_blob);
+
+    smmu_mapping_count = 0;
+    for (int i = 0; i < map_blob->len; i++) {
+        map = &g_array_index(map_blob, AcpiIortMapping, i);
+        if (map->iommu) {
+            smmu_mapping_count++;
+        }
+    }
 
     iort = acpi_data_push(table_data, sizeof(*iort));
 
@@ -280,13 +331,13 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 
         /* SMMUv3 node */
         smmu_offset = iort_node_offset + node_size;
-        node_size = sizeof(*smmu) + sizeof(*idmap);
+        node_size = sizeof(*smmu) + sizeof(*idmap) * smmu_mapping_count;
         iort_length += node_size;
         smmu = acpi_data_push(table_data, node_size);
 
         smmu->type = ACPI_IORT_NODE_SMMU_V3;
         smmu->length = cpu_to_le16(node_size);
-        smmu->mapping_count = cpu_to_le32(1);
+        smmu->mapping_count = cpu_to_le32(smmu_mapping_count);
         smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
         smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
         smmu->flags = cpu_to_le32(ACPI_IORT_SMMU_V3_COHACC_OVERRIDE);
@@ -295,23 +346,28 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         smmu->gerr_gsiv = cpu_to_le32(irq + 2);
         smmu->sync_gsiv = cpu_to_le32(irq + 3);
 
-        /* Identity RID mapping covering the whole input RID range */
-        idmap = &smmu->id_mapping_array[0];
-        idmap->input_base = 0;
-        idmap->id_count = cpu_to_le32(0xFFFF);
-        idmap->output_base = 0;
-        /* output IORT node is the ITS group node (the first node) */
-        idmap->output_reference = cpu_to_le32(iort_node_offset);
+        for (int i = 0, j = 0; i < map_blob->len; i++) {
+            map = &g_array_index(map_blob, AcpiIortMapping, i);
+
+            if (!map->iommu) {
+                continue;
+            }
+
+            idmap = &smmu->id_mapping_array[j++];
+            *idmap = map->idmap;
+            /* output IORT node is the ITS group node (the first node) */
+            idmap->output_reference = cpu_to_le32(iort_node_offset);
+        }
     }
 
     /* Root Complex Node */
-    node_size = sizeof(*rc) + sizeof(*idmap);
+    node_size = sizeof(*rc) + sizeof(*idmap) * map_blob->len;
     iort_length += node_size;
     rc = acpi_data_push(table_data, node_size);
 
     rc->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
     rc->length = cpu_to_le16(node_size);
-    rc->mapping_count = cpu_to_le32(1);
+    rc->mapping_count = cpu_to_le32(map_blob->len);
     rc->mapping_offset = cpu_to_le32(sizeof(*rc));
 
     /* fully coherent device */
@@ -319,20 +375,23 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     rc->memory_properties.memory_flags = 0x3; /* CCA = CPM = DCAS = 1 */
     rc->pci_segment_number = 0; /* MCFG pci_segment */
 
-    /* Identity RID mapping covering the whole input RID range */
-    idmap = &rc->id_mapping_array[0];
-    idmap->input_base = 0;
-    idmap->id_count = cpu_to_le32(0xFFFF);
-    idmap->output_base = 0;
+    for (int i = 0; i < map_blob->len; i++) {
+        map = &g_array_index(map_blob, AcpiIortMapping, i);
+        idmap = &rc->id_mapping_array[i];
 
-    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
-        /* output IORT node is the smmuv3 node */
-        idmap->output_reference = cpu_to_le32(smmu_offset);
-    } else {
-        /* output IORT node is the ITS group node (the first node) */
-        idmap->output_reference = cpu_to_le32(iort_node_offset);
+        *idmap = map->idmap;
+
+        if (vms->iommu == VIRT_IOMMU_SMMUV3 && map->iommu) {
+            /* output IORT node is the smmuv3 node */
+            idmap->output_reference = cpu_to_le32(smmu_offset);
+        } else {
+            /* output IORT node is the ITS group node (the first node) */
+            idmap->output_reference = cpu_to_le32(iort_node_offset);
+        }
     }
 
+    g_array_free(map_blob, true);
+
     /*
      * Update the pointer address in case table_data->data moves during above
      * acpi_data_push operations.
-- 
2.19.1

[PATCH RFC RESEND v2 5/6] hw/i386/acpi-build: Add explicit scope in DMAR table

[Wang Xingang]

From: Xingang Wang <wangxingang5@huawei.com>

In DMAR table, the drhd is set to cover all pci devices when intel_iommu
is on. This patch add explicit scope data, including only the pci devices
that go through iommu.

Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
 hw/i386/acpi-build.c | 68 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index de98750aef..6936889cad 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1988,6 +1988,56 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
                  x86ms->oem_table_id);
 }
 
+/*
+ * Insert DMAR scope for PCI bridges and endpoint devcie
+ */
+static void
+insert_scope(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    GArray *scope_blob = opaque;
+    AcpiDmarDeviceScope *scope = NULL;
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
+        /* Dmar Scope Type: 0x02 for PCI Bridge */
+        build_append_int_noprefix(scope_blob, 0x02, 1);
+    } else {
+        /* Dmar Scope Type: 0x01 for PCI Endpoint Device */
+        build_append_int_noprefix(scope_blob, 0x01, 1);
+    }
+
+    /* length */
+    build_append_int_noprefix(scope_blob,
+                              sizeof(*scope) + sizeof(scope->path[0]), 1);
+    /* reserved */
+    build_append_int_noprefix(scope_blob, 0, 2);
+    /* enumeration_id */
+    build_append_int_noprefix(scope_blob, 0, 1);
+    /* bus */
+    build_append_int_noprefix(scope_blob, pci_bus_num(bus), 1);
+    /* device */
+    build_append_int_noprefix(scope_blob, PCI_SLOT(dev->devfn), 1);
+    /* function */
+    build_append_int_noprefix(scope_blob, PCI_FUNC(dev->devfn), 1);
+}
+
+/* For all PCI host bridges, walk and insert DMAR scope */
+static int
+dmar_host_bridges(Object *obj, void *opaque)
+{
+    GArray *scope_blob = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
+        PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
+
+        if (bus && pci_root_bus_has_iommu(bus)) {
+            pci_for_each_device(bus, pci_bus_num(bus), insert_scope,
+                                scope_blob);
+        }
+    }
+
+    return 0;
+}
+
 /*
  * VT-d spec 8.1 DMA Remapping Reporting Structure
  * (version Oct. 2014 or later)
@@ -2007,6 +2057,15 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker, const char *oem_id,
     /* Root complex IOAPIC use one path[0] only */
     size_t ioapic_scope_size = sizeof(*scope) + sizeof(scope->path[0]);
     IntelIOMMUState *intel_iommu = INTEL_IOMMU_DEVICE(iommu);
+    GArray *scope_blob = g_array_new(false, true, 1);
+
+    /*
+     * A PCI bus walk, for each PCI host bridge.
+     * Insert scope for each PCI bridge and endpoint device which
+     * is attached to a bus with iommu enabled.
+     */
+    object_child_foreach_recursive(object_get_root(),
+                                   dmar_host_bridges, scope_blob);
 
     assert(iommu);
     if (x86_iommu_ir_supported(iommu)) {
@@ -2020,8 +2079,9 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker, const char *oem_id,
     /* DMAR Remapping Hardware Unit Definition structure */
     drhd = acpi_data_push(table_data, sizeof(*drhd) + ioapic_scope_size);
     drhd->type = cpu_to_le16(ACPI_DMAR_TYPE_HARDWARE_UNIT);
-    drhd->length = cpu_to_le16(sizeof(*drhd) + ioapic_scope_size);
-    drhd->flags = ACPI_DMAR_INCLUDE_PCI_ALL;
+    drhd->length =
+        cpu_to_le16(sizeof(*drhd) + ioapic_scope_size + scope_blob->len);
+    drhd->flags = 0;            /* Don't include all pci device */
     drhd->pci_segment = cpu_to_le16(0);
     drhd->address = cpu_to_le64(Q35_HOST_BRIDGE_IOMMU_ADDR);
 
@@ -2035,6 +2095,10 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker, const char *oem_id,
     scope->path[0].device = PCI_SLOT(Q35_PSEUDO_DEVFN_IOAPIC);
     scope->path[0].function = PCI_FUNC(Q35_PSEUDO_DEVFN_IOAPIC);
 
+    /* Add scope found above */
+    g_array_append_vals(table_data, scope_blob->data, scope_blob->len);
+    g_array_free(scope_blob, true);
+
     if (iommu->dt_supported) {
         atsr = acpi_data_push(table_data, sizeof(*atsr));
         atsr->type = cpu_to_le16(ACPI_DMAR_TYPE_ATSR);
-- 
2.19.1

[PATCH RFC RESEND v2 6/6] hw/i386/acpi-build: Add iommu filter in IVRS table

[Wang Xingang]

From: Xingang Wang <wangxingang5@huawei.com>

When building amd IVRS table, only devices attached to root bus with
IOMMU flag should be scanned.

Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
 hw/i386/acpi-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6936889cad..e0f38305da 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2229,7 +2229,7 @@ ivrs_host_bridges(Object *obj, void *opaque)
     if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
         PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
 
-        if (bus) {
+        if (bus && pci_root_bus_has_iommu(bus)) {
             pci_for_each_device(bus, pci_bus_num(bus), insert_ivhd, ivhd_blob);
         }
     }
-- 
2.19.1

Re: [PATCH RFC RESEND v2 0/6] Introduce IOMMU Option For PCI Root Bus

[Wang Xingang]

Hi, everyone!

Do you have any suggestions about this iommu configuration feature?
Please help review these patches, thanks very much.

On 2021/3/25 15:22, Wang Xingang wrote:
> From: Xingang Wang <wangxingang5@huawei.com>
> 
> These patches add support for configure iommu on/off for pci root bus,
> including primary bus and pxb root bus. At present, All root bus
> will go through iommu when iommu is configured, which is not flexible.
> 
> So this add option to enable/disable iommu for primary bus and pxb
> root bus.  When iommu is enabled for the root bus, devices attached to it
> will go through iommu. When iommu is disabled for the root bus, devices
> will not go through iommu accordingly.
> 
> The option example for iommu configuration is like the following:
> 
> primary root bus option:
> arm: -machine virt iommu=smmuv3,primary_bus_iommu=false(or true)
> x86: -machine q35,primary_bus_iommu=false(or true)
> 
> pxb root bus:
>   -device pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,addr=0x3.0x1,iommu=false
> 
> History:
> 
> v1 -> v2:
> - rebase on top of v6.0.0-rc0
> - Fix some issues
> - Took into account Eric's comments, and remove the PCI_BUS_IOMMU flag,
>    replace it with a property in PCIHostState.
> - Add support for x86 iommu option
> 
> Xingang Wang (6):
>    hw/pci/pci_host: Add iommu property for pci host
>    hw/pci: Add iommu option for pci root bus
>    hw/pci: Add pci_root_bus_max_bus
>    hw/arm/virt-acpi-build: Add explicit idmap info in IORT table
>    hw/i386/acpi-build: Add explicit scope in DMAR table
>    hw/i386/acpi-build: Add iommu filter in IVRS table
> 
>   hw/arm/virt-acpi-build.c            | 103 ++++++++++++++++++++++------
>   hw/arm/virt.c                       |  25 +++++++
>   hw/i386/acpi-build.c                |  70 ++++++++++++++++++-
>   hw/i386/pc.c                        |  19 +++++
>   hw/pci-bridge/pci_expander_bridge.c |   3 +
>   hw/pci-host/q35.c                   |   1 +
>   hw/pci/pci.c                        |  52 +++++++++++++-
>   hw/pci/pci_host.c                   |   2 +
>   include/hw/arm/virt.h               |   1 +
>   include/hw/i386/pc.h                |   1 +
>   include/hw/pci/pci.h                |   2 +
>   include/hw/pci/pci_host.h           |   1 +
>   12 files changed, 254 insertions(+), 26 deletions(-)
> 

Re: [PATCH RFC RESEND v2 2/6] hw/pci: Add iommu option for pci root bus

[Auger Eric]

Hi Wang,

On 3/25/21 8:22 AM, Wang Xingang wrote:
> From: Xingang Wang <wangxingang5@huawei.com>
> 
> This add iommu option for pci root bus, including primary bus
> and pxb root bus. The option is valid only if there is a virtual
> iommu device.
> 
> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>

same in this patch I would prefer to inverse the logic and use something
like bypass_iommu.

Sorry it an invasive change for you. Maybe wait for the other's feedbacks.

> ---
>  hw/arm/virt.c                       | 25 +++++++++++++++++++++++++
>  hw/i386/pc.c                        | 19 +++++++++++++++++++
>  hw/pci-bridge/pci_expander_bridge.c |  3 +++
>  hw/pci-host/q35.c                   |  1 +
>  include/hw/arm/virt.h               |  1 +
>  include/hw/i386/pc.h                |  1 +
Also I think this patch would need to be split into several ones (at
least this is what I would do)
- 1 patch for the pci_expander_bridge.c  change ("add an iommy bypass prop")
- 1  patch for virt machine where you add a machine option to bypass the
iommu translation for the root bus.
- 1 patch of pc/q35

Indeed the patch title title does not reflect the machine changes and
the pxb changes
>  6 files changed, 50 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index aa2bbd14e0..446b3b867f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1366,6 +1366,7 @@ static void create_pcie(VirtMachineState *vms)
>      }
>  
>      pci = PCI_HOST_BRIDGE(dev);
> +    pci->iommu = vms->primary_bus_iommu;
>      vms->bus = pci->bus;
>      if (vms->bus) {
>          for (i = 0; i < nb_nics; i++) {
> @@ -2319,6 +2320,20 @@ static void virt_set_iommu(Object *obj, const char *value, Error **errp)
>      }
>  }

>  
> +static bool virt_get_primary_bus_iommu(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    return vms->primary_bus_iommu;
> +}
> +
> +static void virt_set_primary_bus_iommu(Object *obj, bool value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    vms->primary_bus_iommu = value;
> +}
> +
>  static CpuInstanceProperties
>  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>  {
> @@ -2652,6 +2667,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>                                            "Set the IOMMU type. "
>                                            "Valid values are none and smmuv3");
>  
> +    object_class_property_add_bool(oc, "primary_bus_iommu",
> +                                  virt_get_primary_bus_iommu,
> +                                  virt_set_primary_bus_iommu);
> +    object_class_property_set_description(oc, "primary_bus_iommu",
> +                                          "Set on/off to enable/disable "
> +                                          "iommu for primary bus");
> +
>      object_class_property_add_bool(oc, "ras", virt_get_ras,
>                                     virt_set_ras);
>      object_class_property_set_description(oc, "ras",
> @@ -2719,6 +2741,9 @@ static void virt_instance_init(Object *obj)
>      /* Default disallows iommu instantiation */
>      vms->iommu = VIRT_IOMMU_NONE;
>  
> +    /* The primary bus is attached to iommu by default */
> +    vms->primary_bus_iommu = true;
> +
>      /* Default disallows RAS instantiation */
>      vms->ras = false;
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8a84b25a03..b64e4bb7f2 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1529,6 +1529,21 @@ static void pc_machine_set_hpet(Object *obj, bool value, Error **errp)
>      pcms->hpet_enabled = value;
>  }
>  
> +static bool pc_machine_get_primary_bus_iommu(Object *obj, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    return pcms->primary_bus_iommu;
> +}
> +
> +static void pc_machine_set_primary_bus_iommu(Object *obj, bool value,
> +                                             Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    pcms->primary_bus_iommu = value;
> +}
> +
>  static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
>                                              const char *name, void *opaque,
>                                              Error **errp)
> @@ -1628,6 +1643,7 @@ static void pc_machine_initfn(Object *obj)
>  #ifdef CONFIG_HPET
>      pcms->hpet_enabled = true;
>  #endif
> +    pcms->primary_bus_iommu = true;
>  
>      pc_system_flash_create(pcms);
>      pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
> @@ -1752,6 +1768,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_add_bool(oc, "hpet",
>          pc_machine_get_hpet, pc_machine_set_hpet);
>  
> +    object_class_property_add_bool(oc, "primary_bus_iommu",
> +        pc_machine_get_primary_bus_iommu, pc_machine_set_primary_bus_iommu);
> +
>      object_class_property_add(oc, PC_MACHINE_MAX_FW_SIZE, "size",
>          pc_machine_get_max_fw_size, pc_machine_set_max_fw_size,
>          NULL, NULL);
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index aedded1064..f1a0eadc03 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -57,6 +57,7 @@ struct PXBDev {
>  
>      uint8_t bus_nr;
>      uint16_t numa_node;
> +    bool iommu;
>  };
>  
>  static PXBDev *convert_to_pxb(PCIDevice *dev)
> @@ -255,6 +256,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
>      bus->map_irq = pxb_map_irq_fn;
>  
>      PCI_HOST_BRIDGE(ds)->bus = bus;
> +    PCI_HOST_BRIDGE(ds)->iommu = pxb->iommu;
>  
>      pxb_register_bus(dev, bus, &local_err);
>      if (local_err) {
> @@ -301,6 +303,7 @@ static Property pxb_dev_properties[] = {
>      /* Note: 0 is not a legal PXB bus number. */
>      DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
>      DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED),
> +    DEFINE_PROP_BOOL("iommu", PXBDev, iommu, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 2eb729dff5..3b23fd0975 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -64,6 +64,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>                                  s->mch.address_space_io,
>                                  0, TYPE_PCIE_BUS);
>      PC_MACHINE(qdev_get_machine())->bus = pci->bus;
> +    pci->iommu = PC_MACHINE(qdev_get_machine())->primary_bus_iommu;
>      qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal);
>  }
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 921416f918..1fbb19710f 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -147,6 +147,7 @@ struct VirtMachineState {
>      OnOffAuto acpi;
>      VirtGICType gic_version;
>      VirtIOMMUType iommu;
> +    bool primary_bus_iommu;
>      VirtMSIControllerType msi_controller;
>      uint16_t virtio_iommu_bdf;
>      struct arm_boot_info bootinfo;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index dcf060b791..07b08b3ac6 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -45,6 +45,7 @@ typedef struct PCMachineState {
>      bool sata_enabled;
>      bool pit_enabled;
>      bool hpet_enabled;
> +    bool primary_bus_iommu;
>      uint64_t max_fw_size;
>  
>      /* NUMA information: */
> 

Thanks

Eric

Re: [PATCH RFC RESEND v2 1/6] hw/pci/pci_host: Add iommu property for pci host

[Auger Eric]

Hi Wang,

On 3/25/21 8:22 AM, Wang Xingang wrote:
> From: Xingang Wang <wangxingang5@huawei.com>
> 
> The pci host iommu property is useful to check whether
> the iommu is enabled on the pci root bus.
> 
> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> ---
>  hw/pci/pci.c              | 18 +++++++++++++++++-
>  hw/pci/pci_host.c         |  2 ++
>  include/hw/pci/pci.h      |  1 +
>  include/hw/pci/pci_host.h |  1 +
>  4 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index ac9a24889c..e17aa9075f 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -417,6 +417,22 @@ const char *pci_root_bus_path(PCIDevice *dev)
>      return rootbus->qbus.name;
>  }
>  
> +bool pci_root_bus_has_iommu(PCIBus *bus)
"has_iommu" is misleading as it does not mean an IOMMU is actually
instantiated but rather that if there is any, it will translate
transactions coming from this primary bus

I would rather inverse the logic and have a

"bypass_iommu" property defaulting to false

and this function dubbed something like pci_root_bus_bypass_iommu
> +{
> +    PCIBus *rootbus = bus;
> +    PCIHostState *host_bridge;
> +
> +    if (!pci_bus_is_root(bus)) {
> +        rootbus = pci_device_root_bus(bus->parent_dev);
> +    }
> +
> +    host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent);
> +
> +    assert(host_bridge->bus == rootbus);
> +
> +    return host_bridge->iommu;
> +}
> +
>  static void pci_root_bus_init(PCIBus *bus, DeviceState *parent,
>                                MemoryRegion *address_space_mem,
>                                MemoryRegion *address_space_io,
> @@ -2716,7 +2732,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>  
>          iommu_bus = parent_bus;
>      }
> -    if (iommu_bus && iommu_bus->iommu_fn) {
> +    if (pci_root_bus_has_iommu(bus) && iommu_bus && iommu_bus->iommu_fn) {
>          return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn);
>      }
>      return &address_space_memory;
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 8ca5fadcbd..92ce213b18 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -222,6 +222,8 @@ const VMStateDescription vmstate_pcihost = {
>  static Property pci_host_properties_common[] = {
>      DEFINE_PROP_BOOL("x-config-reg-migration-enabled", PCIHostState,
>                       mig_enabled, true),
> +    DEFINE_PROP_BOOL("pci-host-iommu-enabled", PCIHostState,
> +                     iommu, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 6be4e0c460..718b5a454a 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -480,6 +480,7 @@ void pci_for_each_bus(PCIBus *bus,
>  
>  PCIBus *pci_device_root_bus(const PCIDevice *d);
>  const char *pci_root_bus_path(PCIDevice *dev);
> +bool pci_root_bus_has_iommu(PCIBus *bus);
>  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
>  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
>  void pci_bus_get_w64_range(PCIBus *bus, Range *range);
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index 52e038c019..64128e3a19 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -43,6 +43,7 @@ struct PCIHostState {
>      uint32_t config_reg;
>      bool mig_enabled;
>      PCIBus *bus;
> +    bool iommu;
>  
>      QLIST_ENTRY(PCIHostState) next;
>  };
> 
Thanks

Eric

Re: [PATCH RFC RESEND v2 1/6] hw/pci/pci_host: Add iommu property for pci host

[Wang Xingang]

Hi Eric,

On 2021/4/13 1:36, Auger Eric wrote:
> Hi Wang,
> 
> On 3/25/21 8:22 AM, Wang Xingang wrote:
>> From: Xingang Wang <wangxingang5@huawei.com>
>>
>> The pci host iommu property is useful to check whether
>> the iommu is enabled on the pci root bus.
>>
>> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> ---
>>   hw/pci/pci.c              | 18 +++++++++++++++++-
>>   hw/pci/pci_host.c         |  2 ++
>>   include/hw/pci/pci.h      |  1 +
>>   include/hw/pci/pci_host.h |  1 +
>>   4 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index ac9a24889c..e17aa9075f 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -417,6 +417,22 @@ const char *pci_root_bus_path(PCIDevice *dev)
>>       return rootbus->qbus.name;
>>   }
>>   
>> +bool pci_root_bus_has_iommu(PCIBus *bus)
> "has_iommu" is misleading as it does not mean an IOMMU is actually
> instantiated but rather that if there is any, it will translate
> transactions coming from this primary bus
> 
> I would rather inverse the logic and have a
> 
> "bypass_iommu" property defaulting to false
> 
> and this function dubbed something like pci_root_bus_bypass_iommu
>> +{
>> +    PCIBus *rootbus = bus;
>> +    PCIHostState *host_bridge;
>> +
>> +    if (!pci_bus_is_root(bus)) {
>> +        rootbus = pci_device_root_bus(bus->parent_dev);
>> +    }
>> +
>> +    host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent);
>> +
>> +    assert(host_bridge->bus == rootbus);
>> +
>> +    return host_bridge->iommu;
>> +}
>> +

Thanks for your advice, it is misleading, i will replace this.

>>   static void pci_root_bus_init(PCIBus *bus, DeviceState *parent,
>>                                 MemoryRegion *address_space_mem,
>>                                 MemoryRegion *address_space_io,
>> @@ -2716,7 +2732,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>>   
>>           iommu_bus = parent_bus;
>>       }
>> -    if (iommu_bus && iommu_bus->iommu_fn) {
>> +    if (pci_root_bus_has_iommu(bus) && iommu_bus && iommu_bus->iommu_fn) {
>>           return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn);
>>       }
>>       return &address_space_memory;
>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
>> index 8ca5fadcbd..92ce213b18 100644
>> --- a/hw/pci/pci_host.c
>> +++ b/hw/pci/pci_host.c
>> @@ -222,6 +222,8 @@ const VMStateDescription vmstate_pcihost = {
>>   static Property pci_host_properties_common[] = {
>>       DEFINE_PROP_BOOL("x-config-reg-migration-enabled", PCIHostState,
>>                        mig_enabled, true),
>> +    DEFINE_PROP_BOOL("pci-host-iommu-enabled", PCIHostState,
>> +                     iommu, true),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index 6be4e0c460..718b5a454a 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -480,6 +480,7 @@ void pci_for_each_bus(PCIBus *bus,
>>   
>>   PCIBus *pci_device_root_bus(const PCIDevice *d);
>>   const char *pci_root_bus_path(PCIDevice *dev);
>> +bool pci_root_bus_has_iommu(PCIBus *bus);
>>   PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
>>   int pci_qdev_find_device(const char *id, PCIDevice **pdev);
>>   void pci_bus_get_w64_range(PCIBus *bus, Range *range);
>> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
>> index 52e038c019..64128e3a19 100644
>> --- a/include/hw/pci/pci_host.h
>> +++ b/include/hw/pci/pci_host.h
>> @@ -43,6 +43,7 @@ struct PCIHostState {
>>       uint32_t config_reg;
>>       bool mig_enabled;
>>       PCIBus *bus;
>> +    bool iommu;
>>   
>>       QLIST_ENTRY(PCIHostState) next;
>>   };
>>
> Thanks
> 
> Eric
> 
> .
> 

Thanks

Xingang

.

Re: [PATCH RFC RESEND v2 2/6] hw/pci: Add iommu option for pci root bus

[Wang Xingang]

Hi Eric,

On 2021/4/13 1:36, Auger Eric wrote:
> Hi Wang,
> 
> On 3/25/21 8:22 AM, Wang Xingang wrote:
>> From: Xingang Wang <wangxingang5@huawei.com>
>>
>> This add iommu option for pci root bus, including primary bus
>> and pxb root bus. The option is valid only if there is a virtual
>> iommu device.
>>
>> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> 
> same in this patch I would prefer to inverse the logic and use something
> like bypass_iommu.
> 
> Sorry it an invasive change for you. Maybe wait for the other's feedbacks.
> 

Thanks, it is useful.

>> ---
>>   hw/arm/virt.c                       | 25 +++++++++++++++++++++++++
>>   hw/i386/pc.c                        | 19 +++++++++++++++++++
>>   hw/pci-bridge/pci_expander_bridge.c |  3 +++
>>   hw/pci-host/q35.c                   |  1 +
>>   include/hw/arm/virt.h               |  1 +
>>   include/hw/i386/pc.h                |  1 +
> Also I think this patch would need to be split into several ones (at
> least this is what I would do)
> - 1 patch for the pci_expander_bridge.c  change ("add an iommy bypass prop")
> - 1  patch for virt machine where you add a machine option to bypass the
> iommu translation for the root bus.
> - 1 patch of pc/q35
> 
> Indeed the patch title title does not reflect the machine changes and
> the pxb changes

Thanks, I will split it and change the description in the next revision.

>>   6 files changed, 50 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index aa2bbd14e0..446b3b867f 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1366,6 +1366,7 @@ static void create_pcie(VirtMachineState *vms)
>>       }
>>   
>>       pci = PCI_HOST_BRIDGE(dev);
>> +    pci->iommu = vms->primary_bus_iommu;
>>       vms->bus = pci->bus;
>>       if (vms->bus) {
>>           for (i = 0; i < nb_nics; i++) {
>> @@ -2319,6 +2320,20 @@ static void virt_set_iommu(Object *obj, const char *value, Error **errp)
>>       }
>>   }
> 
>>   
>> +static bool virt_get_primary_bus_iommu(Object *obj, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    return vms->primary_bus_iommu;
>> +}
>> +
>> +static void virt_set_primary_bus_iommu(Object *obj, bool value, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    vms->primary_bus_iommu = value;
>> +}
>> +
>>   static CpuInstanceProperties
>>   virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>>   {
>> @@ -2652,6 +2667,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>>                                             "Set the IOMMU type. "
>>                                             "Valid values are none and smmuv3");
>>   
>> +    object_class_property_add_bool(oc, "primary_bus_iommu",
>> +                                  virt_get_primary_bus_iommu,
>> +                                  virt_set_primary_bus_iommu);
>> +    object_class_property_set_description(oc, "primary_bus_iommu",
>> +                                          "Set on/off to enable/disable "
>> +                                          "iommu for primary bus");
>> +
>>       object_class_property_add_bool(oc, "ras", virt_get_ras,
>>                                      virt_set_ras);
>>       object_class_property_set_description(oc, "ras",
>> @@ -2719,6 +2741,9 @@ static void virt_instance_init(Object *obj)
>>       /* Default disallows iommu instantiation */
>>       vms->iommu = VIRT_IOMMU_NONE;
>>   
>> +    /* The primary bus is attached to iommu by default */
>> +    vms->primary_bus_iommu = true;
>> +
>>       /* Default disallows RAS instantiation */
>>       vms->ras = false;
>>   
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 8a84b25a03..b64e4bb7f2 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1529,6 +1529,21 @@ static void pc_machine_set_hpet(Object *obj, bool value, Error **errp)
>>       pcms->hpet_enabled = value;
>>   }
>>   
>> +static bool pc_machine_get_primary_bus_iommu(Object *obj, Error **errp)
>> +{
>> +    PCMachineState *pcms = PC_MACHINE(obj);
>> +
>> +    return pcms->primary_bus_iommu;
>> +}
>> +
>> +static void pc_machine_set_primary_bus_iommu(Object *obj, bool value,
>> +                                             Error **errp)
>> +{
>> +    PCMachineState *pcms = PC_MACHINE(obj);
>> +
>> +    pcms->primary_bus_iommu = value;
>> +}
>> +
>>   static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
>>                                               const char *name, void *opaque,
>>                                               Error **errp)
>> @@ -1628,6 +1643,7 @@ static void pc_machine_initfn(Object *obj)
>>   #ifdef CONFIG_HPET
>>       pcms->hpet_enabled = true;
>>   #endif
>> +    pcms->primary_bus_iommu = true;
>>   
>>       pc_system_flash_create(pcms);
>>       pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
>> @@ -1752,6 +1768,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>       object_class_property_add_bool(oc, "hpet",
>>           pc_machine_get_hpet, pc_machine_set_hpet);
>>   
>> +    object_class_property_add_bool(oc, "primary_bus_iommu",
>> +        pc_machine_get_primary_bus_iommu, pc_machine_set_primary_bus_iommu);
>> +
>>       object_class_property_add(oc, PC_MACHINE_MAX_FW_SIZE, "size",
>>           pc_machine_get_max_fw_size, pc_machine_set_max_fw_size,
>>           NULL, NULL);
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>> index aedded1064..f1a0eadc03 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -57,6 +57,7 @@ struct PXBDev {
>>   
>>       uint8_t bus_nr;
>>       uint16_t numa_node;
>> +    bool iommu;
>>   };
>>   
>>   static PXBDev *convert_to_pxb(PCIDevice *dev)
>> @@ -255,6 +256,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
>>       bus->map_irq = pxb_map_irq_fn;
>>   
>>       PCI_HOST_BRIDGE(ds)->bus = bus;
>> +    PCI_HOST_BRIDGE(ds)->iommu = pxb->iommu;
>>   
>>       pxb_register_bus(dev, bus, &local_err);
>>       if (local_err) {
>> @@ -301,6 +303,7 @@ static Property pxb_dev_properties[] = {
>>       /* Note: 0 is not a legal PXB bus number. */
>>       DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
>>       DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED),
>> +    DEFINE_PROP_BOOL("iommu", PXBDev, iommu, true),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 2eb729dff5..3b23fd0975 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -64,6 +64,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>>                                   s->mch.address_space_io,
>>                                   0, TYPE_PCIE_BUS);
>>       PC_MACHINE(qdev_get_machine())->bus = pci->bus;
>> +    pci->iommu = PC_MACHINE(qdev_get_machine())->primary_bus_iommu;
>>       qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal);
>>   }
>>   
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index 921416f918..1fbb19710f 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -147,6 +147,7 @@ struct VirtMachineState {
>>       OnOffAuto acpi;
>>       VirtGICType gic_version;
>>       VirtIOMMUType iommu;
>> +    bool primary_bus_iommu;
>>       VirtMSIControllerType msi_controller;
>>       uint16_t virtio_iommu_bdf;
>>       struct arm_boot_info bootinfo;
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index dcf060b791..07b08b3ac6 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -45,6 +45,7 @@ typedef struct PCMachineState {
>>       bool sata_enabled;
>>       bool pit_enabled;
>>       bool hpet_enabled;
>> +    bool primary_bus_iommu;
>>       uint64_t max_fw_size;
>>   
>>       /* NUMA information: */
>>
> 
> Thanks
> 
> Eric
> 
> .
> 

Thanks

Xingang

.

Re: [PATCH RFC RESEND v2 3/6] hw/pci: Add pci_root_bus_max_bus

[Auger Eric]

Hi Xingang,

On 3/25/21 8:22 AM, Wang Xingang wrote:
> From: Xingang Wang <wangxingang5@huawei.com>
> 
> This helps to find max bus number of a root bus.
s/max bus number of a root bus/highest bus number of a bridge hierarchy?
> 
> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> ---
>  hw/pci/pci.c         | 34 ++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h |  1 +
>  2 files changed, 35 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e17aa9075f..c7957cbf7c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -538,6 +538,40 @@ int pci_bus_num(PCIBus *s)
>      return PCI_BUS_GET_CLASS(s)->bus_num(s);
>  }
>  
> +int pci_root_bus_max_bus(PCIBus *bus)
> +{
> +    PCIHostState *host;
> +    PCIDevice *dev;
> +    int max_bus = 0;
> +    int type, devfn;
> +    uint8_t subordinate;
> +
> +    if (!pci_bus_is_root(bus)) {
> +        return 0;
> +    }
> +
> +    host = PCI_HOST_BRIDGE(BUS(bus)->parent);
> +    max_bus = pci_bus_num(host->bus);
> +
> +    for (devfn = 0; devfn < ARRAY_SIZE(host->bus->devices); devfn++) {
> +        dev = host->bus->devices[devfn];
> +
> +        if (!dev) {
> +            continue;
> +        }
> +
> +        type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
Seems there is PCI_DEVICE_GET_CLASS(dev)->is_bridge (see
pci_root_bus_in_range). Can't that be used instead?
> +        if (type == PCI_HEADER_TYPE_BRIDGE) {
> +            subordinate = dev->config[PCI_SUBORDINATE_BUS];
> +            if (subordinate > max_bus) {
> +                max_bus = subordinate;
what about the secondary bus number, it is always less than the others?

Thanks

Eric
> +            }
> +        }
> +    }
> +
> +    return max_bus;
> +}
> +
>  int pci_bus_numa_node(PCIBus *bus)
>  {
>      return PCI_BUS_GET_CLASS(bus)->numa_node(bus);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 718b5a454a..e0c69534f4 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -450,6 +450,7 @@ static inline PCIBus *pci_get_bus(const PCIDevice *dev)
>      return PCI_BUS(qdev_get_parent_bus(DEVICE(dev)));
>  }
>  int pci_bus_num(PCIBus *s);
> +int pci_root_bus_max_bus(PCIBus *bus);
>  static inline int pci_dev_bus_num(const PCIDevice *dev)
>  {
>      return pci_bus_num(pci_get_bus(dev));
> 

Re: [PATCH RFC RESEND v2 4/6] hw/arm/virt-acpi-build: Add explicit idmap info in IORT table

[Auger Eric]

Hi Xingang,

On 3/25/21 8:22 AM, Wang Xingang wrote:
> From: Xingang Wang <wangxingang5@huawei.com>
> 
> The idmap of smmuv3 and root complex covers the whole RID space for now,
> this patch add explicit idmap info according to root bus number range.
> This add smmuv3 idmap for certain bus which has enabled the iommu property.
> 
> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 103 ++++++++++++++++++++++++++++++---------
>  1 file changed, 81 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f5a2b2d4cb..5491036c86 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -44,6 +44,7 @@
>  #include "hw/acpi/tpm.h"
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
>  #include "hw/pci-host/gpex.h"
>  #include "hw/arm/virt.h"
>  #include "hw/mem/nvdimm.h"
> @@ -237,6 +238,41 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>      aml_append(scope, dev);
>  }
>  
> +typedef
> +struct AcpiIortMapping {
> +    AcpiIortIdMapping idmap;
> +    bool iommu;
> +} AcpiIortMapping;
> +
> +/* For all PCI host bridges, walk and insert DMAR scope */
this comment should rather be in the caller
also DMAR is not the ARM vocable.

I would add the comment for this function:
build the ID mapping for aa given PCI host bridge
> +static int
> +iort_host_bridges(Object *obj, void *opaque)
> +{
> +    GArray *map_blob = opaque;
> +    AcpiIortMapping map;
> +    AcpiIortIdMapping *idmap = &map.idmap;
> +    int bus_num, max_bus;
> +
> +    if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> +        PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
> +
> +        if (bus) {
> +            bus_num = pci_bus_num(bus);
> +            max_bus = pci_root_bus_max_bus(bus);
> +
> +            idmap->input_base = cpu_to_le32(bus_num << 8);
> +            idmap->id_count = cpu_to_le32((max_bus - bus_num + 1) << 8);
> +            idmap->output_base = cpu_to_le32(bus_num << 8);
> +            idmap->flags = cpu_to_le32(0);
> +
> +            map.iommu = pci_root_bus_has_iommu(bus);
if iommu is not set, we don't need to populate the idmap and we may even
directly continue, ie. not add the element the map_bap_blob, no?
> +            g_array_append_val(map_blob, map);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void
>  build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> @@ -247,6 +283,21 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      AcpiIortSmmu3 *smmu;
>      size_t node_size, iort_node_offset, iort_length, smmu_offset = 0;
>      AcpiIortRC *rc;
> +    int smmu_mapping_count;
> +    GArray *map_blob = g_array_new(false, true, sizeof(AcpiIortMapping));
> +    AcpiIortMapping *map;
> +
> +    /* pci_for_each_bus(vms->bus, insert_map, map_blob); */
comment to be removed
> +    object_child_foreach_recursive(object_get_root(),
> +                                   iort_host_bridges, map_blob);
> +
> +    smmu_mapping_count = 0;
> +    for (int i = 0; i < map_blob->len; i++) {
> +        map = &g_array_index(map_blob, AcpiIortMapping, i);
> +        if (map->iommu) {
> +            smmu_mapping_count++;
> +        }
> +    }
>  
>      iort = acpi_data_push(table_data, sizeof(*iort));
>  
> @@ -280,13 +331,13 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  
>          /* SMMUv3 node */
>          smmu_offset = iort_node_offset + node_size;
> -        node_size = sizeof(*smmu) + sizeof(*idmap);
> +        node_size = sizeof(*smmu) + sizeof(*idmap) * smmu_mapping_count;
>          iort_length += node_size;
>          smmu = acpi_data_push(table_data, node_size);
>  
>          smmu->type = ACPI_IORT_NODE_SMMU_V3;
>          smmu->length = cpu_to_le16(node_size);
> -        smmu->mapping_count = cpu_to_le32(1);
> +        smmu->mapping_count = cpu_to_le32(smmu_mapping_count);
>          smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
>          smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
>          smmu->flags = cpu_to_le32(ACPI_IORT_SMMU_V3_COHACC_OVERRIDE);
> @@ -295,23 +346,28 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          smmu->gerr_gsiv = cpu_to_le32(irq + 2);
>          smmu->sync_gsiv = cpu_to_le32(irq + 3);
>  
> -        /* Identity RID mapping covering the whole input RID range */
> -        idmap = &smmu->id_mapping_array[0];
> -        idmap->input_base = 0;
> -        idmap->id_count = cpu_to_le32(0xFFFF);
> -        idmap->output_base = 0;
> -        /* output IORT node is the ITS group node (the first node) */
> -        idmap->output_reference = cpu_to_le32(iort_node_offset);
> +        for (int i = 0, j = 0; i < map_blob->len; i++) {
> +            map = &g_array_index(map_blob, AcpiIortMapping, i);
> +
> +            if (!map->iommu) {
> +                continue;
> +            }
> +
> +            idmap = &smmu->id_mapping_array[j++];
> +            *idmap = map->idmap;
> +            /* output IORT node is the ITS group node (the first node) */
> +            idmap->output_reference = cpu_to_le32(iort_node_offset);
> +        }
>      }
>  
>      /* Root Complex Node */
> -    node_size = sizeof(*rc) + sizeof(*idmap);
> +    node_size = sizeof(*rc) + sizeof(*idmap) * map_blob->len;
>      iort_length += node_size;
>      rc = acpi_data_push(table_data, node_size);
>  
>      rc->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
>      rc->length = cpu_to_le16(node_size);
> -    rc->mapping_count = cpu_to_le32(1);
> +    rc->mapping_count = cpu_to_le32(map_blob->len);
>      rc->mapping_offset = cpu_to_le32(sizeof(*rc));
>  
>      /* fully coherent device */
> @@ -319,20 +375,23 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      rc->memory_properties.memory_flags = 0x3; /* CCA = CPM = DCAS = 1 */
>      rc->pci_segment_number = 0; /* MCFG pci_segment */
>  
> -    /* Identity RID mapping covering the whole input RID range */
> -    idmap = &rc->id_mapping_array[0];
> -    idmap->input_base = 0;
> -    idmap->id_count = cpu_to_le32(0xFFFF);
> -    idmap->output_base = 0;
> +    for (int i = 0; i < map_blob->len; i++) {
> +        map = &g_array_index(map_blob, AcpiIortMapping, i);
> +        idmap = &rc->id_mapping_array[i];
>  
> -    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> -        /* output IORT node is the smmuv3 node */
> -        idmap->output_reference = cpu_to_le32(smmu_offset);
> -    } else {
> -        /* output IORT node is the ITS group node (the first node) */
> -        idmap->output_reference = cpu_to_le32(iort_node_offset);
> +        *idmap = map->idmap;
We we descibe the link between SMMU SID and ITS deviceids, shouldn't we
leave the whole range? Even if an RID is not translated, it may use MSIs?
> +
> +        if (vms->iommu == VIRT_IOMMU_SMMUV3 && map->iommu) {
> +            /* output IORT node is the smmuv3 node */
> +            idmap->output_reference = cpu_to_le32(smmu_offset);
> +        } else {
> +            /* output IORT node is the ITS group node (the first node) */
> +            idmap->output_reference = cpu_to_le32(iort_node_offset);
is that case we have a direct mapping between RC and ITS (no
instantiated SMMU). Shouldn't the whole RID range being used in that case?

Thanks

Eric
> +        }
>      }
>  
> +    g_array_free(map_blob, true);
> +
>      /*
>       * Update the pointer address in case table_data->data moves during above
>       * acpi_data_push operations.
> 

Re: [PATCH RFC RESEND v2 3/6] hw/pci: Add pci_root_bus_max_bus

[Xingang Wang]

Hi Eric,

On 2021/4/13 16:05, Auger Eric wrote:
> Hi Xingang,
> 
> On 3/25/21 8:22 AM, Wang Xingang wrote:
>> From: Xingang Wang <wangxingang5@huawei.com>
>>
>> This helps to find max bus number of a root bus.
> s/max bus number of a root bus/highest bus number of a bridge hierarchy?

Thanks, I will change the description.

>>
>> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> ---
>>   hw/pci/pci.c         | 34 ++++++++++++++++++++++++++++++++++
>>   include/hw/pci/pci.h |  1 +
>>   2 files changed, 35 insertions(+)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index e17aa9075f..c7957cbf7c 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -538,6 +538,40 @@ int pci_bus_num(PCIBus *s)
>>       return PCI_BUS_GET_CLASS(s)->bus_num(s);
>>   }
>>   
>> +int pci_root_bus_max_bus(PCIBus *bus)
>> +{
>> +    PCIHostState *host;
>> +    PCIDevice *dev;
>> +    int max_bus = 0;
>> +    int type, devfn;
>> +    uint8_t subordinate;
>> +
>> +    if (!pci_bus_is_root(bus)) {
>> +        return 0;
>> +    }
>> +
>> +    host = PCI_HOST_BRIDGE(BUS(bus)->parent);
>> +    max_bus = pci_bus_num(host->bus);
>> +
>> +    for (devfn = 0; devfn < ARRAY_SIZE(host->bus->devices); devfn++) {
>> +        dev = host->bus->devices[devfn];
>> +
>> +        if (!dev) {
>> +            continue;
>> +        }
>> +
>> +        type = dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> Seems there is PCI_DEVICE_GET_CLASS(dev)->is_bridge (see
> pci_root_bus_in_range). Can't that be used instead?

Thanks, I will simplify this.

>> +        if (type == PCI_HEADER_TYPE_BRIDGE) {
>> +            subordinate = dev->config[PCI_SUBORDINATE_BUS];
>> +            if (subordinate > max_bus) {
>> +                max_bus = subordinate;
> what about the secondary bus number, it is always less than the others?
> 

Thanks, the secondary bus number should be taken into account. Maybe a 
pci_root_bus_range to get [min_bus, max_bus] would be better.

> Thanks
> 
> Eric
>> +            }
>> +        }
>> +    }
>> +
>> +    return max_bus;
>> +}
>> +
>>   int pci_bus_numa_node(PCIBus *bus)
>>   {
>>       return PCI_BUS_GET_CLASS(bus)->numa_node(bus);
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index 718b5a454a..e0c69534f4 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -450,6 +450,7 @@ static inline PCIBus *pci_get_bus(const PCIDevice *dev)
>>       return PCI_BUS(qdev_get_parent_bus(DEVICE(dev)));
>>   }
>>   int pci_bus_num(PCIBus *s);
>> +int pci_root_bus_max_bus(PCIBus *bus);
>>   static inline int pci_dev_bus_num(const PCIDevice *dev)
>>   {
>>       return pci_bus_num(pci_get_bus(dev));
>>
> 
> .
> 

Xingang

.

Re: [PATCH RFC RESEND v2 4/6] hw/arm/virt-acpi-build: Add explicit idmap info in IORT table

[Xingang Wang]

Hi Eric,

On 2021/4/13 17:03, Auger Eric wrote:
> Hi Xingang,
> 
> On 3/25/21 8:22 AM, Wang Xingang wrote:
>> From: Xingang Wang <wangxingang5@huawei.com>
>>
>> The idmap of smmuv3 and root complex covers the whole RID space for now,
>> this patch add explicit idmap info according to root bus number range.
>> This add smmuv3 idmap for certain bus which has enabled the iommu property.
>>
>> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> ---
>>   hw/arm/virt-acpi-build.c | 103 ++++++++++++++++++++++++++++++---------
>>   1 file changed, 81 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index f5a2b2d4cb..5491036c86 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -44,6 +44,7 @@
>>   #include "hw/acpi/tpm.h"
>>   #include "hw/pci/pcie_host.h"
>>   #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bus.h"
>>   #include "hw/pci-host/gpex.h"
>>   #include "hw/arm/virt.h"
>>   #include "hw/mem/nvdimm.h"
>> @@ -237,6 +238,41 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>>       aml_append(scope, dev);
>>   }
>>   
>> +typedef
>> +struct AcpiIortMapping {
>> +    AcpiIortIdMapping idmap;
>> +    bool iommu;
>> +} AcpiIortMapping;
>> +
>> +/* For all PCI host bridges, walk and insert DMAR scope */
> this comment should rather be in the caller
> also DMAR is not the ARM vocable.
> 
> I would add the comment for this function:
> build the ID mapping for aa given PCI host bridge

Thanks, I will fix the comment.

>> +static int
>> +iort_host_bridges(Object *obj, void *opaque)
>> +{
>> +    GArray *map_blob = opaque;
>> +    AcpiIortMapping map;
>> +    AcpiIortIdMapping *idmap = &map.idmap;
>> +    int bus_num, max_bus;
>> +
>> +    if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
>> +        PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
>> +
>> +        if (bus) {
>> +            bus_num = pci_bus_num(bus);
>> +            max_bus = pci_root_bus_max_bus(bus);
>> +
>> +            idmap->input_base = cpu_to_le32(bus_num << 8);
>> +            idmap->id_count = cpu_to_le32((max_bus - bus_num + 1) << 8);
>> +            idmap->output_base = cpu_to_le32(bus_num << 8);
>> +            idmap->flags = cpu_to_le32(0);
>> +
>> +            map.iommu = pci_root_bus_has_iommu(bus);
> if iommu is not set, we don't need to populate the idmap and we may even
> directly continue, ie. not add the element the map_bap_blob, no?
Thanks, if we leave the whole range when iommu is not set, this indeed 
should be dropped in map_blob.
>> +            g_array_append_val(map_blob, map);
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static void
>>   build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>   {
>> @@ -247,6 +283,21 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>       AcpiIortSmmu3 *smmu;
>>       size_t node_size, iort_node_offset, iort_length, smmu_offset = 0;
>>       AcpiIortRC *rc;
>> +    int smmu_mapping_count;
>> +    GArray *map_blob = g_array_new(false, true, sizeof(AcpiIortMapping));
>> +    AcpiIortMapping *map;
>> +
>> +    /* pci_for_each_bus(vms->bus, insert_map, map_blob); */
> comment to be removed
Done.
>> +    object_child_foreach_recursive(object_get_root(),
>> +                                   iort_host_bridges, map_blob);
>> +
>> +    smmu_mapping_count = 0;
>> +    for (int i = 0; i < map_blob->len; i++) {
>> +        map = &g_array_index(map_blob, AcpiIortMapping, i);
>> +        if (map->iommu) {
>> +            smmu_mapping_count++;
>> +        }
>> +    }
>>   
>>       iort = acpi_data_push(table_data, sizeof(*iort));
>>   
>> @@ -280,13 +331,13 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>   
>>           /* SMMUv3 node */
>>           smmu_offset = iort_node_offset + node_size;
>> -        node_size = sizeof(*smmu) + sizeof(*idmap);
>> +        node_size = sizeof(*smmu) + sizeof(*idmap) * smmu_mapping_count;
>>           iort_length += node_size;
>>           smmu = acpi_data_push(table_data, node_size);
>>   
>>           smmu->type = ACPI_IORT_NODE_SMMU_V3;
>>           smmu->length = cpu_to_le16(node_size);
>> -        smmu->mapping_count = cpu_to_le32(1);
>> +        smmu->mapping_count = cpu_to_le32(smmu_mapping_count);
>>           smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
>>           smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
>>           smmu->flags = cpu_to_le32(ACPI_IORT_SMMU_V3_COHACC_OVERRIDE);
>> @@ -295,23 +346,28 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>           smmu->gerr_gsiv = cpu_to_le32(irq + 2);
>>           smmu->sync_gsiv = cpu_to_le32(irq + 3);
>>   
>> -        /* Identity RID mapping covering the whole input RID range */
>> -        idmap = &smmu->id_mapping_array[0];
>> -        idmap->input_base = 0;
>> -        idmap->id_count = cpu_to_le32(0xFFFF);
>> -        idmap->output_base = 0;
>> -        /* output IORT node is the ITS group node (the first node) */
>> -        idmap->output_reference = cpu_to_le32(iort_node_offset);
>> +        for (int i = 0, j = 0; i < map_blob->len; i++) {
>> +            map = &g_array_index(map_blob, AcpiIortMapping, i);
>> +
>> +            if (!map->iommu) {
>> +                continue;
>> +            }
>> +
>> +            idmap = &smmu->id_mapping_array[j++];
>> +            *idmap = map->idmap;
>> +            /* output IORT node is the ITS group node (the first node) */
>> +            idmap->output_reference = cpu_to_le32(iort_node_offset);
>> +        }
>>       }
>>   
>>       /* Root Complex Node */
>> -    node_size = sizeof(*rc) + sizeof(*idmap);
>> +    node_size = sizeof(*rc) + sizeof(*idmap) * map_blob->len;
>>       iort_length += node_size;
>>       rc = acpi_data_push(table_data, node_size);
>>   
>>       rc->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
>>       rc->length = cpu_to_le16(node_size);
>> -    rc->mapping_count = cpu_to_le32(1);
>> +    rc->mapping_count = cpu_to_le32(map_blob->len);
>>       rc->mapping_offset = cpu_to_le32(sizeof(*rc));
>>   
>>       /* fully coherent device */
>> @@ -319,20 +375,23 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>       rc->memory_properties.memory_flags = 0x3; /* CCA = CPM = DCAS = 1 */
>>       rc->pci_segment_number = 0; /* MCFG pci_segment */
>>   
>> -    /* Identity RID mapping covering the whole input RID range */
>> -    idmap = &rc->id_mapping_array[0];
>> -    idmap->input_base = 0;
>> -    idmap->id_count = cpu_to_le32(0xFFFF);
>> -    idmap->output_base = 0;
>> +    for (int i = 0; i < map_blob->len; i++) {
>> +        map = &g_array_index(map_blob, AcpiIortMapping, i);
>> +        idmap = &rc->id_mapping_array[i];
>>   
>> -    if (vms->iommu == VIRT_IOMMU_SMMUV3) {
>> -        /* output IORT node is the smmuv3 node */
>> -        idmap->output_reference = cpu_to_le32(smmu_offset);
>> -    } else {
>> -        /* output IORT node is the ITS group node (the first node) */
>> -        idmap->output_reference = cpu_to_le32(iort_node_offset);
>> +        *idmap = map->idmap;
> We we descibe the link between SMMU SID and ITS deviceids, shouldn't we
> leave the whole range? Even if an RID is not translated, it may use MSIs?
Thanks, I did not take into account of this situation, I will fix this.
>> +
>> +        if (vms->iommu == VIRT_IOMMU_SMMUV3 && map->iommu) {
>> +            /* output IORT node is the smmuv3 node */
>> +            idmap->output_reference = cpu_to_le32(smmu_offset);
>> +        } else {
>> +            /* output IORT node is the ITS group node (the first node) */
>> +            idmap->output_reference = cpu_to_le32(iort_node_offset);
> is that case we have a direct mapping between RC and ITS (no
> instantiated SMMU). Shouldn't the whole RID range being used in that case?
> 
> Thanks
> 
> Eric

Thanks, I will change the idmap strategy to cover the whole RID range.
>> +        }
>>       }
>>   
>> +    g_array_free(map_blob, true);
>> +
>>       /*
>>        * Update the pointer address in case table_data->data moves during above
>>        * acpi_data_push operations.
>>
> 
> .
> 

Xingang

.