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

[PATCH 0/6] 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.

With a simple configuration the table contains a virtio-iommu-pci node
and a pci-range node:

	qemu-system-aarch64 -M virt -bios QEMU_EFI.fd
	  -device virtio-iommu ...

	$ iasl -d ...
	[000h 0000   4]                    Signature : "VIOT"

	[024h 0036   2]                   Node count : 0002
	[026h 0038   2]                  Node offset : 0030

	[030h 0048   1]                         Type : 03 [VirtIO-PCI IOMMU]
	[032h 0050   2]                       Length : 0010
	[034h 0052   2]                  PCI Segment : 0000
	[036h 0054   2]               PCI BDF number : 0030

	[040h 0064   1]                         Type : 01 [PCI Range]
	[042h 0066   2]                       Length : 0018
	[044h 0068   4]               Endpoint start : 00000000
	[048h 0072   2]            PCI Segment start : 0000
	[04Ah 0074   2]              PCI Segment end : 0000
	[04Ch 0076   2]                PCI BDF start : 0000
	[04Eh 0078   2]                  PCI BDF end : 00FF
	[050h 0080   2]                  Output node : 0030

With a more complex topology multiple PCI Range nodes describe the system:

	qemu-system-aarch64 -bios QEMU_EFI.fd -device virtio-iommu
	  -M virt,default_bus_bypass_iommu=true
	  -device pxb-pcie,bus_nr=0x10,id=pcie.1000,bus=pcie.0
	  -device pxb-pcie,bus_nr=0x20,id=pcie.2000,bus=pcie.0,bypass_iommu=true
	  -device pxb-pcie,bus_nr=0x30,id=pcie.3000,bus=pcie.0

	[024h 0036   2]                   Node count : 0003
	[026h 0038   2]                  Node offset : 0030

	[030h 0048   1]                         Type : 03 [VirtIO-PCI IOMMU]
	[032h 0050   2]                       Length : 0010
	[034h 0052   2]                  PCI Segment : 0000
	[036h 0054   2]               PCI BDF number : 0020

	[040h 0064   1]                         Type : 01 [PCI Range]
	[042h 0066   2]                       Length : 0018
	[044h 0068   4]               Endpoint start : 00003000
	[048h 0072   2]            PCI Segment start : 0000
	[04Ah 0074   2]              PCI Segment end : 0000
	[04Ch 0076   2]                PCI BDF start : 3000
	[04Eh 0078   2]                  PCI BDF end : 32FF
	[050h 0080   2]                  Output node : 0030

	[058h 0088   1]                         Type : 01 [PCI Range]
	[05Ah 0090   2]                       Length : 0018
	[05Ch 0092   4]               Endpoint start : 00001000
	[060h 0096   2]            PCI Segment start : 0000
	[062h 0098   2]              PCI Segment end : 0000
	[064h 0100   2]                PCI BDF start : 1000
	[066h 0102   2]                  PCI BDF end : 11FF
	[068h 0104   2]                  Output node : 0030


The VIOT table description will be in the next release of ACPI.
In the meantime you can find a description at
https://jpbrucker.net/virtio-iommu/viot/viot-v9.pdf
Linux support for VIOT was added in version 5.14

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

Jean-Philippe Brucker (5):
  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
  pc: Add VIOT table for virtio-iommu

 hw/acpi/viot.h               | 13 ++++++
 include/hw/acpi/acpi-defs.h  | 60 ++++++++++++++++++++++++++
 include/hw/i386/pc.h         |  2 +
 hw/acpi/viot.c               | 82 ++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c     |  7 +++
 hw/arm/virt.c                | 10 +----
 hw/i386/acpi-build.c         |  5 +++
 hw/i386/pc.c                 | 18 +++++++-
 hw/virtio/virtio-iommu-pci.c |  7 ---
 hw/acpi/Kconfig              |  4 ++
 hw/acpi/meson.build          |  1 +
 hw/arm/Kconfig               |  1 +
 hw/i386/Kconfig              |  1 +
 13 files changed, 195 insertions(+), 16 deletions(-)
 create mode 100644 hw/acpi/viot.h
 create mode 100644 hw/acpi/viot.c

