Re: [PATCH 1/2] iommu/amd: Add HATDis feature support

[Joao Martins <joao.m.martins@oracle.com>]

On 06/05/2025 06:12, Ankit Soni wrote:
> On Wed, Apr 30, 2025 at 12:41:04PM +0100, Joao Martins wrote:
>>> With intel patch you mentioned above, it seems that it is mostly handling 
>>> "second stage translation support" disable, which will eventually disable dma 
>>> translation. And in AMD case, HATDis bit indicates host(v1) translation is not
>>> available, then attempt to use guest(v2) translation, and if both page
>>> table modes are not available then disable dma tranlation.
>>
>> OK, I guess it makes sense if HATDis is v1 only.
>>
>> My other call out was that when we disable dma-translation all together (aka
>> both modes), then we shouldn't advertise the IOMMU groups (internally and to
>> userspace) by not calling iommu_device_register()/iommu_device_sysfs_add().
>>
> 
> Sorry for the late reply. I had cross-checked it; if the probe fails,
> then IOMMU groups will not be populated, and eventually, it will not
> have significance for calling iommu_device_register()/iommu_device_sysfs_add().
> 

It would nonetheless populate a ivhd entry in sysfs needlessly but with an empty
devices list (qemu diff at the tail end for how I checked it; it's only missing
the ILLEGAL_DEVICE_TABLE_ENTRY event being generated, but enough to check the
first patch with sw iommu) e.g. as far as I checked:

$ find /sys | grep ivhd
/sys/class/iommu/ivhd0
/sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0
/sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/uevent
/sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/amd-iommu
/sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/amd-iommu/cap
/sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/amd-iommu/features
/sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/devices
/sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/device
/sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/subsystem

-->8--

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 59593582be23..78801672ee3d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2326,7 +2326,11 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker,
const char *oem_id,
     /* IOMMU info */
     build_append_int_noprefix(table_data, 0, 2);
     /* IOMMU Attributes */
-    build_append_int_noprefix(table_data, 0, 4);
+    if (!s->dma_translation) {
+        build_append_int_noprefix(table_data, (1UL << 0) /* HATDis */, 4);
+    } else {
+        build_append_int_noprefix(table_data, 0, 4);
+    }
     /* EFR Register Image */
     build_append_int_noprefix(table_data,
                               amdvi_extended_feature_register(s),
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 34abb61e092e..d033d309a210 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -83,6 +83,9 @@ uint64_t amdvi_extended_feature_register(AMDVIState *s)
     if (s->xtsup) {
         feature |= AMDVI_FEATURE_XT;
     }
+    if (!s->dma_translation) {
+        feature |= AMDVI_HATS_MODE_RESERVED;
+    }

     return feature;
 }
@@ -1665,6 +1668,7 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error
**errp)
 }

 static Property amdvi_properties[] = {
+    DEFINE_PROP_BOOL("dma-translation", AMDVIState, dma_translation, true),
     DEFINE_PROP_BOOL("xtsup", AMDVIState, xtsup, false),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index e0dac4d9a96c..b5358dc30533 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -169,6 +169,7 @@
 /* AMDVI paging mode */
 #define AMDVI_GATS_MODE                 (2ULL <<  12)
 #define AMDVI_HATS_MODE                 (2ULL <<  10)
+#define AMDVI_HATS_MODE_RESERVED        (2ULL <<  11)

 /* IOTLB */
 #define AMDVI_IOTLB_MAX_SIZE 1024
@@ -371,6 +372,7 @@ struct AMDVIState {
     /* Interrupt remapping */
     bool ga_enabled;
     bool xtsup;
+    bool dma_translation;
 };

 uint64_t amdvi_extended_feature_register(AMDVIState *s);