* [PATCH v3 0/2] hw/i386/amd_iommu: Add migration support
@ 2025-02-12 5:44 Suravee Suthikulpanit
2025-02-12 5:44 ` [PATCH v3 1/2] hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow full control over the PCI device creation Suravee Suthikulpanit
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Suravee Suthikulpanit @ 2025-02-12 5:44 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, mtosatti, mst, marcel.apfelbaum, jon.grimm,
santosh.shukla, vasant.hegde, Wei.Huang2, bsd, berrange,
joao.m.martins, alejandro.j.jimenez, Suravee Suthikulpanit
Currently, amd-iommu device does not support migration. This series addresses
an issue due hidden AMDVI-PCI device enumeration. Then introduces migratable
VMStateDescription, which saves necessary parameters for the device.
Changes from v2:
(https://lore.kernel.org/all/20250206051856.323651-1-suravee.suthikulpanit@amd.com)
* Add patch 1/2
Suravee Suthikulpanit (2):
hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow
full control over the PCI device creation
hw/i386/amd_iommu: Allow migration when explicitly create the
AMDVI-PCI device
hw/i386/acpi-build.c | 8 ++--
hw/i386/amd_iommu.c | 111 +++++++++++++++++++++++++++++++++----------
hw/i386/amd_iommu.h | 3 +-
3 files changed, 91 insertions(+), 31 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow full control over the PCI device creation
2025-02-12 5:44 [PATCH v3 0/2] hw/i386/amd_iommu: Add migration support Suravee Suthikulpanit
@ 2025-02-12 5:44 ` Suravee Suthikulpanit
2025-02-21 0:38 ` Michael S. Tsirkin
2025-02-12 5:44 ` [PATCH v3 2/2] hw/i386/amd_iommu: Allow migration when explicitly create the AMDVI-PCI device Suravee Suthikulpanit
2025-02-20 23:21 ` [PATCH v3 0/2] hw/i386/amd_iommu: Add migration support Michael S. Tsirkin
2 siblings, 1 reply; 6+ messages in thread
From: Suravee Suthikulpanit @ 2025-02-12 5:44 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, mtosatti, mst, marcel.apfelbaum, jon.grimm,
santosh.shukla, vasant.hegde, Wei.Huang2, bsd, berrange,
joao.m.martins, alejandro.j.jimenez, Suravee Suthikulpanit
Current amd-iommu model internally creates an AMDVI-PCI device. Here is
a snippet from info qtree:
bus: main-system-bus
type System
dev: amd-iommu, id ""
xtsup = false
pci-id = ""
intremap = "on"
device-iotlb = false
pt = true
...
dev: q35-pcihost, id ""
MCFG = -1 (0xffffffffffffffff)
pci-hole64-size = 34359738368 (32 GiB)
below-4g-mem-size = 134217728 (128 MiB)
above-4g-mem-size = 0 (0 B)
smm-ranges = true
x-pci-hole64-fix = true
x-config-reg-migration-enabled = true
bypass-iommu = false
bus: pcie.0
type PCIE
dev: AMDVI-PCI, id ""
addr = 01.0
romfile = ""
romsize = 4294967295 (0xffffffff)
rombar = -1 (0xffffffffffffffff)
multifunction = false
x-pcie-lnksta-dllla = true
x-pcie-extcap-init = true
failover_pair_id = ""
acpi-index = 0 (0x0)
x-pcie-err-unc-mask = true
x-pcie-ari-nextfn-1 = false
x-max-bounce-buffer-size = 4096 (4 KiB)
x-pcie-ext-tag = true
busnr = 0 (0x0)
class Class 0806, addr 00:01.0, pci id 1022:0000 (sub 1af4:1100)
This prohibits users from specifying the PCI topology for the amd-iommu device,
which becomes a problem when trying to support VM migration since it does not
guarantee the same enumeration of AMD IOMMU device.
Therfore, decouple the AMDVI-PCI from amd-iommu device and introduce pci-id
parameter to link between the two devices.
For example:
-device AMDVI-PCI,id=iommupci0,bus=pcie.0,addr=0x05 \
-device amd-iommu,intremap=on,pt=on,xtsup=on,pci-id=iommupci0 \
For backward-compatibility, internally create the AMDVI-PCI device if not
specified on the CLI.
Co-developed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
hw/i386/acpi-build.c | 8 +++----
hw/i386/amd_iommu.c | 53 +++++++++++++++++++++++++++-----------------
hw/i386/amd_iommu.h | 3 ++-
3 files changed, 39 insertions(+), 25 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 53b7306b43..e70eeaf577 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2333,10 +2333,10 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
build_append_int_noprefix(table_data, ivhd_blob->len + 24, 2);
/* DeviceID */
build_append_int_noprefix(table_data,
- object_property_get_int(OBJECT(&s->pci), "addr",
+ object_property_get_int(OBJECT(s->pci), "addr",
&error_abort), 2);
/* Capability offset */
- build_append_int_noprefix(table_data, s->pci.capab_offset, 2);
+ build_append_int_noprefix(table_data, s->pci->capab_offset, 2);
/* IOMMU base address */
build_append_int_noprefix(table_data, s->mr_mmio.addr, 8);
/* PCI Segment Group */
@@ -2368,10 +2368,10 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
build_append_int_noprefix(table_data, ivhd_blob->len + 40, 2);
/* DeviceID */
build_append_int_noprefix(table_data,
- object_property_get_int(OBJECT(&s->pci), "addr",
+ object_property_get_int(OBJECT(s->pci), "addr",
&error_abort), 2);
/* Capability offset */
- build_append_int_noprefix(table_data, s->pci.capab_offset, 2);
+ build_append_int_noprefix(table_data, s->pci->capab_offset, 2);
/* IOMMU base address */
build_append_int_noprefix(table_data, s->mr_mmio.addr, 8);
/* PCI Segment Group */
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 6b13ce894b..0f552bafa0 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -167,11 +167,11 @@ static void amdvi_generate_msi_interrupt(AMDVIState *s)
{
MSIMessage msg = {};
MemTxAttrs attrs = {
- .requester_id = pci_requester_id(&s->pci.dev)
+ .requester_id = pci_requester_id(&s->pci->dev)
};
- if (msi_enabled(&s->pci.dev)) {
- msg = msi_get_message(&s->pci.dev, 0);
+ if (msi_enabled(&s->pci->dev)) {
+ msg = msi_get_message(&s->pci->dev, 0);
address_space_stl_le(&address_space_memory, msg.address, msg.data,
attrs, NULL);
}
@@ -239,7 +239,7 @@ static void amdvi_page_fault(AMDVIState *s, uint16_t devid,
info |= AMDVI_EVENT_IOPF_I | AMDVI_EVENT_IOPF;
amdvi_encode_event(evt, devid, addr, info);
amdvi_log_event(s, evt);
- pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
+ pci_word_test_and_set_mask(s->pci->dev.config + PCI_STATUS,
PCI_STATUS_SIG_TARGET_ABORT);
}
/*
@@ -256,7 +256,7 @@ static void amdvi_log_devtab_error(AMDVIState *s, uint16_t devid,
amdvi_encode_event(evt, devid, devtab, info);
amdvi_log_event(s, evt);
- pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
+ pci_word_test_and_set_mask(s->pci->dev.config + PCI_STATUS,
PCI_STATUS_SIG_TARGET_ABORT);
}
/* log an event trying to access command buffer
@@ -269,7 +269,7 @@ static void amdvi_log_command_error(AMDVIState *s, hwaddr addr)
amdvi_encode_event(evt, 0, addr, info);
amdvi_log_event(s, evt);
- pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
+ pci_word_test_and_set_mask(s->pci->dev.config + PCI_STATUS,
PCI_STATUS_SIG_TARGET_ABORT);
}
/* log an illegal command event
@@ -310,7 +310,7 @@ static void amdvi_log_pagetab_error(AMDVIState *s, uint16_t devid,
info |= AMDVI_EVENT_PAGE_TAB_HW_ERROR;
amdvi_encode_event(evt, devid, addr, info);
amdvi_log_event(s, evt);
- pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
+ pci_word_test_and_set_mask(s->pci->dev.config + PCI_STATUS,
PCI_STATUS_SIG_TARGET_ABORT);
}
@@ -1607,26 +1607,45 @@ static void amdvi_sysbus_reset(DeviceState *dev)
{
AMDVIState *s = AMD_IOMMU_DEVICE(dev);
- msi_reset(&s->pci.dev);
+ msi_reset(&s->pci->dev);
amdvi_init(s);
}
static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
{
+ DeviceClass *dc = (DeviceClass *) object_get_class(OBJECT(dev));
AMDVIState *s = AMD_IOMMU_DEVICE(dev);
MachineState *ms = MACHINE(qdev_get_machine());
PCMachineState *pcms = PC_MACHINE(ms);
X86MachineState *x86ms = X86_MACHINE(ms);
PCIBus *bus = pcms->pcibus;
- s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
- amdvi_uint64_equal, g_free, g_free);
+ if (s->pci_id) {
+ PCIDevice *pdev = NULL;
+ int ret = pci_qdev_find_device(s->pci_id, &pdev);
- /* This device should take care of IOMMU PCI properties */
- if (!qdev_realize(DEVICE(&s->pci), &bus->qbus, errp)) {
- return;
+ if (ret) {
+ error_report("Cannot find PCI device '%s'", s->pci_id);
+ return;
+ }
+
+ if (!object_dynamic_cast(OBJECT(pdev), TYPE_AMD_IOMMU_PCI)) {
+ error_report("Device '%s' must be an AMDVI-PCI device type", s->pci_id);
+ return;
+ }
+
+ s->pci = AMD_IOMMU_PCI(pdev);
+ } else {
+ s->pci = AMD_IOMMU_PCI(object_new(TYPE_AMD_IOMMU_PCI));
+ /* This device should take care of IOMMU PCI properties */
+ if (!qdev_realize(DEVICE(s->pci), &bus->qbus, errp)) {
+ return;
+ }
}
+ s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
+ amdvi_uint64_equal, g_free, g_free);
+
/* Pseudo address space under root PCI bus. */
x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_IOAPIC_SB_DEVID);
@@ -1663,6 +1682,7 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
static const Property amdvi_properties[] = {
DEFINE_PROP_BOOL("xtsup", AMDVIState, xtsup, false),
+ DEFINE_PROP_STRING("pci-id", AMDVIState, pci_id),
};
static const VMStateDescription vmstate_amdvi_sysbus = {
@@ -1670,12 +1690,6 @@ static const VMStateDescription vmstate_amdvi_sysbus = {
.unmigratable = 1
};
-static void amdvi_sysbus_instance_init(Object *klass)
-{
- AMDVIState *s = AMD_IOMMU_DEVICE(klass);
-
- object_initialize(&s->pci, sizeof(s->pci), TYPE_AMD_IOMMU_PCI);
-}
static void amdvi_sysbus_class_init(ObjectClass *klass, void *data)
{
@@ -1698,7 +1712,6 @@ static const TypeInfo amdvi_sysbus = {
.name = TYPE_AMD_IOMMU_DEVICE,
.parent = TYPE_X86_IOMMU_DEVICE,
.instance_size = sizeof(AMDVIState),
- .instance_init = amdvi_sysbus_instance_init,
.class_init = amdvi_sysbus_class_init
};
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index e0dac4d9a9..ece71ff0b6 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -315,7 +315,8 @@ struct AMDVIPCIState {
struct AMDVIState {
X86IOMMUState iommu; /* IOMMU bus device */
- AMDVIPCIState pci; /* IOMMU PCI device */
+ AMDVIPCIState *pci; /* IOMMU PCI device */
+ char *pci_id; /* ID of AMDVI-PCI device, if user created */
uint32_t version;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] hw/i386/amd_iommu: Allow migration when explicitly create the AMDVI-PCI device
2025-02-12 5:44 [PATCH v3 0/2] hw/i386/amd_iommu: Add migration support Suravee Suthikulpanit
2025-02-12 5:44 ` [PATCH v3 1/2] hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow full control over the PCI device creation Suravee Suthikulpanit
@ 2025-02-12 5:44 ` Suravee Suthikulpanit
2025-02-20 23:21 ` [PATCH v3 0/2] hw/i386/amd_iommu: Add migration support Michael S. Tsirkin
2 siblings, 0 replies; 6+ messages in thread
From: Suravee Suthikulpanit @ 2025-02-12 5:44 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, mtosatti, mst, marcel.apfelbaum, jon.grimm,
santosh.shukla, vasant.hegde, Wei.Huang2, bsd, berrange,
joao.m.martins, alejandro.j.jimenez, Suravee Suthikulpanit
Add migration support for AMD IOMMU model by saving necessary AMDVIState
parameters for MMIO registers, device table, command buffer, and event
buffers.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
hw/i386/amd_iommu.c | 58 ++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 52 insertions(+), 6 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 0f552bafa0..dda1a5781f 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1611,6 +1611,57 @@ static void amdvi_sysbus_reset(DeviceState *dev)
amdvi_init(s);
}
+static const VMStateDescription vmstate_amdvi_sysbus = {
+ .name = "amd-iommu",
+ .unmigratable = 1
+};
+
+static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
+ .name = "amd-iommu",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .priority = MIG_PRI_IOMMU,
+ .fields = (VMStateField[]) {
+ /* Updated in amdvi_handle_control_write() */
+ VMSTATE_BOOL(enabled, AMDVIState),
+ VMSTATE_BOOL(ga_enabled, AMDVIState),
+ VMSTATE_BOOL(ats_enabled, AMDVIState),
+ VMSTATE_BOOL(cmdbuf_enabled, AMDVIState),
+ VMSTATE_BOOL(completion_wait_intr, AMDVIState),
+ VMSTATE_BOOL(evtlog_enabled, AMDVIState),
+ VMSTATE_BOOL(evtlog_intr, AMDVIState),
+ /* Updated in amdvi_handle_devtab_write() */
+ VMSTATE_UINT64(devtab, AMDVIState),
+ VMSTATE_UINT64(devtab_len, AMDVIState),
+ /* Updated in amdvi_handle_cmdbase_write() */
+ VMSTATE_UINT64(cmdbuf, AMDVIState),
+ VMSTATE_UINT64(cmdbuf_len, AMDVIState),
+ /* Updated in amdvi_handle_cmdhead_write() */
+ VMSTATE_UINT32(cmdbuf_head, AMDVIState),
+ /* Updated in amdvi_handle_cmdtail_write() */
+ VMSTATE_UINT32(cmdbuf_tail, AMDVIState),
+ /* Updated in amdvi_handle_evtbase_write() */
+ VMSTATE_UINT64(evtlog, AMDVIState),
+ VMSTATE_UINT32(evtlog_len, AMDVIState),
+ /* Updated in amdvi_handle_evthead_write() */
+ VMSTATE_UINT32(evtlog_head, AMDVIState),
+ /* Updated in amdvi_handle_evttail_write() */
+ VMSTATE_UINT32(evtlog_tail, AMDVIState),
+ /* Updated in amdvi_handle_pprbase_write() */
+ VMSTATE_UINT64(ppr_log, AMDVIState),
+ VMSTATE_UINT32(pprlog_len, AMDVIState),
+ /* Updated in amdvi_handle_pprhead_write() */
+ VMSTATE_UINT32(pprlog_head, AMDVIState),
+ /* Updated in amdvi_handle_tailhead_write() */
+ VMSTATE_UINT32(pprlog_tail, AMDVIState),
+ /* MMIO registers */
+ VMSTATE_UINT8_ARRAY(mmior, AMDVIState, AMDVI_MMIO_SIZE),
+ VMSTATE_UINT8_ARRAY(romask, AMDVIState, AMDVI_MMIO_SIZE),
+ VMSTATE_UINT8_ARRAY(w1cmask, AMDVIState, AMDVI_MMIO_SIZE),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
{
DeviceClass *dc = (DeviceClass *) object_get_class(OBJECT(dev));
@@ -1635,6 +1686,7 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
}
s->pci = AMD_IOMMU_PCI(pdev);
+ dc->vmsd = &vmstate_amdvi_sysbus_migratable;
} else {
s->pci = AMD_IOMMU_PCI(object_new(TYPE_AMD_IOMMU_PCI));
/* This device should take care of IOMMU PCI properties */
@@ -1685,12 +1737,6 @@ static const Property amdvi_properties[] = {
DEFINE_PROP_STRING("pci-id", AMDVIState, pci_id),
};
-static const VMStateDescription vmstate_amdvi_sysbus = {
- .name = "amd-iommu",
- .unmigratable = 1
-};
-
-
static void amdvi_sysbus_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/2] hw/i386/amd_iommu: Add migration support
2025-02-12 5:44 [PATCH v3 0/2] hw/i386/amd_iommu: Add migration support Suravee Suthikulpanit
2025-02-12 5:44 ` [PATCH v3 1/2] hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow full control over the PCI device creation Suravee Suthikulpanit
2025-02-12 5:44 ` [PATCH v3 2/2] hw/i386/amd_iommu: Allow migration when explicitly create the AMDVI-PCI device Suravee Suthikulpanit
@ 2025-02-20 23:21 ` Michael S. Tsirkin
2 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2025-02-20 23:21 UTC (permalink / raw)
To: Suravee Suthikulpanit
Cc: qemu-devel, pbonzini, mtosatti, marcel.apfelbaum, jon.grimm,
santosh.shukla, vasant.hegde, Wei.Huang2, bsd, berrange,
joao.m.martins, alejandro.j.jimenez
On Wed, Feb 12, 2025 at 05:44:48AM +0000, Suravee Suthikulpanit wrote:
> Currently, amd-iommu device does not support migration. This series addresses
> an issue due hidden AMDVI-PCI device enumeration. Then introduces migratable
> VMStateDescription, which saves necessary parameters for the device.
>
> Changes from v2:
> (https://lore.kernel.org/all/20250206051856.323651-1-suravee.suthikulpanit@amd.com)
> * Add patch 1/2
Fails build on 32 bit:
https://gitlab.com/mstredhat/qemu/-/jobs/9202574769
In file included from ../include/qemu/osdep.h:53,
from ../hw/i386/amd_iommu.c:23:
../include/qemu/compiler.h:70:35: error: invalid operands to binary - (have ‘uint64_t *’ {aka ‘long long unsigned int *’} and ‘size_t *’ {aka ‘unsigned int *’})
70 | #define type_check(t1,t2) ((t1*)0 - (t2*)0)
| ^
../include/migration/vmstate.h:269:6: note: in expansion of macro ‘type_check’
269 | type_check(_type, typeof_field(_state, _field)))
| ^~~~~~~~~~
../include/migration/vmstate.h:320:21: note: in expansion of macro ‘vmstate_offset_value’
320 | .offset = vmstate_offset_value(_state, _field, _type), \
| ^~~~~~~~~~~~~~~~~~~~
../include/migration/vmstate.h:853:5: note: in expansion of macro ‘VMSTATE_SINGLE_TEST’
853 | VMSTATE_SINGLE_TEST(_field, _state, NULL, _version, _info, _type)
| ^~~~~~~~~~~~~~~~~~~
../include/migration/vmstate.h:903:5: note: in expansion of macro ‘VMSTATE_SINGLE’
903 | VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64, uint64_t)
| ^~~~~~~~~~~~~~
../include/migration/vmstate.h:937:5: note: in expansion of macro ‘VMSTATE_UINT64_V’
937 | VMSTATE_UINT64_V(_f, _s, 0)
| ^~~~~~~~~~~~~~~~
../hw/i386/amd_iommu.c:1635:7: note: in expansion of macro ‘VMSTATE_UINT64’
1635 | VMSTATE_UINT64(devtab_len, AMDVIState),
| ^~~~~~~~~~~~~~
> Suravee Suthikulpanit (2):
> hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow
> full control over the PCI device creation
> hw/i386/amd_iommu: Allow migration when explicitly create the
> AMDVI-PCI device
>
> hw/i386/acpi-build.c | 8 ++--
> hw/i386/amd_iommu.c | 111 +++++++++++++++++++++++++++++++++----------
> hw/i386/amd_iommu.h | 3 +-
> 3 files changed, 91 insertions(+), 31 deletions(-)
>
> --
> 2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow full control over the PCI device creation
2025-02-12 5:44 ` [PATCH v3 1/2] hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow full control over the PCI device creation Suravee Suthikulpanit
@ 2025-02-21 0:38 ` Michael S. Tsirkin
2025-02-25 13:39 ` Suthikulpanit, Suravee
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2025-02-21 0:38 UTC (permalink / raw)
To: Suravee Suthikulpanit
Cc: qemu-devel, pbonzini, mtosatti, marcel.apfelbaum, jon.grimm,
santosh.shukla, vasant.hegde, Wei.Huang2, bsd, berrange,
joao.m.martins, alejandro.j.jimenez
On Wed, Feb 12, 2025 at 05:44:49AM +0000, Suravee Suthikulpanit wrote:
> Current amd-iommu model internally creates an AMDVI-PCI device. Here is
> a snippet from info qtree:
>
> bus: main-system-bus
> type System
> dev: amd-iommu, id ""
> xtsup = false
> pci-id = ""
> intremap = "on"
> device-iotlb = false
> pt = true
> ...
> dev: q35-pcihost, id ""
> MCFG = -1 (0xffffffffffffffff)
> pci-hole64-size = 34359738368 (32 GiB)
> below-4g-mem-size = 134217728 (128 MiB)
> above-4g-mem-size = 0 (0 B)
> smm-ranges = true
> x-pci-hole64-fix = true
> x-config-reg-migration-enabled = true
> bypass-iommu = false
> bus: pcie.0
> type PCIE
> dev: AMDVI-PCI, id ""
> addr = 01.0
> romfile = ""
> romsize = 4294967295 (0xffffffff)
> rombar = -1 (0xffffffffffffffff)
> multifunction = false
> x-pcie-lnksta-dllla = true
> x-pcie-extcap-init = true
> failover_pair_id = ""
> acpi-index = 0 (0x0)
> x-pcie-err-unc-mask = true
> x-pcie-ari-nextfn-1 = false
> x-max-bounce-buffer-size = 4096 (4 KiB)
> x-pcie-ext-tag = true
> busnr = 0 (0x0)
> class Class 0806, addr 00:01.0, pci id 1022:0000 (sub 1af4:1100)
>
> This prohibits users from specifying the PCI topology for the amd-iommu device,
> which becomes a problem when trying to support VM migration since it does not
> guarantee the same enumeration of AMD IOMMU device.
>
> Therfore, decouple the AMDVI-PCI from amd-iommu device and introduce pci-id
> parameter to link between the two devices.
>
> For example:
> -device AMDVI-PCI,id=iommupci0,bus=pcie.0,addr=0x05 \
> -device amd-iommu,intremap=on,pt=on,xtsup=on,pci-id=iommupci0 \
>
> For backward-compatibility, internally create the AMDVI-PCI device if not
> specified on the CLI.
>
> Co-developed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
breaks build:
https://gitlab.com/mstredhat/qemu/-/jobs/9202802751
./hw/i386/amd_iommu.c: In function ‘amdvi_sysbus_realize’:
../hw/i386/amd_iommu.c:1616:18: error: unused variable ‘dc’ [-Werror=unused-variable]
1616 | DeviceClass *dc = (DeviceClass *) object_get_class(OBJECT(dev));
| ^~
cc1: all warnings being treated as errors
> ---
> hw/i386/acpi-build.c | 8 +++----
> hw/i386/amd_iommu.c | 53 +++++++++++++++++++++++++++-----------------
> hw/i386/amd_iommu.h | 3 ++-
> 3 files changed, 39 insertions(+), 25 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 53b7306b43..e70eeaf577 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2333,10 +2333,10 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> build_append_int_noprefix(table_data, ivhd_blob->len + 24, 2);
> /* DeviceID */
> build_append_int_noprefix(table_data,
> - object_property_get_int(OBJECT(&s->pci), "addr",
> + object_property_get_int(OBJECT(s->pci), "addr",
> &error_abort), 2);
> /* Capability offset */
> - build_append_int_noprefix(table_data, s->pci.capab_offset, 2);
> + build_append_int_noprefix(table_data, s->pci->capab_offset, 2);
> /* IOMMU base address */
> build_append_int_noprefix(table_data, s->mr_mmio.addr, 8);
> /* PCI Segment Group */
> @@ -2368,10 +2368,10 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> build_append_int_noprefix(table_data, ivhd_blob->len + 40, 2);
> /* DeviceID */
> build_append_int_noprefix(table_data,
> - object_property_get_int(OBJECT(&s->pci), "addr",
> + object_property_get_int(OBJECT(s->pci), "addr",
> &error_abort), 2);
> /* Capability offset */
> - build_append_int_noprefix(table_data, s->pci.capab_offset, 2);
> + build_append_int_noprefix(table_data, s->pci->capab_offset, 2);
> /* IOMMU base address */
> build_append_int_noprefix(table_data, s->mr_mmio.addr, 8);
> /* PCI Segment Group */
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 6b13ce894b..0f552bafa0 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -167,11 +167,11 @@ static void amdvi_generate_msi_interrupt(AMDVIState *s)
> {
> MSIMessage msg = {};
> MemTxAttrs attrs = {
> - .requester_id = pci_requester_id(&s->pci.dev)
> + .requester_id = pci_requester_id(&s->pci->dev)
> };
>
> - if (msi_enabled(&s->pci.dev)) {
> - msg = msi_get_message(&s->pci.dev, 0);
> + if (msi_enabled(&s->pci->dev)) {
> + msg = msi_get_message(&s->pci->dev, 0);
> address_space_stl_le(&address_space_memory, msg.address, msg.data,
> attrs, NULL);
> }
> @@ -239,7 +239,7 @@ static void amdvi_page_fault(AMDVIState *s, uint16_t devid,
> info |= AMDVI_EVENT_IOPF_I | AMDVI_EVENT_IOPF;
> amdvi_encode_event(evt, devid, addr, info);
> amdvi_log_event(s, evt);
> - pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
> + pci_word_test_and_set_mask(s->pci->dev.config + PCI_STATUS,
> PCI_STATUS_SIG_TARGET_ABORT);
> }
> /*
> @@ -256,7 +256,7 @@ static void amdvi_log_devtab_error(AMDVIState *s, uint16_t devid,
>
> amdvi_encode_event(evt, devid, devtab, info);
> amdvi_log_event(s, evt);
> - pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
> + pci_word_test_and_set_mask(s->pci->dev.config + PCI_STATUS,
> PCI_STATUS_SIG_TARGET_ABORT);
> }
> /* log an event trying to access command buffer
> @@ -269,7 +269,7 @@ static void amdvi_log_command_error(AMDVIState *s, hwaddr addr)
>
> amdvi_encode_event(evt, 0, addr, info);
> amdvi_log_event(s, evt);
> - pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
> + pci_word_test_and_set_mask(s->pci->dev.config + PCI_STATUS,
> PCI_STATUS_SIG_TARGET_ABORT);
> }
> /* log an illegal command event
> @@ -310,7 +310,7 @@ static void amdvi_log_pagetab_error(AMDVIState *s, uint16_t devid,
> info |= AMDVI_EVENT_PAGE_TAB_HW_ERROR;
> amdvi_encode_event(evt, devid, addr, info);
> amdvi_log_event(s, evt);
> - pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
> + pci_word_test_and_set_mask(s->pci->dev.config + PCI_STATUS,
> PCI_STATUS_SIG_TARGET_ABORT);
> }
>
> @@ -1607,26 +1607,45 @@ static void amdvi_sysbus_reset(DeviceState *dev)
> {
> AMDVIState *s = AMD_IOMMU_DEVICE(dev);
>
> - msi_reset(&s->pci.dev);
> + msi_reset(&s->pci->dev);
> amdvi_init(s);
> }
>
> static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
> {
> + DeviceClass *dc = (DeviceClass *) object_get_class(OBJECT(dev));
> AMDVIState *s = AMD_IOMMU_DEVICE(dev);
> MachineState *ms = MACHINE(qdev_get_machine());
> PCMachineState *pcms = PC_MACHINE(ms);
> X86MachineState *x86ms = X86_MACHINE(ms);
> PCIBus *bus = pcms->pcibus;
>
> - s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
> - amdvi_uint64_equal, g_free, g_free);
> + if (s->pci_id) {
> + PCIDevice *pdev = NULL;
> + int ret = pci_qdev_find_device(s->pci_id, &pdev);
>
> - /* This device should take care of IOMMU PCI properties */
> - if (!qdev_realize(DEVICE(&s->pci), &bus->qbus, errp)) {
> - return;
> + if (ret) {
> + error_report("Cannot find PCI device '%s'", s->pci_id);
> + return;
> + }
> +
> + if (!object_dynamic_cast(OBJECT(pdev), TYPE_AMD_IOMMU_PCI)) {
> + error_report("Device '%s' must be an AMDVI-PCI device type", s->pci_id);
> + return;
> + }
> +
> + s->pci = AMD_IOMMU_PCI(pdev);
> + } else {
> + s->pci = AMD_IOMMU_PCI(object_new(TYPE_AMD_IOMMU_PCI));
> + /* This device should take care of IOMMU PCI properties */
> + if (!qdev_realize(DEVICE(s->pci), &bus->qbus, errp)) {
> + return;
> + }
> }
>
> + s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
> + amdvi_uint64_equal, g_free, g_free);
> +
> /* Pseudo address space under root PCI bus. */
> x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_IOAPIC_SB_DEVID);
>
> @@ -1663,6 +1682,7 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>
> static const Property amdvi_properties[] = {
> DEFINE_PROP_BOOL("xtsup", AMDVIState, xtsup, false),
> + DEFINE_PROP_STRING("pci-id", AMDVIState, pci_id),
> };
>
> static const VMStateDescription vmstate_amdvi_sysbus = {
> @@ -1670,12 +1690,6 @@ static const VMStateDescription vmstate_amdvi_sysbus = {
> .unmigratable = 1
> };
>
> -static void amdvi_sysbus_instance_init(Object *klass)
> -{
> - AMDVIState *s = AMD_IOMMU_DEVICE(klass);
> -
> - object_initialize(&s->pci, sizeof(s->pci), TYPE_AMD_IOMMU_PCI);
> -}
>
> static void amdvi_sysbus_class_init(ObjectClass *klass, void *data)
> {
> @@ -1698,7 +1712,6 @@ static const TypeInfo amdvi_sysbus = {
> .name = TYPE_AMD_IOMMU_DEVICE,
> .parent = TYPE_X86_IOMMU_DEVICE,
> .instance_size = sizeof(AMDVIState),
> - .instance_init = amdvi_sysbus_instance_init,
> .class_init = amdvi_sysbus_class_init
> };
>
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index e0dac4d9a9..ece71ff0b6 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -315,7 +315,8 @@ struct AMDVIPCIState {
>
> struct AMDVIState {
> X86IOMMUState iommu; /* IOMMU bus device */
> - AMDVIPCIState pci; /* IOMMU PCI device */
> + AMDVIPCIState *pci; /* IOMMU PCI device */
> + char *pci_id; /* ID of AMDVI-PCI device, if user created */
>
> uint32_t version;
>
> --
> 2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow full control over the PCI device creation
2025-02-21 0:38 ` Michael S. Tsirkin
@ 2025-02-25 13:39 ` Suthikulpanit, Suravee
0 siblings, 0 replies; 6+ messages in thread
From: Suthikulpanit, Suravee @ 2025-02-25 13:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, pbonzini, mtosatti, marcel.apfelbaum, jon.grimm,
santosh.shukla, vasant.hegde, Wei.Huang2, bsd, berrange,
joao.m.martins, alejandro.j.jimenez
On 2/21/2025 7:38 AM, Michael S. Tsirkin wrote:
> On Wed, Feb 12, 2025 at 05:44:49AM +0000, Suravee Suthikulpanit wrote:
>> Current amd-iommu model internally creates an AMDVI-PCI device. Here is
>> a snippet from info qtree:
>>
>> bus: main-system-bus
>> type System
>> dev: amd-iommu, id ""
>> xtsup = false
>> pci-id = ""
>> intremap = "on"
>> device-iotlb = false
>> pt = true
>> ...
>> dev: q35-pcihost, id ""
>> MCFG = -1 (0xffffffffffffffff)
>> pci-hole64-size = 34359738368 (32 GiB)
>> below-4g-mem-size = 134217728 (128 MiB)
>> above-4g-mem-size = 0 (0 B)
>> smm-ranges = true
>> x-pci-hole64-fix = true
>> x-config-reg-migration-enabled = true
>> bypass-iommu = false
>> bus: pcie.0
>> type PCIE
>> dev: AMDVI-PCI, id ""
>> addr = 01.0
>> romfile = ""
>> romsize = 4294967295 (0xffffffff)
>> rombar = -1 (0xffffffffffffffff)
>> multifunction = false
>> x-pcie-lnksta-dllla = true
>> x-pcie-extcap-init = true
>> failover_pair_id = ""
>> acpi-index = 0 (0x0)
>> x-pcie-err-unc-mask = true
>> x-pcie-ari-nextfn-1 = false
>> x-max-bounce-buffer-size = 4096 (4 KiB)
>> x-pcie-ext-tag = true
>> busnr = 0 (0x0)
>> class Class 0806, addr 00:01.0, pci id 1022:0000 (sub 1af4:1100)
>>
>> This prohibits users from specifying the PCI topology for the amd-iommu device,
>> which becomes a problem when trying to support VM migration since it does not
>> guarantee the same enumeration of AMD IOMMU device.
>>
>> Therfore, decouple the AMDVI-PCI from amd-iommu device and introduce pci-id
>> parameter to link between the two devices.
>>
>> For example:
>> -device AMDVI-PCI,id=iommupci0,bus=pcie.0,addr=0x05 \
>> -device amd-iommu,intremap=on,pt=on,xtsup=on,pci-id=iommupci0 \
>>
>> For backward-compatibility, internally create the AMDVI-PCI device if not
>> specified on the CLI.
>>
>> Co-developed-by: Daniel P. Berrangé<berrange@redhat.com>
>> Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>
> breaks build:
>
> https://gitlab.com/mstredhat/qemu/-/jobs/9202802751
>
> ./hw/i386/amd_iommu.c: In function ‘amdvi_sysbus_realize’:
> ../hw/i386/amd_iommu.c:1616:18: error: unused variable ‘dc’ [-Werror=unused-variable]
> 1616 | DeviceClass *dc = (DeviceClass *) object_get_class(OBJECT(dev));
> | ^~
> cc1: all warnings being treated as errors
I'll send out v4 with the fix.
Suravee
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-25 13:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-12 5:44 [PATCH v3 0/2] hw/i386/amd_iommu: Add migration support Suravee Suthikulpanit
2025-02-12 5:44 ` [PATCH v3 1/2] hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow full control over the PCI device creation Suravee Suthikulpanit
2025-02-21 0:38 ` Michael S. Tsirkin
2025-02-25 13:39 ` Suthikulpanit, Suravee
2025-02-12 5:44 ` [PATCH v3 2/2] hw/i386/amd_iommu: Allow migration when explicitly create the AMDVI-PCI device Suravee Suthikulpanit
2025-02-20 23:21 ` [PATCH v3 0/2] hw/i386/amd_iommu: Add migration support Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).