-- 
2.32.0

[PATCH 1/6] 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.32.0

[PATCH 2/6] 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/viot.h      | 13 +++++++
 hw/acpi/viot.c      | 82 +++++++++++++++++++++++++++++++++++++++++++++
 hw/acpi/Kconfig     |  4 +++
 hw/acpi/meson.build |  1 +
 4 files changed, 100 insertions(+)
 create mode 100644 hw/acpi/viot.h
 create mode 100644 hw/acpi/viot.c

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..5cd10e9553
--- /dev/null
+++ b/hw/acpi/viot.c
@@ -0,0 +1,82 @@
+/*
+ * 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"
+
+/* Build PCI range for a given PCI host bridge */
+static int viot_host_bridges(Object *obj, void *opaque)
+{
+    GArray *pci_range_blob = opaque;
+
+    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);
+
+            AcpiViotPciRange pci_range = {
+                .type = ACPI_VIOT_NODE_PCI_RANGE,
+                .length = cpu_to_le16(sizeof(pci_range)),
+                .bdf_start = cpu_to_le16(PCI_BUILD_BDF(min_bus, 0)),
+                .bdf_end = cpu_to_le16(PCI_BUILD_BDF(max_bus, 0xff)),
+                .endpoint_start = cpu_to_le32(PCI_BUILD_BDF(min_bus, 0)),
+            };
+
+            g_array_append_val(pci_range_blob, pci_range);
+        }
+    }
+
+    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)
+{
+    int i;
+    AcpiViot *viot;
+    AcpiViotPciRange *pci_range;
+    AcpiViotVirtioIommuPci *viommu;
+    int viommu_off = sizeof(*viot);
+    int viot_start = table_data->len;
+    GArray *pci_ranges = g_array_new(false, true, sizeof(*pci_range));
+
+    viot = acpi_data_push(table_data, sizeof(*viot));
+    viot->node_offset = cpu_to_le16(viommu_off);
+
+    viommu = acpi_data_push(table_data, sizeof(*viommu));
+    viommu->type = ACPI_VIOT_NODE_VIRTIO_IOMMU_PCI;
+    viommu->length = cpu_to_le16(sizeof(*viommu));
+    viommu->bdf = cpu_to_le16(virtio_iommu_bdf);
+
+    /* Build the list of PCI ranges that this viommu manages */
+    object_child_foreach_recursive(object_get_root(), viot_host_bridges,
+                                   pci_ranges);
+
+    for (i = 0; i < pci_ranges->len; i++) {
+        pci_range = &g_array_index(pci_ranges, AcpiViotPciRange, i);
+        pci_range->output_node = cpu_to_le16(viommu_off);
+    }
+    viot->node_count = cpu_to_le16(pci_ranges->len + 1);
+
+    g_array_append_vals(table_data, pci_ranges->data,
+                        pci_ranges->len * sizeof(*pci_range));
+    g_array_free(pci_ranges, 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/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/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.32.0

[PATCH 3/6] 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/virt-acpi-build.c | 7 +++++++
 hw/arm/Kconfig           | 1 +
 2 files changed, 8 insertions(+)

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,
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
-- 
2.32.0

[PATCH 4/6] 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.32.0

[PATCH 5/6] 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>
---
 include/hw/i386/pc.h | 2 ++
 hw/i386/acpi-build.c | 5 +++++
 hw/i386/pc.c         | 7 +++++++
 hw/i386/Kconfig      | 1 +
 4 files changed, 15 insertions(+)

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);
     }
 }
 
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
-- 
2.32.0

[PATCH 6/6] 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 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 694fc9ce07..fb24f000e7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1376,6 +1376,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);
     }
 }
 
@@ -1436,7 +1444,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.32.0

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

[Igor Mammedov]

On Tue, 10 Aug 2021 10:45:02 +0200
Jean-Philippe Brucker <jean-philippe@linaro.org> 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.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>


