[PATCH v2 0/8] virtio-iommu: Add ACPI support

[PATCH v2 0/8] virtio-iommu: Add ACPI support

[Jean-Philippe Brucker]

Allow instantiating a virtio-iommu device on ACPI systems by adding a
Virtual I/O Translation table (VIOT). Enable x86 support for VIOT.

Changes since [v1]:
* Patch 2, use build_append_int_noprefix() to generate the ACPI table.
* Patch 7, check that only one IOMMU is instantiated on x86.
* Added patch 5 that rejects multiple instantations of virtio-iommu on
  Arm. It's not crucial to the series but it matches the check added to
  x86 and is very simple.
* Added patch 8 documenting that interrupt remapping is not supported.

There is an important caveat at the moment: when virtio-iommu is
instantiated, device DMA faults until the guest configures the IOMMU.
Firmware is therefore unable to access storage devices and load the
bootloader and OS. Upcoming patches will align virtio-iommu with other
vIOMMUs and let DMA bypass the IOMMU during boot. In the meantime there
are several ways to circumvent the problem:
* Use plain old virtio-blk as storage, without enabling the
  'iommu_platform' property. DMA from the device bypasses the IOMMU.
* Place the storage device behind a PCI bus that bypasses the IOMMU,
  using the new 'bypass_iommu' bridge property.
  See docs/bypass-iommu.txt
* Use non-PCI storage devices, for example virtio-blk-device on the
  arm64 virt machine.

You can find a description of the VIOT table, which will be included in next
ACPI version, here: https://jpbrucker.net/virtio-iommu/viot/viot-v9.pdf

[v1] https://lore.kernel.org/qemu-devel/20210810084505.2257983-1-jean-philippe@linaro.org/

Eric Auger (1):
  pc: Allow instantiating a virtio-iommu device

Jean-Philippe Brucker (7):
  acpi: Add VIOT structure definitions
  hw/acpi: Add VIOT table
  hw/arm/virt-acpi-build: Add VIOT table for virtio-iommu
  hw/arm/virt: Remove device tree restriction for virtio-iommu
  hw/arm/virt: Reject instantiation of multiple IOMMUs
  pc: Add VIOT table for virtio-iommu
  docs: Add '-device virtio-iommu' entry

 hw/acpi/Kconfig              |   4 ++
 hw/arm/Kconfig               |   1 +
 hw/i386/Kconfig              |   1 +
 hw/acpi/viot.h               |  13 ++++
 include/hw/acpi/acpi-defs.h  |  60 +++++++++++++++++++
 include/hw/i386/pc.h         |   2 +
 hw/acpi/viot.c               | 112 +++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c     |   7 +++
 hw/arm/virt.c                |  15 +++--
 hw/i386/acpi-build.c         |   5 ++
 hw/i386/pc.c                 |  28 ++++++++-
 hw/virtio/virtio-iommu-pci.c |   7 ---
 hw/acpi/meson.build          |   1 +
 qemu-options.hx              |   3 +
 14 files changed, 243 insertions(+), 16 deletions(-)
 create mode 100644 hw/acpi/viot.h
 create mode 100644 hw/acpi/viot.c

-- 
2.33.0

[PATCH v2 1/8] acpi: Add VIOT structure definitions

[Jean-Philippe Brucker]

The ACPI Virtual I/O Translation table (VIOT) table describes I/O
topology for paravirtual devices. At the moment it describes the
relation between virtio-iommu devices and their endpoints. Add the
structure definitions for VIOT.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
Following the latest spec draft, and related acpica change
https://jpbrucker.net/virtio-iommu/viot/viot-v9.pdf
https://github.com/acpica/acpica/commit/fc4e33319c1ee08f20f5c44853dd8426643f6dfd
---
 include/hw/acpi/acpi-defs.h | 60 +++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index cf9f44299c..adbf7d7b77 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -618,4 +618,64 @@ struct AcpiIortRC {
 } QEMU_PACKED;
 typedef struct AcpiIortRC AcpiIortRC;
 
+/*
+ * Virtual I/O Translation Table
+ */
+struct AcpiViot {
+    ACPI_TABLE_HEADER_DEF
+    uint16_t node_count;
+    uint16_t node_offset;
+    uint8_t reserved[8];
+} QEMU_PACKED;
+typedef struct AcpiViot AcpiViot;
+
+#define ACPI_VIOT_NODE_HEADER_DEF   /* Fields common to all nodes */ \
+    uint8_t type;          \
+    uint8_t reserved;      \
+    uint16_t length;
+
+/* Values for node Type above */
+enum {
+    ACPI_VIOT_NODE_PCI_RANGE = 0x01,
+    ACPI_VIOT_NODE_MMIO = 0x02,
+    ACPI_VIOT_NODE_VIRTIO_IOMMU_PCI = 0x03,
+    ACPI_VIOT_NODE_VIRTIO_IOMMU_MMIO = 0x04,
+};
+
+struct AcpiViotPciRange {
+    ACPI_VIOT_NODE_HEADER_DEF
+    uint32_t endpoint_start;
+    uint16_t segment_start;
+    uint16_t segment_end;
+    uint16_t bdf_start;
+    uint16_t bdf_end;
+    uint16_t output_node;
+    uint8_t reserved1[6];
+} QEMU_PACKED;
+typedef struct AcpiViotPciRange AcpiViotPciRange;
+
+struct AcpiViotMmio {
+    ACPI_VIOT_NODE_HEADER_DEF
+    uint32_t endpoint;
+    uint64_t base_address;
+    uint16_t output_node;
+    uint8_t reserved1[6];
+} QEMU_PACKED;
+typedef struct AcpiViotMmio AcpiViotMmio;
+
+struct AcpiViotVirtioIommuPci {
+    ACPI_VIOT_NODE_HEADER_DEF
+    uint16_t segment;
+    uint16_t bdf;
+    uint8_t reserved1[8];
+} QEMU_PACKED;
+typedef struct AcpiViotVirtioIommuPci AcpiViotVirtioIommuPci;
+
+struct AcpiViotVirtioIommuMmio {
+    ACPI_VIOT_NODE_HEADER_DEF
+    uint8_t reserved1[4];
+    uint64_t base_address;
+} QEMU_PACKED;
+typedef struct AcpiViotVirtioIommuMmio AcpiViotVirtioIommuMmio;
+
 #endif
-- 
2.33.0

[PATCH v2 2/8] hw/acpi: Add VIOT table

[Jean-Philippe Brucker]

Add a function that generates a Virtual I/O Translation table (VIOT),
describing the topology of paravirtual IOMMUs. The table is created when
instantiating a virtio-iommu device. It contains a virtio-iommu node and
PCI Range nodes for endpoints managed by the IOMMU. By default, a single
node describes all PCI devices. When passing the "default_bus_bypass_iommu"
machine option and "bypass_iommu" PXB option, only buses that do not
bypass the IOMMU are described by PCI Range nodes.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/acpi/Kconfig     |   4 ++
 hw/acpi/viot.h      |  13 +++++
 hw/acpi/viot.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++
 hw/acpi/meson.build |   1 +
 4 files changed, 130 insertions(+)
 create mode 100644 hw/acpi/viot.h
 create mode 100644 hw/acpi/viot.c

diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index cfc4ede8d9..abad79c103 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -41,6 +41,10 @@ config ACPI_VMGENID
     default y
     depends on PC
 
