[RFC RESEND PATCH 0/4] hw/arm/virt-acpi-build: Introduce iommu option for pci root bus

[RFC RESEND PATCH 0/4] hw/arm/virt-acpi-build: 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.

Xingang Wang (4):
  pci: Add PCI_BUS_IOMMU property
  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/arm/virt-acpi-build.c            | 92 +++++++++++++++++++++--------
 hw/arm/virt.c                       | 29 +++++++++
 hw/pci-bridge/pci_expander_bridge.c |  6 ++
 hw/pci/pci.c                        | 35 ++++++++++-
 include/hw/arm/virt.h               |  1 +
 include/hw/pci/pci.h                |  1 +
 include/hw/pci/pci_bus.h            | 13 ++++
 7 files changed, 153 insertions(+), 24 deletions(-)

-- 
2.19.1

[RFC RESEND PATCH 1/4] pci: Add PCI_BUS_IOMMU property

[Wang Xingang]

From: Xingang Wang <wangxingang5@huawei.com>

This Property can be useful to check whether this bus is attached to iommu.

Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
 include/hw/pci/pci_bus.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 347440d42c..42109e8a06 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -24,6 +24,8 @@ enum PCIBusFlags {
     PCI_BUS_IS_ROOT                                         = 0x0001,
     /* PCIe extended configuration space is accessible on this bus */
     PCI_BUS_EXTENDED_CONFIG_SPACE                           = 0x0002,
+    /* Iommu is enabled on this bus */
+    PCI_BUS_IOMMU                                           = 0x0004,
 };
 
 struct PCIBus {
@@ -63,4 +65,15 @@ static inline bool pci_bus_allows_extended_config_space(PCIBus *bus)
     return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
 }
 
+static inline bool pci_bus_has_iommu(PCIBus *bus)
+{
+    PCIBus *root_bus = bus;
+
+    while (root_bus && !pci_bus_is_root(root_bus)) {
+        root_bus = pci_get_bus(root_bus->parent_dev);
+    }
+
+    return !!(root_bus->flags & PCI_BUS_IOMMU);
+}
+
 #endif /* QEMU_PCI_BUS_H */
-- 
2.19.1

[RFC RESEND PATCH 2/4] 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. Default option is set to true, and the option
is valid only if the iommu option for machine is properly set.

Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
 hw/arm/virt.c                       | 29 +++++++++++++++++++++++++++++
 hw/pci-bridge/pci_expander_bridge.c |  6 ++++++
 hw/pci/pci.c                        |  2 +-
 include/hw/arm/virt.h               |  1 +
 4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 371147f3ae..0c9e549759 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -79,6 +79,7 @@
 #include "hw/virtio/virtio-iommu.h"
 #include "hw/char/pl011.h"
 #include "qemu/guest-random.h"
+#include "include/hw/pci/pci_bus.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -1232,6 +1233,10 @@ static void create_smmu(const VirtMachineState *vms,
 
     dev = qdev_new("arm-smmuv3");
 
+    if (vms->primary_bus_iommu) {
+        bus->flags |= PCI_BUS_IOMMU;
+    }
+
     object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
                              &error_abort);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
@@ -2305,6 +2310,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)
 {
@@ -2629,6 +2648,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",
@@ -2696,6 +2722,9 @@ static void virt_instance_init(Object *obj)
     /* Default disallows iommu instantiation */
     vms->iommu = VIRT_IOMMU_NONE;
 
+    /* Iommu is enabled by default for primary bus */
+    vms->primary_bus_iommu = true;
+
     /* Default disallows RAS instantiation */
     vms->ras = false;
 
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index aedded1064..0412656265 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)
@@ -254,6 +255,10 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
     bus->address_space_io = pci_get_bus(dev)->address_space_io;
     bus->map_irq = pxb_map_irq_fn;
 
+    if (pxb->iommu) {
+        bus->flags |= PCI_BUS_IOMMU;
+    }
+
     PCI_HOST_BRIDGE(ds)->bus = bus;
 
     pxb_register_bus(dev, bus, &local_err);
@@ -301,6 +306,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/pci.c b/hw/pci/pci.c
index a9ebef8a35..dc969989c9 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2712,7 +2712,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
 
         iommu_bus = parent_bus;
     }