using packed structures for composing ACPI tables is discouraged,
pls use build_append_int_noprefix() API instead. You can look at
build_amd_iommu() as an example.

PS:
Also note field comments format.
/it should be verbatim copy of entry name from respective table in spec/


> ---
>  hw/acpi/viot.h      | 13 +++++++
>  hw/acpi/viot.c      | 82 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/Kconfig     |  4 +++
>  hw/acpi/meson.build |  1 +
>  4 files changed, 100 insertions(+)
>  create mode 100644 hw/acpi/viot.h
>  create mode 100644 hw/acpi/viot.c
> 
> 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..5cd10e9553
> --- /dev/null
> +++ b/hw/acpi/viot.c
> @@ -0,0 +1,82 @@
> +/*
> + * 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"
> +
> +/* Build PCI range for a given PCI host bridge */
> +static int viot_host_bridges(Object *obj, void *opaque)
> +{
> +    GArray *pci_range_blob = opaque;
> +
> +    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);
> +
> +            AcpiViotPciRange pci_range = {
> +                .type = ACPI_VIOT_NODE_PCI_RANGE,
> +                .length = cpu_to_le16(sizeof(pci_range)),
> +                .bdf_start = cpu_to_le16(PCI_BUILD_BDF(min_bus, 0)),
> +                .bdf_end = cpu_to_le16(PCI_BUILD_BDF(max_bus, 0xff)),
> +                .endpoint_start = cpu_to_le32(PCI_BUILD_BDF(min_bus, 0)),
> +            };
> +
> +            g_array_append_val(pci_range_blob, pci_range);
> +        }
> +    }
> +
> +    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)
> +{
> +    int i;
> +    AcpiViot *viot;
> +    AcpiViotPciRange *pci_range;
> +    AcpiViotVirtioIommuPci *viommu;
> +    int viommu_off = sizeof(*viot);
> +    int viot_start = table_data->len;
> +    GArray *pci_ranges = g_array_new(false, true, sizeof(*pci_range));
> +
> +    viot = acpi_data_push(table_data, sizeof(*viot));
> +    viot->node_offset = cpu_to_le16(viommu_off);
> +
> +    viommu = acpi_data_push(table_data, sizeof(*viommu));
> +    viommu->type = ACPI_VIOT_NODE_VIRTIO_IOMMU_PCI;
> +    viommu->length = cpu_to_le16(sizeof(*viommu));
> +    viommu->bdf = cpu_to_le16(virtio_iommu_bdf);
> +
> +    /* Build the list of PCI ranges that this viommu manages */
> +    object_child_foreach_recursive(object_get_root(), viot_host_bridges,
> +                                   pci_ranges);
> +
> +    for (i = 0; i < pci_ranges->len; i++) {
> +        pci_range = &g_array_index(pci_ranges, AcpiViotPciRange, i);
> +        pci_range->output_node = cpu_to_le16(viommu_off);
> +    }
> +    viot->node_count = cpu_to_le16(pci_ranges->len + 1);
> +
> +    g_array_append_vals(table_data, pci_ranges->data,
> +                        pci_ranges->len * sizeof(*pci_range));
> +    g_array_free(pci_ranges, 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/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/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'))

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

[Eric Auger]

Hi Jean,

On 8/10/21 10:45 AM, 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>
> ---
>  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");
We may check the vms->iommu is not already set to something else (to
VIRT_IOMMU_SMMUV3 for instance).

Thanks

Eric
>          return;
>      }
>      for (int i = 0; i < s->nb_reserved_regions; i++) {

Re: [PATCH 6/6] pc: Allow instantiating a virtio-iommu device

[Eric Auger]

Hi Jean,

On 8/10/21 10:45 AM, 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.
>
> 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>

I think we need to handle the case where the end-user gets lost with
iommu options and use an invalid combination such as

-M q35,iommu=on,int_remap=on,kernel_irqchip=off -device -device virtio-iommu-pci

We may also document somewhere that the virtio-iommu-pci
does not support irq remapping as this may be an important limitation on x86.

Thanks

Eric

> ---
>  hw/i386/pc.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 694fc9ce07..fb24f000e7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1376,6 +1376,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);
>      }
>  }
>  
> @@ -1436,7 +1444,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);
>      }
>  