+config ACPI_VIOT
+    bool
+    depends on ACPI
+
 config ACPI_HW_REDUCED
     bool
     select ACPI
diff --git a/hw/acpi/viot.h b/hw/acpi/viot.h
new file mode 100644
index 0000000000..4cef29a640
--- /dev/null
+++ b/hw/acpi/viot.h
@@ -0,0 +1,13 @@
+/*
+ * ACPI Virtual I/O Translation Table implementation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef VIOT_H
+#define VIOT_H
+
+void build_viot(GArray *table_data, BIOSLinker *linker,
+                uint16_t virtio_iommu_bdf, const char *oem_id,
+                const char *oem_table_id);
+
+#endif /* VIOT_H */
diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
new file mode 100644
index 0000000000..81bb2e93ff
--- /dev/null
+++ b/hw/acpi/viot.c
@@ -0,0 +1,112 @@
+/*
+ * ACPI Virtual I/O Translation table implementation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "qemu/osdep.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/viot.h"
+#include "hw/pci/pci.h"
+#include "hw/pci/pci_host.h"
+
+struct viot_pci_ranges {
+    GArray *blob;
+    size_t count;
+    uint16_t output_node;
+};
+
+/* Build PCI range for a given PCI host bridge */
+static int viot_host_bridges(Object *obj, void *opaque)
+{
+    struct viot_pci_ranges *pci_ranges = opaque;
+    GArray *blob = pci_ranges->blob;
+
+    if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
+        PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
+
+        if (bus && !pci_bus_bypass_iommu(bus)) {
+            int min_bus, max_bus;
+
+            pci_bus_range(bus, &min_bus, &max_bus);
+
+            /* Type */
+            build_append_int_noprefix(blob, ACPI_VIOT_NODE_PCI_RANGE, 1);
+            /* Reserved */
+            build_append_int_noprefix(blob, 0, 1);
+            /* Length */
+            build_append_int_noprefix(blob, sizeof(AcpiViotPciRange), 2);
+            /* Endpoint start */
+            build_append_int_noprefix(blob, PCI_BUILD_BDF(min_bus, 0), 4);
+            /* PCI Segment start */
+            build_append_int_noprefix(blob, 0, 2);
+            /* PCI Segment end */
+            build_append_int_noprefix(blob, 0, 2);
+            /* PCI BDF start */
+            build_append_int_noprefix(blob, PCI_BUILD_BDF(min_bus, 0), 2);
+            /* PCI BDF end */
+            build_append_int_noprefix(blob, PCI_BUILD_BDF(max_bus, 0xff), 2);
+            /* Output node */
+            build_append_int_noprefix(blob, pci_ranges->output_node, 2);
+            /* Reserved */
+            build_append_int_noprefix(blob, 0, 6);
+
+            pci_ranges->count++;
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * Generate a VIOT table with one PCI-based virtio-iommu that manages PCI
+ * endpoints.
+ */
+void build_viot(GArray *table_data, BIOSLinker *linker,
+                uint16_t virtio_iommu_bdf, const char *oem_id,
+                const char *oem_table_id)
+{
+    /* virtio-iommu node follows the header */
+    int viommu_off = sizeof(AcpiViot);
+    int viot_start = table_data->len;
+    struct viot_pci_ranges pci_ranges = {
+        .output_node = viommu_off,
+        .blob = g_array_new(false, true, 1),
+    };
+
+    /* Build the list of PCI ranges that this viommu manages */
+    object_child_foreach_recursive(object_get_root(), viot_host_bridges,
+                                   &pci_ranges);
+
+    /* VIOT header */
+    acpi_data_push(table_data, sizeof(AcpiTableHeader));
+    /* Node count */
+    build_append_int_noprefix(table_data, pci_ranges.count + 1, 2);
+    /* Node offset */
+    build_append_int_noprefix(table_data, viommu_off, 2);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 8);
+
+    /* Virtio-iommu based on virtio-pci */
+    /* Type */
+    build_append_int_noprefix(table_data, ACPI_VIOT_NODE_VIRTIO_IOMMU_PCI, 1);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 1);
+    /* Length */
+    build_append_int_noprefix(table_data, sizeof(AcpiViotVirtioIommuPci), 2);
+    /* PCI Segment */
+    build_append_int_noprefix(table_data, 0, 2);
+    /* PCI BDF number */
+    build_append_int_noprefix(table_data, virtio_iommu_bdf, 2);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 8);
+
+    /* PCI ranges found above */
+    g_array_append_vals(table_data, pci_ranges.blob->data,
+                        pci_ranges.blob->len);
+    g_array_free(pci_ranges.blob, true);
+
+    build_header(linker, table_data, (void *)(table_data->data + viot_start),
+                 "VIOT", table_data->len - viot_start, 0, oem_id, oem_table_id);
+}
+
diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
index 29f804d13e..a510988b27 100644
--- a/hw/acpi/meson.build
+++ b/hw/acpi/meson.build
@@ -16,6 +16,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device
 acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c'))
 acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c'))
 acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('piix4.c', 'pcihp.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_VIOT', if_true: files('viot.c'))
 acpi_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('ich9.c', 'tco.c'))
 acpi_ss.add(when: 'CONFIG_IPMI', if_true: files('ipmi.c'), if_false: files('ipmi-stub.c'))
 acpi_ss.add(when: 'CONFIG_PC', if_false: files('acpi-x86-stub.c'))
-- 
2.33.0

[PATCH v2 3/8] hw/arm/virt-acpi-build: Add VIOT table for virtio-iommu

[Jean-Philippe Brucker]

When a virtio-iommu is instantiated, describe it using the ACPI VIOT
table.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/Kconfig           | 1 +
 hw/arm/virt-acpi-build.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 4ba0aca067..7da0422446 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -29,6 +29,7 @@ config ARM_VIRT
     select ACPI_HW_REDUCED
     select ACPI_NVDIMM
     select ACPI_APEI
+    select ACPI_VIOT
 
 config CHEETAH
     bool
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 037cc1fd82..e2fa677d80 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -55,6 +55,7 @@
 #include "kvm_arm.h"
 #include "migration/vmstate.h"
 #include "hw/acpi/ghes.h"
+#include "hw/acpi/viot.h"
 
 #define ARM_SPI_BASE 32
 
@@ -849,6 +850,12 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     }
 #endif
 
+    if (vms->iommu == VIRT_IOMMU_VIRTIO) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_viot(tables_blob, tables->linker, vms->virtio_iommu_bdf,
+                   vms->oem_id, vms->oem_table_id);
+    }
+
     /* XSDT is pointed to by RSDP */
     xsdt = tables_blob->len;
     build_xsdt(tables_blob, tables->linker, table_offsets, vms->oem_id,
-- 
2.33.0

[PATCH v2 4/8] hw/arm/virt: Remove device tree restriction for virtio-iommu

[Jean-Philippe Brucker]

virtio-iommu is now supported with ACPI VIOT as well as device tree.
Remove the restriction that prevents from instantiating a virtio-iommu
device under ACPI.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c                | 10 ++--------
 hw/virtio/virtio-iommu-pci.c |  7 -------
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 81eda46b0b..b4598d3fe6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2551,16 +2551,10 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
     MachineClass *mc = MACHINE_GET_CLASS(machine);
 
     if (device_is_dynamic_sysbus(mc, dev) ||
-       (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))) {
+        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
         return HOTPLUG_HANDLER(machine);
     }