-    if (iommu_bus && iommu_bus->iommu_fn) {
+    if (pci_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/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ee9a93101e..babe829486 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;
-- 
2.19.1

[RFC RESEND PATCH 3/4] 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         | 33 +++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h |  1 +
 2 files changed, 34 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index dc969989c9..ed92ce0971 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -516,6 +516,39 @@ 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;
+    int max_bus = 0;
+    int type;
+    int devfn;
+
+    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++) {
+        PCIDevice *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) {
+            uint8_t 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 1bc231480f..238b91817a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -449,6 +449,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

[RFC RESEND PATCH 4/4] 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 | 92 ++++++++++++++++++++++++++++++----------
 1 file changed, 69 insertions(+), 23 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f9c9df916c..38ab700ad9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -54,6 +54,7 @@
 #include "kvm_arm.h"
 #include "migration/vmstate.h"
 #include "hw/acpi/ghes.h"
+#include "hw/pci/pci_bus.h"
 
 #define ARM_SPI_BASE 32
 
@@ -247,9 +248,36 @@ 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;
+    PCIBus *bus = vms->bus;
+    GArray *root_bus_array;
+    size_t root_bus_count = 0;
+    size_t root_bus_smmu_count = 0;
+    int bus_num, max_bus, index;
+
+    root_bus_array = g_array_new(false, true, sizeof(PCIBus *));
 
     iort = acpi_data_push(table_data, sizeof(*iort));
 
+    g_array_append_val(root_bus_array, bus);
+    root_bus_count++;
+    if (vms->iommu == VIRT_IOMMU_SMMUV3 && pci_bus_has_iommu(bus)) {
+        root_bus_smmu_count++;
+    }
+
+    QLIST_FOREACH(bus, &bus->child, sibling) {
+
+        if (!pci_bus_is_root(bus)) {
+            continue;
+        }
+
+        g_array_append_val(root_bus_array, bus);
+        root_bus_count++;
+
+        if (vms->iommu == VIRT_IOMMU_SMMUV3 && pci_bus_has_iommu(bus)) {
+            root_bus_smmu_count++;
+        }
+    }
+
     if (vms->iommu == VIRT_IOMMU_SMMUV3) {
         nb_nodes = 3; /* RC, ITS, SMMUv3 */
     } else {
@@ -280,13 +308,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) * root_bus_smmu_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(root_bus_smmu_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 +323,34 @@ 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);
+        index = 0;
+        for (int i = 0; i < root_bus_count; i++) {
+            bus = g_array_index(root_bus_array, PCIBus *, i);
+
+            if (!pci_bus_has_iommu(bus)) {
+                continue;
+            }
+
+            bus_num = pci_bus_num(bus);
+            max_bus = pci_root_bus_max_bus(bus);
+
+            idmap = &smmu->id_mapping_array[index++];
+            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);
+            /* 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) * root_bus_count;
     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(root_bus_count);
     rc->mapping_offset = cpu_to_le32(sizeof(*rc));
 
     /* fully coherent device */
@@ -319,18 +358,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;
-
-    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);
+    for (int i = 0; i < root_bus_count; i++) {
+        bus = g_array_index(root_bus_array, PCIBus *, i);
+        bus_num = pci_bus_num(bus);
+        max_bus = pci_root_bus_max_bus(bus);
+
+        idmap = &rc->id_mapping_array[i];
+        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);
+
+        if (vms->iommu == VIRT_IOMMU_SMMUV3 && pci_bus_has_iommu(bus)) {
+            /* 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);
+        }
     }
 
     /*
@@ -343,6 +387,8 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     build_header(linker, table_data, (void *)(table_data->data + iort_start),
                  "IORT", table_data->len - iort_start, 0, vms->oem_id,
                  vms->oem_table_id);
+
+    g_array_free(root_bus_array, true);
 }
 
 static void
-- 
2.19.1

Re: [RFC RESEND PATCH 0/4] hw/arm/virt-acpi-build: Introduce iommu option for pci root bus

[Wang Xingang]

Hi, everyone! Do you have any suggestions? Please help review these 
patches, thanks very much.

On 2021/2/27 16:33, 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.
> 
> Xingang Wang (4):
>    pci: Add PCI_BUS_IOMMU property
>    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/arm/virt-acpi-build.c            | 92 +++++++++++++++++++++--------
>   hw/arm/virt.c                       | 29 +++++++++
>   hw/pci-bridge/pci_expander_bridge.c |  6 ++
>   hw/pci/pci.c                        | 35 ++++++++++-
>   include/hw/arm/virt.h               |  1 +
>   include/hw/pci/pci.h                |  1 +
>   include/hw/pci/pci_bus.h            | 13 ++++
>   7 files changed, 153 insertions(+), 24 deletions(-)
> 

Re: [RFC RESEND PATCH 0/4] hw/arm/virt-acpi-build: Introduce iommu option for pci root bus

[Auger Eric]

Hi,
On 2/27/21 9:33 AM, 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.

Please could you give an example of the qemu command line for which the
new option is useful for you. This would help me to understand your
pcie/pci topology and also make sure I test it with the smmu.

Thank you in advance

Best Regards

Eric
> 
> Xingang Wang (4):
>   pci: Add PCI_BUS_IOMMU property
>   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/arm/virt-acpi-build.c            | 92 +++++++++++++++++++++--------
>  hw/arm/virt.c                       | 29 +++++++++
>  hw/pci-bridge/pci_expander_bridge.c |  6 ++
>  hw/pci/pci.c                        | 35 ++++++++++-
>  include/hw/arm/virt.h               |  1 +
>  include/hw/pci/pci.h                |  1 +
>  include/hw/pci/pci_bus.h            | 13 ++++
>  7 files changed, 153 insertions(+), 24 deletions(-)
> 

Re: [RFC RESEND PATCH 0/4] hw/arm/virt-acpi-build: Introduce iommu option for pci root bus

[Wang Xingang]

Hi Eric,

On 2021/3/9 22:36, Auger Eric wrote:
> Hi,
> On 2/27/21 9:33 AM, 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.
> 
> Please could you give an example of the qemu command line for which the
> new option is useful for you. This would help me to understand your
> pcie/pci topology and also make sure I test it with the smmu.
> 
> Thank you in advance
> 
> Best Regards
> 
> Eric
>>
>> Xingang Wang (4):
>>    pci: Add PCI_BUS_IOMMU property
>>    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/arm/virt-acpi-build.c            | 92 +++++++++++++++++++++--------
>>   hw/arm/virt.c                       | 29 +++++++++
>>   hw/pci-bridge/pci_expander_bridge.c |  6 ++
>>   hw/pci/pci.c                        | 35 ++++++++++-
>>   include/hw/arm/virt.h               |  1 +
>>   include/hw/pci/pci.h                |  1 +
>>   include/hw/pci/pci_bus.h            | 13 ++++
>>   7 files changed, 153 insertions(+), 24 deletions(-)
>>
> 
> .
> 

Thanks for your advice.

I test this with the following script, in which i add two options.

The option `primary_bus_iommu=false(or true)` for `-machine 
virt,iommu=smmuv3`, this helps to enable/disable whether primary bus go 
through iommu.

The other option `iommu=false` or `iommu=true` for `-device pxb-pcie` 
helps to enable/disable whether pxb root bus go through iommu.

> #!/bin/sh
> 
> /path/to/qemu/build/aarch64-softmmu/qemu-system-aarch64 \
> -enable-kvm \
> -cpu host \
> -kernel /path/to/linux/arch/arm64/boot/Image \
> -m 16G \
> -smp 8,sockets=8,cores=1,threads=1 \
> -machine virt,kernel_irqchip=on,gic-version=3,iommu=smmuv3,primary_bus_iommu=false \
> -drive file=./QEMU_EFI-pflash.raw,if=pflash,format=raw,unit=0,readonly=on \
> -device pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,addr=0x3.0x1,iommu=false \
> -device pxb-pcie,bus_nr=0x20,id=pci.20,bus=pcie.0,addr=0x3.0x2,iommu=true \
> -device pxb-pcie,bus_nr=0x23,id=pci.30,bus=pcie.0,addr=0x3.0x3,iommu=true \
> -device pxb-pcie,bus_nr=0x40,id=pci.40,bus=pcie.0,addr=0x3.0x4,iommu=false \
> -device pcie-pci-bridge,id=pci.11,bus=pci.10,addr=0x1 \
> -device pcie-pci-bridge,id=pci.21,bus=pci.20,addr=0x1 \
> -device pcie-root-port,port=0x20,chassis=10,id=pci.2,bus=pcie.0,addr=0x2 \
> -device pcie-root-port,port=0x20,chassis=11,id=pci.12,bus=pci.10,addr=0x2 \
> -device pcie-root-port,port=0x20,chassis=19,id=pci.19,bus=pci.11,addr=0x3 \
> -device pcie-root-port,port=0x20,chassis=12,id=pci.22,bus=pci.20,addr=0x2 \
> -device pcie-root-port,port=0x20,chassis=13,id=pci.42,bus=pci.40,addr=0x2 \
> -device virtio-scsi-pci,id=scsi0,bus=pci.12,addr=0x1 \
> -device vfio-pci,host=b5:00.2,bus=pci.42,addr=0x0,id=acc2 \
> -net none \
> -initrd /path/to/rootfs.cpio.gz \
> -nographic \
> -append "rdinit=init console=ttyAMA0 earlycon=pl011,0x9000000 nokaslr" \

I test the command line with an accelerator. The IORT table will have 
some changes, so only the root bus with iommu=true will go through smmuv3.

Thanks,
Xingang
.

Re: [RFC RESEND PATCH 0/4] hw/arm/virt-acpi-build: Introduce iommu option for pci root bus

[Auger Eric]

Hi Xingang,

On 3/10/21 3:13 AM, Wang Xingang wrote:
> Hi Eric,
> 
> On 2021/3/9 22:36, Auger Eric wrote:
>> Hi,
>> On 2/27/21 9:33 AM, 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.
>>
>> Please could you give an example of the qemu command line for which the
>> new option is useful for you. This would help me to understand your
>> pcie/pci topology and also make sure I test it with the smmu.
>>
>> Thank you in advance
>>
>> Best Regards
>>
>> Eric
>>>
>>> Xingang Wang (4):
>>>    pci: Add PCI_BUS_IOMMU property
>>>    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/arm/virt-acpi-build.c            | 92 +++++++++++++++++++++--------
>>>   hw/arm/virt.c                       | 29 +++++++++
>>>   hw/pci-bridge/pci_expander_bridge.c |  6 ++
>>>   hw/pci/pci.c                        | 35 ++++++++++-
>>>   include/hw/arm/virt.h               |  1 +
>>>   include/hw/pci/pci.h                |  1 +
>>>   include/hw/pci/pci_bus.h            | 13 ++++
>>>   7 files changed, 153 insertions(+), 24 deletions(-)
>>>
>>
>> .
>>
> 
> Thanks for your advice.
> 
> I test this with the following script, in which i add two options.
> 
> The option `primary_bus_iommu=false(or true)` for `-machine
> virt,iommu=smmuv3`, this helps to enable/disable whether primary bus go
> through iommu.
> 
> The other option `iommu=false` or `iommu=true` for `-device pxb-pcie`
> helps to enable/disable whether pxb root bus go through iommu.
> 
>> #!/bin/sh
>>
>> /path/to/qemu/build/aarch64-softmmu/qemu-system-aarch64 \
>> -enable-kvm \
>> -cpu host \
>> -kernel /path/to/linux/arch/arm64/boot/Image \
>> -m 16G \
>> -smp 8,sockets=8,cores=1,threads=1 \
>> -machine
>> virt,kernel_irqchip=on,gic-version=3,iommu=smmuv3,primary_bus_iommu=false
>> \
>> -drive
>> file=./QEMU_EFI-pflash.raw,if=pflash,format=raw,unit=0,readonly=on \
>> -device
>> pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,addr=0x3.0x1,iommu=false \
>> -device
>> pxb-pcie,bus_nr=0x20,id=pci.20,bus=pcie.0,addr=0x3.0x2,iommu=true \
>> -device
>> pxb-pcie,bus_nr=0x23,id=pci.30,bus=pcie.0,addr=0x3.0x3,iommu=true \
>> -device
>> pxb-pcie,bus_nr=0x40,id=pci.40,bus=pcie.0,addr=0x3.0x4,iommu=false \
>> -device pcie-pci-bridge,id=pci.11,bus=pci.10,addr=0x1 \
>> -device pcie-pci-bridge,id=pci.21,bus=pci.20,addr=0x1 \
>> -device
>> pcie-root-port,port=0x20,chassis=10,id=pci.2,bus=pcie.0,addr=0x2 \
>> -device
>> pcie-root-port,port=0x20,chassis=11,id=pci.12,bus=pci.10,addr=0x2 \
>> -device
>> pcie-root-port,port=0x20,chassis=19,id=pci.19,bus=pci.11,addr=0x3 \
>> -device
>> pcie-root-port,port=0x20,chassis=12,id=pci.22,bus=pci.20,addr=0x2 \
>> -device
>> pcie-root-port,port=0x20,chassis=13,id=pci.42,bus=pci.40,addr=0x2 \
>> -device virtio-scsi-pci,id=scsi0,bus=pci.12,addr=0x1 \
>> -device vfio-pci,host=b5:00.2,bus=pci.42,addr=0x0,id=acc2 \
>> -net none \
>> -initrd /path/to/rootfs.cpio.gz \
>> -nographic \
>> -append "rdinit=init console=ttyAMA0 earlycon=pl011,0x9000000 nokaslr" \
> 
> I test the command line with an accelerator. The IORT table will have
> some changes, so only the root bus with iommu=true will go through smmuv3.

Thank you for sharing your command line.

On my end without using ",iommu=smmuv3" and the new options, my guest
crashes.

    0.833665] ACPI: PCI Root Bridge [PC0A] (domain 0000 [bus 0a-0b])
[    0.837630] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
ClockPM Segments MSI HPX-Type3]
[    0.843377] acpi PNP0A08:00: _OSC: platform does not support [LTR]
[    0.846796] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
AER PCIeCapability]
[    0.851082] acpi PNP0A08:00: ECAM area [mem
0x4010a00000-0x4010bfffff] reserved by PNP0C02:00
[    0.854742] acpi PNP0A08:00: ECAM at [mem 0x4010a00000-0x4010bfffff]
for [bus 0a-0b]
[    0.859569] ------------[ cut here ]------------
[    0.862470] kernel BUG at mm/ioremap.c:76!
[    0.865066] Internal error: Oops - BUG: 0 [#1] SMP
[    0.868130] Modules linked in:
[    0.870060] CPU: 6 PID: 1 Comm: swapper/0 Not tainted
5.11.0-rc6-guest-upstream+ #26
[    0.874920] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0
02/06/2015
[    0.879283] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO BTYPE=--)
[    0.883055] pc : ioremap_page_range+0x33c/0x3e0
[    0.885942] lr : ioremap_page_range+0x30/0x3e0
[    0.888737] sp : ffff80001272f800
[    0.890824] x29: ffff80001272f800 x28: ffffffbffe801000
[    0.894168] x27: ffffffc020040000 x26: ffff8000111b01f8
[    0.897543] x25: 0400000000000001 x24: ffffffbffe800000
[    0.900882] x23: 000000003eff3000 x22: ffffffbffe801000
[    0.904221] x21: ffffffbffe801000 x20: ffff0003f2270020
[    0.907612] x19: 0000000000000001 x18: 0000000000000030
[    0.910952] x17: 0000000000000000 x16: 0000000000000001
[    0.914283] x15: ffffffffffffffff x14: ffff8000116d49c8
[    0.917679] x13: 000000003eff3000 x12: 0000000000000041
[    0.921018] x11: ffff800011f2f000 x10: 000000000000002e
[    0.924359] x9 : ffff800010c372c4 x8 : ffffffbffe800000
[    0.927744] x7 : ffff0003f224eff8 x6 : 0000000000000001
[    0.931092] x5 : ffffffbffe800fff x4 : ffff8000116de650
[    0.934430] x3 : 0068000000000f17 x2 : 0140000000000000
[    0.937813] x1 : 00000040407f0000 x0 : ffff0003ffdcccc0
[    0.941165] Call trace:
[    0.942713]  ioremap_page_range+0x33c/0x3e0
[    0.945374]  pci_remap_iospace+0x7c/0x90
[    0.947881]  acpi_pci_probe_root_resources+0x180/0x238
[    0.951122]  pci_acpi_root_prepare_resources+0x28/0xc8
[    0.954357]  acpi_pci_root_create+0x9c/0x2f8
[    0.956990]  pci_acpi_scan_root+0x150/0x240
[    0.959639]  acpi_pci_root_add+0x34c/0x4e0
[    0.962220]  acpi_bus_attach+0x15c/0x2c0
[    0.964692]  acpi_bus_attach+0x9c/0x2c0
[    0.967135]  acpi_bus_attach+0x9c/0x2c0
[    0.969582]  acpi_bus_scan+0x64/0x118
[    0.971888]  acpi_scan_init+0x10c/0x244
[    0.974302]  acpi_init+0x2bc/0x328
[    0.976463]  do_one_initcall+0x54/0x268
[    0.978913]  kernel_init_freeable+0x22c/0x2c4
[    0.981658]  kernel_init+0x1c/0x128
[    0.983864]  ret_from_fork+0x10/0x34
[    0.986139] Code: a9446bf9 a8cb7bfd d50323bf d65f03c0 (d4210000)
[    0.990037] ---[ end trace fc68f309d1db57e3 ]---
[    0.992939] Kernel panic - not syncing: Oops - BUG: Fatal exception
[    0.996893] SMP: stopping secondary CPUs
[    0.999487] ---[ end Kernel panic - not syncing: Oops - BUG: Fatal
exception ]---

Do you have any idea. I am using

http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/4198/QEMU-AARCH64/RELEASE_GCC5/

Thanks

Eric




> 
> Thanks,
> Xingang
> .
> 

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

[Auger Eric]

Hi Xingang,

On 2/27/21 9:33 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. Default option is set to true, and the option
> is valid only if the iommu option for machine is properly set.
> 
> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> ---
>  hw/arm/virt.c                       | 29 +++++++++++++++++++++++++++++
>  hw/pci-bridge/pci_expander_bridge.c |  6 ++++++
>  hw/pci/pci.c                        |  2 +-
>  include/hw/arm/virt.h               |  1 +
>  4 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 371147f3ae..0c9e549759 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -79,6 +79,7 @@
>  #include "hw/virtio/virtio-iommu.h"
>  #include "hw/char/pl011.h"
>  #include "qemu/guest-random.h"
> +#include "include/hw/pci/pci_bus.h"
>  
>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -1232,6 +1233,10 @@ static void create_smmu(const VirtMachineState *vms,
>  
>      dev = qdev_new("arm-smmuv3");
>  
> +    if (vms->primary_bus_iommu) {
> +        bus->flags |= PCI_BUS_IOMMU;
> +    }
> +
>      object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>                               &error_abort);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> @@ -2305,6 +2310,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)
>  {
> @@ -2629,6 +2648,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",
> @@ -2696,6 +2722,9 @@ static void virt_instance_init(Object *obj)
>      /* Default disallows iommu instantiation */
>      vms->iommu = VIRT_IOMMU_NONE;
>  
> +    /* Iommu is enabled by default for primary bus */
> +    vms->primary_bus_iommu = true;
> +
>      /* Default disallows RAS instantiation */
>      vms->ras = false;
>  
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index aedded1064..0412656265 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)
> @@ -254,6 +255,10 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
>      bus->address_space_io = pci_get_bus(dev)->address_space_io;
>      bus->map_irq = pxb_map_irq_fn;
>  
> +    if (pxb->iommu) {
> +        bus->flags |= PCI_BUS_IOMMU;
> +    }
> +
>      PCI_HOST_BRIDGE(ds)->bus = bus;
>  
>      pxb_register_bus(dev, bus, &local_err);
> @@ -301,6 +306,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),
looks a bit odd to me that we have a property for the PXE-PCIe extra
root complex and not for the gpex device. Wouldn't it make sense to add
one for the GPEX too? In the positive you still could have a machine
option that would force the GPEX property value?
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a9ebef8a35..dc969989c9 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2712,7 +2712,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>  
>          iommu_bus = parent_bus;
>      }
> -    if (iommu_bus && iommu_bus->iommu_fn) {
> +    if (pci_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/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index ee9a93101e..babe829486 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;
> 
Thanks

Eric

Re: [RFC RESEND PATCH 1/4] pci: Add PCI_BUS_IOMMU property

[Auger Eric]

Hi Xingang,

On 2/27/21 9:33 AM, Wang Xingang wrote:
> From: Xingang Wang <wangxingang5@huawei.com>
> 
> This Property can be useful to check whether this bus is attached to iommu.

Strictly speaking this is not a Property (QEMU property) but a flag
> 
> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> ---
>  include/hw/pci/pci_bus.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 347440d42c..42109e8a06 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -24,6 +24,8 @@ enum PCIBusFlags {
>      PCI_BUS_IS_ROOT                                         = 0x0001,
>      /* PCIe extended configuration space is accessible on this bus */
>      PCI_BUS_EXTENDED_CONFIG_SPACE                           = 0x0002,
> +    /* Iommu is enabled on this bus */
s/Iommu/IOMMU here and elsewhere
> +    PCI_BUS_IOMMU                                           = 0x0004,
>  };
>  
>  struct PCIBus {
> @@ -63,4 +65,15 @@ static inline bool pci_bus_allows_extended_config_space(PCIBus *bus)
>      return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
>  }
>  
> +static inline bool pci_bus_has_iommu(PCIBus *bus)
> +{
> +    PCIBus *root_bus = bus;
> +
> +    while (root_bus && !pci_bus_is_root(root_bus)) {
> +        root_bus = pci_get_bus(root_bus->parent_dev);
> +    }
> +
> +    return !!(root_bus->flags & PCI_BUS_IOMMU);
> +}
> +
>  #endif /* QEMU_PCI_BUS_H */
> 
Eric

Re: [RFC RESEND PATCH 0/4] hw/arm/virt-acpi-build: Introduce iommu option for pci root bus

[Wang Xingang]

Hi Eric,

On 2021/3/10 18:18, Auger Eric wrote:
> Hi Xingang,
> 
> On 3/10/21 3:13 AM, Wang Xingang wrote:
>> Hi Eric,
>>
>> On 2021/3/9 22:36, Auger Eric wrote:
>>> Hi,
>>> On 2/27/21 9:33 AM, 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.
>>>
>>> Please could you give an example of the qemu command line for which the
>>> new option is useful for you. This would help me to understand your
>>> pcie/pci topology and also make sure I test it with the smmu.
>>>
>>> Thank you in advance
>>>
>>> Best Regards
>>>
>>> Eric
>>>>
>>>> Xingang Wang (4):
>>>>     pci: Add PCI_BUS_IOMMU property
>>>>     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/arm/virt-acpi-build.c            | 92 +++++++++++++++++++++--------
>>>>    hw/arm/virt.c                       | 29 +++++++++
>>>>    hw/pci-bridge/pci_expander_bridge.c |  6 ++
>>>>    hw/pci/pci.c                        | 35 ++++++++++-
>>>>    include/hw/arm/virt.h               |  1 +
>>>>    include/hw/pci/pci.h                |  1 +
>>>>    include/hw/pci/pci_bus.h            | 13 ++++
>>>>    7 files changed, 153 insertions(+), 24 deletions(-)
>>>>
>>>
>>> .
>>>
>>
>> Thanks for your advice.
>>
>> I test this with the following script, in which i add two options.
>>
>> The option `primary_bus_iommu=false(or true)` for `-machine
>> virt,iommu=smmuv3`, this helps to enable/disable whether primary bus go
>> through iommu.
>>
>> The other option `iommu=false` or `iommu=true` for `-device pxb-pcie`
>> helps to enable/disable whether pxb root bus go through iommu.
>>
>>> #!/bin/sh
>>>
>>> /path/to/qemu/build/aarch64-softmmu/qemu-system-aarch64 \
>>> -enable-kvm \
>>> -cpu host \
>>> -kernel /path/to/linux/arch/arm64/boot/Image \
>>> -m 16G \
>>> -smp 8,sockets=8,cores=1,threads=1 \
>>> -machine
>>> virt,kernel_irqchip=on,gic-version=3,iommu=smmuv3,primary_bus_iommu=false
>>> \
>>> -drive
>>> file=./QEMU_EFI-pflash.raw,if=pflash,format=raw,unit=0,readonly=on \
>>> -device
>>> pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,addr=0x3.0x1,iommu=false \
>>> -device
>>> pxb-pcie,bus_nr=0x20,id=pci.20,bus=pcie.0,addr=0x3.0x2,iommu=true \
>>> -device
>>> pxb-pcie,bus_nr=0x23,id=pci.30,bus=pcie.0,addr=0x3.0x3,iommu=true \
>>> -device
>>> pxb-pcie,bus_nr=0x40,id=pci.40,bus=pcie.0,addr=0x3.0x4,iommu=false \
>>> -device pcie-pci-bridge,id=pci.11,bus=pci.10,addr=0x1 \
>>> -device pcie-pci-bridge,id=pci.21,bus=pci.20,addr=0x1 \
>>> -device
>>> pcie-root-port,port=0x20,chassis=10,id=pci.2,bus=pcie.0,addr=0x2 \
>>> -device
>>> pcie-root-port,port=0x20,chassis=11,id=pci.12,bus=pci.10,addr=0x2 \
>>> -device
>>> pcie-root-port,port=0x20,chassis=19,id=pci.19,bus=pci.11,addr=0x3 \
>>> -device
>>> pcie-root-port,port=0x20,chassis=12,id=pci.22,bus=pci.20,addr=0x2 \
>>> -device
>>> pcie-root-port,port=0x20,chassis=13,id=pci.42,bus=pci.40,addr=0x2 \
>>> -device virtio-scsi-pci,id=scsi0,bus=pci.12,addr=0x1 \
>>> -device vfio-pci,host=b5:00.2,bus=pci.42,addr=0x0,id=acc2 \
>>> -net none \
>>> -initrd /path/to/rootfs.cpio.gz \
>>> -nographic \
>>> -append "rdinit=init console=ttyAMA0 earlycon=pl011,0x9000000 nokaslr" \
>>
>> I test the command line with an accelerator. The IORT table will have
>> some changes, so only the root bus with iommu=true will go through smmuv3.
> 
> Thank you for sharing your command line.
> 
> On my end without using ",iommu=smmuv3" and the new options, my guest
> crashes.
> 
>      0.833665] ACPI: PCI Root Bridge [PC0A] (domain 0000 [bus 0a-0b])
> [    0.837630] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
> ClockPM Segments MSI HPX-Type3]
> [    0.843377] acpi PNP0A08:00: _OSC: platform does not support [LTR]
> [    0.846796] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
> AER PCIeCapability]
> [    0.851082] acpi PNP0A08:00: ECAM area [mem
> 0x4010a00000-0x4010bfffff] reserved by PNP0C02:00
> [    0.854742] acpi PNP0A08:00: ECAM at [mem 0x4010a00000-0x4010bfffff]
> for [bus 0a-0b]
> [    0.859569] ------------[ cut here ]------------
> [    0.862470] kernel BUG at mm/ioremap.c:76!
> [    0.865066] Internal error: Oops - BUG: 0 [#1] SMP
> [    0.868130] Modules linked in:
> [    0.870060] CPU: 6 PID: 1 Comm: swapper/0 Not tainted
> 5.11.0-rc6-guest-upstream+ #26
> [    0.874920] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0
> 02/06/2015
> [    0.879283] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO BTYPE=--)
> [    0.883055] pc : ioremap_page_range+0x33c/0x3e0
> [    0.885942] lr : ioremap_page_range+0x30/0x3e0
> [    0.888737] sp : ffff80001272f800
> [    0.890824] x29: ffff80001272f800 x28: ffffffbffe801000
> [    0.894168] x27: ffffffc020040000 x26: ffff8000111b01f8
> [    0.897543] x25: 0400000000000001 x24: ffffffbffe800000
> [    0.900882] x23: 000000003eff3000 x22: ffffffbffe801000
> [    0.904221] x21: ffffffbffe801000 x20: ffff0003f2270020
> [    0.907612] x19: 0000000000000001 x18: 0000000000000030
> [    0.910952] x17: 0000000000000000 x16: 0000000000000001
> [    0.914283] x15: ffffffffffffffff x14: ffff8000116d49c8
> [    0.917679] x13: 000000003eff3000 x12: 0000000000000041
> [    0.921018] x11: ffff800011f2f000 x10: 000000000000002e
> [    0.924359] x9 : ffff800010c372c4 x8 : ffffffbffe800000
> [    0.927744] x7 : ffff0003f224eff8 x6 : 0000000000000001
> [    0.931092] x5 : ffffffbffe800fff x4 : ffff8000116de650
> [    0.934430] x3 : 0068000000000f17 x2 : 0140000000000000
> [    0.937813] x1 : 00000040407f0000 x0 : ffff0003ffdcccc0
> [    0.941165] Call trace:
> [    0.942713]  ioremap_page_range+0x33c/0x3e0
> [    0.945374]  pci_remap_iospace+0x7c/0x90
> [    0.947881]  acpi_pci_probe_root_resources+0x180/0x238
> [    0.951122]  pci_acpi_root_prepare_resources+0x28/0xc8
> [    0.954357]  acpi_pci_root_create+0x9c/0x2f8
> [    0.956990]  pci_acpi_scan_root+0x150/0x240
> [    0.959639]  acpi_pci_root_add+0x34c/0x4e0
> [    0.962220]  acpi_bus_attach+0x15c/0x2c0
> [    0.964692]  acpi_bus_attach+0x9c/0x2c0
> [    0.967135]  acpi_bus_attach+0x9c/0x2c0
> [    0.969582]  acpi_bus_scan+0x64/0x118
> [    0.971888]  acpi_scan_init+0x10c/0x244
> [    0.974302]  acpi_init+0x2bc/0x328
> [    0.976463]  do_one_initcall+0x54/0x268
> [    0.978913]  kernel_init_freeable+0x22c/0x2c4
> [    0.981658]  kernel_init+0x1c/0x128
> [    0.983864]  ret_from_fork+0x10/0x34
> [    0.986139] Code: a9446bf9 a8cb7bfd d50323bf d65f03c0 (d4210000)
> [    0.990037] ---[ end trace fc68f309d1db57e3 ]---
> [    0.992939] Kernel panic - not syncing: Oops - BUG: Fatal exception
> [    0.996893] SMP: stopping secondary CPUs
> [    0.999487] ---[ end Kernel panic - not syncing: Oops - BUG: Fatal
> exception ]---
> 
> Do you have any idea. I am using
> 
> http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/4198/QEMU-AARCH64/RELEASE_GCC5/
> 
> Thanks
> 
> Eric
> 
> 
> 
> 
>>
>> Thanks,
>> Xingang
>> .
>>
> 
> .
> 

