Re: [PATCH] hw/i386/amd_iommu: Allow migration

[Joao Martins]

On 21/11/2024 11:42, Joao Martins wrote:> On 20/11/2024 07:31, Suravee
Suthikulpanit wrote:
>> 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 | 36 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 13af7211e1..3d2bb9d81e 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -1673,7 +1673,41 @@ static Property amdvi_properties[] = {
>>
>>  static const VMStateDescription vmstate_amdvi_sysbus = {
>>      .name = "amd-iommu",
>> -    .unmigratable = 1
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .priority = MIG_PRI_IOMMU,
>> +    .fields = (VMStateField[]) {
>> +      /* Updated in  amdvi_handle_control_write() */
>> +      VMSTATE_BOOL(enabled, AMDVIState),
>
> no xtsup ?  I guess you are relying on the dest command line having xtsup=on
> like intel-iommu
>

Having said, I think I found a flaw here that sort of "ignores" the default
command line param of 'device-iotlb' (broad x86-iommu param). By default it
seems we enable device-iotlb in amd-iommu regardless, even though it's disabled
by default in qemu command line params.

Should we enable migration I think stuff like that starts to be important to
honor given the compability issues we would have to deal apriori. See below on
how to fix, happy to formally send if what I explained makes sense to all

-------->8---------

Subject: i386/amd-iommu: Set IotblSup based on x86_iommu::dt_supported

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9fcc2897b84a..f30e103d649b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2310,7 +2310,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker,
const char *oem_id,
     /* virtualization flags */
     build_append_int_noprefix(table_data,
                              (1UL << 0) | /* HtTunEn      */
-                             (1UL << 4) | /* iotblSup     */
+                             (s->iommu.dt_supported ?
+                              (1UL << 4) : 0) |  /* iotblSup     */
                              (1UL << 6) | /* PrefSup      */
                              (1UL << 7),  /* PPRSup       */
                              1);
@@ -2347,7 +2348,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker,
const char *oem_id,
     /* virtualization flags */
     build_append_int_noprefix(table_data,
                              (1UL << 0) | /* HtTunEn      */
-                             (1UL << 4),  /* iotblSup     */
+                             (s->iommu.dt_supported ?
+                              (1UL << 4) : 0),  /* iotblSup     */
                              1);

     /* IVHD length */
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 13af7211e11d..7e6140b9a0a2 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -77,6 +77,18 @@ typedef struct AMDVIIOTLBEntry {
     uint64_t page_mask;         /* physical page size  */
 } AMDVIIOTLBEntry;

+static uint64_t amdvi_capab_feature_register(AMDVIState *s)
+{
+    uint64_t feature = AMDVI_CAPAB_FEATURES;
+    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
+
+    if (x86_iommu->dt_supported) {
+        feature |= AMDVI_CAPAB_FLAG_IOTLBSUP;
+    }
+
+    return feature;
+}
+
 uint64_t amdvi_extended_feature_register(AMDVIState *s)
 {
     uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
@@ -1563,6 +1575,7 @@ static void amdvi_init(AMDVIState *s)
 static void amdvi_pci_realize(PCIDevice *pdev, Error **errp)
 {
     AMDVIPCIState *s = AMD_IOMMU_PCI(pdev);
+    AMDVIState *amdvi = container_of(s, AMDVIState, pci);
     int ret;

     ret = pci_add_capability(pdev, AMDVI_CAPAB_ID_SEC, 0,
@@ -1591,7 +1604,7 @@ static void amdvi_pci_realize(PCIDevice *pdev, Error 
**errp)
     pci_config_set_prog_interface(pdev->config, 0);

     /* reset AMDVI specific capabilities, all r/o */
-    pci_set_long(pdev->config + s->capab_offset, AMDVI_CAPAB_FEATURES);
+    pci_set_long(pdev->config + s->capab_offset,
amdvi_capab_feature_register(amdvi));
     pci_set_long(pdev->config + s->capab_offset + AMDVI_CAPAB_BAR_LOW,
                  AMDVI_BASE_ADDR & ~(0xffff0000));
     pci_set_long(pdev->config + s->capab_offset + AMDVI_CAPAB_BAR_HIGH,
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index e0dac4d9a96c..98e155549532 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -182,7 +182,7 @@

 /* capabilities header */
 #define AMDVI_CAPAB_FEATURES (AMDVI_CAPAB_FLAT_EXT | \
-        AMDVI_CAPAB_FLAG_NPCACHE | AMDVI_CAPAB_FLAG_IOTLBSUP \
+        AMDVI_CAPAB_FLAG_NPCACHE \
         | AMDVI_CAPAB_ID_SEC | AMDVI_CAPAB_INIT_TYPE | \
         AMDVI_CAPAB_FLAG_HTTUNNEL |  AMDVI_CAPAB_EFR_SUP)