-    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
-        VirtMachineState *vms = VIRT_MACHINE(machine);
-
-        if (!vms->bootinfo.firmware_loaded || !virt_is_acpi_enabled(vms)) {
-            return HOTPLUG_HANDLER(machine);
-        }
-    }
     return NULL;
 }
 
diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
index 770c286be7..f30eb16cbf 100644
--- a/hw/virtio/virtio-iommu-pci.c
+++ b/hw/virtio/virtio-iommu-pci.c
@@ -48,16 +48,9 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     VirtIOIOMMU *s = VIRTIO_IOMMU(vdev);
 
     if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) {
-        MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
-
-        error_setg(errp,
-                   "%s machine fails to create iommu-map device tree bindings",
-                   mc->name);
         error_append_hint(errp,
                           "Check your machine implements a hotplug handler "
                           "for the virtio-iommu-pci device\n");
-        error_append_hint(errp, "Check the guest is booted without FW or with "
-                          "-no-acpi\n");
         return;
     }
     for (int i = 0; i < s->nb_reserved_regions; i++) {
-- 
2.33.0

[PATCH v2 5/8] hw/arm/virt: Reject instantiation of multiple IOMMUs

[Jean-Philippe Brucker]

We do not support instantiating multiple IOMMUs. Before adding a
virtio-iommu, check that no other IOMMU is present. This will detect
both "iommu=smmuv3" machine parameter and another virtio-iommu instance.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b4598d3fe6..5ca225291f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2475,6 +2475,11 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
     if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
         PCIDevice *pdev = PCI_DEVICE(dev);
 
+        if (vms->iommu != VIRT_IOMMU_NONE) {
+            error_setg(errp, "virt machine does not support multiple IOMMUs");
+            return;
+        }
+
         vms->iommu = VIRT_IOMMU_VIRTIO;
         vms->virtio_iommu_bdf = pci_get_bdf(pdev);
         create_virtio_iommu_dt_bindings(vms);
-- 
2.33.0

[PATCH v2 6/8] pc: Add VIOT table for virtio-iommu

[Jean-Philippe Brucker]

The ACPI Virtual I/O Translation table (VIOT) describes the relation
between a virtio-iommu and the endpoints it manages. When a virtio-iommu
device is instantiated, add a VIOT table.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/i386/Kconfig      | 1 +
 include/hw/i386/pc.h | 2 ++
 hw/i386/acpi-build.c | 5 +++++
 hw/i386/pc.c         | 7 +++++++
 4 files changed, 15 insertions(+)

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index ddedcef0b2..13db05d557 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -54,6 +54,7 @@ config PC_ACPI
     select ACPI_X86
     select ACPI_CPU_HOTPLUG
     select ACPI_MEMORY_HOTPLUG
+    select ACPI_VIOT
     select SMBUS_EEPROM
     select PFLASH_CFI01
     depends on ACPI_SMBUS
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 88dffe7517..979b8d0b7c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -45,6 +45,8 @@ typedef struct PCMachineState {
     bool pit_enabled;
     bool hpet_enabled;
     bool default_bus_bypass_iommu;
+    bool virtio_iommu;
+    uint16_t virtio_iommu_bdf;
     uint64_t max_fw_size;
 
     /* NUMA information: */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a33ac8b91e..078b7f5c6f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -71,6 +71,7 @@
 
 #include "hw/acpi/ipmi.h"
 #include "hw/acpi/hmat.h"
+#include "hw/acpi/viot.h"
 
 /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
  * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
@@ -2559,6 +2560,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
             build_dmar_q35(tables_blob, tables->linker, x86ms->oem_id,
                            x86ms->oem_table_id);
         }
+    } else if (pcms->virtio_iommu) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_viot(tables_blob, tables->linker, pcms->virtio_iommu_bdf,
+                   x86ms->oem_id, x86ms->oem_table_id);
     }
     if (machine->nvdimms_state->is_enabled) {
         nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c2b9d62a35..694fc9ce07 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -84,6 +84,7 @@
 #include "hw/i386/intel_iommu.h"
 #include "hw/net/ne2000-isa.h"
 #include "standard-headers/asm-x86/bootparam.h"
+#include "hw/virtio/virtio-iommu.h"
 #include "hw/virtio/virtio-pmem-pci.h"
 #include "hw/virtio/virtio-mem-pci.h"
 #include "hw/mem/memory-device.h"
@@ -1388,6 +1389,12 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
                object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
         pc_virtio_md_pci_plug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
+        PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+        PCIDevice *pdev = PCI_DEVICE(dev);
+
+        pcms->virtio_iommu = true;
+        pcms->virtio_iommu_bdf = pci_get_bdf(pdev);
     }
 }
 
-- 
2.33.0

[PATCH v2 7/8] pc: Allow instantiating a virtio-iommu device

[Jean-Philippe Brucker]

From: Eric Auger <eric.auger@redhat.com>

Add a hotplug handler for virtio-iommu on x86 and set the necessary
reserved region property. On x86, the [0xfee00000, 0xfeefffff] DMA
region is reserved for MSIs. DMA transactions to this range either
trigger IRQ remapping in the IOMMU or bypasses IOMMU translation.

Although virtio-iommu does not support IRQ remapping it must be informed
of the reserved region so that it can forward DMA transactions targeting
this region.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/i386/pc.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 694fc9ce07..c1e1cffe16 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -797,6 +797,11 @@ void pc_machine_done(Notifier *notifier, void *data)
                      "irqchip support.");
         exit(EXIT_FAILURE);
     }
+
+    if (pcms->virtio_iommu && x86_iommu_get_default()) {
+        error_report("QEMU does not support multiple vIOMMUs for x86 yet.");
+        exit(EXIT_FAILURE);
+    }
 }
 
 void pc_guest_info_init(PCMachineState *pcms)
@@ -1376,6 +1381,14 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
                object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
         pc_virtio_md_pci_pre_plug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
+        /* Declare the reserved MSI region */
+        char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
+                                              VIRTIO_IOMMU_RESV_MEM_T_MSI);
+
+        qdev_prop_set_uint32(dev, "len-reserved-regions", 1);
+        qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str);
+        g_free(resv_prop_str);
     }
 }
 
@@ -1393,6 +1406,11 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
         PCMachineState *pcms = PC_MACHINE(hotplug_dev);
         PCIDevice *pdev = PCI_DEVICE(dev);
 
+        if (pcms->virtio_iommu) {
+            error_setg(errp,
+                       "QEMU does not support multiple vIOMMUs for x86 yet.");
+            return;
+        }
         pcms->virtio_iommu = true;
         pcms->virtio_iommu_bdf = pci_get_bdf(pdev);
     }
@@ -1436,7 +1454,8 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
         object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
         object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
-        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
+        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
         return HOTPLUG_HANDLER(machine);
     }
 
-- 
2.33.0

[PATCH v2 8/8] docs: Add '-device virtio-iommu' entry

[Jean-Philippe Brucker]

Document the virtio-iommu device for qemu-system-x86_64. In particular
note the lack of interrupt remapping, which may be an important
limitation on x86.

Suggested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 qemu-options.hx | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 83aa59a920..9a1906a748 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -976,6 +976,9 @@ SRST
     Please also refer to the wiki page for general scenarios of VT-d
     emulation in QEMU: https://wiki.qemu.org/Features/VT-d.
 