I retest with the QEMU_EFI.fd and QEMU.img.gz you provided, and i don't 
have this issue. Could you please provide more information about how you 
test, and is everything ok with the iommu=smmuv3 on ?

 From the debug info, it might be the problem of building the ACPI IORT 
table. And could you please retest without the last patch, and see if 
everything is ok.

Thanks.

Xingang

.

Re: [RFC RESEND PATCH 1/4] pci: Add PCI_BUS_IOMMU property

[Wang Xingang]

Hi Eric,

On 2021/3/10 18:25, Auger Eric wrote:
> Hi Xingang,
> 
> On 2/27/21 9:33 AM, Wang Xingang wrote:
>> From: Xingang Wang <wangxingang5@huawei.com>
>>
>> This Property can be useful to check whether this bus is attached to iommu.
> 
> Strictly speaking this is not a Property (QEMU property) but a flag
>>
>> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> ---
>>   include/hw/pci/pci_bus.h | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
>> index 347440d42c..42109e8a06 100644
>> --- a/include/hw/pci/pci_bus.h
>> +++ b/include/hw/pci/pci_bus.h
>> @@ -24,6 +24,8 @@ enum PCIBusFlags {
>>       PCI_BUS_IS_ROOT                                         = 0x0001,
>>       /* PCIe extended configuration space is accessible on this bus */
>>       PCI_BUS_EXTENDED_CONFIG_SPACE                           = 0x0002,
>> +    /* Iommu is enabled on this bus */
> s/Iommu/IOMMU here and elsewhere
>> +    PCI_BUS_IOMMU                                           = 0x0004,
>>   };
>>   
>>   struct PCIBus {
>> @@ -63,4 +65,15 @@ static inline bool pci_bus_allows_extended_config_space(PCIBus *bus)
>>       return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
>>   }
>>   
>> +static inline bool pci_bus_has_iommu(PCIBus *bus)
>> +{
>> +    PCIBus *root_bus = bus;
>> +
>> +    while (root_bus && !pci_bus_is_root(root_bus)) {
>> +        root_bus = pci_get_bus(root_bus->parent_dev);
>> +    }
>> +
>> +    return !!(root_bus->flags & PCI_BUS_IOMMU);
>> +}
>> +
>>   #endif /* QEMU_PCI_BUS_H */
>>
> Eric
> 
> .
> 