Re: [PATCH 0/6] virtio-iommu: Add ACPI support

[Eric Auger]

Hi Jean,

On 8/10/21 10:45 AM, Jean-Philippe Brucker wrote:
> Allow instantiating a virtio-iommu device on ACPI systems by adding a
> Virtual I/O Translation table (VIOT). Enable x86 support for VIOT.

Don't you need your other patch
"virtio-iommu: Default to bypass during boot"?

Without this latter, for me the guest fails to boot.

Thanks

Eric
>
> With a simple configuration the table contains a virtio-iommu-pci node
> and a pci-range node:
>
> 	qemu-system-aarch64 -M virt -bios QEMU_EFI.fd
> 	  -device virtio-iommu ...
>
> 	$ iasl -d ...
> 	[000h 0000   4]                    Signature : "VIOT"
>
> 	[024h 0036   2]                   Node count : 0002
> 	[026h 0038   2]                  Node offset : 0030
>
> 	[030h 0048   1]                         Type : 03 [VirtIO-PCI IOMMU]
> 	[032h 0050   2]                       Length : 0010
> 	[034h 0052   2]                  PCI Segment : 0000
> 	[036h 0054   2]               PCI BDF number : 0030
>
> 	[040h 0064   1]                         Type : 01 [PCI Range]
> 	[042h 0066   2]                       Length : 0018
> 	[044h 0068   4]               Endpoint start : 00000000
> 	[048h 0072   2]            PCI Segment start : 0000
> 	[04Ah 0074   2]              PCI Segment end : 0000
> 	[04Ch 0076   2]                PCI BDF start : 0000
> 	[04Eh 0078   2]                  PCI BDF end : 00FF
> 	[050h 0080   2]                  Output node : 0030
>
> With a more complex topology multiple PCI Range nodes describe the system:
>
> 	qemu-system-aarch64 -bios QEMU_EFI.fd -device virtio-iommu
> 	  -M virt,default_bus_bypass_iommu=true
> 	  -device pxb-pcie,bus_nr=0x10,id=pcie.1000,bus=pcie.0
> 	  -device pxb-pcie,bus_nr=0x20,id=pcie.2000,bus=pcie.0,bypass_iommu=true
> 	  -device pxb-pcie,bus_nr=0x30,id=pcie.3000,bus=pcie.0
>
> 	[024h 0036   2]                   Node count : 0003
> 	[026h 0038   2]                  Node offset : 0030
>
> 	[030h 0048   1]                         Type : 03 [VirtIO-PCI IOMMU]
> 	[032h 0050   2]                       Length : 0010
> 	[034h 0052   2]                  PCI Segment : 0000
> 	[036h 0054   2]               PCI BDF number : 0020
>
> 	[040h 0064   1]                         Type : 01 [PCI Range]
> 	[042h 0066   2]                       Length : 0018
> 	[044h 0068   4]               Endpoint start : 00003000
> 	[048h 0072   2]            PCI Segment start : 0000
> 	[04Ah 0074   2]              PCI Segment end : 0000
> 	[04Ch 0076   2]                PCI BDF start : 3000
> 	[04Eh 0078   2]                  PCI BDF end : 32FF
> 	[050h 0080   2]                  Output node : 0030
>
> 	[058h 0088   1]                         Type : 01 [PCI Range]
> 	[05Ah 0090   2]                       Length : 0018
> 	[05Ch 0092   4]               Endpoint start : 00001000
> 	[060h 0096   2]            PCI Segment start : 0000
> 	[062h 0098   2]              PCI Segment end : 0000
> 	[064h 0100   2]                PCI BDF start : 1000
> 	[066h 0102   2]                  PCI BDF end : 11FF
> 	[068h 0104   2]                  Output node : 0030
>
>
> The VIOT table description will be in the next release of ACPI.
> In the meantime you can find a description at
> https://jpbrucker.net/virtio-iommu/viot/viot-v9.pdf
> Linux support for VIOT was added in version 5.14
>
> Eric Auger (1):
>   pc: Allow instantiating a virtio-iommu device
>
> Jean-Philippe Brucker (5):
>   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
>   pc: Add VIOT table for virtio-iommu
>
>  hw/acpi/viot.h               | 13 ++++++
>  include/hw/acpi/acpi-defs.h  | 60 ++++++++++++++++++++++++++
>  include/hw/i386/pc.h         |  2 +
>  hw/acpi/viot.c               | 82 ++++++++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c     |  7 +++
>  hw/arm/virt.c                | 10 +----
>  hw/i386/acpi-build.c         |  5 +++
>  hw/i386/pc.c                 | 18 +++++++-
>  hw/virtio/virtio-iommu-pci.c |  7 ---
>  hw/acpi/Kconfig              |  4 ++
>  hw/acpi/meson.build          |  1 +
>  hw/arm/Kconfig               |  1 +
>  hw/i386/Kconfig              |  1 +
>  13 files changed, 195 insertions(+), 16 deletions(-)
>  create mode 100644 hw/acpi/viot.h
>  create mode 100644 hw/acpi/viot.c
>