+``-device virtio-iommu``
+    Enable a paravirtual IOMMU, that manages DMA isolation and remapping
+    for all PCI devices, but does not support interrupt remapping.
 ERST
 
 DEF("name", HAS_ARG, QEMU_OPTION_name,
-- 
2.33.0

Re: [PATCH v2 1/8] acpi: Add VIOT structure definitions

[Eric Auger]

Hi Jean,

On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> The ACPI Virtual I/O Translation table (VIOT) table describes I/O
> topology for paravirtual devices. At the moment it describes the
> relation between virtio-iommu devices and their endpoints. Add the
> structure definitions for VIOT.

I don't think those additions are needed anymore. Looking at Igor's
conversions in  [PATCH v2 00/35] acpi: refactor error prone
build_header() and packed structures usage in ACPI tables you will see
he removes them and hardcode the sizes/offsets in
hw/arm/virt-acpu-build.c and removes the associated structs. Even the
enum values are not used anymore which is arguable but this seems to be
the default style now. Anyway there are some structs like AcpiViotMmio
which are not used in this series. Thanks Eric
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> Following the latest spec draft, and related acpica change
> https://jpbrucker.net/virtio-iommu/viot/viot-v9.pdf
> https://github.com/acpica/acpica/commit/fc4e33319c1ee08f20f5c44853dd8426643f6dfd
> ---
>  include/hw/acpi/acpi-defs.h | 60 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index cf9f44299c..adbf7d7b77 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -618,4 +618,64 @@ struct AcpiIortRC {
>  } QEMU_PACKED;
>  typedef struct AcpiIortRC AcpiIortRC;
>  
> +/*
> + * Virtual I/O Translation Table
> + */
> +struct AcpiViot {
> +    ACPI_TABLE_HEADER_DEF
> +    uint16_t node_count;
> +    uint16_t node_offset;
> +    uint8_t reserved[8];
> +} QEMU_PACKED;
> +typedef struct AcpiViot AcpiViot;
> +
> +#define ACPI_VIOT_NODE_HEADER_DEF   /* Fields common to all nodes */ \
> +    uint8_t type;          \
> +    uint8_t reserved;      \
> +    uint16_t length;
> +
> +/* Values for node Type above */
> +enum {
> +    ACPI_VIOT_NODE_PCI_RANGE = 0x01,
> +    ACPI_VIOT_NODE_MMIO = 0x02,
> +    ACPI_VIOT_NODE_VIRTIO_IOMMU_PCI = 0x03,
> +    ACPI_VIOT_NODE_VIRTIO_IOMMU_MMIO = 0x04,
> +};
> +
> +struct AcpiViotPciRange {
> +    ACPI_VIOT_NODE_HEADER_DEF
> +    uint32_t endpoint_start;
> +    uint16_t segment_start;
> +    uint16_t segment_end;
> +    uint16_t bdf_start;
> +    uint16_t bdf_end;
> +    uint16_t output_node;
> +    uint8_t reserved1[6];
> +} QEMU_PACKED;
> +typedef struct AcpiViotPciRange AcpiViotPciRange;
> +
> +struct AcpiViotMmio {
> +    ACPI_VIOT_NODE_HEADER_DEF
> +    uint32_t endpoint;
> +    uint64_t base_address;
> +    uint16_t output_node;
> +    uint8_t reserved1[6];
> +} QEMU_PACKED;
> +typedef struct AcpiViotMmio AcpiViotMmio;
> +
> +struct AcpiViotVirtioIommuPci {
> +    ACPI_VIOT_NODE_HEADER_DEF
> +    uint16_t segment;
> +    uint16_t bdf;
> +    uint8_t reserved1[8];
> +} QEMU_PACKED;
> +typedef struct AcpiViotVirtioIommuPci AcpiViotVirtioIommuPci;
> +
> +struct AcpiViotVirtioIommuMmio {
> +    ACPI_VIOT_NODE_HEADER_DEF
> +    uint8_t reserved1[4];
> +    uint64_t base_address;
> +} QEMU_PACKED;
> +typedef struct AcpiViotVirtioIommuMmio AcpiViotVirtioIommuMmio;
> +
>  #endif

Re: [PATCH v2 2/8] hw/acpi: Add VIOT table

[Eric Auger]

Hi Jean,

On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> Add a function that generates a Virtual I/O Translation table (VIOT),
> describing the topology of paravirtual IOMMUs. The table is created when
> instantiating a virtio-iommu device. It contains a virtio-iommu node and
> PCI Range nodes for endpoints managed by the IOMMU. By default, a single
> node describes all PCI devices. When passing the "default_bus_bypass_iommu"
> machine option and "bypass_iommu" PXB option, only buses that do not
> bypass the IOMMU are described by PCI Range nodes.

You may consider to add a test for the new VIOT table (However this may
be added afterwards). See tests/qtest/bios-tables-test.c for the process
and examples.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/acpi/Kconfig     |   4 ++
>  hw/acpi/viot.h      |  13 +++++
>  hw/acpi/viot.c      | 112 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/meson.build |   1 +
>  4 files changed, 130 insertions(+)
>  create mode 100644 hw/acpi/viot.h
>  create mode 100644 hw/acpi/viot.c
>
> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> index cfc4ede8d9..abad79c103 100644
> --- a/hw/acpi/Kconfig
> +++ b/hw/acpi/Kconfig
> @@ -41,6 +41,10 @@ config ACPI_VMGENID
>      default y
>      depends on PC
>  
> +config ACPI_VIOT
> +    bool
> +    depends on ACPI
> +
>  config ACPI_HW_REDUCED
>      bool
>      select ACPI
> diff --git a/hw/acpi/viot.h b/hw/acpi/viot.h
> new file mode 100644
> index 0000000000..4cef29a640
> --- /dev/null
> +++ b/hw/acpi/viot.h
> @@ -0,0 +1,13 @@
> +/*
> + * ACPI Virtual I/O Translation Table implementation
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef VIOT_H
> +#define VIOT_H
> +
> +void build_viot(GArray *table_data, BIOSLinker *linker,
> +                uint16_t virtio_iommu_bdf, const char *oem_id,
> +                const char *oem_table_id);
> +
> +#endif /* VIOT_H */
> diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
> new file mode 100644
> index 0000000000..81bb2e93ff
> --- /dev/null
> +++ b/hw/acpi/viot.c
> @@ -0,0 +1,112 @@
> +/*
> + * ACPI Virtual I/O Translation table implementation
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#include "qemu/osdep.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/viot.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_host.h"
> +
> +struct viot_pci_ranges {
> +    GArray *blob;
> +    size_t count;
> +    uint16_t output_node;
> +};
> +
> +/* Build PCI range for a given PCI host bridge */
chapter 1.3
> +static int viot_host_bridges(Object *obj, void *opaque)
> +{
> +    struct viot_pci_ranges *pci_ranges = opaque;
> +    GArray *blob = pci_ranges->blob;
> +
> +    if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> +        PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
> +
> +        if (bus && !pci_bus_bypass_iommu(bus)) {
> +            int min_bus, max_bus;
> +
> +            pci_bus_range(bus, &min_bus, &max_bus);
> +
> +            /* Type */
> +            build_append_int_noprefix(blob, ACPI_VIOT_NODE_PCI_RANGE, 1);
s/ACPI_VIOT_NODE_PCI_RANGE/1
> +            /* Reserved */
> +            build_append_int_noprefix(blob, 0, 1);
> +            /* Length */
> +            build_append_int_noprefix(blob, sizeof(AcpiViotPciRange), 2);
hardcode the size
> +            /* Endpoint start */
> +            build_append_int_noprefix(blob, PCI_BUILD_BDF(min_bus, 0), 4);
> +            /* PCI Segment start */
> +            build_append_int_noprefix(blob, 0, 2);
> +            /* PCI Segment end */
> +            build_append_int_noprefix(blob, 0, 2);
> +            /* PCI BDF start */
> +            build_append_int_noprefix(blob, PCI_BUILD_BDF(min_bus, 0), 2);
> +            /* PCI BDF end */
> +            build_append_int_noprefix(blob, PCI_BUILD_BDF(max_bus, 0xff), 2);
> +            /* Output node */
> +            build_append_int_noprefix(blob, pci_ranges->output_node, 2);
> +            /* Reserved */
> +            build_append_int_noprefix(blob, 0, 6);
> +
> +            pci_ranges->count++;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * Generate a VIOT table with one PCI-based virtio-iommu that manages PCI
> + * endpoints.
Add a link to the doc + Table refs?

https://jpbrucker.net/virtio-iommu/viot/viot-v9.pdf

> + */
> +void build_viot(GArray *table_data, BIOSLinker *linker,
> +                uint16_t virtio_iommu_bdf, const char *oem_id,
> +                const char *oem_table_id)
> +{
> +    /* virtio-iommu node follows the header */
> +    int viommu_off = sizeof(AcpiViot);
> +    int viot_start = table_data->len;
> +    struct viot_pci_ranges pci_ranges = {
> +        .output_node = viommu_off,
> +        .blob = g_array_new(false, true, 1),
> +    };
> +
> +    /* Build the list of PCI ranges that this viommu manages */
> +    object_child_foreach_recursive(object_get_root(), viot_host_bridges,
> +                                   &pci_ranges);
> +
> +    /* VIOT header */
> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
Depending on the order of landing, this may be replaced by Igor's

acpi_init_table() and associated with acpi_table_composed().

> +    /* Node count */
> +    build_append_int_noprefix(table_data, pci_ranges.count + 1, 2);
> +    /* Node offset */
> +    build_append_int_noprefix(table_data, viommu_off, 2);
> +    /* Reserved */
> +    build_append_int_noprefix(table_data, 0, 8);
> +
> +    /* Virtio-iommu based on virtio-pci */
chapter 1.1. By the way may be worth adding table titles in the spec.
> +    /* Type */
> +    build_append_int_noprefix(table_data, ACPI_VIOT_NODE_VIRTIO_IOMMU_PCI, 1);
s/ACPI_VIOT_NODE_VIRTIO_IOMMU_PCI/3 or put the defines here instead of
in the header.
> +    /* Reserved */
> +    build_append_int_noprefix(table_data, 0, 1);
> +    /* Length */
> +    build_append_int_noprefix(table_data, sizeof(AcpiViotVirtioIommuPci), 2);
hardcode the size of AcpiViotVirtioIommuPci
> +    /* PCI Segment */
> +    build_append_int_noprefix(table_data, 0, 2);
> +    /* PCI BDF number */
> +    build_append_int_noprefix(table_data, virtio_iommu_bdf, 2);
> +    /* Reserved */
> +    build_append_int_noprefix(table_data, 0, 8);
> +
> +    /* PCI ranges found above */
> +    g_array_append_vals(table_data, pci_ranges.blob->data,
> +                        pci_ranges.blob->len);
> +    g_array_free(pci_ranges.blob, true);
> +
> +    build_header(linker, table_data, (void *)(table_data->data + viot_start),
> +                 "VIOT", table_data->len - viot_start, 0, oem_id, oem_table_id);

acpi_table_composed() or whatever its new name

> +}
> +
> diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
> index 29f804d13e..a510988b27 100644
> --- a/hw/acpi/meson.build
> +++ b/hw/acpi/meson.build
> @@ -16,6 +16,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device
>  acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('piix4.c', 'pcihp.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_VIOT', if_true: files('viot.c'))
there is a conflict with main now if I did not mess up things

<<<<<<< HEAD
acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c'))
acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_true: files('pcihp.c'))
acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_false:
files('acpi-pci-hotplug-stub.c'))
=======
acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('piix4.c', 'pcihp.c'))
acpi_ss.add(when: 'CONFIG_ACPI_VIOT', if_true: files('viot.c'))
>>>>>>> hw/acpi: Add VIOT table