Thanks, i will fix this.

Xingang

.

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

[Wang Xingang]

Hi Eric,

On 2021/3/10 18:24, Auger Eric wrote:
> Hi Xingang,
> 
> On 2/27/21 9:33 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. Default option is set to true, and the option
>> is valid only if the iommu option for machine is properly set.
>>
>> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> ---
>>   hw/arm/virt.c                       | 29 +++++++++++++++++++++++++++++
>>   hw/pci-bridge/pci_expander_bridge.c |  6 ++++++
>>   hw/pci/pci.c                        |  2 +-
>>   include/hw/arm/virt.h               |  1 +
>>   4 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 371147f3ae..0c9e549759 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -79,6 +79,7 @@
>>   #include "hw/virtio/virtio-iommu.h"
>>   #include "hw/char/pl011.h"
>>   #include "qemu/guest-random.h"
>> +#include "include/hw/pci/pci_bus.h"
>>   
>>   #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>>       static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
>> @@ -1232,6 +1233,10 @@ static void create_smmu(const VirtMachineState *vms,
>>   
>>       dev = qdev_new("arm-smmuv3");
>>   
>> +    if (vms->primary_bus_iommu) {
>> +        bus->flags |= PCI_BUS_IOMMU;
>> +    }
>> +
>>       object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>>                                &error_abort);
>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> @@ -2305,6 +2310,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)
>>   {
>> @@ -2629,6 +2648,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",
>> @@ -2696,6 +2722,9 @@ static void virt_instance_init(Object *obj)
>>       /* Default disallows iommu instantiation */
>>       vms->iommu = VIRT_IOMMU_NONE;
>>   
>> +    /* Iommu is enabled by default for primary bus */
>> +    vms->primary_bus_iommu = true;
>> +
>>       /* Default disallows RAS instantiation */
>>       vms->ras = false;
>>   
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>> index aedded1064..0412656265 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)
>> @@ -254,6 +255,10 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
>>       bus->address_space_io = pci_get_bus(dev)->address_space_io;
>>       bus->map_irq = pxb_map_irq_fn;
>>   
>> +    if (pxb->iommu) {
>> +        bus->flags |= PCI_BUS_IOMMU;
>> +    }
>> +
>>       PCI_HOST_BRIDGE(ds)->bus = bus;
>>   
>>       pxb_register_bus(dev, bus, &local_err);
>> @@ -301,6 +306,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),
> looks a bit odd to me that we have a property for the PXE-PCIe extra
> root complex and not for the gpex device. Wouldn't it make sense to add
> one for the GPEX too? In the positive you still could have a machine
> option that would force the GPEX property value?