Re: [PATCH 6/6] pc: Allow instantiating a virtio-iommu device

[Jean-Philippe Brucker]

On Tue, Aug 17, 2021 at 04:11:49PM +0200, Eric Auger wrote:
> Hi Jean,
> 
> On 8/10/21 10:45 AM, 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.
> >
> > 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>
> 
> I think we need to handle the case where the end-user gets lost with
> iommu options and use an invalid combination such as
> 
> -M q35,iommu=on,int_remap=on,kernel_irqchip=off -device -device virtio-iommu-pci

I guess that would be
"-M q35,kernel_irqchip=off -device intel-iommu,intremap=on -device virtio-iommu-pci"

I'll add the checks, similar to the one in x86_iommu_set_default().

> We may also document somewhere that the virtio-iommu-pci
> does not support irq remapping as this may be an important limitation on x86.

I'll mention it in the commit message, unless you had another place in
mind?

Thanks,
Jean

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

[Jean-Philippe Brucker]

On Tue, Aug 17, 2021 at 03:42:22PM +0200, Eric Auger wrote:
> > 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");
> We may check the vms->iommu is not already set to something else (to
> VIRT_IOMMU_SMMUV3 for instance).

Since that check is machine specific, virt_machine_device_plug_cb() in
hw/arm/virt.c may be a good place for it. The change feels unrelated to
this series but it's simple enough that I'm tempted to just append the
patch at the end. It also deals with trying to instantiate multiple
virtio-iommu devices, which isn't supported either.

Thanks,
Jean

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

[Jean-Philippe Brucker]

On Tue, Aug 10, 2021 at 11:22:27AM +0200, Igor Mammedov wrote:
> On Tue, 10 Aug 2021 10:45:02 +0200
> Jean-Philippe Brucker <jean-philippe@linaro.org> 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.
> > 
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> 
> using packed structures for composing ACPI tables is discouraged,
> pls use build_append_int_noprefix() API instead. You can look at
> build_amd_iommu() as an example.
> 
> PS:
> Also note field comments format.
> /it should be verbatim copy of entry name from respective table in spec/

Got it, I'll switch to build_append_int_noprefix()

Thanks,
Jean

Re: [PATCH 0/6] virtio-iommu: Add ACPI support

[Jean-Philippe Brucker]

Hi Eric,

On Tue, Aug 17, 2021 at 04:58:01PM +0200, Eric Auger wrote:
> Hi Jean,
> 
> On 8/10/21 10:45 AM, Jean-Philippe Brucker wrote:
> > Allow instantiating a virtio-iommu device on ACPI systems by adding a
> > Virtual I/O Translation table (VIOT). Enable x86 support for VIOT.
> 
> Don't you need your other patch
> "virtio-iommu: Default to bypass during boot"?
> 
> Without this latter, for me the guest fails to boot.