Thanks

Eric


>  acpi_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('ich9.c', 'tco.c'))
>  acpi_ss.add(when: 'CONFIG_IPMI', if_true: files('ipmi.c'), if_false: files('ipmi-stub.c'))
>  acpi_ss.add(when: 'CONFIG_PC', if_false: files('acpi-x86-stub.c'))

Re: [PATCH v2 3/8] hw/arm/virt-acpi-build: Add VIOT table for virtio-iommu

[Eric Auger]

Hi Jean,

On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> When a virtio-iommu is instantiated, describe it using the ACPI VIOT
> table.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  hw/arm/Kconfig           | 1 +
>  hw/arm/virt-acpi-build.c | 7 +++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 4ba0aca067..7da0422446 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -29,6 +29,7 @@ config ARM_VIRT
>      select ACPI_HW_REDUCED
>      select ACPI_NVDIMM
>      select ACPI_APEI
> +    select ACPI_VIOT
>  
>  config CHEETAH
>      bool
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 037cc1fd82..e2fa677d80 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -55,6 +55,7 @@
>  #include "kvm_arm.h"
>  #include "migration/vmstate.h"
>  #include "hw/acpi/ghes.h"
> +#include "hw/acpi/viot.h"
>  
>  #define ARM_SPI_BASE 32
>  
> @@ -849,6 +850,12 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      }
>  #endif
>  
> +    if (vms->iommu == VIRT_IOMMU_VIRTIO) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_viot(tables_blob, tables->linker, vms->virtio_iommu_bdf,
> +                   vms->oem_id, vms->oem_table_id);
> +    }
> +
>      /* XSDT is pointed to by RSDP */
>      xsdt = tables_blob->len;
>      build_xsdt(tables_blob, tables->linker, table_offsets, vms->oem_id,

Re: [PATCH v2 5/8] hw/arm/virt: Reject instantiation of multiple IOMMUs

[Eric Auger]

Hi Jean,
On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> We do not support instantiating multiple IOMMUs. Before adding a
> virtio-iommu, check that no other IOMMU is present. This will detect
> both "iommu=smmuv3" machine parameter and another virtio-iommu instance.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

You may add
Fixes: 70e89132c9 ("hw/arm/virt: Add the virtio-iommu device tree mappings")
as the problem already exists with dt.

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  hw/arm/virt.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b4598d3fe6..5ca225291f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2475,6 +2475,11 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>      if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>          PCIDevice *pdev = PCI_DEVICE(dev);
>  
> +        if (vms->iommu != VIRT_IOMMU_NONE) {
> +            error_setg(errp, "virt machine does not support multiple IOMMUs");
> +            return;
> +        }
> +
>          vms->iommu = VIRT_IOMMU_VIRTIO;
>          vms->virtio_iommu_bdf = pci_get_bdf(pdev);
>          create_virtio_iommu_dt_bindings(vms);

Re: [PATCH v2 4/8] hw/arm/virt: Remove device tree restriction for virtio-iommu

[Eric Auger]

Hi Jean,

On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> virtio-iommu is now supported with ACPI VIOT as well as device tree.
> Remove the restriction that prevents from instantiating a virtio-iommu
> device under ACPI.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> ---
>  hw/arm/virt.c                | 10 ++--------
>  hw/virtio/virtio-iommu-pci.c |  7 -------
>  2 files changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 81eda46b0b..b4598d3fe6 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2551,16 +2551,10 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>  
>      if (device_is_dynamic_sysbus(mc, dev) ||
> -       (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))) {
> +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>          return HOTPLUG_HANDLER(machine);
>      }
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> -        VirtMachineState *vms = VIRT_MACHINE(machine);
> -
> -        if (!vms->bootinfo.firmware_loaded || !virt_is_acpi_enabled(vms)) {
> -            return HOTPLUG_HANDLER(machine);
> -        }
> -    }
>      return NULL;
>  }
>  
> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
> index 770c286be7..f30eb16cbf 100644
> --- a/hw/virtio/virtio-iommu-pci.c
> +++ b/hw/virtio/virtio-iommu-pci.c
> @@ -48,16 +48,9 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>      VirtIOIOMMU *s = VIRTIO_IOMMU(vdev);
>  
>      if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) {
> -        MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> -
> -        error_setg(errp,
> -                   "%s machine fails to create iommu-map device tree bindings",
> -                   mc->name);
>          error_append_hint(errp,
>                            "Check your machine implements a hotplug handler "
>                            "for the virtio-iommu-pci device\n");
> -        error_append_hint(errp, "Check the guest is booted without FW or with "
> -                          "-no-acpi\n");
>          return;
>      }
>      for (int i = 0; i < s->nb_reserved_regions; i++) {

Re: [PATCH v2 8/8] docs: Add '-device virtio-iommu' entry

[Eric Auger]

Hi Jean,

On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> Document the virtio-iommu device for qemu-system-x86_64. In particular
Nit: this is not only for qemu-system-x86_6. This also documents the
option usage for aarch64. Only the interrupt remapping note is x86-64
specific.

I think it also would be worth to mention the interaction with the
bypass iommu option as you mention "for all devices".

Thanks

Eric
> note the lack of interrupt remapping, which may be an important
> limitation on x86.
>
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  qemu-options.hx | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 83aa59a920..9a1906a748 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -976,6 +976,9 @@ SRST
>      Please also refer to the wiki page for general scenarios of VT-d
>      emulation in QEMU: https://wiki.qemu.org/Features/VT-d.
>  
> +``-device virtio-iommu``
> +    Enable a paravirtual IOMMU, that manages DMA isolation and remapping
> +    for all PCI devices, but does not support interrupt remapping.
>  ERST
>  
>  DEF("name", HAS_ARG, QEMU_OPTION_name,

Re: [PATCH v2 8/8] docs: Add '-device virtio-iommu' entry

[Daniel P. Berrangé]

On Fri, Sep 03, 2021 at 04:32:09PM +0200, Jean-Philippe Brucker wrote:
> Document the virtio-iommu device for qemu-system-x86_64. In particular
> note the lack of interrupt remapping, which may be an important
> limitation on x86.
> 
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  qemu-options.hx | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 83aa59a920..9a1906a748 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -976,6 +976,9 @@ SRST
>      Please also refer to the wiki page for general scenarios of VT-d
>      emulation in QEMU: https://wiki.qemu.org/Features/VT-d.
>  
> +``-device virtio-iommu``
> +    Enable a paravirtual IOMMU, that manages DMA isolation and remapping
> +    for all PCI devices, but does not support interrupt remapping.

It would be desirable to document why this is better/worse/equiv to
the intel-iommu device documented just before, so that people have a
better idea of which they should be trying to use.

I'm going to assume intel-iommu is more likely to "just work" out of
the box since it models real hardware that OS are likely to already
support ?  Is that right though ?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 7/8] pc: Allow instantiating a virtio-iommu device

[Eric Auger]

Hi Jean,

On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> From: Eric Auger <eric.auger@redhat.com>
>
> Add a hotplug handler for virtio-iommu on x86 and set the necessary
> reserved region property. On x86, the [0xfee00000, 0xfeefffff] DMA
> region is reserved for MSIs. DMA transactions to this range either
> trigger IRQ remapping in the IOMMU or bypasses IOMMU translation.
s/bypasses/bypass.
> Although virtio-iommu does not support IRQ remapping it must be informed
> of the reserved region so that it can forward DMA transactions targeting
> this region.

>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
Feel free to remove my SoB. I have done much here.
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/i386/pc.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 694fc9ce07..c1e1cffe16 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -797,6 +797,11 @@ void pc_machine_done(Notifier *notifier, void *data)
>                       "irqchip support.");
>          exit(EXIT_FAILURE);
>      }
> +
> +    if (pcms->virtio_iommu && x86_iommu_get_default()) {
> +        error_report("QEMU does not support multiple vIOMMUs for x86 yet.");
> +        exit(EXIT_FAILURE);
> +    }
>  }