Indeed it makes sense to add one property for GPEX too.However, the 
iommu property for PXBDev only helps to add option in qemu command line.
When it is necessary to check whether the iommu is enabled on the root 
bus, it would be better to access the bus flag. In qemu, the pxb is not 
related to GPEX currently, and i do not find proper position to add the 
iommu property for GPEX, you might have some good idea for that.

>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index a9ebef8a35..dc969989c9 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2712,7 +2712,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>>   
>>           iommu_bus = parent_bus;
>>       }
>> -    if (iommu_bus && iommu_bus->iommu_fn) {
>> +    if (pci_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/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index ee9a93101e..babe829486 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;
>>
> Thanks
> 
> Eric
> 
> .
> 

Thanks

Xingang

.

Re: [RFC RESEND PATCH 0/4] hw/arm/virt-acpi-build: Introduce iommu option for pci root bus

[Auger Eric]

Hi Xingang,

On 3/11/21 12:57 PM, Wang Xingang wrote:
> Hi Eric,
> 
> On 2021/3/10 18:18, Auger Eric wrote:
>> Hi Xingang,
>>
>> On 3/10/21 3:13 AM, Wang Xingang wrote:
>>> Hi Eric,
>>>
>>> On 2021/3/9 22:36, Auger Eric wrote:
>>>> Hi,
>>>> On 2/27/21 9:33 AM, 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.
>>>>
>>>> Please could you give an example of the qemu command line for which the
>>>> new option is useful for you. This would help me to understand your
>>>> pcie/pci topology and also make sure I test it with the smmu.

It looks like a guest issue. I have switched to a fedora guest and it
works now with the following command line:

./build/qemu-system-aarch64 -M virt,gic-version=host -cpu host \
-smp 8 -m 16G -display none --enable-kvm -serial \
-drive
file=/home/augere/VM/IMAGES/aarch64-vm0-fed30.raw,format=raw,if=none,cache=writethrough,id=drv0
\
-netdev
tap,id=nic0,script=/home/augere/TEST/SCRIPTS/qemu-ifup,downscript=/home/augere/TEST/SCRIPTS/qemu-ifdown,vhost=on
\
-drive if=pflash,format=raw,file=/home/augere/VM/UEFI/flash0.img,readonly \
-drive if=pflash,format=raw,file=/home/augere/VM/UEFI/flash1.img \
-net none -d guest_errors \
-device
virtio-blk-pci,bus=pcie.0,scsi=off,drive=drv0,id=virtio-disk0,bootindex=1,werror=stop,rerror=stop
\
-device pxb-pcie,id=bridge1,bus=pcie.0,bus_nr=254 \
-device pcie-root-port,port=0x0,chassis=4,id=pcie.4,bus=bridge1 \
-device virtio-net-pci,bus=pcie.4,netdev=nic0,mac=6a:f5:10:b1:3d:d2

It also works with your patches.

Thanks

Eric

>>>>
>>>> Thank you in advance
>>>>
>>>> Best Regards
>>>>
>>>> Eric
>>>>>
>>>>> Xingang Wang (4):
>>>>>     pci: Add PCI_BUS_IOMMU property
>>>>>     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/arm/virt-acpi-build.c            | 92
>>>>> +++++++++++++++++++++--------
>>>>>    hw/arm/virt.c                       | 29 +++++++++
>>>>>    hw/pci-bridge/pci_expander_bridge.c |  6 ++
>>>>>    hw/pci/pci.c                        | 35 ++++++++++-
>>>>>    include/hw/arm/virt.h               |  1 +
>>>>>    include/hw/pci/pci.h                |  1 +
>>>>>    include/hw/pci/pci_bus.h            | 13 ++++
>>>>>    7 files changed, 153 insertions(+), 24 deletions(-)
>>>>>
>>>>
>>>> .
>>>>
>>>
>>> Thanks for your advice.
>>>
>>> I test this with the following script, in which i add two options.
>>>
>>> The option `primary_bus_iommu=false(or true)` for `-machine
>>> virt,iommu=smmuv3`, this helps to enable/disable whether primary bus go
>>> through iommu.
>>>
>>> The other option `iommu=false` or `iommu=true` for `-device pxb-pcie`
>>> helps to enable/disable whether pxb root bus go through iommu.
>>>
>>>> #!/bin/sh
>>>>
>>>> /path/to/qemu/build/aarch64-softmmu/qemu-system-aarch64 \
>>>> -enable-kvm \
>>>> -cpu host \
>>>> -kernel /path/to/linux/arch/arm64/boot/Image \
>>>> -m 16G \
>>>> -smp 8,sockets=8,cores=1,threads=1 \
>>>> -machine
>>>> virt,kernel_irqchip=on,gic-version=3,iommu=smmuv3,primary_bus_iommu=false
>>>>
>>>> \
>>>> -drive
>>>> file=./QEMU_EFI-pflash.raw,if=pflash,format=raw,unit=0,readonly=on \
>>>> -device
>>>> pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,addr=0x3.0x1,iommu=false \
>>>> -device
>>>> pxb-pcie,bus_nr=0x20,id=pci.20,bus=pcie.0,addr=0x3.0x2,iommu=true \
>>>> -device
>>>> pxb-pcie,bus_nr=0x23,id=pci.30,bus=pcie.0,addr=0x3.0x3,iommu=true \
>>>> -device
>>>> pxb-pcie,bus_nr=0x40,id=pci.40,bus=pcie.0,addr=0x3.0x4,iommu=false \
>>>> -device pcie-pci-bridge,id=pci.11,bus=pci.10,addr=0x1 \
>>>> -device pcie-pci-bridge,id=pci.21,bus=pci.20,addr=0x1 \
>>>> -device
>>>> pcie-root-port,port=0x20,chassis=10,id=pci.2,bus=pcie.0,addr=0x2 \
>>>> -device
>>>> pcie-root-port,port=0x20,chassis=11,id=pci.12,bus=pci.10,addr=0x2 \
>>>> -device
>>>> pcie-root-port,port=0x20,chassis=19,id=pci.19,bus=pci.11,addr=0x3 \
>>>> -device
>>>> pcie-root-port,port=0x20,chassis=12,id=pci.22,bus=pci.20,addr=0x2 \
>>>> -device
>>>> pcie-root-port,port=0x20,chassis=13,id=pci.42,bus=pci.40,addr=0x2 \
>>>> -device virtio-scsi-pci,id=scsi0,bus=pci.12,addr=0x1 \
>>>> -device vfio-pci,host=b5:00.2,bus=pci.42,addr=0x0,id=acc2 \
>>>> -net none \
>>>> -initrd /path/to/rootfs.cpio.gz \
>>>> -nographic \
>>>> -append "rdinit=init console=ttyAMA0 earlycon=pl011,0x9000000
>>>> nokaslr" \
>>>
>>> I test the command line with an accelerator. The IORT table will have
>>> some changes, so only the root bus with iommu=true will go through
>>> smmuv3.
>>
>> Thank you for sharing your command line.
>>
>> On my end without using ",iommu=smmuv3" and the new options, my guest
>> crashes.
>>
>>      0.833665] ACPI: PCI Root Bridge [PC0A] (domain 0000 [bus 0a-0b])
>> [    0.837630] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
>> ClockPM Segments MSI HPX-Type3]
>> [    0.843377] acpi PNP0A08:00: _OSC: platform does not support [LTR]
>> [    0.846796] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
>> AER PCIeCapability]
>> [    0.851082] acpi PNP0A08:00: ECAM area [mem
>> 0x4010a00000-0x4010bfffff] reserved by PNP0C02:00
>> [    0.854742] acpi PNP0A08:00: ECAM at [mem 0x4010a00000-0x4010bfffff]
>> for [bus 0a-0b]
>> [    0.859569] ------------[ cut here ]------------
>> [    0.862470] kernel BUG at mm/ioremap.c:76!
>> [    0.865066] Internal error: Oops - BUG: 0 [#1] SMP
>> [    0.868130] Modules linked in:
>> [    0.870060] CPU: 6 PID: 1 Comm: swapper/0 Not tainted
>> 5.11.0-rc6-guest-upstream+ #26
>> [    0.874920] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0
>> 02/06/2015
>> [    0.879283] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO BTYPE=--)
>> [    0.883055] pc : ioremap_page_range+0x33c/0x3e0
>> [    0.885942] lr : ioremap_page_range+0x30/0x3e0
>> [    0.888737] sp : ffff80001272f800
>> [    0.890824] x29: ffff80001272f800 x28: ffffffbffe801000
>> [    0.894168] x27: ffffffc020040000 x26: ffff8000111b01f8
>> [    0.897543] x25: 0400000000000001 x24: ffffffbffe800000
>> [    0.900882] x23: 000000003eff3000 x22: ffffffbffe801000
>> [    0.904221] x21: ffffffbffe801000 x20: ffff0003f2270020
>> [    0.907612] x19: 0000000000000001 x18: 0000000000000030
>> [    0.910952] x17: 0000000000000000 x16: 0000000000000001
>> [    0.914283] x15: ffffffffffffffff x14: ffff8000116d49c8
>> [    0.917679] x13: 000000003eff3000 x12: 0000000000000041
>> [    0.921018] x11: ffff800011f2f000 x10: 000000000000002e
>> [    0.924359] x9 : ffff800010c372c4 x8 : ffffffbffe800000
>> [    0.927744] x7 : ffff0003f224eff8 x6 : 0000000000000001
>> [    0.931092] x5 : ffffffbffe800fff x4 : ffff8000116de650
>> [    0.934430] x3 : 0068000000000f17 x2 : 0140000000000000
>> [    0.937813] x1 : 00000040407f0000 x0 : ffff0003ffdcccc0
>> [    0.941165] Call trace:
>> [    0.942713]  ioremap_page_range+0x33c/0x3e0
>> [    0.945374]  pci_remap_iospace+0x7c/0x90
>> [    0.947881]  acpi_pci_probe_root_resources+0x180/0x238
>> [    0.951122]  pci_acpi_root_prepare_resources+0x28/0xc8
>> [    0.954357]  acpi_pci_root_create+0x9c/0x2f8
>> [    0.956990]  pci_acpi_scan_root+0x150/0x240
>> [    0.959639]  acpi_pci_root_add+0x34c/0x4e0
>> [    0.962220]  acpi_bus_attach+0x15c/0x2c0
>> [    0.964692]  acpi_bus_attach+0x9c/0x2c0
>> [    0.967135]  acpi_bus_attach+0x9c/0x2c0
>> [    0.969582]  acpi_bus_scan+0x64/0x118
>> [    0.971888]  acpi_scan_init+0x10c/0x244
>> [    0.974302]  acpi_init+0x2bc/0x328
>> [    0.976463]  do_one_initcall+0x54/0x268
>> [    0.978913]  kernel_init_freeable+0x22c/0x2c4
>> [    0.981658]  kernel_init+0x1c/0x128
>> [    0.983864]  ret_from_fork+0x10/0x34
>> [    0.986139] Code: a9446bf9 a8cb7bfd d50323bf d65f03c0 (d4210000)
>> [    0.990037] ---[ end trace fc68f309d1db57e3 ]---
>> [    0.992939] Kernel panic - not syncing: Oops - BUG: Fatal exception
>> [    0.996893] SMP: stopping secondary CPUs
>> [    0.999487] ---[ end Kernel panic - not syncing: Oops - BUG: Fatal
>> exception ]---
>>
>> Do you have any idea. I am using
>>
>> http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/4198/QEMU-AARCH64/RELEASE_GCC5/
>>
>>
>> Thanks
>>
>> Eric
>>
>>
>>
>>
>>>
>>> Thanks,
>>> Xingang
>>> .
>>>
>>
>> .
>>
> 
> I retest with the QEMU_EFI.fd and QEMU.img.gz you provided, and i don't
> have this issue. Could you please provide more information about how you
> test, and is everything ok with the iommu=smmuv3 on ?
> 
> From the debug info, it might be the problem of building the ACPI IORT
> table. And could you please retest without the last patch, and see if
> everything is ok.
> 
> Thanks.
> 
> Xingang
> 
> .
> 

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