Good point, I think I've been lucky during my testing. My bootloader and
kernel are on virtio-blk-pci devices and as I wasn't explicitly enabling
the "iommu_platform" parameter, they would bypass the IOMMU. When enabling
the parameter, boot hangs since the IOMMU isn't enabled when the
bootloader needs to fetch the kernel, and DMA faults. That parameter is
specific to virtio devices. Using another storage for bootloader and
kernel will result in failure to boot.

I've been postponing the boot-bypass patch since it requires a
specification change to be done right, but it's next on my list.

Thanks,
Jean

Re: [PATCH 6/6] pc: Allow instantiating a virtio-iommu device

[Eric Auger]

Hi Jean,

On 8/27/21 3:26 PM, Jean-Philippe Brucker wrote:
> On Tue, Aug 17, 2021 at 04:11:49PM +0200, Eric Auger wrote:
>> Hi Jean,
>>
>> On 8/10/21 10:45 AM, 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.
>>>
>>> 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>
>> I think we need to handle the case where the end-user gets lost with
>> iommu options and use an invalid combination such as
>>
>> -M q35,iommu=on,int_remap=on,kernel_irqchip=off -device -device virtio-iommu-pci
> I guess that would be
> "-M q35,kernel_irqchip=off -device intel-iommu,intremap=on -device virtio-iommu-pci"
>
> I'll add the checks, similar to the one in x86_iommu_set_default().
yes that what I meant sorry.
>
>> We may also document somewhere that the virtio-iommu-pci
>> does not support irq remapping as this may be an important limitation on x86.
> I'll mention it in the commit message, unless you had another place in
> mind?

you may add an entry in qemu-options.hx too as Peter did for the intel
iommu in
7395b3e3e7  docs: Add '-device intel-iommu' entry (7 weeks ago) <Peter Xu>

Thanks

Eric
>
> Thanks,
> Jean
>

Re: [PATCH 0/6] virtio-iommu: Add ACPI support

[Eric Auger]

Hi Jean,

On 8/27/21 3:30 PM, Jean-Philippe Brucker wrote:
> Hi Eric,
>
> On Tue, Aug 17, 2021 at 04:58:01PM +0200, Eric Auger wrote:
>> Hi Jean,
>>
>> On 8/10/21 10:45 AM, Jean-Philippe Brucker wrote:
>>> Allow instantiating a virtio-iommu device on ACPI systems by adding a
>>> Virtual I/O Translation table (VIOT). Enable x86 support for VIOT.
>> Don't you need your other patch
>> "virtio-iommu: Default to bypass during boot"?
>>
>> Without this latter, for me the guest fails to boot.
> Good point, I think I've been lucky during my testing. My bootloader and
> kernel are on virtio-blk-pci devices and as I wasn't explicitly enabling
> the "iommu_platform" parameter, they would bypass the IOMMU. When enabling
> the parameter, boot hangs since the IOMMU isn't enabled when the
> bootloader needs to fetch the kernel, and DMA faults. That parameter is
> specific to virtio devices. Using another storage for bootloader and
> kernel will result in failure to boot.
>
> I've been postponing the boot-bypass patch since it requires a
> specification change to be done right, but it's next on my list.
The boot-bypass feature seems a critical feature to overcome the current
v3 limitation. Are there big spec changes required? Maybe we shall work
on this in parallel of this series because, to me, it will delay the
integration of virtio-iommu in libvirt for instance.

Thanks

Eric
>
> Thanks,
> Jean
>

Re: [PATCH 0/6] virtio-iommu: Add ACPI support

[Jean-Philippe Brucker]

On Wed, Sep 29, 2021 at 11:18:39AM +0200, Eric Auger wrote:
> > I've been postponing the boot-bypass patch since it requires a
> > specification change to be done right, but it's next on my list.
> The boot-bypass feature seems a critical feature to overcome the current
> v3 limitation. Are there big spec changes required? Maybe we shall work
> on this in parallel of this series because, to me, it will delay the
> integration of virtio-iommu in libvirt for instance.

I agree. The spec change is ready but I wanted to tidy up the driver
implementation first. I'll send something out this week

Thanks,
Jean