* [PATCH v4 00/11] virtio-iommu: Add ACPI support
@ 2021-10-01 17:33 Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 01/11] hw/acpi: Add VIOT table Jean-Philippe Brucker
` (11 more replies)
0 siblings, 12 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
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 v3 [1]:
* Cleaned the IOMMU-uniqueness checks. Added patch 6 to have a
single check on x86.
* Added patch 5 that allows to gracefully propagate errors when setting
resv_mem properties.
* Squashed table blobs into patch 11.
* Addressed all other comments from v3.
Caveats:
* Series depends on "acpi: refactor error prone build_header() and
packed structures usage in ACPI tables" [2]
* Since virtio-iommu doesn't support boot-bypass at the moment, firmware
can't access storage behind the IOMMU to load bootloader or kernel.
This will be solved by another series currently in flight [3]. In the
meantime you can use a storage device that bypasses the IOMMU such as
virtio-blk-pci (without iommu_platform property) or a bypass bridge
(docs/iommu-bypass.txt).
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
[1] https://lore.kernel.org/qemu-devel/20210914142004.2433568-1-jean-philippe@linaro.org/
[2] https://lore.kernel.org/qemu-devel/20210924122802.1455362-1-imammedo@redhat.com/
[3] https://lore.kernel.org/qemu-devel/20210930185050.262759-1-jean-philippe@linaro.org/
Jean-Philippe Brucker (11):
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
hw/arm/virt: Use object_property_set instead of qdev_prop_set
hw/i386: Move vIOMMU uniqueness check into pc.c
pc: Allow instantiating a virtio-iommu device
tests/acpi: allow updates of VIOT expected data files
tests/acpi: add test cases for VIOT
tests/acpi: add expected blob for VIOT test on virt machine
tests/acpi: add expected blobs for VIOT test on q35 machine
hw/acpi/viot.h | 13 ++++
include/hw/i386/pc.h | 2 +
hw/acpi/viot.c | 112 +++++++++++++++++++++++++++++++++
hw/arm/virt-acpi-build.c | 7 +++
hw/arm/virt.c | 20 +++---
hw/i386/acpi-build.c | 5 ++
hw/i386/pc.c | 30 ++++++++-
hw/i386/x86-iommu.c | 6 --
hw/virtio/virtio-iommu-pci.c | 7 ---
tests/qtest/bios-tables-test.c | 38 +++++++++++
hw/acpi/Kconfig | 4 ++
hw/acpi/meson.build | 1 +
hw/arm/Kconfig | 1 +
hw/i386/Kconfig | 1 +
tests/data/acpi/q35/DSDT.viot | Bin 0 -> 9398 bytes
tests/data/acpi/q35/VIOT.viot | Bin 0 -> 112 bytes
tests/data/acpi/virt/VIOT | Bin 0 -> 88 bytes
17 files changed, 223 insertions(+), 24 deletions(-)
create mode 100644 hw/acpi/viot.h
create mode 100644 hw/acpi/viot.c
create mode 100644 tests/data/acpi/q35/DSDT.viot
create mode 100644 tests/data/acpi/q35/VIOT.viot
create mode 100644 tests/data/acpi/virt/VIOT
--
2.33.0
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 01/11] hw/acpi: Add VIOT table
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-06 8:09 ` Igor Mammedov
2021-10-01 17:33 ` [PATCH v4 02/11] hw/arm/virt-acpi-build: Add VIOT table for virtio-iommu Jean-Philippe Brucker
` (10 subsequent siblings)
11 siblings, 1 reply; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
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.
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
hw/acpi/viot.h | 13 +++++
hw/acpi/viot.c | 112 ++++++++++++++++++++++++++++++++++++++++++++
hw/acpi/Kconfig | 4 ++
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/viot.h b/hw/acpi/viot.h
new file mode 100644
index 0000000000..9fe565bb87
--- /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(MachineState *ms, 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..e33d468e11
--- /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 build_pci_range_node(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 (PCI range) */
+ build_append_int_noprefix(blob, 1, 1);
+ /* Reserved */
+ build_append_int_noprefix(blob, 0, 1);
+ /* Length */
+ build_append_int_noprefix(blob, 24, 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(MachineState *ms, GArray *table_data, BIOSLinker *linker,
+ uint16_t virtio_iommu_bdf, const char *oem_id,
+ const char *oem_table_id)
+{
+ /* The virtio-iommu node follows the 48-bytes header */
+ int viommu_off = 48;
+ AcpiTable table = { .sig = "VIOT", .rev = 0,
+ .oem_id = oem_id, .oem_table_id = oem_table_id };
+ struct viot_pci_ranges pci_ranges = {
+ .output_node = viommu_off,
+ .blob = g_array_new(false, true /* clear */, 1),
+ };
+
+ /* Build the list of PCI ranges that this viommu manages */
+ object_child_foreach_recursive(OBJECT(ms), build_pci_range_node,
+ &pci_ranges);
+
+ /* ACPI table header */
+ acpi_table_begin(&table, table_data);
+ /* 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 node */
+ /* Type (virtio-pci IOMMU) */
+ build_append_int_noprefix(table_data, 3, 1);
+ /* Reserved */
+ build_append_int_noprefix(table_data, 0, 1);
+ /* Length */
+ build_append_int_noprefix(table_data, 16, 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);
+
+ acpi_table_end(linker, &table);
+}
+
diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index 3b5e118c54..622b0b50b7 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -51,6 +51,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 7d8c0eb43e..adf6347bc4 100644
--- a/hw/acpi/meson.build
+++ b/hw/acpi/meson.build
@@ -20,6 +20,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files(
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_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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 02/11] hw/arm/virt-acpi-build: Add VIOT table for virtio-iommu
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 01/11] hw/acpi: Add VIOT table Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 03/11] hw/arm/virt: Remove device tree restriction " Jean-Philippe Brucker
` (9 subsequent siblings)
11 siblings, 0 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
When a virtio-iommu is instantiated, describe it using the ACPI VIOT
table.
Reviewed-by: Eric Auger <eric.auger@redhat.com>
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 6cec97352b..e26639e1e1 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
@@ -934,6 +935,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(ms, 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 18832abf7d..a05d75faca 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -27,6 +27,7 @@ config ARM_VIRT
select DIMM
select ACPI_HW_REDUCED
select ACPI_APEI
+ select ACPI_VIOT
config CHEETAH
bool
--
2.33.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 03/11] hw/arm/virt: Remove device tree restriction for virtio-iommu
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 01/11] hw/acpi: Add VIOT table Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 02/11] hw/arm/virt-acpi-build: Add VIOT table for virtio-iommu Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-05 11:57 ` Eric Auger
2021-10-01 17:33 ` [PATCH v4 04/11] hw/arm/virt: Reject instantiation of multiple IOMMUs Jean-Philippe Brucker
` (8 subsequent siblings)
11 siblings, 1 reply; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
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.
Reviewed-by: Eric Auger <eric.auger@redhat.com>
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 1d59f0e59f..56e8fc7059 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2561,16 +2561,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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 04/11] hw/arm/virt: Reject instantiation of multiple IOMMUs
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
` (2 preceding siblings ...)
2021-10-01 17:33 ` [PATCH v4 03/11] hw/arm/virt: Remove device tree restriction " Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-06 6:35 ` Igor Mammedov
2021-10-01 17:33 ` [PATCH v4 05/11] hw/arm/virt: Use object_property_set instead of qdev_prop_set Jean-Philippe Brucker
` (7 subsequent siblings)
11 siblings, 1 reply; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
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.
Fixes: 70e89132c9 ("hw/arm/virt: Add the virtio-iommu device tree mappings")
Reviewed-by: Eric Auger <eric.auger@redhat.com>
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 56e8fc7059..36f0261ef4 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2441,6 +2441,11 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
hwaddr db_start = 0, db_end = 0;
char *resv_prop_str;
+ if (vms->iommu != VIRT_IOMMU_NONE) {
+ error_setg(errp, "virt machine does not support multiple IOMMUs");
+ return;
+ }
+
switch (vms->msi_controller) {
case VIRT_MSI_CTRL_NONE:
return;
--
2.33.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 05/11] hw/arm/virt: Use object_property_set instead of qdev_prop_set
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
` (3 preceding siblings ...)
2021-10-01 17:33 ` [PATCH v4 04/11] hw/arm/virt: Reject instantiation of multiple IOMMUs Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-05 9:27 ` Eric Auger
2021-10-06 6:36 ` Igor Mammedov
2021-10-01 17:33 ` [PATCH v4 06/11] hw/i386: Move vIOMMU uniqueness check into pc.c Jean-Philippe Brucker
` (6 subsequent siblings)
11 siblings, 2 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
To propagate errors to the caller of the pre_plug callback, use the
object_poperty_set*() functions directly instead of the qdev_prop_set*()
helpers.
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
hw/arm/virt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 36f0261ef4..ac307b6030 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2465,8 +2465,9 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
db_start, db_end,
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);
+ object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp);
+ object_property_set_str(OBJECT(dev), "reserved-regions[0]",
+ resv_prop_str, errp);
g_free(resv_prop_str);
}
}
--
2.33.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 06/11] hw/i386: Move vIOMMU uniqueness check into pc.c
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
` (4 preceding siblings ...)
2021-10-01 17:33 ` [PATCH v4 05/11] hw/arm/virt: Use object_property_set instead of qdev_prop_set Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-05 11:41 ` Eric Auger
2021-10-01 17:33 ` [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device Jean-Philippe Brucker
` (5 subsequent siblings)
11 siblings, 1 reply; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
We're about to need this check for a third vIOMMU, virtio-iommu, which
doesn't inherit X86IOMMUState as it doesn't support IRQ remapping and is
a virtio device. Move the check into the pre_plug callback to be shared
by all three vIOMMUs.
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
hw/i386/pc.c | 10 +++++++++-
hw/i386/x86-iommu.c | 6 ------
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 557d49c9f8..789ccb6ef4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1367,6 +1367,13 @@ static void pc_virtio_md_pci_unplug(HotplugHandler *hotplug_dev,
static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
+ if (object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) &&
+ x86_iommu_get_default()) {
+ error_setg(errp, "QEMU does not support multiple vIOMMUs "
+ "for x86 yet.");
+ return;
+ }
+
if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
pc_memory_pre_plug(hotplug_dev, dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
@@ -1428,7 +1435,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_X86_IOMMU_DEVICE)) {
return HOTPLUG_HANDLER(machine);
}
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 86ad03972e..550e551993 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -84,12 +84,6 @@ static void x86_iommu_set_default(X86IOMMUState *x86_iommu)
{
assert(x86_iommu);
- if (x86_iommu_default) {
- error_report("QEMU does not support multiple vIOMMUs "
- "for x86 yet.");
- exit(1);
- }
-
x86_iommu_default = x86_iommu;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
` (5 preceding siblings ...)
2021-10-01 17:33 ` [PATCH v4 06/11] hw/i386: Move vIOMMU uniqueness check into pc.c Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-05 19:18 ` Eric Auger
` (2 more replies)
2021-10-01 17:33 ` [PATCH v4 08/11] tests/acpi: allow updates of VIOT expected data files Jean-Philippe Brucker
` (4 subsequent siblings)
11 siblings, 3 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
Allow instantiating a virtio-iommu device by adding an ACPI Virtual I/O
Translation table (VIOT), which describes the relation between the
virtio-iommu and the endpoints it manages.
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: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
include/hw/i386/pc.h | 2 ++
hw/i386/acpi-build.c | 5 +++++
hw/i386/pc.c | 24 ++++++++++++++++++++++--
hw/i386/Kconfig | 1 +
4 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 82cf7b7e30..f3ba1ee4c0 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;
/* ACPI Memory hotplug IO base address */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d1c28440f4..4e46585709 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
@@ -2593,6 +2594,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(machine, 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 789ccb6ef4..31710bc4fb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -83,6 +83,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"
@@ -1367,8 +1368,11 @@ static void pc_virtio_md_pci_unplug(HotplugHandler *hotplug_dev,
static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
- if (object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) &&
- x86_iommu_get_default()) {
+ PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+
+ if ((object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) ||
+ object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) &&
+ (x86_iommu_get_default() || pcms->virtio_iommu)) {
error_setg(errp, "QEMU does not support multiple vIOMMUs "
"for x86 yet.");
return;
@@ -1381,6 +1385,15 @@ 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 APIC range as the reserved MSI region */
+ char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
+ VIRTIO_IOMMU_RESV_MEM_T_MSI);
+
+ object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp);
+ object_property_set_str(OBJECT(dev), "reserved-regions[0]",
+ resv_prop_str, errp);
+ g_free(resv_prop_str);
}
}
@@ -1394,6 +1407,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);
}
}
@@ -1436,6 +1455,7 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
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_IOMMU_PCI) ||
object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE)) {
return HOTPLUG_HANDLER(machine);
}
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.33.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 08/11] tests/acpi: allow updates of VIOT expected data files
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
` (6 preceding siblings ...)
2021-10-01 17:33 ` [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-06 8:12 ` Igor Mammedov
2021-10-01 17:33 ` [PATCH v4 09/11] tests/acpi: add test cases for VIOT Jean-Philippe Brucker
` (3 subsequent siblings)
11 siblings, 1 reply; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
Create empty data files and allow updates for the upcoming VIOT tests.
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
tests/data/acpi/q35/DSDT.viot | 0
tests/data/acpi/q35/VIOT.viot | 0
tests/data/acpi/virt/VIOT | 0
4 files changed, 3 insertions(+)
create mode 100644 tests/data/acpi/q35/DSDT.viot
create mode 100644 tests/data/acpi/q35/VIOT.viot
create mode 100644 tests/data/acpi/virt/VIOT
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..29b5b1eabc 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,4 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/VIOT",
+"tests/data/acpi/q35/DSDT.viot",
+"tests/data/acpi/q35/VIOT.viot",
diff --git a/tests/data/acpi/q35/DSDT.viot b/tests/data/acpi/q35/DSDT.viot
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/data/acpi/q35/VIOT.viot b/tests/data/acpi/q35/VIOT.viot
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/data/acpi/virt/VIOT b/tests/data/acpi/virt/VIOT
new file mode 100644
index 0000000000..e69de29bb2
--
2.33.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 09/11] tests/acpi: add test cases for VIOT
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
` (7 preceding siblings ...)
2021-10-01 17:33 ` [PATCH v4 08/11] tests/acpi: allow updates of VIOT expected data files Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-05 10:27 ` Ani Sinha
` (2 more replies)
2021-10-01 17:33 ` [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine Jean-Philippe Brucker
` (2 subsequent siblings)
11 siblings, 3 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
Add two test cases for VIOT, one on the q35 machine and the other on
virt. To test complex topologies the q35 test has two PCIe buses that
bypass the IOMMU (and are therefore not described by VIOT), and two
buses that are translated by virtio-iommu.
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
tests/qtest/bios-tables-test.c | 38 ++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 4f11d03055..b6cb383bd9 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1403,6 +1403,42 @@ static void test_acpi_virt_tcg(void)
free_test_data(&data);
}
+static void test_acpi_q35_viot(void)
+{
+ test_data data = {
+ .machine = MACHINE_Q35,
+ .variant = ".viot",
+ };
+
+ /*
+ * To keep things interesting, two buses bypass the IOMMU.
+ * VIOT should only describes the other two buses.
+ */
+ test_acpi_one("-machine default_bus_bypass_iommu=on "
+ "-device virtio-iommu "
+ "-device pxb-pcie,bus_nr=0x10,id=pcie.100,bus=pcie.0 "
+ "-device pxb-pcie,bus_nr=0x20,id=pcie.200,bus=pcie.0,bypass_iommu=on "
+ "-device pxb-pcie,bus_nr=0x30,id=pcie.300,bus=pcie.0",
+ &data);
+ free_test_data(&data);
+}
+
+static void test_acpi_virt_viot(void)
+{
+ test_data data = {
+ .machine = "virt",
+ .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
+ .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
+ .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
+ .ram_start = 0x40000000ULL,
+ .scan_len = 128ULL * 1024 * 1024,
+ };
+
+ test_acpi_one("-cpu cortex-a57 "
+ "-device virtio-iommu", &data);
+ free_test_data(&data);
+}
+
static void test_oem_fields(test_data *data)
{
int i;
@@ -1567,12 +1603,14 @@ int main(int argc, char *argv[])
if (strcmp(arch, "x86_64") == 0) {
qtest_add_func("acpi/microvm/pcie", test_acpi_microvm_pcie_tcg);
}
+ qtest_add_func("acpi/q35/viot", test_acpi_q35_viot);
} else if (strcmp(arch, "aarch64") == 0) {
qtest_add_func("acpi/virt", test_acpi_virt_tcg);
qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
qtest_add_func("acpi/virt/memhp", test_acpi_virt_tcg_memhp);
qtest_add_func("acpi/virt/pxb", test_acpi_virt_tcg_pxb);
qtest_add_func("acpi/virt/oem-fields", test_acpi_oem_fields_virt);
+ qtest_add_func("acpi/virt/viot", test_acpi_virt_viot);
}
ret = g_test_run();
boot_sector_cleanup(disk);
--
2.33.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
` (8 preceding siblings ...)
2021-10-01 17:33 ` [PATCH v4 09/11] tests/acpi: add test cases for VIOT Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-05 10:04 ` Ani Sinha
2021-10-05 19:38 ` Eric Auger
2021-10-01 17:33 ` [PATCH v4 11/11] tests/acpi: add expected blobs for VIOT test on q35 machine Jean-Philippe Brucker
2021-10-05 15:45 ` [PATCH v4 00/11] virtio-iommu: Add ACPI support Michael S. Tsirkin
11 siblings, 2 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
The VIOT blob contains the following:
[000h 0000 4] Signature : "VIOT" [Virtual I/O Translation Table]
[004h 0004 4] Table Length : 00000058
[008h 0008 1] Revision : 00
[009h 0009 1] Checksum : 66
[00Ah 0010 6] Oem ID : "BOCHS "
[010h 0016 8] Oem Table ID : "BXPC "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "BXPC"
[020h 0032 4] Asl Compiler Revision : 00000001
[024h 0036 2] Node count : 0002
[026h 0038 2] Node offset : 0030
[028h 0040 8] Reserved : 0000000000000000
[030h 0048 1] Type : 03 [VirtIO-PCI IOMMU]
[031h 0049 1] Reserved : 00
[032h 0050 2] Length : 0010
[034h 0052 2] PCI Segment : 0000
[036h 0054 2] PCI BDF number : 0008
[038h 0056 8] Reserved : 0000000000000000
[040h 0064 1] Type : 01 [PCI Range]
[041h 0065 1] Reserved : 00
[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
[052h 0082 6] Reserved : 000000000000
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
tests/qtest/bios-tables-test-allowed-diff.h | 1 -
tests/data/acpi/virt/VIOT | Bin 0 -> 88 bytes
2 files changed, 1 deletion(-)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 29b5b1eabc..fa213e4738 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,4 +1,3 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/virt/VIOT",
"tests/data/acpi/q35/DSDT.viot",
"tests/data/acpi/q35/VIOT.viot",
diff --git a/tests/data/acpi/virt/VIOT b/tests/data/acpi/virt/VIOT
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..921f40d88c28ba2171a4d664e119914335309e7d 100644
GIT binary patch
literal 88
zcmWIZ^bd((0D?3pe`k+i1*eDrX9XZ&1PX!JAexE60Hgv8m>C3sGzXN&z`)2L0cSHX
I{D-Rq0Q5fy0RR91
literal 0
HcmV?d00001
--
2.33.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 11/11] tests/acpi: add expected blobs for VIOT test on q35 machine
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
` (9 preceding siblings ...)
2021-10-01 17:33 ` [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine Jean-Philippe Brucker
@ 2021-10-01 17:33 ` Jean-Philippe Brucker
2021-10-05 10:07 ` Ani Sinha
2021-10-05 19:41 ` Eric Auger
2021-10-05 15:45 ` [PATCH v4 00/11] virtio-iommu: Add ACPI support Michael S. Tsirkin
11 siblings, 2 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-01 17:33 UTC (permalink / raw)
To: eric.auger, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, Jean-Philippe Brucker, qemu-arm, ani, pbonzini
Add expected blobs of the VIOT and DSDT table for the VIOT test on the
q35 machine.
Since the test instantiates a virtio device and two PCIe expander
bridges, DSDT.viot has more blocks than the base DSDT (long diff not
shown here). The VIOT table generated for the q35 test is:
[000h 0000 4] Signature : "VIOT" [Virtual I/O Translation Table]
[004h 0004 4] Table Length : 00000070
[008h 0008 1] Revision : 00
[009h 0009 1] Checksum : 3D
[00Ah 0010 6] Oem ID : "BOCHS "
[010h 0016 8] Oem Table ID : "BXPC "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "BXPC"
[020h 0032 4] Asl Compiler Revision : 00000001
[024h 0036 2] Node count : 0003
[026h 0038 2] Node offset : 0030
[028h 0040 8] Reserved : 0000000000000000
[030h 0048 1] Type : 03 [VirtIO-PCI IOMMU]
[031h 0049 1] Reserved : 00
[032h 0050 2] Length : 0010
[034h 0052 2] PCI Segment : 0000
[036h 0054 2] PCI BDF number : 0010
[038h 0056 8] Reserved : 0000000000000000
[040h 0064 1] Type : 01 [PCI Range]
[041h 0065 1] Reserved : 00
[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 : 30FF
[050h 0080 2] Output node : 0030
[052h 0082 6] Reserved : 000000000000
[058h 0088 1] Type : 01 [PCI Range]
[059h 0089 1] Reserved : 00
[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 : 10FF
[068h 0104 2] Output node : 0030
[06Ah 0106 6] Reserved : 000000000000
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
tests/qtest/bios-tables-test-allowed-diff.h | 2 --
tests/data/acpi/q35/DSDT.viot | Bin 0 -> 9398 bytes
tests/data/acpi/q35/VIOT.viot | Bin 0 -> 112 bytes
3 files changed, 2 deletions(-)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index fa213e4738..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,3 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/DSDT.viot",
-"tests/data/acpi/q35/VIOT.viot",
diff --git a/tests/data/acpi/q35/DSDT.viot b/tests/data/acpi/q35/DSDT.viot
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..b41270ff6d63493c2ae379ddd1d3e28f190a6c01 100644
GIT binary patch
literal 9398
zcmeHNO>7&-8J*>iv|O&FB}G~OOGG$M|57BBoWHhc5OS9yDTx$CQgH$r;8Idr*-4Q_
z5(9Az1F`}niVsB-#zBvCpa8wKr(7GLxfJNZhXOUwQxCo5S`_gq>icGPq#2R|qEj!C
zfZhFO-<xmV?9RU7?0QYF^~FWTjE@VeZn>E)yj*v)_%j$|bWD4v61&3MJ6@sGF_Mv(
z(Y~GJ$Ji9i%ul_-ddc|xw*RT`zx{!4bOW~WnR9oe8@#vYZ!iK~-v}&=4xHj-r&;K<
zcU`OQR&r*iT=DGueakdEt~iRCoxImzW@o+PvCPVNXSM0Z?!3la@A7=V7VmARrY)yk
z{pY1`=FY$P>E*ZcU;gqRzq<396$4-adlUOh0d4%7zBT9fosWB0jax+L=jQv<ANTla
zQRdK@z^9UXwkV>i=J#J~?>_G}@-A=VM7>texw(0?%WX7MbJqC}W*M`obLj6+2L}g#
z7KhBa!JMioR2I#0z1Wf}4QL}(?VWPHRb@6~_rFcDSo^j^@$^f@nwPCNyiPXrY^T}E
zvw%wcfQq{B`j+GO?T>ms>-oupgMHSY{HWJupLA{Zum8sP*}gR;+Lp2=-%n6m?tjZ-
zjG;9@c#>K}{oUR@TWRJyyo-^34o#_78fy{Dw`^y5>Zzy%5~{uX^m4%iSX`qhT8~!A
zG^eeZlHoI-8Ai$2Vq4f>h#*^g_hNN*{g5>^t+7liet~+Zy}PhdZ_UfPW8!)n8rHEU
zO2#|UccP|wVTaee;I38=IdP!Tn<F?6qYtCZyx?%o<BgVk*qUT`{dm;@3z%VEU<SIV
zmOARAox0m>8Og6~%fzLjz(wD!XR-0J?VV<E38Ua|wuK9qq`)oYic2_As8t(A^6!F1
zfm^7pSF`ns_j0yv6jt12mU+DH7MCLJ$0#~D2(}3k+%T>(s-yiwD&A+AC-UHoLQ!1-
zZTt}HXS}hx*Q`$VSHhuj|GB^ZyZOw!)sJSsuAcdeTMekL*MH;pAM0IX{WHC*Rs<v9
z7Qc^d+_nd7KNU4@(}vxf?a%bCS>r)E9$^!#8~A%&#`e2rz2YvijNQTB2(~G5e*20+
zH;dzb%?EP5(W<AH-`YthW4JrnN^QBw#Ib?nMV7Xy+=?J5$smfL%+eDvv;!ka;KGPl
z08WZ?oCl~3iHZ6-Ho}>}h7mC(G{QI&P|ie1Otgk$qns&Q5M{)a(5PSn%9#j>DYIZ)
z2`sNC#+ect6HM87gsRTCrZdi&5*imw*?5Gi&M{5r7-vf8n649{s&ib^Ij-p(*L5OP
zb()$^Q`2ecIuWWm@dQ$OI-%)I=sFRqIxS77rRlVEod{K(Nlj-`)0xzDB2;zaS*To3
zThnRlIuWWmCp4WCn$8JbCqh-{q^5IH(>bZ@M5yYV(sWK~I;V7<2vwbqrqj`MI=W7T
zs?L<AGo|TF={garI@6lYw5Bty>qMyPoYr(sYdWWOod{K(8BJ$K)0xqAB2;zGXgX&!
zoin;lgsRR{n$A<2&QrQhgsM)=Byji1=g_RCb5_@hP}O-_(|KCcd0N+rP}O;cGxOn-
z@C;`b!iU`%!E}#8VtOI=tj0X6G0*Bugevo##yqDn&*@BrD)YR?Jg+g&>r8|y^AU~t
zh{k+GXChRYk8-ATnMXNOKI0!1O!?qONKAPJ=d_%2TFyB=Cqj|agn{N211&WxNX^aE
zz%des28sY_MG!?Glfpm+j$4w!h$Y)+AgO>J8Yn_34F)Q((m)j`8K{6B8Yn`vMjEKV
zh7sjd4OBo64HO|-#IZ0?feoWjBZrcK3aAWKoiI>QEoZ_&6(|`fLg|WRpa`W-7^uK<
zCJa=8l7R{+&q)S~Q0jz%3M^;BKouw%sDN@N87M-j69y`<oCyO}pk$x|%9&)K2<7#J
zfeI{V!ax-$8K{7ACK)I~sS^e&u$&14RiI>`0?L_Wpa`W-7^uK<CJa=8l7R{+XOe*;
zlsaLc0?U~&Pz6c`DxjQ628vMXgn<eyXTm@gC>f}LawZulLa7r5DzKah16818paRO7
zWS|J8P8g`bawZH^fs%m=C})y^B9uB|paRR8Fi-_b1}dPONd}5g>V$y`EN8+%6(|{~
zfN~}oC_<?d1}d<e2?JH2WS|1dnPi{{rA`>Az;Y%GRDqI#3Mglifg+STVW0xbnJ`cV
zN(L&RoJj_XQ0jz%3M^;BKouw%sDN@N87M-j69y`<oCyO}pk$x|%9&)K2&GOKsK9b2
z3{-)VfeI*Rl7S+WI$@v!%b74x1xf}gpqxnticso=fg(~26p?D62vq|`s2Hfign=qd
zGEjv{2C6V&pb8TPsxZkw6($*|!i0e;Oc<!bBm-5LWS|NY2C6V&pbC==RAG{VDohwC
zB6)RTponnAcxRG<BE)5oAu(m&!axzpzJ-Azl6^}Cict0~87M+56`k$CLRsJL@gJ-E
z^n>)?813@y+dqFQO21NRRfE<$jCx}&564|A(8i^WXELn4tQF~akv1jTl+Da^WeuwV
z{lracXB&$wiG3@XdAhRZHWC0da;Zrx`QaE#@Rgt&><*P#acnEW&24Ln(GeQaz|zag
zIOYg>x#VGExrP>lOf<WhI%Q3NILQ)GxEsUso8d`})r-s&UkBQ!CZIL>OseBmn(P)G
zTRXgDiq@kT5N?CVvz=z6Y24Wk+d%25Jj`Ag$d1L6@0`7$WH0dS1+-7iUdW8jUTE)D
zNxN7|!*XA!-HMzby{n{md3tvsy?ac0H%#vhh_|qy^_|swN_vl{_Xg5?$E5ed^u@vS
zu@j^(D(Q<neQ_Xt@tE|*F#Y0S`uGXbFDmI5dHThH^oz%&UkuZi2Gft7Abm+mU*hRY
z1L;f0q%Vc(%Y*61PmsQ>q%ZUI<$?6&W73zAK1PqS!Ss$uFCQM5R$#b`^;^9xGscHJ
zb~LWT>IR3gmC~!jTw|y@Q8u!=p>dhktHT6js5((Lvbv#hh9zJcXHpnuB}3JTvXRvd
zjhl>K9i}Qn)rqo^)eVh<8NE8pUxum^Wh1K_y01^^)nQUIRGlarS>4cmI;&TQna)sk
zqD-&OId`<4y*t&c7VxSCuT|_Q*8H8-7sT6%vUTNX#lPMtHsxJvVQ|8mCfxdZw*1E1
z!Ryz5oqhAwx8At^=H{z!v9{sXSC+%Ob;7br#sm9-ZP-}VhbPg}`2*Xs+qZv2L7rvZ
zda-7>m0Htq8`YN#EP&@|vuLoW>sCE1vZo0db(_VNEZ|<gmBm_frB*QmMuoWRFJ0k!
z&>+#siM(5{<Z|;lYKC}m6wjpG*5WIXzSY}i@4LBN?rNqyhSmFgqW##Fr(*Ke1_>nr
zwER9;jAQ}x`)7NDCrCnIO<~!6=*-IUzMrYaN!G~{8G}`!hLQDW7EifNw`LwA!*pxK
z%jw8Lz%~P8>oYIi+cE;S6&QZNx}UAszw7zN-Axqv!RBB}E_kpI7o|UYcd!(fW%KiX
zqSv(!kmmDX)cO9!p`N4b9&GMW8@ud!7p=PYT>qIN7{4=c`8KU+Fw`+P3EuThda+oH
zi8qT>;Rf5JOAc+D?fKE-hU!9lu5XO)=4!%SZMN)Vx_R{7GE_f0FSt6QZvrbaSW3`q
zgN5kkk*#aL_dMOx82|Xg@W6d-?$h0k5m3#yQc25XSZ$zrXl4>GO<PG4!?v$p%bdgD
zDb<UeVs3$jBc28l-BZqLj85@^AC9Tqvn=y7VCNZi#Niu3@y1FisJk`ZQVroO&0G-=
zm;6lKy@aO@o+B1^g+h&X!>@b)udjGBFD773Ef%CBJAAa%#_x8mo#5TB6=55}%{RLn
z_<g}P`%Z5YpoDnu<FPF&Fb)gyC`k1-t;k_XAuNd=mbe&^w{C1$4x{&Dds~bJH@m2N
zZ_8l8)}Bv;W8W83fEg>jH^CxURAD*%(^bVgO08=b!miDVuEp;Au03Jb!fQr$?R?m^
zj_6wKzVF%-cCEu`M$7No>Wr{%{+5`3Jh7CtzL2*!V_1s=xMV!EAKHeAas!hT{C$JF
z|Dk;tPhik6_8<P$1wT)}lMfE)cQXA&nD#w}iTB!n6ld5Uv@>7gLyd36nJ-_s^8~)(
z2x}8%^!TKg;&_s3e-zVK3vLWgOWYs0BlssFc|0w}T#3q+Ns{=IAt}Z)pCo0+vSSAS
z^kFGhFV`ERDnTcWm~ai(^_BgVZJfq4;m22yzImyW9*Y@hJC2|3q|KNW!SjovXXp$m
zaei@h8QQx~E?#!UM0jGYSY3y&ED#!Bcc?9P;!d2Vbxt9{AL`DMNmsGGn{4KC;v*7t
z$xoWO=S6@kW-!dooAVySR}45<kO!COppBX6d`JW~A_|VkQC_fcgoufPBSf5K#>jWr
F{{VUX)Oi2^
literal 0
HcmV?d00001
diff --git a/tests/data/acpi/q35/VIOT.viot b/tests/data/acpi/q35/VIOT.viot
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..9b179266ccbf84f1c250ee646812d17e27987764 100644
GIT binary patch
literal 112
zcmWIZ^baXu00LVle`k+i1*eDrX9XZ&1PX!JAex!M0Hgv8m>C3sGzdcgBZCA3T-xBj
Q0Zb)W9Hva*zW_`e0M!8s0RR91
literal 0
HcmV?d00001
--
2.33.0
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v4 05/11] hw/arm/virt: Use object_property_set instead of qdev_prop_set
2021-10-01 17:33 ` [PATCH v4 05/11] hw/arm/virt: Use object_property_set instead of qdev_prop_set Jean-Philippe Brucker
@ 2021-10-05 9:27 ` Eric Auger
2021-10-06 6:36 ` Igor Mammedov
1 sibling, 0 replies; 45+ messages in thread
From: Eric Auger @ 2021-10-05 9:27 UTC (permalink / raw)
To: Jean-Philippe Brucker, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, qemu-arm, ani, pbonzini
On 10/1/21 7:33 PM, Jean-Philippe Brucker wrote:
> To propagate errors to the caller of the pre_plug callback, use the
> object_poperty_set*() functions directly instead of the qdev_prop_set*()
> helpers.
>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
> ---
> hw/arm/virt.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 36f0261ef4..ac307b6030 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2465,8 +2465,9 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> db_start, db_end,
> 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);
> + object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp);
> + object_property_set_str(OBJECT(dev), "reserved-regions[0]",
> + resv_prop_str, errp);
> g_free(resv_prop_str);
> }
> }
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine
2021-10-01 17:33 ` [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine Jean-Philippe Brucker
@ 2021-10-05 10:04 ` Ani Sinha
2021-10-08 15:33 ` Jean-Philippe Brucker
2021-10-05 19:38 ` Eric Auger
1 sibling, 1 reply; 45+ messages in thread
From: Ani Sinha @ 2021-10-05 10:04 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, pbonzini, ani, imammedo
On Fri, 1 Oct 2021, Jean-Philippe Brucker wrote:
> The VIOT blob contains the following:
>
> [000h 0000 4] Signature : "VIOT" [Virtual I/O Translation Table]
> [004h 0004 4] Table Length : 00000058
> [008h 0008 1] Revision : 00
> [009h 0009 1] Checksum : 66
> [00Ah 0010 6] Oem ID : "BOCHS "
> [010h 0016 8] Oem Table ID : "BXPC "
> [018h 0024 4] Oem Revision : 00000001
> [01Ch 0028 4] Asl Compiler ID : "BXPC"
> [020h 0032 4] Asl Compiler Revision : 00000001
>
> [024h 0036 2] Node count : 0002
> [026h 0038 2] Node offset : 0030
> [028h 0040 8] Reserved : 0000000000000000
>
> [030h 0048 1] Type : 03 [VirtIO-PCI IOMMU]
> [031h 0049 1] Reserved : 00
> [032h 0050 2] Length : 0010
>
> [034h 0052 2] PCI Segment : 0000
> [036h 0054 2] PCI BDF number : 0008
> [038h 0056 8] Reserved : 0000000000000000
>
> [040h 0064 1] Type : 01 [PCI Range]
> [041h 0065 1] Reserved : 00
> [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
> [052h 0082 6] Reserved : 000000000000
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Acked-by: Ani Sinha <ani@anisinha.ca>
Without looking at the other patches, the disassembly looks good (with
latest iasl from upstream git).
One suggestion : maybe also add the raw table data as well of length 88.
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 1 -
> tests/data/acpi/virt/VIOT | Bin 0 -> 88 bytes
> 2 files changed, 1 deletion(-)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 29b5b1eabc..fa213e4738 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,4 +1,3 @@
> /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/virt/VIOT",
> "tests/data/acpi/q35/DSDT.viot",
> "tests/data/acpi/q35/VIOT.viot",
> diff --git a/tests/data/acpi/virt/VIOT b/tests/data/acpi/virt/VIOT
> index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..921f40d88c28ba2171a4d664e119914335309e7d 100644
> GIT binary patch
> literal 88
> zcmWIZ^bd((0D?3pe`k+i1*eDrX9XZ&1PX!JAexE60Hgv8m>C3sGzXN&z`)2L0cSHX
> I{D-Rq0Q5fy0RR91
>
> literal 0
> HcmV?d00001
>
> --
> 2.33.0
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 11/11] tests/acpi: add expected blobs for VIOT test on q35 machine
2021-10-01 17:33 ` [PATCH v4 11/11] tests/acpi: add expected blobs for VIOT test on q35 machine Jean-Philippe Brucker
@ 2021-10-05 10:07 ` Ani Sinha
2021-10-05 19:41 ` Eric Auger
1 sibling, 0 replies; 45+ messages in thread
From: Ani Sinha @ 2021-10-05 10:07 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, pbonzini, ani, imammedo
On Fri, 1 Oct 2021, Jean-Philippe Brucker wrote:
> Add expected blobs of the VIOT and DSDT table for the VIOT test on the
> q35 machine.
>
> Since the test instantiates a virtio device and two PCIe expander
> bridges, DSDT.viot has more blocks than the base DSDT (long diff not
> shown here).
For documentation and bisection of issues in future, I think its better to
provide the DSDT table ASL diff here as well.
>The VIOT table generated for the q35 test is:
>
> [000h 0000 4] Signature : "VIOT" [Virtual I/O Translation Table]
> [004h 0004 4] Table Length : 00000070
> [008h 0008 1] Revision : 00
> [009h 0009 1] Checksum : 3D
> [00Ah 0010 6] Oem ID : "BOCHS "
> [010h 0016 8] Oem Table ID : "BXPC "
> [018h 0024 4] Oem Revision : 00000001
> [01Ch 0028 4] Asl Compiler ID : "BXPC"
> [020h 0032 4] Asl Compiler Revision : 00000001
>
> [024h 0036 2] Node count : 0003
> [026h 0038 2] Node offset : 0030
> [028h 0040 8] Reserved : 0000000000000000
>
> [030h 0048 1] Type : 03 [VirtIO-PCI IOMMU]
> [031h 0049 1] Reserved : 00
> [032h 0050 2] Length : 0010
>
> [034h 0052 2] PCI Segment : 0000
> [036h 0054 2] PCI BDF number : 0010
> [038h 0056 8] Reserved : 0000000000000000
>
> [040h 0064 1] Type : 01 [PCI Range]
> [041h 0065 1] Reserved : 00
> [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 : 30FF
> [050h 0080 2] Output node : 0030
> [052h 0082 6] Reserved : 000000000000
>
> [058h 0088 1] Type : 01 [PCI Range]
> [059h 0089 1] Reserved : 00
> [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 : 10FF
> [068h 0104 2] Output node : 0030
> [06Ah 0106 6] Reserved : 000000000000
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 2 --
> tests/data/acpi/q35/DSDT.viot | Bin 0 -> 9398 bytes
> tests/data/acpi/q35/VIOT.viot | Bin 0 -> 112 bytes
> 3 files changed, 2 deletions(-)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index fa213e4738..dfb8523c8b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,3 +1 @@
> /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/q35/DSDT.viot",
> -"tests/data/acpi/q35/VIOT.viot",
> diff --git a/tests/data/acpi/q35/DSDT.viot b/tests/data/acpi/q35/DSDT.viot
> index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..b41270ff6d63493c2ae379ddd1d3e28f190a6c01 100644
> GIT binary patch
> literal 9398
> zcmeHNO>7&-8J*>iv|O&FB}G~OOGG$M|57BBoWHhc5OS9yDTx$CQgH$r;8Idr*-4Q_
> z5(9Az1F`}niVsB-#zBvCpa8wKr(7GLxfJNZhXOUwQxCo5S`_gq>icGPq#2R|qEj!C
> zfZhFO-<xmV?9RU7?0QYF^~FWTjE@VeZn>E)yj*v)_%j$|bWD4v61&3MJ6@sGF_Mv(
> z(Y~GJ$Ji9i%ul_-ddc|xw*RT`zx{!4bOW~WnR9oe8@#vYZ!iK~-v}&=4xHj-r&;K<
> zcU`OQR&r*iT=DGueakdEt~iRCoxImzW@o+PvCPVNXSM0Z?!3la@A7=V7VmARrY)yk
> z{pY1`=FY$P>E*ZcU;gqRzq<396$4-adlUOh0d4%7zBT9fosWB0jax+L=jQv<ANTla
> zQRdK@z^9UXwkV>i=J#J~?>_G}@-A=VM7>texw(0?%WX7MbJqC}W*M`obLj6+2L}g#
> z7KhBa!JMioR2I#0z1Wf}4QL}(?VWPHRb@6~_rFcDSo^j^@$^f@nwPCNyiPXrY^T}E
> zvw%wcfQq{B`j+GO?T>ms>-oupgMHSY{HWJupLA{Zum8sP*}gR;+Lp2=-%n6m?tjZ-
> zjG;9@c#>K}{oUR@TWRJyyo-^34o#_78fy{Dw`^y5>Zzy%5~{uX^m4%iSX`qhT8~!A
> zG^eeZlHoI-8Ai$2Vq4f>h#*^g_hNN*{g5>^t+7liet~+Zy}PhdZ_UfPW8!)n8rHEU
> zO2#|UccP|wVTaee;I38=IdP!Tn<F?6qYtCZyx?%o<BgVk*qUT`{dm;@3z%VEU<SIV
> zmOARAox0m>8Og6~%fzLjz(wD!XR-0J?VV<E38Ua|wuK9qq`)oYic2_As8t(A^6!F1
> zfm^7pSF`ns_j0yv6jt12mU+DH7MCLJ$0#~D2(}3k+%T>(s-yiwD&A+AC-UHoLQ!1-
> zZTt}HXS}hx*Q`$VSHhuj|GB^ZyZOw!)sJSsuAcdeTMekL*MH;pAM0IX{WHC*Rs<v9
> z7Qc^d+_nd7KNU4@(}vxf?a%bCS>r)E9$^!#8~A%&#`e2rz2YvijNQTB2(~G5e*20+
> zH;dzb%?EP5(W<AH-`YthW4JrnN^QBw#Ib?nMV7Xy+=?J5$smfL%+eDvv;!ka;KGPl
> z08WZ?oCl~3iHZ6-Ho}>}h7mC(G{QI&P|ie1Otgk$qns&Q5M{)a(5PSn%9#j>DYIZ)
> z2`sNC#+ect6HM87gsRTCrZdi&5*imw*?5Gi&M{5r7-vf8n649{s&ib^Ij-p(*L5OP
> zb()$^Q`2ecIuWWm@dQ$OI-%)I=sFRqIxS77rRlVEod{K(Nlj-`)0xzDB2;zaS*To3
> zThnRlIuWWmCp4WCn$8JbCqh-{q^5IH(>bZ@M5yYV(sWK~I;V7<2vwbqrqj`MI=W7T
> zs?L<AGo|TF={garI@6lYw5Bty>qMyPoYr(sYdWWOod{K(8BJ$K)0xqAB2;zGXgX&!
> zoin;lgsRR{n$A<2&QrQhgsM)=Byji1=g_RCb5_@hP}O-_(|KCcd0N+rP}O;cGxOn-
> z@C;`b!iU`%!E}#8VtOI=tj0X6G0*Bugevo##yqDn&*@BrD)YR?Jg+g&>r8|y^AU~t
> zh{k+GXChRYk8-ATnMXNOKI0!1O!?qONKAPJ=d_%2TFyB=Cqj|agn{N211&WxNX^aE
> zz%des28sY_MG!?Glfpm+j$4w!h$Y)+AgO>J8Yn_34F)Q((m)j`8K{6B8Yn`vMjEKV
> zh7sjd4OBo64HO|-#IZ0?feoWjBZrcK3aAWKoiI>QEoZ_&6(|`fLg|WRpa`W-7^uK<
> zCJa=8l7R{+&q)S~Q0jz%3M^;BKouw%sDN@N87M-j69y`<oCyO}pk$x|%9&)K2<7#J
> zfeI{V!ax-$8K{7ACK)I~sS^e&u$&14RiI>`0?L_Wpa`W-7^uK<CJa=8l7R{+XOe*;
> zlsaLc0?U~&Pz6c`DxjQ628vMXgn<eyXTm@gC>f}LawZulLa7r5DzKah16818paRO7
> zWS|J8P8g`bawZH^fs%m=C})y^B9uB|paRR8Fi-_b1}dPONd}5g>V$y`EN8+%6(|{~
> zfN~}oC_<?d1}d<e2?JH2WS|1dnPi{{rA`>Az;Y%GRDqI#3Mglifg+STVW0xbnJ`cV
> zN(L&RoJj_XQ0jz%3M^;BKouw%sDN@N87M-j69y`<oCyO}pk$x|%9&)K2&GOKsK9b2
> z3{-)VfeI*Rl7S+WI$@v!%b74x1xf}gpqxnticso=fg(~26p?D62vq|`s2Hfign=qd
> zGEjv{2C6V&pb8TPsxZkw6($*|!i0e;Oc<!bBm-5LWS|NY2C6V&pbC==RAG{VDohwC
> zB6)RTponnAcxRG<BE)5oAu(m&!axzpzJ-Azl6^}Cict0~87M+56`k$CLRsJL@gJ-E
> z^n>)?813@y+dqFQO21NRRfE<$jCx}&564|A(8i^WXELn4tQF~akv1jTl+Da^WeuwV
> z{lracXB&$wiG3@XdAhRZHWC0da;Zrx`QaE#@Rgt&><*P#acnEW&24Ln(GeQaz|zag
> zIOYg>x#VGExrP>lOf<WhI%Q3NILQ)GxEsUso8d`})r-s&UkBQ!CZIL>OseBmn(P)G
> zTRXgDiq@kT5N?CVvz=z6Y24Wk+d%25Jj`Ag$d1L6@0`7$WH0dS1+-7iUdW8jUTE)D
> zNxN7|!*XA!-HMzby{n{md3tvsy?ac0H%#vhh_|qy^_|swN_vl{_Xg5?$E5ed^u@vS
> zu@j^(D(Q<neQ_Xt@tE|*F#Y0S`uGXbFDmI5dHThH^oz%&UkuZi2Gft7Abm+mU*hRY
> z1L;f0q%Vc(%Y*61PmsQ>q%ZUI<$?6&W73zAK1PqS!Ss$uFCQM5R$#b`^;^9xGscHJ
> zb~LWT>IR3gmC~!jTw|y@Q8u!=p>dhktHT6js5((Lvbv#hh9zJcXHpnuB}3JTvXRvd
> zjhl>K9i}Qn)rqo^)eVh<8NE8pUxum^Wh1K_y01^^)nQUIRGlarS>4cmI;&TQna)sk
> zqD-&OId`<4y*t&c7VxSCuT|_Q*8H8-7sT6%vUTNX#lPMtHsxJvVQ|8mCfxdZw*1E1
> z!Ryz5oqhAwx8At^=H{z!v9{sXSC+%Ob;7br#sm9-ZP-}VhbPg}`2*Xs+qZv2L7rvZ
> zda-7>m0Htq8`YN#EP&@|vuLoW>sCE1vZo0db(_VNEZ|<gmBm_frB*QmMuoWRFJ0k!
> z&>+#siM(5{<Z|;lYKC}m6wjpG*5WIXzSY}i@4LBN?rNqyhSmFgqW##Fr(*Ke1_>nr
> zwER9;jAQ}x`)7NDCrCnIO<~!6=*-IUzMrYaN!G~{8G}`!hLQDW7EifNw`LwA!*pxK
> z%jw8Lz%~P8>oYIi+cE;S6&QZNx}UAszw7zN-Axqv!RBB}E_kpI7o|UYcd!(fW%KiX
> zqSv(!kmmDX)cO9!p`N4b9&GMW8@ud!7p=PYT>qIN7{4=c`8KU+Fw`+P3EuThda+oH
> zi8qT>;Rf5JOAc+D?fKE-hU!9lu5XO)=4!%SZMN)Vx_R{7GE_f0FSt6QZvrbaSW3`q
> zgN5kkk*#aL_dMOx82|Xg@W6d-?$h0k5m3#yQc25XSZ$zrXl4>GO<PG4!?v$p%bdgD
> zDb<UeVs3$jBc28l-BZqLj85@^AC9Tqvn=y7VCNZi#Niu3@y1FisJk`ZQVroO&0G-=
> zm;6lKy@aO@o+B1^g+h&X!>@b)udjGBFD773Ef%CBJAAa%#_x8mo#5TB6=55}%{RLn
> z_<g}P`%Z5YpoDnu<FPF&Fb)gyC`k1-t;k_XAuNd=mbe&^w{C1$4x{&Dds~bJH@m2N
> zZ_8l8)}Bv;W8W83fEg>jH^CxURAD*%(^bVgO08=b!miDVuEp;Au03Jb!fQr$?R?m^
> zj_6wKzVF%-cCEu`M$7No>Wr{%{+5`3Jh7CtzL2*!V_1s=xMV!EAKHeAas!hT{C$JF
> z|Dk;tPhik6_8<P$1wT)}lMfE)cQXA&nD#w}iTB!n6ld5Uv@>7gLyd36nJ-_s^8~)(
> z2x}8%^!TKg;&_s3e-zVK3vLWgOWYs0BlssFc|0w}T#3q+Ns{=IAt}Z)pCo0+vSSAS
> z^kFGhFV`ERDnTcWm~ai(^_BgVZJfq4;m22yzImyW9*Y@hJC2|3q|KNW!SjovXXp$m
> zaei@h8QQx~E?#!UM0jGYSY3y&ED#!Bcc?9P;!d2Vbxt9{AL`DMNmsGGn{4KC;v*7t
> z$xoWO=S6@kW-!dooAVySR}45<kO!COppBX6d`JW~A_|VkQC_fcgoufPBSf5K#>jWr
> F{{VUX)Oi2^
>
> literal 0
> HcmV?d00001
>
> diff --git a/tests/data/acpi/q35/VIOT.viot b/tests/data/acpi/q35/VIOT.viot
> index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..9b179266ccbf84f1c250ee646812d17e27987764 100644
> GIT binary patch
> literal 112
> zcmWIZ^baXu00LVle`k+i1*eDrX9XZ&1PX!JAex!M0Hgv8m>C3sGzdcgBZCA3T-xBj
> Q0Zb)W9Hva*zW_`e0M!8s0RR91
>
> literal 0
> HcmV?d00001
>
> --
> 2.33.0
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 09/11] tests/acpi: add test cases for VIOT
2021-10-01 17:33 ` [PATCH v4 09/11] tests/acpi: add test cases for VIOT Jean-Philippe Brucker
@ 2021-10-05 10:27 ` Ani Sinha
2021-10-08 15:27 ` Jean-Philippe Brucker
2021-10-05 19:40 ` Eric Auger
2021-10-06 8:14 ` Igor Mammedov
2 siblings, 1 reply; 45+ messages in thread
From: Ani Sinha @ 2021-10-05 10:27 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, pbonzini, ani, imammedo
On Fri, 1 Oct 2021, Jean-Philippe Brucker wrote:
> Add two test cases for VIOT, one on the q35 machine and the other on
> virt. To test complex topologies the q35 test has two PCIe buses that
> bypass the IOMMU (and are therefore not described by VIOT), and two
> buses that are translated by virtio-iommu.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
This might be a stupid question but what about virtio-mmio and single mmio
cases? I see none of your tables has nodes for those and here too you do
not add test cases for it.
> ---
> tests/qtest/bios-tables-test.c | 38 ++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 4f11d03055..b6cb383bd9 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1403,6 +1403,42 @@ static void test_acpi_virt_tcg(void)
> free_test_data(&data);
> }
>
> +static void test_acpi_q35_viot(void)
> +{
> + test_data data = {
> + .machine = MACHINE_Q35,
> + .variant = ".viot",
> + };
> +
> + /*
> + * To keep things interesting, two buses bypass the IOMMU.
> + * VIOT should only describes the other two buses.
> + */
> + test_acpi_one("-machine default_bus_bypass_iommu=on "
> + "-device virtio-iommu "
> + "-device pxb-pcie,bus_nr=0x10,id=pcie.100,bus=pcie.0 "
> + "-device pxb-pcie,bus_nr=0x20,id=pcie.200,bus=pcie.0,bypass_iommu=on "
> + "-device pxb-pcie,bus_nr=0x30,id=pcie.300,bus=pcie.0",
> + &data);
> + free_test_data(&data);
> +}
> +
> +static void test_acpi_virt_viot(void)
> +{
> + test_data data = {
> + .machine = "virt",
> + .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
> + .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
> + .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
> + .ram_start = 0x40000000ULL,
> + .scan_len = 128ULL * 1024 * 1024,
> + };
> +
> + test_acpi_one("-cpu cortex-a57 "
> + "-device virtio-iommu", &data);
> + free_test_data(&data);
> +}
> +
> static void test_oem_fields(test_data *data)
> {
> int i;
> @@ -1567,12 +1603,14 @@ int main(int argc, char *argv[])
> if (strcmp(arch, "x86_64") == 0) {
> qtest_add_func("acpi/microvm/pcie", test_acpi_microvm_pcie_tcg);
> }
> + qtest_add_func("acpi/q35/viot", test_acpi_q35_viot);
> } else if (strcmp(arch, "aarch64") == 0) {
> qtest_add_func("acpi/virt", test_acpi_virt_tcg);
> qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
> qtest_add_func("acpi/virt/memhp", test_acpi_virt_tcg_memhp);
> qtest_add_func("acpi/virt/pxb", test_acpi_virt_tcg_pxb);
> qtest_add_func("acpi/virt/oem-fields", test_acpi_oem_fields_virt);
> + qtest_add_func("acpi/virt/viot", test_acpi_virt_viot);
> }
> ret = g_test_run();
> boot_sector_cleanup(disk);
> --
> 2.33.0
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 06/11] hw/i386: Move vIOMMU uniqueness check into pc.c
2021-10-01 17:33 ` [PATCH v4 06/11] hw/i386: Move vIOMMU uniqueness check into pc.c Jean-Philippe Brucker
@ 2021-10-05 11:41 ` Eric Auger
0 siblings, 0 replies; 45+ messages in thread
From: Eric Auger @ 2021-10-05 11:41 UTC (permalink / raw)
To: Jean-Philippe Brucker, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, qemu-arm, ani, pbonzini
Hi jean,
On 10/1/21 7:33 PM, Jean-Philippe Brucker wrote:
> We're about to need this check for a third vIOMMU, virtio-iommu, which
> doesn't inherit X86IOMMUState as it doesn't support IRQ remapping and is
> a virtio device. Move the check into the pre_plug callback to be shared
> by all three vIOMMUs.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Eric
> ---
> hw/i386/pc.c | 10 +++++++++-
> hw/i386/x86-iommu.c | 6 ------
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 557d49c9f8..789ccb6ef4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1367,6 +1367,13 @@ static void pc_virtio_md_pci_unplug(HotplugHandler *hotplug_dev,
> static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> + if (object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) &&
> + x86_iommu_get_default()) {
> + error_setg(errp, "QEMU does not support multiple vIOMMUs "
> + "for x86 yet.");
> + return;
> + }
> +
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> pc_memory_pre_plug(hotplug_dev, dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> @@ -1428,7 +1435,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_X86_IOMMU_DEVICE)) {
> return HOTPLUG_HANDLER(machine);
> }
>
> diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
> index 86ad03972e..550e551993 100644
> --- a/hw/i386/x86-iommu.c
> +++ b/hw/i386/x86-iommu.c
> @@ -84,12 +84,6 @@ static void x86_iommu_set_default(X86IOMMUState *x86_iommu)
> {
> assert(x86_iommu);
>
> - if (x86_iommu_default) {
> - error_report("QEMU does not support multiple vIOMMUs "
> - "for x86 yet.");
> - exit(1);
> - }
> -
> x86_iommu_default = x86_iommu;
> }
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 03/11] hw/arm/virt: Remove device tree restriction for virtio-iommu
2021-10-01 17:33 ` [PATCH v4 03/11] hw/arm/virt: Remove device tree restriction " Jean-Philippe Brucker
@ 2021-10-05 11:57 ` Eric Auger
2021-10-08 15:20 ` Jean-Philippe Brucker
0 siblings, 1 reply; 45+ messages in thread
From: Eric Auger @ 2021-10-05 11:57 UTC (permalink / raw)
To: Jean-Philippe Brucker, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, qemu-arm, ani, pbonzini
Hi Jean,
On 10/1/21 7:33 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.
>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 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 1d59f0e59f..56e8fc7059 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2561,16 +2561,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);
actually this does not work. To add a hint you need the *errp to be set.
Otherwise when running through this path you will get
emu-system-x86_64: ../util/error.c:158: error_append_hint: Assertion
`err && errp != &error_abort && errp != &error_fatal' failed.
replace the error_append_hint with an error_setg (without the \n)
Thanks
Eric
> 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++) {
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/11] virtio-iommu: Add ACPI support
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
` (10 preceding siblings ...)
2021-10-01 17:33 ` [PATCH v4 11/11] tests/acpi: add expected blobs for VIOT test on q35 machine Jean-Philippe Brucker
@ 2021-10-05 15:45 ` Michael S. Tsirkin
2021-10-08 15:17 ` Jean-Philippe Brucker
11 siblings, 1 reply; 45+ messages in thread
From: Michael S. Tsirkin @ 2021-10-05 15:45 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, richard.henderson,
qemu-devel, eric.auger, qemu-arm, imammedo, ani, pbonzini
Looks like this can not be applied yet because the bypass bit
isn't in yet. what's up with that?
--
MST
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device
2021-10-01 17:33 ` [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device Jean-Philippe Brucker
@ 2021-10-05 19:18 ` Eric Auger
2021-10-06 7:19 ` Igor Mammedov
2021-10-08 10:46 ` Michael S. Tsirkin
2 siblings, 0 replies; 45+ messages in thread
From: Eric Auger @ 2021-10-05 19:18 UTC (permalink / raw)
To: Jean-Philippe Brucker, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, qemu-arm, ani, pbonzini
Hi Jean,
On 10/1/21 7:33 PM, Jean-Philippe Brucker wrote:
> Allow instantiating a virtio-iommu device by adding an ACPI Virtual I/O
> Translation table (VIOT), which describes the relation between the
> virtio-iommu and the endpoints it manages.
>
> 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: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
tested by a protecting a virtio-net-pci device plugged onto a pxb-pcie
and setting
default-bus-bypass-iommu=true on pcie.0.
As described in the cover letter,
without [PATCH 0/3] virtio-iommu: Support VIRTIO_IOMMU_F_BYPASS_CONFIG
the ahci emits some failure if it is protected by the virtio-iommu:
qemu-system-x86_64: virtio_iommu_translate sid=250 is not known!!
qemu-system-x86_64: no buffer available in event queue to report event
qemu-system-x86_64: AHCI: Failed to start FIS receive engine: bad FIS
receive buffer address
../..
Invalid access at addr 0x7FFA6900, size 4, region '(null)', reason: rejected
But this is expected.
So feel free to add
Tested-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
> ---
> include/hw/i386/pc.h | 2 ++
> hw/i386/acpi-build.c | 5 +++++
> hw/i386/pc.c | 24 ++++++++++++++++++++++--
> hw/i386/Kconfig | 1 +
> 4 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 82cf7b7e30..f3ba1ee4c0 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;
>
> /* ACPI Memory hotplug IO base address */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d1c28440f4..4e46585709 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
> @@ -2593,6 +2594,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(machine, 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 789ccb6ef4..31710bc4fb 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -83,6 +83,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"
> @@ -1367,8 +1368,11 @@ static void pc_virtio_md_pci_unplug(HotplugHandler *hotplug_dev,
> static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> - if (object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) &&
> - x86_iommu_get_default()) {
> + PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +
> + if ((object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) ||
> + object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) &&
> + (x86_iommu_get_default() || pcms->virtio_iommu)) {
> error_setg(errp, "QEMU does not support multiple vIOMMUs "
> "for x86 yet.");
> return;
> @@ -1381,6 +1385,15 @@ 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 APIC range as the reserved MSI region */
> + char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
> + VIRTIO_IOMMU_RESV_MEM_T_MSI);
> +
> + object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp);
> + object_property_set_str(OBJECT(dev), "reserved-regions[0]",
> + resv_prop_str, errp);
> + g_free(resv_prop_str);
> }
> }
>
> @@ -1394,6 +1407,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);
> }
> }
>
> @@ -1436,6 +1455,7 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
> 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_IOMMU_PCI) ||
> object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE)) {
> return HOTPLUG_HANDLER(machine);
> }
> 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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine
2021-10-01 17:33 ` [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine Jean-Philippe Brucker
2021-10-05 10:04 ` Ani Sinha
@ 2021-10-05 19:38 ` Eric Auger
2021-10-08 15:30 ` Jean-Philippe Brucker
1 sibling, 1 reply; 45+ messages in thread
From: Eric Auger @ 2021-10-05 19:38 UTC (permalink / raw)
To: Jean-Philippe Brucker, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, qemu-arm, ani, pbonzini
Hi Jean,
On 10/1/21 7:33 PM, Jean-Philippe Brucker wrote:
> The VIOT blob contains the following:
>
> [000h 0000 4] Signature : "VIOT" [Virtual I/O Translation Table]
> [004h 0004 4] Table Length : 00000058
> [008h 0008 1] Revision : 00
> [009h 0009 1] Checksum : 66
> [00Ah 0010 6] Oem ID : "BOCHS "
> [010h 0016 8] Oem Table ID : "BXPC "
> [018h 0024 4] Oem Revision : 00000001
> [01Ch 0028 4] Asl Compiler ID : "BXPC"
> [020h 0032 4] Asl Compiler Revision : 00000001
>
> [024h 0036 2] Node count : 0002
> [026h 0038 2] Node offset : 0030
> [028h 0040 8] Reserved : 0000000000000000
>
> [030h 0048 1] Type : 03 [VirtIO-PCI IOMMU]
> [031h 0049 1] Reserved : 00
> [032h 0050 2] Length : 0010
>
> [034h 0052 2] PCI Segment : 0000
> [036h 0054 2] PCI BDF number : 0008
> [038h 0056 8] Reserved : 0000000000000000
>
> [040h 0064 1] Type : 01 [PCI Range]
> [041h 0065 1] Reserved : 00
> [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
> [052h 0082 6] Reserved : 000000000000
I noticed the spec does not clearly say the virtio-iommu-pci BDF does
not need to be excluded from the PCI range.
Shouldn't it be clarified?
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 1 -
> tests/data/acpi/virt/VIOT | Bin 0 -> 88 bytes
> 2 files changed, 1 deletion(-)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 29b5b1eabc..fa213e4738 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,4 +1,3 @@
> /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/virt/VIOT",
> "tests/data/acpi/q35/DSDT.viot",
> "tests/data/acpi/q35/VIOT.viot",
> diff --git a/tests/data/acpi/virt/VIOT b/tests/data/acpi/virt/VIOT
> index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..921f40d88c28ba2171a4d664e119914335309e7d 100644
> GIT binary patch
> literal 88
> zcmWIZ^bd((0D?3pe`k+i1*eDrX9XZ&1PX!JAexE60Hgv8m>C3sGzXN&z`)2L0cSHX
> I{D-Rq0Q5fy0RR91
>
> literal 0
> HcmV?d00001
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 09/11] tests/acpi: add test cases for VIOT
2021-10-01 17:33 ` [PATCH v4 09/11] tests/acpi: add test cases for VIOT Jean-Philippe Brucker
2021-10-05 10:27 ` Ani Sinha
@ 2021-10-05 19:40 ` Eric Auger
2021-10-06 8:14 ` Igor Mammedov
2 siblings, 0 replies; 45+ messages in thread
From: Eric Auger @ 2021-10-05 19:40 UTC (permalink / raw)
To: Jean-Philippe Brucker, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, qemu-arm, ani, pbonzini
On 10/1/21 7:33 PM, Jean-Philippe Brucker wrote:
> Add two test cases for VIOT, one on the q35 machine and the other on
> virt. To test complex topologies the q35 test has two PCIe buses that
> bypass the IOMMU (and are therefore not described by VIOT), and two
> buses that are translated by virtio-iommu.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> tests/qtest/bios-tables-test.c | 38 ++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 4f11d03055..b6cb383bd9 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1403,6 +1403,42 @@ static void test_acpi_virt_tcg(void)
> free_test_data(&data);
> }
>
> +static void test_acpi_q35_viot(void)
> +{
> + test_data data = {
> + .machine = MACHINE_Q35,
> + .variant = ".viot",
> + };
> +
> + /*
> + * To keep things interesting, two buses bypass the IOMMU.
> + * VIOT should only describes the other two buses.
> + */
> + test_acpi_one("-machine default_bus_bypass_iommu=on "
> + "-device virtio-iommu "
may be better to use virtio-iommu-pci?
> + "-device pxb-pcie,bus_nr=0x10,id=pcie.100,bus=pcie.0 "
> + "-device pxb-pcie,bus_nr=0x20,id=pcie.200,bus=pcie.0,bypass_iommu=on "
> + "-device pxb-pcie,bus_nr=0x30,id=pcie.300,bus=pcie.0",
> + &data);
> + free_test_data(&data);
> +}
> +
> +static void test_acpi_virt_viot(void)
> +{
> + test_data data = {
> + .machine = "virt",
> + .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
> + .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
> + .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
> + .ram_start = 0x40000000ULL,
> + .scan_len = 128ULL * 1024 * 1024,
> + };
> +
> + test_acpi_one("-cpu cortex-a57 "
> + "-device virtio-iommu", &data);
> + free_test_data(&data);
> +}
> +
> static void test_oem_fields(test_data *data)
> {
> int i;
> @@ -1567,12 +1603,14 @@ int main(int argc, char *argv[])
> if (strcmp(arch, "x86_64") == 0) {
> qtest_add_func("acpi/microvm/pcie", test_acpi_microvm_pcie_tcg);
> }
> + qtest_add_func("acpi/q35/viot", test_acpi_q35_viot);
> } else if (strcmp(arch, "aarch64") == 0) {
> qtest_add_func("acpi/virt", test_acpi_virt_tcg);
> qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
> qtest_add_func("acpi/virt/memhp", test_acpi_virt_tcg_memhp);
> qtest_add_func("acpi/virt/pxb", test_acpi_virt_tcg_pxb);
> qtest_add_func("acpi/virt/oem-fields", test_acpi_oem_fields_virt);
> + qtest_add_func("acpi/virt/viot", test_acpi_virt_viot);
> }
> ret = g_test_run();
> boot_sector_cleanup(disk);
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 11/11] tests/acpi: add expected blobs for VIOT test on q35 machine
2021-10-01 17:33 ` [PATCH v4 11/11] tests/acpi: add expected blobs for VIOT test on q35 machine Jean-Philippe Brucker
2021-10-05 10:07 ` Ani Sinha
@ 2021-10-05 19:41 ` Eric Auger
1 sibling, 0 replies; 45+ messages in thread
From: Eric Auger @ 2021-10-05 19:41 UTC (permalink / raw)
To: Jean-Philippe Brucker, imammedo
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, qemu-arm, ani, pbonzini
On 10/1/21 7:33 PM, Jean-Philippe Brucker wrote:
> Add expected blobs of the VIOT and DSDT table for the VIOT test on the
> q35 machine.
>
> Since the test instantiates a virtio device and two PCIe expander
> bridges, DSDT.viot has more blocks than the base DSDT (long diff not
> shown here). The VIOT table generated for the q35 test is:
>
> [000h 0000 4] Signature : "VIOT" [Virtual I/O Translation Table]
> [004h 0004 4] Table Length : 00000070
> [008h 0008 1] Revision : 00
> [009h 0009 1] Checksum : 3D
> [00Ah 0010 6] Oem ID : "BOCHS "
> [010h 0016 8] Oem Table ID : "BXPC "
> [018h 0024 4] Oem Revision : 00000001
> [01Ch 0028 4] Asl Compiler ID : "BXPC"
> [020h 0032 4] Asl Compiler Revision : 00000001
>
> [024h 0036 2] Node count : 0003
> [026h 0038 2] Node offset : 0030
> [028h 0040 8] Reserved : 0000000000000000
>
> [030h 0048 1] Type : 03 [VirtIO-PCI IOMMU]
> [031h 0049 1] Reserved : 00
> [032h 0050 2] Length : 0010
>
> [034h 0052 2] PCI Segment : 0000
> [036h 0054 2] PCI BDF number : 0010
> [038h 0056 8] Reserved : 0000000000000000
>
> [040h 0064 1] Type : 01 [PCI Range]
> [041h 0065 1] Reserved : 00
> [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 : 30FF
> [050h 0080 2] Output node : 0030
> [052h 0082 6] Reserved : 000000000000
>
> [058h 0088 1] Type : 01 [PCI Range]
> [059h 0089 1] Reserved : 00
> [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 : 10FF
> [068h 0104 2] Output node : 0030
> [06Ah 0106 6] Reserved : 000000000000
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 2 --
> tests/data/acpi/q35/DSDT.viot | Bin 0 -> 9398 bytes
> tests/data/acpi/q35/VIOT.viot | Bin 0 -> 112 bytes
> 3 files changed, 2 deletions(-)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index fa213e4738..dfb8523c8b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,3 +1 @@
> /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/q35/DSDT.viot",
> -"tests/data/acpi/q35/VIOT.viot",
> diff --git a/tests/data/acpi/q35/DSDT.viot b/tests/data/acpi/q35/DSDT.viot
> index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..b41270ff6d63493c2ae379ddd1d3e28f190a6c01 100644
> GIT binary patch
> literal 9398
> zcmeHNO>7&-8J*>iv|O&FB}G~OOGG$M|57BBoWHhc5OS9yDTx$CQgH$r;8Idr*-4Q_
> z5(9Az1F`}niVsB-#zBvCpa8wKr(7GLxfJNZhXOUwQxCo5S`_gq>icGPq#2R|qEj!C
> zfZhFO-<xmV?9RU7?0QYF^~FWTjE@VeZn>E)yj*v)_%j$|bWD4v61&3MJ6@sGF_Mv(
> z(Y~GJ$Ji9i%ul_-ddc|xw*RT`zx{!4bOW~WnR9oe8@#vYZ!iK~-v}&=4xHj-r&;K<
> zcU`OQR&r*iT=DGueakdEt~iRCoxImzW@o+PvCPVNXSM0Z?!3la@A7=V7VmARrY)yk
> z{pY1`=FY$P>E*ZcU;gqRzq<396$4-adlUOh0d4%7zBT9fosWB0jax+L=jQv<ANTla
> zQRdK@z^9UXwkV>i=J#J~?>_G}@-A=VM7>texw(0?%WX7MbJqC}W*M`obLj6+2L}g#
> z7KhBa!JMioR2I#0z1Wf}4QL}(?VWPHRb@6~_rFcDSo^j^@$^f@nwPCNyiPXrY^T}E
> zvw%wcfQq{B`j+GO?T>ms>-oupgMHSY{HWJupLA{Zum8sP*}gR;+Lp2=-%n6m?tjZ-
> zjG;9@c#>K}{oUR@TWRJyyo-^34o#_78fy{Dw`^y5>Zzy%5~{uX^m4%iSX`qhT8~!A
> zG^eeZlHoI-8Ai$2Vq4f>h#*^g_hNN*{g5>^t+7liet~+Zy}PhdZ_UfPW8!)n8rHEU
> zO2#|UccP|wVTaee;I38=IdP!Tn<F?6qYtCZyx?%o<BgVk*qUT`{dm;@3z%VEU<SIV
> zmOARAox0m>8Og6~%fzLjz(wD!XR-0J?VV<E38Ua|wuK9qq`)oYic2_As8t(A^6!F1
> zfm^7pSF`ns_j0yv6jt12mU+DH7MCLJ$0#~D2(}3k+%T>(s-yiwD&A+AC-UHoLQ!1-
> zZTt}HXS}hx*Q`$VSHhuj|GB^ZyZOw!)sJSsuAcdeTMekL*MH;pAM0IX{WHC*Rs<v9
> z7Qc^d+_nd7KNU4@(}vxf?a%bCS>r)E9$^!#8~A%&#`e2rz2YvijNQTB2(~G5e*20+
> zH;dzb%?EP5(W<AH-`YthW4JrnN^QBw#Ib?nMV7Xy+=?J5$smfL%+eDvv;!ka;KGPl
> z08WZ?oCl~3iHZ6-Ho}>}h7mC(G{QI&P|ie1Otgk$qns&Q5M{)a(5PSn%9#j>DYIZ)
> z2`sNC#+ect6HM87gsRTCrZdi&5*imw*?5Gi&M{5r7-vf8n649{s&ib^Ij-p(*L5OP
> zb()$^Q`2ecIuWWm@dQ$OI-%)I=sFRqIxS77rRlVEod{K(Nlj-`)0xzDB2;zaS*To3
> zThnRlIuWWmCp4WCn$8JbCqh-{q^5IH(>bZ@M5yYV(sWK~I;V7<2vwbqrqj`MI=W7T
> zs?L<AGo|TF={garI@6lYw5Bty>qMyPoYr(sYdWWOod{K(8BJ$K)0xqAB2;zGXgX&!
> zoin;lgsRR{n$A<2&QrQhgsM)=Byji1=g_RCb5_@hP}O-_(|KCcd0N+rP}O;cGxOn-
> z@C;`b!iU`%!E}#8VtOI=tj0X6G0*Bugevo##yqDn&*@BrD)YR?Jg+g&>r8|y^AU~t
> zh{k+GXChRYk8-ATnMXNOKI0!1O!?qONKAPJ=d_%2TFyB=Cqj|agn{N211&WxNX^aE
> zz%des28sY_MG!?Glfpm+j$4w!h$Y)+AgO>J8Yn_34F)Q((m)j`8K{6B8Yn`vMjEKV
> zh7sjd4OBo64HO|-#IZ0?feoWjBZrcK3aAWKoiI>QEoZ_&6(|`fLg|WRpa`W-7^uK<
> zCJa=8l7R{+&q)S~Q0jz%3M^;BKouw%sDN@N87M-j69y`<oCyO}pk$x|%9&)K2<7#J
> zfeI{V!ax-$8K{7ACK)I~sS^e&u$&14RiI>`0?L_Wpa`W-7^uK<CJa=8l7R{+XOe*;
> zlsaLc0?U~&Pz6c`DxjQ628vMXgn<eyXTm@gC>f}LawZulLa7r5DzKah16818paRO7
> zWS|J8P8g`bawZH^fs%m=C})y^B9uB|paRR8Fi-_b1}dPONd}5g>V$y`EN8+%6(|{~
> zfN~}oC_<?d1}d<e2?JH2WS|1dnPi{{rA`>Az;Y%GRDqI#3Mglifg+STVW0xbnJ`cV
> zN(L&RoJj_XQ0jz%3M^;BKouw%sDN@N87M-j69y`<oCyO}pk$x|%9&)K2&GOKsK9b2
> z3{-)VfeI*Rl7S+WI$@v!%b74x1xf}gpqxnticso=fg(~26p?D62vq|`s2Hfign=qd
> zGEjv{2C6V&pb8TPsxZkw6($*|!i0e;Oc<!bBm-5LWS|NY2C6V&pbC==RAG{VDohwC
> zB6)RTponnAcxRG<BE)5oAu(m&!axzpzJ-Azl6^}Cict0~87M+56`k$CLRsJL@gJ-E
> z^n>)?813@y+dqFQO21NRRfE<$jCx}&564|A(8i^WXELn4tQF~akv1jTl+Da^WeuwV
> z{lracXB&$wiG3@XdAhRZHWC0da;Zrx`QaE#@Rgt&><*P#acnEW&24Ln(GeQaz|zag
> zIOYg>x#VGExrP>lOf<WhI%Q3NILQ)GxEsUso8d`})r-s&UkBQ!CZIL>OseBmn(P)G
> zTRXgDiq@kT5N?CVvz=z6Y24Wk+d%25Jj`Ag$d1L6@0`7$WH0dS1+-7iUdW8jUTE)D
> zNxN7|!*XA!-HMzby{n{md3tvsy?ac0H%#vhh_|qy^_|swN_vl{_Xg5?$E5ed^u@vS
> zu@j^(D(Q<neQ_Xt@tE|*F#Y0S`uGXbFDmI5dHThH^oz%&UkuZi2Gft7Abm+mU*hRY
> z1L;f0q%Vc(%Y*61PmsQ>q%ZUI<$?6&W73zAK1PqS!Ss$uFCQM5R$#b`^;^9xGscHJ
> zb~LWT>IR3gmC~!jTw|y@Q8u!=p>dhktHT6js5((Lvbv#hh9zJcXHpnuB}3JTvXRvd
> zjhl>K9i}Qn)rqo^)eVh<8NE8pUxum^Wh1K_y01^^)nQUIRGlarS>4cmI;&TQna)sk
> zqD-&OId`<4y*t&c7VxSCuT|_Q*8H8-7sT6%vUTNX#lPMtHsxJvVQ|8mCfxdZw*1E1
> z!Ryz5oqhAwx8At^=H{z!v9{sXSC+%Ob;7br#sm9-ZP-}VhbPg}`2*Xs+qZv2L7rvZ
> zda-7>m0Htq8`YN#EP&@|vuLoW>sCE1vZo0db(_VNEZ|<gmBm_frB*QmMuoWRFJ0k!
> z&>+#siM(5{<Z|;lYKC}m6wjpG*5WIXzSY}i@4LBN?rNqyhSmFgqW##Fr(*Ke1_>nr
> zwER9;jAQ}x`)7NDCrCnIO<~!6=*-IUzMrYaN!G~{8G}`!hLQDW7EifNw`LwA!*pxK
> z%jw8Lz%~P8>oYIi+cE;S6&QZNx}UAszw7zN-Axqv!RBB}E_kpI7o|UYcd!(fW%KiX
> zqSv(!kmmDX)cO9!p`N4b9&GMW8@ud!7p=PYT>qIN7{4=c`8KU+Fw`+P3EuThda+oH
> zi8qT>;Rf5JOAc+D?fKE-hU!9lu5XO)=4!%SZMN)Vx_R{7GE_f0FSt6QZvrbaSW3`q
> zgN5kkk*#aL_dMOx82|Xg@W6d-?$h0k5m3#yQc25XSZ$zrXl4>GO<PG4!?v$p%bdgD
> zDb<UeVs3$jBc28l-BZqLj85@^AC9Tqvn=y7VCNZi#Niu3@y1FisJk`ZQVroO&0G-=
> zm;6lKy@aO@o+B1^g+h&X!>@b)udjGBFD773Ef%CBJAAa%#_x8mo#5TB6=55}%{RLn
> z_<g}P`%Z5YpoDnu<FPF&Fb)gyC`k1-t;k_XAuNd=mbe&^w{C1$4x{&Dds~bJH@m2N
> zZ_8l8)}Bv;W8W83fEg>jH^CxURAD*%(^bVgO08=b!miDVuEp;Au03Jb!fQr$?R?m^
> zj_6wKzVF%-cCEu`M$7No>Wr{%{+5`3Jh7CtzL2*!V_1s=xMV!EAKHeAas!hT{C$JF
> z|Dk;tPhik6_8<P$1wT)}lMfE)cQXA&nD#w}iTB!n6ld5Uv@>7gLyd36nJ-_s^8~)(
> z2x}8%^!TKg;&_s3e-zVK3vLWgOWYs0BlssFc|0w}T#3q+Ns{=IAt}Z)pCo0+vSSAS
> z^kFGhFV`ERDnTcWm~ai(^_BgVZJfq4;m22yzImyW9*Y@hJC2|3q|KNW!SjovXXp$m
> zaei@h8QQx~E?#!UM0jGYSY3y&ED#!Bcc?9P;!d2Vbxt9{AL`DMNmsGGn{4KC;v*7t
> z$xoWO=S6@kW-!dooAVySR}45<kO!COppBX6d`JW~A_|VkQC_fcgoufPBSf5K#>jWr
> F{{VUX)Oi2^
>
> literal 0
> HcmV?d00001
>
> diff --git a/tests/data/acpi/q35/VIOT.viot b/tests/data/acpi/q35/VIOT.viot
> index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..9b179266ccbf84f1c250ee646812d17e27987764 100644
> GIT binary patch
> literal 112
> zcmWIZ^baXu00LVle`k+i1*eDrX9XZ&1PX!JAex!M0Hgv8m>C3sGzdcgBZCA3T-xBj
> Q0Zb)W9Hva*zW_`e0M!8s0RR91
>
> literal 0
> HcmV?d00001
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 04/11] hw/arm/virt: Reject instantiation of multiple IOMMUs
2021-10-01 17:33 ` [PATCH v4 04/11] hw/arm/virt: Reject instantiation of multiple IOMMUs Jean-Philippe Brucker
@ 2021-10-06 6:35 ` Igor Mammedov
0 siblings, 0 replies; 45+ messages in thread
From: Igor Mammedov @ 2021-10-06 6:35 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, ani, pbonzini
On Fri, 1 Oct 2021 18:33:52 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> 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.
>
> Fixes: 70e89132c9 ("hw/arm/virt: Add the virtio-iommu device tree mappings")
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/arm/virt.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 56e8fc7059..36f0261ef4 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2441,6 +2441,11 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> hwaddr db_start = 0, db_end = 0;
> char *resv_prop_str;
>
> + if (vms->iommu != VIRT_IOMMU_NONE) {
> + error_setg(errp, "virt machine does not support multiple IOMMUs");
> + return;
> + }
> +
> switch (vms->msi_controller) {
> case VIRT_MSI_CTRL_NONE:
> return;
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 05/11] hw/arm/virt: Use object_property_set instead of qdev_prop_set
2021-10-01 17:33 ` [PATCH v4 05/11] hw/arm/virt: Use object_property_set instead of qdev_prop_set Jean-Philippe Brucker
2021-10-05 9:27 ` Eric Auger
@ 2021-10-06 6:36 ` Igor Mammedov
1 sibling, 0 replies; 45+ messages in thread
From: Igor Mammedov @ 2021-10-06 6:36 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, ani, pbonzini
On Fri, 1 Oct 2021 18:33:53 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> To propagate errors to the caller of the pre_plug callback, use the
> object_poperty_set*() functions directly instead of the qdev_prop_set*()
> helpers.
>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/arm/virt.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 36f0261ef4..ac307b6030 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2465,8 +2465,9 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> db_start, db_end,
> 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);
> + object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp);
> + object_property_set_str(OBJECT(dev), "reserved-regions[0]",
> + resv_prop_str, errp);
> g_free(resv_prop_str);
> }
> }
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device
2021-10-01 17:33 ` [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device Jean-Philippe Brucker
2021-10-05 19:18 ` Eric Auger
@ 2021-10-06 7:19 ` Igor Mammedov
2021-10-08 15:24 ` Jean-Philippe Brucker
2021-10-08 10:46 ` Michael S. Tsirkin
2 siblings, 1 reply; 45+ messages in thread
From: Igor Mammedov @ 2021-10-06 7:19 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, ani, pbonzini
On Fri, 1 Oct 2021 18:33:55 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> Allow instantiating a virtio-iommu device by adding an ACPI Virtual I/O
> Translation table (VIOT), which describes the relation between the
> virtio-iommu and the endpoints it manages.
>
> 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: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> include/hw/i386/pc.h | 2 ++
> hw/i386/acpi-build.c | 5 +++++
> hw/i386/pc.c | 24 ++++++++++++++++++++++--
> hw/i386/Kconfig | 1 +
> 4 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 82cf7b7e30..f3ba1ee4c0 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;
>
> /* ACPI Memory hotplug IO base address */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d1c28440f4..4e46585709 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
> @@ -2593,6 +2594,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(machine, tables_blob, tables->linker, pcms->virtio_iommu_bdf,
I'd drop PCMachineState::virtio_iommu_bdf and s/pcms->virtio_iommu_bdf/pci_get_bdf(iommu)/
> + 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 789ccb6ef4..31710bc4fb 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -83,6 +83,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"
> @@ -1367,8 +1368,11 @@ static void pc_virtio_md_pci_unplug(HotplugHandler *hotplug_dev,
> static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> - if (object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) &&
> - x86_iommu_get_default()) {
> + PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +
> + if ((object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) ||
> + object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) &&
> + (x86_iommu_get_default() || pcms->virtio_iommu)) {
this check is getting uglier,
may be instead of introducing pcms->virtio_iommu boolean, better approach
would be adding 'Device* PCMachineState::iommu' and setting it to IOMMU
so the check would reduce to:
if ((object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) ||
object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)))
{
if (pcms->iommu)
err
else set pcms->iommu in plug handler or here
}
that also will let to cleanup/get rid of x86_iommu_[s|g]et_default()
and x86_iommu_default 'global'.
Maybe replace previous patch with one that would remove
x86_iommu_[s|g]et_default().
> error_setg(errp, "QEMU does not support multiple vIOMMUs "
> "for x86 yet.");
> return;
> @@ -1381,6 +1385,15 @@ 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 APIC range as the reserved MSI region */
> + char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
> + VIRTIO_IOMMU_RESV_MEM_T_MSI);
> +
> + object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp);
> + object_property_set_str(OBJECT(dev), "reserved-regions[0]",
> + resv_prop_str, errp);
> + g_free(resv_prop_str);
> }
> }
>
> @@ -1394,6 +1407,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);
> }
> }
>
> @@ -1436,6 +1455,7 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
> 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_IOMMU_PCI) ||
> object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE)) {
> return HOTPLUG_HANDLER(machine);
> }
> 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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 01/11] hw/acpi: Add VIOT table
2021-10-01 17:33 ` [PATCH v4 01/11] hw/acpi: Add VIOT table Jean-Philippe Brucker
@ 2021-10-06 8:09 ` Igor Mammedov
2021-10-08 15:20 ` Jean-Philippe Brucker
0 siblings, 1 reply; 45+ messages in thread
From: Igor Mammedov @ 2021-10-06 8:09 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, Cornelia Huck, ehabkost, shannon.zhaosl, mst,
richard.henderson, qemu-devel, eric.auger, qemu-arm, ani,
pbonzini
On Fri, 1 Oct 2021 18:33:49 +0100
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
perhaps
s/when instantiating ... ./if a virtio-iommu device present/
> 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.
modulo comments, patch looks fine to me from ACPI point of view.
but I don't know if values used for describing PCI structures
make any sense so this might need an ACK from a person who knows
PCI innards better.
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> hw/acpi/viot.h | 13 +++++
> hw/acpi/viot.c | 112 ++++++++++++++++++++++++++++++++++++++++++++
> hw/acpi/Kconfig | 4 ++
> 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/viot.h b/hw/acpi/viot.h
> new file mode 100644
> index 0000000000..9fe565bb87
> --- /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(MachineState *ms, 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..e33d468e11
> --- /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 build_pci_range_node(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 (PCI range) */
see [1] below
> + build_append_int_noprefix(blob, 1, 1);
> + /* Reserved */
> + build_append_int_noprefix(blob, 0, 1);
> + /* Length */
> + build_append_int_noprefix(blob, 24, 2);
spec should be fixed to state length value for fixed length structures
like it's done in ACPI specs, I who we should poke at to make this happen.
zzzz
> + /* 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);
zzzz
see comment [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.
> + */
this comment needs to state spec name/version, otherwise it's not clear
what code below is based on (example: build_dmar_q35).
Also since there is no final spec yet and spec doesn't have permanent
hosting place (i.e. hosted by one of specs org), I'd consider
link in cover letter 'dead' and not suitable for long term use.
So we should shovel spec docs/specs and point to it in this comment
> +void build_viot(MachineState *ms, GArray *table_data, BIOSLinker *linker,
> + uint16_t virtio_iommu_bdf, const char *oem_id,
> + const char *oem_table_id)
> +{
> + /* The virtio-iommu node follows the 48-bytes header */
> + int viommu_off = 48;
> + AcpiTable table = { .sig = "VIOT", .rev = 0,
> + .oem_id = oem_id, .oem_table_id = oem_table_id };
> + struct viot_pci_ranges pci_ranges = {
> + .output_node = viommu_off,
> + .blob = g_array_new(false, true /* clear */, 1),
> + };
> +
> + /* Build the list of PCI ranges that this viommu manages */
> + object_child_foreach_recursive(OBJECT(ms), build_pci_range_node,
> + &pci_ranges);
> +
> + /* ACPI table header */
> + acpi_table_begin(&table, table_data);
> + /* 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 node */
> + /* Type (virtio-pci IOMMU) */
(1)
/* Type */
> + build_append_int_noprefix(table_data, 3, 1);
s:3,:3 /* virtio-pci IOMMU */,:
check-patch will spit out warning but that kind comment
is common practice with ACPI code
> + /* Reserved */
> + build_append_int_noprefix(table_data, 0, 1);
> + /* Length */
> + build_append_int_noprefix(table_data, 16, 2);
> + /* PCI Segment */
> + build_append_int_noprefix(table_data, 0, 2);
(2)
can we fetch _SEG value from device instead of hard-codding value here?
I might be obvious to PCI folks,
but it would be better have at least a comment explaining
where these values come from
Michael,
what do you think?
> + /* 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);
> +
> + acpi_table_end(linker, &table);
> +}
> +
> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> index 3b5e118c54..622b0b50b7 100644
> --- a/hw/acpi/Kconfig
> +++ b/hw/acpi/Kconfig
> @@ -51,6 +51,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 7d8c0eb43e..adf6347bc4 100644
> --- a/hw/acpi/meson.build
> +++ b/hw/acpi/meson.build
> @@ -20,6 +20,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files(
> 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_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'))
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 08/11] tests/acpi: allow updates of VIOT expected data files
2021-10-01 17:33 ` [PATCH v4 08/11] tests/acpi: allow updates of VIOT expected data files Jean-Philippe Brucker
@ 2021-10-06 8:12 ` Igor Mammedov
2021-10-08 15:26 ` Jean-Philippe Brucker
0 siblings, 1 reply; 45+ messages in thread
From: Igor Mammedov @ 2021-10-06 8:12 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, ani, pbonzini
On Fri, 1 Oct 2021 18:33:56 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> Create empty data files and allow updates for the upcoming VIOT tests.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
> tests/data/acpi/q35/DSDT.viot | 0
does default tests/data/acpi/q35/DSDT differs from
DSDT.viot?
> tests/data/acpi/q35/VIOT.viot | 0
> tests/data/acpi/virt/VIOT | 0
> 4 files changed, 3 insertions(+)
> create mode 100644 tests/data/acpi/q35/DSDT.viot
> create mode 100644 tests/data/acpi/q35/VIOT.viot
> create mode 100644 tests/data/acpi/virt/VIOT
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..29b5b1eabc 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,4 @@
> /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/virt/VIOT",
> +"tests/data/acpi/q35/DSDT.viot",
> +"tests/data/acpi/q35/VIOT.viot",
> diff --git a/tests/data/acpi/q35/DSDT.viot b/tests/data/acpi/q35/DSDT.viot
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/data/acpi/q35/VIOT.viot b/tests/data/acpi/q35/VIOT.viot
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/data/acpi/virt/VIOT b/tests/data/acpi/virt/VIOT
> new file mode 100644
> index 0000000000..e69de29bb2
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 09/11] tests/acpi: add test cases for VIOT
2021-10-01 17:33 ` [PATCH v4 09/11] tests/acpi: add test cases for VIOT Jean-Philippe Brucker
2021-10-05 10:27 ` Ani Sinha
2021-10-05 19:40 ` Eric Auger
@ 2021-10-06 8:14 ` Igor Mammedov
2 siblings, 0 replies; 45+ messages in thread
From: Igor Mammedov @ 2021-10-06 8:14 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, ani, pbonzini
On Fri, 1 Oct 2021 18:33:57 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> Add two test cases for VIOT, one on the q35 machine and the other on
> virt. To test complex topologies the q35 test has two PCIe buses that
> bypass the IOMMU (and are therefore not described by VIOT), and two
> buses that are translated by virtio-iommu.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> tests/qtest/bios-tables-test.c | 38 ++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 4f11d03055..b6cb383bd9 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1403,6 +1403,42 @@ static void test_acpi_virt_tcg(void)
> free_test_data(&data);
> }
>
> +static void test_acpi_q35_viot(void)
> +{
> + test_data data = {
> + .machine = MACHINE_Q35,
> + .variant = ".viot",
> + };
> +
> + /*
> + * To keep things interesting, two buses bypass the IOMMU.
> + * VIOT should only describes the other two buses.
> + */
> + test_acpi_one("-machine default_bus_bypass_iommu=on "
> + "-device virtio-iommu "
> + "-device pxb-pcie,bus_nr=0x10,id=pcie.100,bus=pcie.0 "
> + "-device pxb-pcie,bus_nr=0x20,id=pcie.200,bus=pcie.0,bypass_iommu=on "
> + "-device pxb-pcie,bus_nr=0x30,id=pcie.300,bus=pcie.0",
> + &data);
> + free_test_data(&data);
> +}
> +
> +static void test_acpi_virt_viot(void)
> +{
> + test_data data = {
> + .machine = "virt",
> + .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
> + .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
> + .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
> + .ram_start = 0x40000000ULL,
> + .scan_len = 128ULL * 1024 * 1024,
> + };
> +
> + test_acpi_one("-cpu cortex-a57 "
> + "-device virtio-iommu", &data);
> + free_test_data(&data);
> +}
> +
> static void test_oem_fields(test_data *data)
> {
> int i;
> @@ -1567,12 +1603,14 @@ int main(int argc, char *argv[])
> if (strcmp(arch, "x86_64") == 0) {
> qtest_add_func("acpi/microvm/pcie", test_acpi_microvm_pcie_tcg);
> }
> + qtest_add_func("acpi/q35/viot", test_acpi_q35_viot);
> } else if (strcmp(arch, "aarch64") == 0) {
> qtest_add_func("acpi/virt", test_acpi_virt_tcg);
> qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
> qtest_add_func("acpi/virt/memhp", test_acpi_virt_tcg_memhp);
> qtest_add_func("acpi/virt/pxb", test_acpi_virt_tcg_pxb);
> qtest_add_func("acpi/virt/oem-fields", test_acpi_oem_fields_virt);
> + qtest_add_func("acpi/virt/viot", test_acpi_virt_viot);
> }
> ret = g_test_run();
> boot_sector_cleanup(disk);
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device
2021-10-01 17:33 ` [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device Jean-Philippe Brucker
2021-10-05 19:18 ` Eric Auger
2021-10-06 7:19 ` Igor Mammedov
@ 2021-10-08 10:46 ` Michael S. Tsirkin
2 siblings, 0 replies; 45+ messages in thread
From: Michael S. Tsirkin @ 2021-10-08 10:46 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, richard.henderson,
qemu-devel, eric.auger, qemu-arm, imammedo, ani, pbonzini
On Fri, Oct 01, 2021 at 06:33:55PM +0100, Jean-Philippe Brucker wrote:
> Allow instantiating a virtio-iommu device by adding an ACPI Virtual I/O
> Translation table (VIOT), which describes the relation between the
> virtio-iommu and the endpoints it manages.
>
> 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: Jean-Philippe Brucker <jean-philippe@linaro.org>
If you like, we can start by merging the pc bits when they are ready.
These are not widely used so have less of a chance to break someone's
setup.
> ---
> include/hw/i386/pc.h | 2 ++
> hw/i386/acpi-build.c | 5 +++++
> hw/i386/pc.c | 24 ++++++++++++++++++++++--
> hw/i386/Kconfig | 1 +
> 4 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 82cf7b7e30..f3ba1ee4c0 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;
>
> /* ACPI Memory hotplug IO base address */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d1c28440f4..4e46585709 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
> @@ -2593,6 +2594,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(machine, 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 789ccb6ef4..31710bc4fb 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -83,6 +83,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"
> @@ -1367,8 +1368,11 @@ static void pc_virtio_md_pci_unplug(HotplugHandler *hotplug_dev,
> static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> - if (object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) &&
> - x86_iommu_get_default()) {
> + PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +
> + if ((object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) ||
> + object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) &&
> + (x86_iommu_get_default() || pcms->virtio_iommu)) {
> error_setg(errp, "QEMU does not support multiple vIOMMUs "
> "for x86 yet.");
> return;
> @@ -1381,6 +1385,15 @@ 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 APIC range as the reserved MSI region */
> + char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d",
> + VIRTIO_IOMMU_RESV_MEM_T_MSI);
> +
> + object_property_set_uint(OBJECT(dev), "len-reserved-regions", 1, errp);
> + object_property_set_str(OBJECT(dev), "reserved-regions[0]",
> + resv_prop_str, errp);
> + g_free(resv_prop_str);
> }
> }
>
> @@ -1394,6 +1407,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);
> }
> }
>
> @@ -1436,6 +1455,7 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
> 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_IOMMU_PCI) ||
> object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE)) {
> return HOTPLUG_HANDLER(machine);
> }
> 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.33.0
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/11] virtio-iommu: Add ACPI support
2021-10-05 15:45 ` [PATCH v4 00/11] virtio-iommu: Add ACPI support Michael S. Tsirkin
@ 2021-10-08 15:17 ` Jean-Philippe Brucker
2021-10-11 10:10 ` Haiwei Li
2021-10-18 15:25 ` Michael S. Tsirkin
0 siblings, 2 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-08 15:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: peter.maydell, ehabkost, shannon.zhaosl, richard.henderson,
qemu-devel, eric.auger, qemu-arm, imammedo, ani, pbonzini
On Tue, Oct 05, 2021 at 11:45:42AM -0400, Michael S. Tsirkin wrote:
> Looks like this can not be applied yet because the bypass bit
> isn't in yet. what's up with that?
The boot-bypass bit isn't a hard dependency for this series, but it will
be needed for full support eventually. It will be delayed by spec and
Linux header changes
In the meantime we can work around the problem by having the boot disks
bypass the IOMMU (virtio without iommu-platform, or iommu-bypass bus).
Thanks,
Jean
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 01/11] hw/acpi: Add VIOT table
2021-10-06 8:09 ` Igor Mammedov
@ 2021-10-08 15:20 ` Jean-Philippe Brucker
0 siblings, 0 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-08 15:20 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, Cornelia Huck, ehabkost, shannon.zhaosl, mst,
richard.henderson, qemu-devel, eric.auger, qemu-arm, ani,
pbonzini
On Wed, Oct 06, 2021 at 10:09:50AM +0200, Igor Mammedov wrote:
> On Fri, 1 Oct 2021 18:33:49 +0100
> 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
>
> perhaps
> s/when instantiating ... ./if a virtio-iommu device present/
>
> > 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.
>
>
> modulo comments, patch looks fine to me from ACPI point of view.
>
> but I don't know if values used for describing PCI structures
> make any sense so this might need an ACK from a person who knows
> PCI innards better.
For what it's worth I mainly looked at other similar tables (IORT, DMAR
and IVRS) to figure out what values I should use
[...]
> > +static int build_pci_range_node(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 (PCI range) */
> see [1] below
>
> > + build_append_int_noprefix(blob, 1, 1);
> > + /* Reserved */
> > + build_append_int_noprefix(blob, 0, 1);
> > + /* Length */
> > + build_append_int_noprefix(blob, 24, 2);
>
> spec should be fixed to state length value for fixed length structures
> like it's done in ACPI specs, I who we should poke at to make this happen.
That doesn't seem to be applied rigorously. Several fixed-size structures
don't state their sizes, for example "5.2.25.7 NVDIMM Block Data Window
Region Structure", "5.2.25.9 Platform Capabilities Structure", "5.2.26.1.1
ACPI_NAMESPACE_DEVICE based Secure Device Structure".
>
> zzzz
> > + /* 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);
> zzzz
> see comment [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.
> > + */
>
> this comment needs to state spec name/version, otherwise it's not clear
> what code below is based on (example: build_dmar_q35).
>
> Also since there is no final spec yet and spec doesn't have permanent
> hosting place (i.e. hosted by one of specs org), I'd consider
> link in cover letter 'dead' and not suitable for long term use.
Yes, I'll throw those documents out once the final spec is out
> So we should shovel spec docs/specs and point to it in this comment
I could write "Defined in the ACPI Specification (Version TBD)"
For all I know it could be version 6.5 or 7.0...
>
> > +void build_viot(MachineState *ms, GArray *table_data, BIOSLinker *linker,
> > + uint16_t virtio_iommu_bdf, const char *oem_id,
> > + const char *oem_table_id)
> > +{
> > + /* The virtio-iommu node follows the 48-bytes header */
> > + int viommu_off = 48;
> > + AcpiTable table = { .sig = "VIOT", .rev = 0,
> > + .oem_id = oem_id, .oem_table_id = oem_table_id };
> > + struct viot_pci_ranges pci_ranges = {
> > + .output_node = viommu_off,
> > + .blob = g_array_new(false, true /* clear */, 1),
> > + };
> > +
> > + /* Build the list of PCI ranges that this viommu manages */
> > + object_child_foreach_recursive(OBJECT(ms), build_pci_range_node,
> > + &pci_ranges);
> > +
> > + /* ACPI table header */
> > + acpi_table_begin(&table, table_data);
> > + /* 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 node */
> > + /* Type (virtio-pci IOMMU) */
>
> (1)
> /* Type */
> > + build_append_int_noprefix(table_data, 3, 1);
> s:3,:3 /* virtio-pci IOMMU */,:
>
> check-patch will spit out warning but that kind comment
> is common practice with ACPI code
>
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0, 1);
> > + /* Length */
> > + build_append_int_noprefix(table_data, 16, 2);
> > + /* PCI Segment */
> > + build_append_int_noprefix(table_data, 0, 2);
> (2)
> can we fetch _SEG value from device instead of hard-codding value here?
Looking for "segment" and "domain" I couldn't find any dynamic segment
number, 0 seems to be hardcoded everywhere (hw/acpi/pci.c,
hw/i386/acpi-build.c, hw/arm/virt.c, hw/arm/virt-acpi-build.c).
>
> I might be obvious to PCI folks,
> but it would be better have at least a comment explaining
> where these values come from
I could add that "QEMU only implements segment 0"
Thanks,
Jean
>
> Michael,
> what do you think?
>
> > + /* 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);
> > +
> > + acpi_table_end(linker, &table);
> > +}
> > +
> > diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> > index 3b5e118c54..622b0b50b7 100644
> > --- a/hw/acpi/Kconfig
> > +++ b/hw/acpi/Kconfig
> > @@ -51,6 +51,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 7d8c0eb43e..adf6347bc4 100644
> > --- a/hw/acpi/meson.build
> > +++ b/hw/acpi/meson.build
> > @@ -20,6 +20,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files(
> > 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_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'))
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 03/11] hw/arm/virt: Remove device tree restriction for virtio-iommu
2021-10-05 11:57 ` Eric Auger
@ 2021-10-08 15:20 ` Jean-Philippe Brucker
0 siblings, 0 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-08 15:20 UTC (permalink / raw)
To: Eric Auger
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, qemu-arm, pbonzini, ani, imammedo
On Tue, Oct 05, 2021 at 01:57:35PM +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);
> actually this does not work. To add a hint you need the *errp to be set.
> Otherwise when running through this path you will get
>
> emu-system-x86_64: ../util/error.c:158: error_append_hint: Assertion
> `err && errp != &error_abort && errp != &error_fatal' failed.
>
> replace the error_append_hint with an error_setg (without the \n)
Woops sorry, will fix
Thanks,
Jean
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device
2021-10-06 7:19 ` Igor Mammedov
@ 2021-10-08 15:24 ` Jean-Philippe Brucker
0 siblings, 0 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-08 15:24 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, ani, pbonzini
On Wed, Oct 06, 2021 at 09:19:54AM +0200, Igor Mammedov wrote:
> > @@ -1367,8 +1368,11 @@ static void pc_virtio_md_pci_unplug(HotplugHandler *hotplug_dev,
> > static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > DeviceState *dev, Error **errp)
> > {
> > - if (object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) &&
> > - x86_iommu_get_default()) {
> > + PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > +
> > + if ((object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) ||
> > + object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) &&
> > + (x86_iommu_get_default() || pcms->virtio_iommu)) {
>
> this check is getting uglier,
> may be instead of introducing pcms->virtio_iommu boolean, better approach
> would be adding 'Device* PCMachineState::iommu' and setting it to IOMMU
> so the check would reduce to:
> if ((object_dynamic_cast(OBJECT(dev), TYPE_X86_IOMMU_DEVICE) ||
> object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)))
> {
> if (pcms->iommu)
> err
> else set pcms->iommu in plug handler or here
> }
>
> that also will let to cleanup/get rid of x86_iommu_[s|g]et_default()
> and x86_iommu_default 'global'.
> Maybe replace previous patch with one that would remove
> x86_iommu_[s|g]et_default().
Ok, I can't figure out a nice way to do this at the moment, will think
more about it. Callers of x86_iommu_get_default() still need a helper, and
most but not all assume that the returned object is for IRQ remapping. At
least adding Device *iommu to pcms should be nicer, but not sure about
removing the x86_iommu_get/set_default helpers.
Thanks,
Jean
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 08/11] tests/acpi: allow updates of VIOT expected data files
2021-10-06 8:12 ` Igor Mammedov
@ 2021-10-08 15:26 ` Jean-Philippe Brucker
2021-10-11 15:55 ` Igor Mammedov
0 siblings, 1 reply; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-08 15:26 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, ani, pbonzini
On Wed, Oct 06, 2021 at 10:12:15AM +0200, Igor Mammedov wrote:
> On Fri, 1 Oct 2021 18:33:56 +0100
> Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
>
> > Create empty data files and allow updates for the upcoming VIOT tests.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> > tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
> > tests/data/acpi/q35/DSDT.viot | 0
>
> does default tests/data/acpi/q35/DSDT differs from
> DSDT.viot?
Yes the VIOT test has one more PCI device (virtio-iommu) and PXB devices,
so there are additional descriptors in the DSDT
Thanks,
Jean
>
> > tests/data/acpi/q35/VIOT.viot | 0
> > tests/data/acpi/virt/VIOT | 0
> > 4 files changed, 3 insertions(+)
> > create mode 100644 tests/data/acpi/q35/DSDT.viot
> > create mode 100644 tests/data/acpi/q35/VIOT.viot
> > create mode 100644 tests/data/acpi/virt/VIOT
> >
> > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> > index dfb8523c8b..29b5b1eabc 100644
> > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > @@ -1 +1,4 @@
> > /* List of comma-separated changed AML files to ignore */
> > +"tests/data/acpi/virt/VIOT",
> > +"tests/data/acpi/q35/DSDT.viot",
> > +"tests/data/acpi/q35/VIOT.viot",
> > diff --git a/tests/data/acpi/q35/DSDT.viot b/tests/data/acpi/q35/DSDT.viot
> > new file mode 100644
> > index 0000000000..e69de29bb2
> > diff --git a/tests/data/acpi/q35/VIOT.viot b/tests/data/acpi/q35/VIOT.viot
> > new file mode 100644
> > index 0000000000..e69de29bb2
> > diff --git a/tests/data/acpi/virt/VIOT b/tests/data/acpi/virt/VIOT
> > new file mode 100644
> > index 0000000000..e69de29bb2
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 09/11] tests/acpi: add test cases for VIOT
2021-10-05 10:27 ` Ani Sinha
@ 2021-10-08 15:27 ` Jean-Philippe Brucker
0 siblings, 0 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-08 15:27 UTC (permalink / raw)
To: Ani Sinha
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, pbonzini, imammedo
On Tue, Oct 05, 2021 at 03:57:17PM +0530, Ani Sinha wrote:
>
>
> On Fri, 1 Oct 2021, Jean-Philippe Brucker wrote:
>
> > Add two test cases for VIOT, one on the q35 machine and the other on
> > virt. To test complex topologies the q35 test has two PCIe buses that
> > bypass the IOMMU (and are therefore not described by VIOT), and two
> > buses that are translated by virtio-iommu.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>
> This might be a stupid question but what about virtio-mmio and single mmio
> cases? I see none of your tables has nodes for those and here too you do
> not add test cases for it.
No it's a good question, there is no support for either at the moment.
virtio-mmio based virtio-iommu is relatively easy to implement, just
requires a little more machine support. QEMU doesn't support putting MMIO
endpoints behind an IOMMU at the moment, and implementing that is more
complicated.
To test the Linux VIOT driver I did both prototypes, but I don't have a
compelling reason or time to upstream them at the moment
https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/acpi-2021-06-02
Thanks,
Jean
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine
2021-10-05 19:38 ` Eric Auger
@ 2021-10-08 15:30 ` Jean-Philippe Brucker
0 siblings, 0 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-08 15:30 UTC (permalink / raw)
To: Eric Auger
Cc: peter.maydell, ehabkost, mst, richard.henderson, qemu-devel,
shannon.zhaosl, qemu-arm, pbonzini, ani, imammedo
On Tue, Oct 05, 2021 at 09:38:13PM +0200, Eric Auger wrote:
> Hi Jean,
>
> On 10/1/21 7:33 PM, Jean-Philippe Brucker wrote:
> > The VIOT blob contains the following:
> >
> > [000h 0000 4] Signature : "VIOT" [Virtual I/O Translation Table]
> > [004h 0004 4] Table Length : 00000058
> > [008h 0008 1] Revision : 00
> > [009h 0009 1] Checksum : 66
> > [00Ah 0010 6] Oem ID : "BOCHS "
> > [010h 0016 8] Oem Table ID : "BXPC "
> > [018h 0024 4] Oem Revision : 00000001
> > [01Ch 0028 4] Asl Compiler ID : "BXPC"
> > [020h 0032 4] Asl Compiler Revision : 00000001
> >
> > [024h 0036 2] Node count : 0002
> > [026h 0038 2] Node offset : 0030
> > [028h 0040 8] Reserved : 0000000000000000
> >
> > [030h 0048 1] Type : 03 [VirtIO-PCI IOMMU]
> > [031h 0049 1] Reserved : 00
> > [032h 0050 2] Length : 0010
> >
> > [034h 0052 2] PCI Segment : 0000
> > [036h 0054 2] PCI BDF number : 0008
> > [038h 0056 8] Reserved : 0000000000000000
> >
> > [040h 0064 1] Type : 01 [PCI Range]
> > [041h 0065 1] Reserved : 00
> > [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
> > [052h 0082 6] Reserved : 000000000000
> I noticed the spec does not clearly say the virtio-iommu-pci BDF does
> not need to be excluded from the PCI range.
> Shouldn't it be clarified?
Possibly, but I didn't want to complicate things. As the spec doesn't
specify any exception the driver should handle it
>
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks!
Jean
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine
2021-10-05 10:04 ` Ani Sinha
@ 2021-10-08 15:33 ` Jean-Philippe Brucker
0 siblings, 0 replies; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-08 15:33 UTC (permalink / raw)
To: Ani Sinha
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, pbonzini, imammedo
On Tue, Oct 05, 2021 at 03:34:57PM +0530, Ani Sinha wrote:
>
>
> On Fri, 1 Oct 2021, Jean-Philippe Brucker wrote:
>
> > The VIOT blob contains the following:
> >
> > [000h 0000 4] Signature : "VIOT" [Virtual I/O Translation Table]
> > [004h 0004 4] Table Length : 00000058
> > [008h 0008 1] Revision : 00
> > [009h 0009 1] Checksum : 66
> > [00Ah 0010 6] Oem ID : "BOCHS "
> > [010h 0016 8] Oem Table ID : "BXPC "
> > [018h 0024 4] Oem Revision : 00000001
> > [01Ch 0028 4] Asl Compiler ID : "BXPC"
> > [020h 0032 4] Asl Compiler Revision : 00000001
> >
> > [024h 0036 2] Node count : 0002
> > [026h 0038 2] Node offset : 0030
> > [028h 0040 8] Reserved : 0000000000000000
> >
> > [030h 0048 1] Type : 03 [VirtIO-PCI IOMMU]
> > [031h 0049 1] Reserved : 00
> > [032h 0050 2] Length : 0010
> >
> > [034h 0052 2] PCI Segment : 0000
> > [036h 0054 2] PCI BDF number : 0008
> > [038h 0056 8] Reserved : 0000000000000000
> >
> > [040h 0064 1] Type : 01 [PCI Range]
> > [041h 0065 1] Reserved : 00
> > [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
> > [052h 0082 6] Reserved : 000000000000
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>
> Acked-by: Ani Sinha <ani@anisinha.ca>
>
> Without looking at the other patches, the disassembly looks good (with
> latest iasl from upstream git).
> One suggestion : maybe also add the raw table data as well of length 88.
Hmm, the raw VIOT table is included at the end of the patch (encoded by
git in base85). Or did you mean any other data?
Thanks,
Jean
>
>
> > ---
> > tests/qtest/bios-tables-test-allowed-diff.h | 1 -
> > tests/data/acpi/virt/VIOT | Bin 0 -> 88 bytes
> > 2 files changed, 1 deletion(-)
> >
> > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> > index 29b5b1eabc..fa213e4738 100644
> > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > @@ -1,4 +1,3 @@
> > /* List of comma-separated changed AML files to ignore */
> > -"tests/data/acpi/virt/VIOT",
> > "tests/data/acpi/q35/DSDT.viot",
> > "tests/data/acpi/q35/VIOT.viot",
> > diff --git a/tests/data/acpi/virt/VIOT b/tests/data/acpi/virt/VIOT
> > index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..921f40d88c28ba2171a4d664e119914335309e7d 100644
> > GIT binary patch
> > literal 88
> > zcmWIZ^bd((0D?3pe`k+i1*eDrX9XZ&1PX!JAexE60Hgv8m>C3sGzXN&z`)2L0cSHX
> > I{D-Rq0Q5fy0RR91
> >
> > literal 0
> > HcmV?d00001
> >
> > --
> > 2.33.0
> >
> >
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/11] virtio-iommu: Add ACPI support
2021-10-08 15:17 ` Jean-Philippe Brucker
@ 2021-10-11 10:10 ` Haiwei Li
2021-10-11 17:34 ` Jean-Philippe Brucker
2021-10-18 15:25 ` Michael S. Tsirkin
1 sibling, 1 reply; 45+ messages in thread
From: Haiwei Li @ 2021-10-11 10:10 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, Michael S. Tsirkin, richard.henderson,
qemu-devel, shannon.zhaosl, qemu-arm, eric.auger, Paolo Bonzini,
ani, imammedo
On Fri, Oct 8, 2021 at 11:18 PM Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> On Tue, Oct 05, 2021 at 11:45:42AM -0400, Michael S. Tsirkin wrote:
> > Looks like this can not be applied yet because the bypass bit
> > isn't in yet. what's up with that?
>
> The boot-bypass bit isn't a hard dependency for this series, but it will
> be needed for full support eventually. It will be delayed by spec and
> Linux header changes
>
> In the meantime we can work around the problem by having the boot disks
> bypass the IOMMU (virtio without iommu-platform, or iommu-bypass bus).
Hi Jean,
I have tested on x86 platform with environment:
qemu, your personal repo: http://jpbrucker.net/git/qemu, branch:
virtio-iommu/acpi-2021-10-01
linux kernel, 5.14
qemu command, with '-device virtio-iommu' and boot disk
'virtio-blk-pci,iommu_platform=off'
But the guest boot failed. The print is as follows:
Begin: Loading essential drivers ... done.
Begin: Running /scripts/init-premount ... done.
Begin: Mounting root file system ... Begin: Running /scripts/local-top ... done.
Begin: Running /scripts/local-premount ... [ 7.490238] Btrfs
loaded, crc32c=crc32c-generic, zoned=no
Scanning for Btrfs filesystems
done.
[ 10.593238] _warn_unseeded_randomness: 300 callbacks suppressed
[ 10.593243] random: get_random_u32 called from
bucket_table_alloc.isra.0+0x123/0x430 with crng_init=0
[ 37.534015] random: get_random_u64 called from
dup_task_struct+0x415/0xa10 with crng_init=0
[ 37.539950] random: get_random_u64 called from
arch_pick_mmap_layout+0x411/0x5e0 with crng_init=0
[ 37.539973] random: get_random_u64 called from
arch_pick_mmap_layout+0x381/0x5e0 with crng_init=0
Begin: Waiting for root file system ... [ 38.624374]
_warn_unseeded_randomness: 38 callbacks suppressed
[ 38.624389] random: get_random_u64 called from
dup_task_struct+0x415/0xa10 with crng_init=0
Begin: Running /scripts/local-block ... [ 38.630760] random:
get_random_u64 called from arch_pick_mmap_layout+0x410
[ 38.630767] random: get_random_u64 called from
arch_pick_mmap_layout+0x381/0x5e0 with crng_init=0
mdadm: No arrays found in config file or automatically
done.
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
[ 39.633914] _warn_unseeded_randomness: 1278 callbacks suppressed
[ 39.633918] random: get_random_u64 called from
dup_task_struct+0x415/0xa10 with crng_init=0
[ 39.634867] random: get_random_u64 called from
arch_pick_mmap_layout+0x411/0x5e0 with crng_init=0
[ 39.634875] random: get_random_u64 called from
arch_pick_mmap_layout+0x381/0x5e0 with crng_init=0
mdadm: error opening /dev/md?*: No such file or directory
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
mdadm: No arrays found in config file or automatically
done.
Gave up waiting for root file system device. Common problems:
- Boot args (cat /proc/cmdline)
- Check rootdelay= (did the system wait long enough?)
- Missing modules (cat /proc/modules; ls /dev)
ALERT! UUID=3caf26b5-4d08-43e0-8634-7573269c4f70 does not exist.
Dropping to a shell!
Any suggestions? Thanks.
--
Haiwei
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 08/11] tests/acpi: allow updates of VIOT expected data files
2021-10-08 15:26 ` Jean-Philippe Brucker
@ 2021-10-11 15:55 ` Igor Mammedov
0 siblings, 0 replies; 45+ messages in thread
From: Igor Mammedov @ 2021-10-11 15:55 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, mst, richard.henderson,
qemu-devel, eric.auger, qemu-arm, ani, pbonzini
On Fri, 8 Oct 2021 16:26:22 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> On Wed, Oct 06, 2021 at 10:12:15AM +0200, Igor Mammedov wrote:
> > On Fri, 1 Oct 2021 18:33:56 +0100
> > Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> >
> > > Create empty data files and allow updates for the upcoming VIOT tests.
> > >
> > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > ---
> > > tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
> > > tests/data/acpi/q35/DSDT.viot | 0
> >
> > does default tests/data/acpi/q35/DSDT differs from
> > DSDT.viot?
>
> Yes the VIOT test has one more PCI device (virtio-iommu) and PXB devices,
> so there are additional descriptors in the DSDT
also see tests/qtest/bios-tables-test.c step 6
(---include diff--- part)
>
> Thanks,
> Jean
>
> >
> > > tests/data/acpi/q35/VIOT.viot | 0
> > > tests/data/acpi/virt/VIOT | 0
> > > 4 files changed, 3 insertions(+)
> > > create mode 100644 tests/data/acpi/q35/DSDT.viot
> > > create mode 100644 tests/data/acpi/q35/VIOT.viot
> > > create mode 100644 tests/data/acpi/virt/VIOT
> > >
> > > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> > > index dfb8523c8b..29b5b1eabc 100644
> > > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > > @@ -1 +1,4 @@
> > > /* List of comma-separated changed AML files to ignore */
> > > +"tests/data/acpi/virt/VIOT",
> > > +"tests/data/acpi/q35/DSDT.viot",
> > > +"tests/data/acpi/q35/VIOT.viot",
> > > diff --git a/tests/data/acpi/q35/DSDT.viot b/tests/data/acpi/q35/DSDT.viot
> > > new file mode 100644
> > > index 0000000000..e69de29bb2
> > > diff --git a/tests/data/acpi/q35/VIOT.viot b/tests/data/acpi/q35/VIOT.viot
> > > new file mode 100644
> > > index 0000000000..e69de29bb2
> > > diff --git a/tests/data/acpi/virt/VIOT b/tests/data/acpi/virt/VIOT
> > > new file mode 100644
> > > index 0000000000..e69de29bb2
> >
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/11] virtio-iommu: Add ACPI support
2021-10-11 10:10 ` Haiwei Li
@ 2021-10-11 17:34 ` Jean-Philippe Brucker
2021-10-13 0:56 ` Haiwei Li
0 siblings, 1 reply; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-11 17:34 UTC (permalink / raw)
To: Haiwei Li
Cc: peter.maydell, ehabkost, Michael S. Tsirkin, richard.henderson,
qemu-devel, shannon.zhaosl, qemu-arm, eric.auger, Paolo Bonzini,
ani, imammedo
Hi Haiwei,
On Mon, Oct 11, 2021 at 06:10:07PM +0800, Haiwei Li wrote:
[...]
> Gave up waiting for root file system device. Common problems:
> - Boot args (cat /proc/cmdline)
> - Check rootdelay= (did the system wait long enough?)
> - Missing modules (cat /proc/modules; ls /dev)
> ALERT! UUID=3caf26b5-4d08-43e0-8634-7573269c4f70 does not exist.
> Dropping to a shell!
>
> Any suggestions? Thanks.
It's possible that the rootfs is on a disk behind the IOMMU, and the IOMMU
driver doesn't get loaded. That could happen, for example, if the
virtio-iommu module is not present in the initramfs. Since IOMMU drivers
are typically built into the kernel rather than modules, distro tools that
build the initramfs might not pick up IOMMU modules. I'm guessing this
could be the issue here because of the hints and "Dropping to a shell"
line.
The clean solution will be to patch the initramfs tools to learn about
IOMMU drivers (I'm somewhat working on that). In the meantime, if this is
indeed the problem, you could try explicitly adding the virtio-iommu
module to the initramfs, or building the kernel with CONFIG_VIRTIO_IOMMU=y
rather than =m, though that requires VIRTIO and VIRTIO_PCI to be built-in
as well.
Thanks,
Jean
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/11] virtio-iommu: Add ACPI support
2021-10-11 17:34 ` Jean-Philippe Brucker
@ 2021-10-13 0:56 ` Haiwei Li
0 siblings, 0 replies; 45+ messages in thread
From: Haiwei Li @ 2021-10-13 0:56 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, Michael S. Tsirkin, richard.henderson,
qemu-devel, shannon.zhaosl, qemu-arm, eric.auger, Paolo Bonzini,
ani, imammedo
On Tue, Oct 12, 2021 at 1:34 AM Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> Hi Haiwei,
>
> On Mon, Oct 11, 2021 at 06:10:07PM +0800, Haiwei Li wrote:
> [...]
> > Gave up waiting for root file system device. Common problems:
> > - Boot args (cat /proc/cmdline)
> > - Check rootdelay= (did the system wait long enough?)
> > - Missing modules (cat /proc/modules; ls /dev)
> > ALERT! UUID=3caf26b5-4d08-43e0-8634-7573269c4f70 does not exist.
> > Dropping to a shell!
> >
> > Any suggestions? Thanks.
>
> It's possible that the rootfs is on a disk behind the IOMMU, and the IOMMU
> driver doesn't get loaded. That could happen, for example, if the
> virtio-iommu module is not present in the initramfs. Since IOMMU drivers
> are typically built into the kernel rather than modules, distro tools that
> build the initramfs might not pick up IOMMU modules. I'm guessing this
> could be the issue here because of the hints and "Dropping to a shell"
> line.
>
> The clean solution will be to patch the initramfs tools to learn about
> IOMMU drivers (I'm somewhat working on that). In the meantime, if this is
> indeed the problem, you could try explicitly adding the virtio-iommu
> module to the initramfs, or building the kernel with CONFIG_VIRTIO_IOMMU=y
> rather than =m, though that requires VIRTIO and VIRTIO_PCI to be built-in
> as well.
Thanks, Jean. It works.
--
Haiwei
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/11] virtio-iommu: Add ACPI support
2021-10-08 15:17 ` Jean-Philippe Brucker
2021-10-11 10:10 ` Haiwei Li
@ 2021-10-18 15:25 ` Michael S. Tsirkin
2021-10-19 15:39 ` Jean-Philippe Brucker
1 sibling, 1 reply; 45+ messages in thread
From: Michael S. Tsirkin @ 2021-10-18 15:25 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, richard.henderson,
qemu-devel, eric.auger, qemu-arm, imammedo, ani, pbonzini
On Fri, Oct 08, 2021 at 04:17:37PM +0100, Jean-Philippe Brucker wrote:
> On Tue, Oct 05, 2021 at 11:45:42AM -0400, Michael S. Tsirkin wrote:
> > Looks like this can not be applied yet because the bypass bit
> > isn't in yet. what's up with that?
>
> The boot-bypass bit isn't a hard dependency for this series, but it will
> be needed for full support eventually. It will be delayed by spec and
> Linux header changes
>
> In the meantime we can work around the problem by having the boot disks
> bypass the IOMMU (virtio without iommu-platform, or iommu-bypass bus).
>
> Thanks,
> Jean
OK... how do we want to apply all this?
If my tree I either need ack from an ARM maintainers, or
post a partial patchset with just x86 bits.
--
MST
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/11] virtio-iommu: Add ACPI support
2021-10-18 15:25 ` Michael S. Tsirkin
@ 2021-10-19 15:39 ` Jean-Philippe Brucker
2021-10-20 15:17 ` Michael S. Tsirkin
0 siblings, 1 reply; 45+ messages in thread
From: Jean-Philippe Brucker @ 2021-10-19 15:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: peter.maydell, ehabkost, shannon.zhaosl, richard.henderson,
qemu-devel, eric.auger, qemu-arm, imammedo, ani, pbonzini
On Mon, Oct 18, 2021 at 11:25:05AM -0400, Michael S. Tsirkin wrote:
> On Fri, Oct 08, 2021 at 04:17:37PM +0100, Jean-Philippe Brucker wrote:
> > On Tue, Oct 05, 2021 at 11:45:42AM -0400, Michael S. Tsirkin wrote:
> > > Looks like this can not be applied yet because the bypass bit
> > > isn't in yet. what's up with that?
> >
> > The boot-bypass bit isn't a hard dependency for this series, but it will
> > be needed for full support eventually. It will be delayed by spec and
> > Linux header changes
> >
> > In the meantime we can work around the problem by having the boot disks
> > bypass the IOMMU (virtio without iommu-platform, or iommu-bypass bus).
> >
> > Thanks,
> > Jean
>
> OK... how do we want to apply all this?
> If my tree I either need ack from an ARM maintainers, or
> post a partial patchset with just x86 bits.
Either works for me, with preference for keeping a single series
(otherwise I need to split patch 8, or add the tests later). I'll send v5
whole.
Thanks,
Jean
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/11] virtio-iommu: Add ACPI support
2021-10-19 15:39 ` Jean-Philippe Brucker
@ 2021-10-20 15:17 ` Michael S. Tsirkin
0 siblings, 0 replies; 45+ messages in thread
From: Michael S. Tsirkin @ 2021-10-20 15:17 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: peter.maydell, ehabkost, shannon.zhaosl, richard.henderson,
qemu-devel, eric.auger, qemu-arm, imammedo, ani, pbonzini
On Tue, Oct 19, 2021 at 04:39:20PM +0100, Jean-Philippe Brucker wrote:
> On Mon, Oct 18, 2021 at 11:25:05AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Oct 08, 2021 at 04:17:37PM +0100, Jean-Philippe Brucker wrote:
> > > On Tue, Oct 05, 2021 at 11:45:42AM -0400, Michael S. Tsirkin wrote:
> > > > Looks like this can not be applied yet because the bypass bit
> > > > isn't in yet. what's up with that?
> > >
> > > The boot-bypass bit isn't a hard dependency for this series, but it will
> > > be needed for full support eventually. It will be delayed by spec and
> > > Linux header changes
> > >
> > > In the meantime we can work around the problem by having the boot disks
> > > bypass the IOMMU (virtio without iommu-platform, or iommu-bypass bus).
> > >
> > > Thanks,
> > > Jean
> >
> > OK... how do we want to apply all this?
> > If my tree I either need ack from an ARM maintainers, or
> > post a partial patchset with just x86 bits.
>
> Either works for me, with preference for keeping a single series
> (otherwise I need to split patch 8, or add the tests later). I'll send v5
> whole.
>
> Thanks,
> Jean
Then you will need an ack from arm maintainers.
--
MST
^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2021-10-20 15:19 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 17:33 [PATCH v4 00/11] virtio-iommu: Add ACPI support Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 01/11] hw/acpi: Add VIOT table Jean-Philippe Brucker
2021-10-06 8:09 ` Igor Mammedov
2021-10-08 15:20 ` Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 02/11] hw/arm/virt-acpi-build: Add VIOT table for virtio-iommu Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 03/11] hw/arm/virt: Remove device tree restriction " Jean-Philippe Brucker
2021-10-05 11:57 ` Eric Auger
2021-10-08 15:20 ` Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 04/11] hw/arm/virt: Reject instantiation of multiple IOMMUs Jean-Philippe Brucker
2021-10-06 6:35 ` Igor Mammedov
2021-10-01 17:33 ` [PATCH v4 05/11] hw/arm/virt: Use object_property_set instead of qdev_prop_set Jean-Philippe Brucker
2021-10-05 9:27 ` Eric Auger
2021-10-06 6:36 ` Igor Mammedov
2021-10-01 17:33 ` [PATCH v4 06/11] hw/i386: Move vIOMMU uniqueness check into pc.c Jean-Philippe Brucker
2021-10-05 11:41 ` Eric Auger
2021-10-01 17:33 ` [PATCH v4 07/11] pc: Allow instantiating a virtio-iommu device Jean-Philippe Brucker
2021-10-05 19:18 ` Eric Auger
2021-10-06 7:19 ` Igor Mammedov
2021-10-08 15:24 ` Jean-Philippe Brucker
2021-10-08 10:46 ` Michael S. Tsirkin
2021-10-01 17:33 ` [PATCH v4 08/11] tests/acpi: allow updates of VIOT expected data files Jean-Philippe Brucker
2021-10-06 8:12 ` Igor Mammedov
2021-10-08 15:26 ` Jean-Philippe Brucker
2021-10-11 15:55 ` Igor Mammedov
2021-10-01 17:33 ` [PATCH v4 09/11] tests/acpi: add test cases for VIOT Jean-Philippe Brucker
2021-10-05 10:27 ` Ani Sinha
2021-10-08 15:27 ` Jean-Philippe Brucker
2021-10-05 19:40 ` Eric Auger
2021-10-06 8:14 ` Igor Mammedov
2021-10-01 17:33 ` [PATCH v4 10/11] tests/acpi: add expected blob for VIOT test on virt machine Jean-Philippe Brucker
2021-10-05 10:04 ` Ani Sinha
2021-10-08 15:33 ` Jean-Philippe Brucker
2021-10-05 19:38 ` Eric Auger
2021-10-08 15:30 ` Jean-Philippe Brucker
2021-10-01 17:33 ` [PATCH v4 11/11] tests/acpi: add expected blobs for VIOT test on q35 machine Jean-Philippe Brucker
2021-10-05 10:07 ` Ani Sinha
2021-10-05 19:41 ` Eric Auger
2021-10-05 15:45 ` [PATCH v4 00/11] virtio-iommu: Add ACPI support Michael S. Tsirkin
2021-10-08 15:17 ` Jean-Philippe Brucker
2021-10-11 10:10 ` Haiwei Li
2021-10-11 17:34 ` Jean-Philippe Brucker
2021-10-13 0:56 ` Haiwei Li
2021-10-18 15:25 ` Michael S. Tsirkin
2021-10-19 15:39 ` Jean-Philippe Brucker
2021-10-20 15:17 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.