[Auger Eric]

Hi Xingang

On 3/11/21 1:24 PM, Wang Xingang wrote:
> Hi Eric,
> 
> On 2021/3/10 18:24, Auger Eric wrote:
>> Hi Xingang,
>>
>> On 2/27/21 9:33 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. Default option is set to true, and the option
>>> is valid only if the iommu option for machine is properly set.
>>>
>>> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
>>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>>> ---
>>>   hw/arm/virt.c                       | 29 +++++++++++++++++++++++++++++
>>>   hw/pci-bridge/pci_expander_bridge.c |  6 ++++++
>>>   hw/pci/pci.c                        |  2 +-
>>>   include/hw/arm/virt.h               |  1 +
>>>   4 files changed, 37 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 371147f3ae..0c9e549759 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -79,6 +79,7 @@
>>>   #include "hw/virtio/virtio-iommu.h"
>>>   #include "hw/char/pl011.h"
>>>   #include "qemu/guest-random.h"
>>> +#include "include/hw/pci/pci_bus.h"
>>>     #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>>>       static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
>>> @@ -1232,6 +1233,10 @@ static void create_smmu(const VirtMachineState
>>> *vms,
>>>         dev = qdev_new("arm-smmuv3");
>>>   +    if (vms->primary_bus_iommu) {
>>> +        bus->flags |= PCI_BUS_IOMMU;
>>> +    }
>>> +
>>>       object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
>>>                                &error_abort);
>>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>> @@ -2305,6 +2310,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)
>>>   {
>>> @@ -2629,6 +2648,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",
>>> @@ -2696,6 +2722,9 @@ static void virt_instance_init(Object *obj)
>>>       /* Default disallows iommu instantiation */
>>>       vms->iommu = VIRT_IOMMU_NONE;
>>>   +    /* Iommu is enabled by default for primary bus */
>>> +    vms->primary_bus_iommu = true;
>>> +
>>>       /* Default disallows RAS instantiation */
>>>       vms->ras = false;
>>>   diff --git a/hw/pci-bridge/pci_expander_bridge.c
>>> b/hw/pci-bridge/pci_expander_bridge.c
>>> index aedded1064..0412656265 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)
>>> @@ -254,6 +255,10 @@ static void pxb_dev_realize_common(PCIDevice
>>> *dev, bool pcie, Error **errp)
>>>       bus->address_space_io = pci_get_bus(dev)->address_space_io;
>>>       bus->map_irq = pxb_map_irq_fn;
>>>   +    if (pxb->iommu) {
>>> +        bus->flags |= PCI_BUS_IOMMU;
>>> +    }
>>> +
>>>       PCI_HOST_BRIDGE(ds)->bus = bus;
>>>         pxb_register_bus(dev, bus, &local_err);
>>> @@ -301,6 +306,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),
>> looks a bit odd to me that we have a property for the PXE-PCIe extra
>> root complex and not for the gpex device. Wouldn't it make sense to add
>> one for the GPEX too? In the positive you still could have a machine
>> option that would force the GPEX property value?
> 
> Indeed it makes sense to add one property for GPEX too.However, the
> iommu property for PXBDev only helps to add option in qemu command line.
> When it is necessary to check whether the iommu is enabled on the root
> bus, it would be better to access the bus flag. In qemu, the pxb is not
> related to GPEX currently, and i do not find proper position to add the
> iommu property for GPEX, you might have some good idea for that.

What I had in mind was to add a similar property at GPEX level. Maybe I
would instead introduce an option that disallow the iommu on its
hierarchy. You would also have a virt machine  "primary_bus_iommu"
option that would control the GPEX property through an
object_property_set_int() call

But I would be curious about others' thoughts.

Thanks

Eric
> 
>>>       DEFINE_PROP_END_OF_LIST(),
>>>   };
>>>   diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index a9ebef8a35..dc969989c9 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -2712,7 +2712,7 @@ AddressSpace
>>> *pci_device_iommu_address_space(PCIDevice *dev)
>>>             iommu_bus = parent_bus;
>>>       }
>>> -    if (iommu_bus && iommu_bus->iommu_fn) {
>>> +    if (pci_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/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>> index ee9a93101e..babe829486 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;
>>>
>> Thanks
>>
>> Eric
>>
>> .
>>
> 
> Thanks
> 
> Xingang
> 
> .
>