I think you shall detect the case of dual instantiation of intel_iommu
and virtio-iommu. Maybe pc_hotplug_allowed() can be used for that. Note
that both devices can be refered to in either order.
>  
>  void pc_guest_info_init(PCMachineState *pcms)
> @@ -1376,6 +1381,14 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
>                 object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
>          pc_virtio_md_pci_pre_plug(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> +        /* Declare the reserved MSI region */
> +        char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
> +                                              VIRTIO_IOMMU_RESV_MEM_T_MSI);
> +
> +        qdev_prop_set_uint32(dev, "len-reserved-regions", 1);
> +        qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str);
> +        g_free(resv_prop_str);
>      }
>  }
>  
> @@ -1393,6 +1406,11 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>          PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>          PCIDevice *pdev = PCI_DEVICE(dev);
>  
> +        if (pcms->virtio_iommu) {
> +            error_setg(errp,
> +                       "QEMU does not support multiple vIOMMUs for x86 yet.");
> +            return;
> +        }
>          pcms->virtio_iommu = true;
>          pcms->virtio_iommu_bdf = pci_get_bdf(pdev);
>      }
> @@ -1436,7 +1454,8 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
>          object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
>          object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
> -        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>          return HOTPLUG_HANDLER(machine);
>      }
>  
Thanks

Eric

Re: [PATCH v2 8/8] docs: Add '-device virtio-iommu' entry

[Eric Auger]

Hi,

On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> Document the virtio-iommu device for qemu-system-x86_64. In particular
> note the lack of interrupt remapping, which may be an important
> limitation on x86.
>
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  qemu-options.hx | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 83aa59a920..9a1906a748 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -976,6 +976,9 @@ SRST
>      Please also refer to the wiki page for general scenarios of VT-d
>      emulation in QEMU: https://wiki.qemu.org/Features/VT-d.
>  
> +``-device virtio-iommu``
> +    Enable a paravirtual IOMMU, that manages DMA isolation and remapping
> +    for all PCI devices, but does not support interrupt remapping.
maybe you should also document which machines do support the
virtio-iommu (as done for intel-iommu). By the way intel-iommu only is
supported on q35. Is it the same for the virtio-iommu. I think we should
restrict the virtio-iommu to q35 too

Thanks

Eric
>  ERST
>  
>  DEF("name", HAS_ARG, QEMU_OPTION_name,

Re: [PATCH v2 6/8] pc: Add VIOT table for virtio-iommu

[Eric Auger]

Hi jean,

On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> The ACPI Virtual I/O Translation table (VIOT) describes the relation
> between a virtio-iommu and the endpoints it manages. When a virtio-iommu
> device is instantiated, add a VIOT table.

As there is no used of pcms->virtio_iommu and virtio_iommu_bdf yet, maybe squash this into next patch?

Thanks

Eric

>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/i386/Kconfig      | 1 +
>  include/hw/i386/pc.h | 2 ++
>  hw/i386/acpi-build.c | 5 +++++
>  hw/i386/pc.c         | 7 +++++++
>  4 files changed, 15 insertions(+)
>
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index ddedcef0b2..13db05d557 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -54,6 +54,7 @@ config PC_ACPI
>      select ACPI_X86
>      select ACPI_CPU_HOTPLUG
>      select ACPI_MEMORY_HOTPLUG
> +    select ACPI_VIOT
>      select SMBUS_EEPROM
>      select PFLASH_CFI01
>      depends on ACPI_SMBUS
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 88dffe7517..979b8d0b7c 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -45,6 +45,8 @@ typedef struct PCMachineState {
>      bool pit_enabled;
>      bool hpet_enabled;
>      bool default_bus_bypass_iommu;
> +    bool virtio_iommu;
> +    uint16_t virtio_iommu_bdf;
>      uint64_t max_fw_size;
>  
>      /* NUMA information: */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a33ac8b91e..078b7f5c6f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -71,6 +71,7 @@
>  
>  #include "hw/acpi/ipmi.h"
>  #include "hw/acpi/hmat.h"
> +#include "hw/acpi/viot.h"
>  
>  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
>   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
> @@ -2559,6 +2560,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>              build_dmar_q35(tables_blob, tables->linker, x86ms->oem_id,
>                             x86ms->oem_table_id);
>          }
> +    } else if (pcms->virtio_iommu) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_viot(tables_blob, tables->linker, pcms->virtio_iommu_bdf,
> +                   x86ms->oem_id, x86ms->oem_table_id);
>      }
>      if (machine->nvdimms_state->is_enabled) {
>          nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c2b9d62a35..694fc9ce07 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -84,6 +84,7 @@
>  #include "hw/i386/intel_iommu.h"
>  #include "hw/net/ne2000-isa.h"
>  #include "standard-headers/asm-x86/bootparam.h"
> +#include "hw/virtio/virtio-iommu.h"
>  #include "hw/virtio/virtio-pmem-pci.h"
>  #include "hw/virtio/virtio-mem-pci.h"
>  #include "hw/mem/memory-device.h"
> @@ -1388,6 +1389,12 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
>                 object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
>          pc_virtio_md_pci_plug(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> +        PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +        PCIDevice *pdev = PCI_DEVICE(dev);
> +
> +        pcms->virtio_iommu = true;
> +        pcms->virtio_iommu_bdf = pci_get_bdf(pdev);
>      }
>  }
>  

Re: [PATCH v2 2/8] hw/acpi: Add VIOT table

[Jean-Philippe Brucker]

On Mon, Sep 06, 2021 at 02:58:55PM +0200, Eric Auger wrote:
> Hi Jean,
> 
> On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> > Add a function that generates a Virtual I/O Translation table (VIOT),
> > describing the topology of paravirtual IOMMUs. The table is created when
> > instantiating a virtio-iommu device. It contains a virtio-iommu node and
> > PCI Range nodes for endpoints managed by the IOMMU. By default, a single
> > node describes all PCI devices. When passing the "default_bus_bypass_iommu"
> > machine option and "bypass_iommu" PXB option, only buses that do not
> > bypass the IOMMU are described by PCI Range nodes.
> 
> You may consider to add a test for the new VIOT table (However this may
> be added afterwards). See tests/qtest/bios-tables-test.c for the process
> and examples.

Sure, it adds 5 patches to the series

> > +/*
> > + * Generate a VIOT table with one PCI-based virtio-iommu that manages PCI
> > + * endpoints.
> Add a link to the doc + Table refs?
> 
> https://jpbrucker.net/virtio-iommu/viot/viot-v9.pdf

No, this document is only temporary until the next ACPI release. Although
the titles and text should be the same, I don't know the refs in the final
document but they will be different

> 
> > + */
> > +void build_viot(GArray *table_data, BIOSLinker *linker,
> > +                uint16_t virtio_iommu_bdf, const char *oem_id,
> > +                const char *oem_table_id)
> > +{
> > +    /* virtio-iommu node follows the header */
> > +    int viommu_off = sizeof(AcpiViot);
> > +    int viot_start = table_data->len;
> > +    struct viot_pci_ranges pci_ranges = {
> > +        .output_node = viommu_off,
> > +        .blob = g_array_new(false, true, 1),
> > +    };
> > +
> > +    /* Build the list of PCI ranges that this viommu manages */
> > +    object_child_foreach_recursive(object_get_root(), viot_host_bridges,
> > +                                   &pci_ranges);
> > +
> > +    /* VIOT header */
> > +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
> Depending on the order of landing, this may be replaced by Igor's
> 
> acpi_init_table() and associated with acpi_table_composed().

Ok I'll prepare that as a fixup, and send it or squash it after the ACPI
series lands

> <<<<<<< HEAD
> acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_true: files('pcihp.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_false:
> files('acpi-pci-hotplug-stub.c'))
> =======
> acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('piix4.c', 'pcihp.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_VIOT', if_true: files('viot.c'))
> >>>>>>> hw/acpi: Add VIOT table

Indeed, fixed

Thanks,
Jean

Re: [PATCH v2 6/8] pc: Add VIOT table for virtio-iommu

[Jean-Philippe Brucker]

On Mon, Sep 06, 2021 at 05:02:05PM +0200, Eric Auger wrote:
> Hi jean,
> 
> On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> > The ACPI Virtual I/O Translation table (VIOT) describes the relation
> > between a virtio-iommu and the endpoints it manages. When a virtio-iommu
> > device is instantiated, add a VIOT table.
> 
> As there is no used of pcms->virtio_iommu and virtio_iommu_bdf yet, maybe squash this into next patch?

Agreed

Thanks,
Jean

Re: [PATCH v2 7/8] pc: Allow instantiating a virtio-iommu device

[Jean-Philippe Brucker]

On Mon, Sep 06, 2021 at 04:57:39PM +0200, Eric Auger wrote:
> Hi Jean,
> 
> On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
> > From: Eric Auger <eric.auger@redhat.com>
> >
> > Add a hotplug handler for virtio-iommu on x86 and set the necessary
> > reserved region property. On x86, the [0xfee00000, 0xfeefffff] DMA
> > region is reserved for MSIs. DMA transactions to this range either
> > trigger IRQ remapping in the IOMMU or bypasses IOMMU translation.
> s/bypasses/bypass.
> > Although virtio-iommu does not support IRQ remapping it must be informed
> > of the reserved region so that it can forward DMA transactions targeting
> > this region.
> 
> >
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Feel free to remove my SoB. I have done much here.
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  hw/i386/pc.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 694fc9ce07..c1e1cffe16 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -797,6 +797,11 @@ void pc_machine_done(Notifier *notifier, void *data)
> >                       "irqchip support.");
> >          exit(EXIT_FAILURE);
> >      }
> > +
> > +    if (pcms->virtio_iommu && x86_iommu_get_default()) {
> > +        error_report("QEMU does not support multiple vIOMMUs for x86 yet.");
> > +        exit(EXIT_FAILURE);
> > +    }
> >  }
> 
> I think you shall detect the case of dual instantiation of intel_iommu
> and virtio-iommu.

Those are handled by x86_iommu_set_default() and
pc_machine_device_plug_cb() (below) respectively. I considered factoring
it but I think it just adds complexity.

Thanks,
Jean

> Maybe pc_hotplug_allowed() can be used for that. Note
> that both devices can be refered to in either order.
> >  
> >  void pc_guest_info_init(PCMachineState *pcms)
> > @@ -1376,6 +1381,14 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
> >                 object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> >          pc_virtio_md_pci_pre_plug(hotplug_dev, dev, errp);
> > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> > +        /* Declare the reserved MSI region */
> > +        char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
> > +                                              VIRTIO_IOMMU_RESV_MEM_T_MSI);
> > +
> > +        qdev_prop_set_uint32(dev, "len-reserved-regions", 1);
> > +        qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str);
> > +        g_free(resv_prop_str);
> >      }
> >  }
> >  
> > @@ -1393,6 +1406,11 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> >          PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >          PCIDevice *pdev = PCI_DEVICE(dev);
> >  
> > +        if (pcms->virtio_iommu) {
> > +            error_setg(errp,
> > +                       "QEMU does not support multiple vIOMMUs for x86 yet.");
> > +            return;
> > +        }
> >          pcms->virtio_iommu = true;
> >          pcms->virtio_iommu_bdf = pci_get_bdf(pdev);
> >      }
> > @@ -1436,7 +1454,8 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
> >      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> >          object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
> >          object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
> > -        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> > +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI) ||
> > +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> >          return HOTPLUG_HANDLER(machine);
> >      }
> >  
> Thanks
> 
> Eric
> 

Re: [PATCH v2 7/8] pc: Allow instantiating a virtio-iommu device

[Eric Auger]

Hi jean,

On 9/8/21 11:54 AM, Jean-Philippe Brucker wrote:
> On Mon, Sep 06, 2021 at 04:57:39PM +0200, Eric Auger wrote:
>> Hi Jean,
>>
>> On 9/3/21 4:32 PM, Jean-Philippe Brucker wrote:
>>> From: Eric Auger <eric.auger@redhat.com>
>>>
>>> Add a hotplug handler for virtio-iommu on x86 and set the necessary
>>> reserved region property. On x86, the [0xfee00000, 0xfeefffff] DMA
>>> region is reserved for MSIs. DMA transactions to this range either
>>> trigger IRQ remapping in the IOMMU or bypasses IOMMU translation.
>> s/bypasses/bypass.
>>> Although virtio-iommu does not support IRQ remapping it must be informed
>>> of the reserved region so that it can forward DMA transactions targeting
>>> this region.
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Feel free to remove my SoB. I have done much here.
>>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>>> ---
>>>  hw/i386/pc.c | 21 ++++++++++++++++++++-
>>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 694fc9ce07..c1e1cffe16 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -797,6 +797,11 @@ void pc_machine_done(Notifier *notifier, void *data)
>>>                       "irqchip support.");
>>>          exit(EXIT_FAILURE);
>>>      }
>>> +
>>> +    if (pcms->virtio_iommu && x86_iommu_get_default()) {
>>> +        error_report("QEMU does not support multiple vIOMMUs for x86 yet.");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>>  }
>> I think you shall detect the case of dual instantiation of intel_iommu
>> and virtio-iommu.
> Those are handled by x86_iommu_set_default() and
Oh I did not notice the x86_iommu_get_default() stuff.

sorry for the noise

Eric


> pc_machine_device_plug_cb() (below) respectively. I considered factoring
> it but I think it just adds complexity.
>
> Thanks,
> Jean
>
>> Maybe pc_hotplug_allowed() can be used for that. Note
>> that both devices can be refered to in either order.
>>>  
>>>  void pc_guest_info_init(PCMachineState *pcms)
>>> @@ -1376,6 +1381,14 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
>>>                 object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
>>>          pc_virtio_md_pci_pre_plug(hotplug_dev, dev, errp);
>>> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>>> +        /* Declare the reserved MSI region */
>>> +        char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
>>> +                                              VIRTIO_IOMMU_RESV_MEM_T_MSI);
>>> +
>>> +        qdev_prop_set_uint32(dev, "len-reserved-regions", 1);
>>> +        qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str);
>>> +        g_free(resv_prop_str);
>>>      }
>>>  }
>>>  
>>> @@ -1393,6 +1406,11 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>>>          PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>>>          PCIDevice *pdev = PCI_DEVICE(dev);
>>>  
>>> +        if (pcms->virtio_iommu) {
>>> +            error_setg(errp,
>>> +                       "QEMU does not support multiple vIOMMUs for x86 yet.");
>>> +            return;
>>> +        }
>>>          pcms->virtio_iommu = true;
>>>          pcms->virtio_iommu_bdf = pci_get_bdf(pdev);
>>>      }
>>> @@ -1436,7 +1454,8 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
>>>          object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
>>>          object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_PMEM_PCI) ||
>>> -        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
>>> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI) ||
>>> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>>>          return HOTPLUG_HANDLER(machine);
>>>      }
>>>  
>> Thanks
>>
>> Eric
>>

Re: [PATCH v2 8/8] docs: Add '-device virtio-iommu' entry

[Jean-Philippe Brucker]

On Mon, Sep 06, 2021 at 03:45:28PM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 03, 2021 at 04:32:09PM +0200, Jean-Philippe Brucker wrote:
> > Document the virtio-iommu device for qemu-system-x86_64. In particular
> > note the lack of interrupt remapping, which may be an important
> > limitation on x86.
> > 
> > Suggested-by: Eric Auger <eric.auger@redhat.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  qemu-options.hx | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 83aa59a920..9a1906a748 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -976,6 +976,9 @@ SRST
> >      Please also refer to the wiki page for general scenarios of VT-d
> >      emulation in QEMU: https://wiki.qemu.org/Features/VT-d.
> >  
> > +``-device virtio-iommu``
> > +    Enable a paravirtual IOMMU, that manages DMA isolation and remapping
> > +    for all PCI devices, but does not support interrupt remapping.
> 
> It would be desirable to document why this is better/worse/equiv to
> the intel-iommu device documented just before, so that people have a
> better idea of which they should be trying to use.
> 
> I'm going to assume intel-iommu is more likely to "just work" out of
> the box since it models real hardware that OS are likely to already
> support ?  Is that right though ?

That's right, and there isn't much point in using virtio-iommu on q35 at
the moment, we're still laying the foundation. It only implements caching
mode for now so it complements the vSMMU on arm which doesn't have that,
but on x86 QEMU it doesn't add anything useful. There are extensions to
get better performance for different type of endpoint, and other features,
but those are all work in progress.

I thinks I'll just drop the documentation patch until the device is more
mature and I have something useful to write here. The other IOMMUs are not
described either at the moment. x86 users will just instantiate the
intel-iommu as usual if they need an IOMMU.

Thanks,
Jean