Re: [PATCH 05/18] amd_iommu: Toggle memory regions based on address translation mode

[Alejandro Jimenez]

On 4/22/25 8:17 AM, Sairaj Kodilkar wrote:


On 4/14/2025 7:32 AM, Alejandro Jimenez wrote:
Enable the appropriate memory region for an address space depending on
the address translation mode selected for it. This is currently based on
a generic x86 IOMMMU property, and only done during the address space
initialization. Extract the code into a helper and toggle the regions
based on whether DMA remapping is available as a global capability, and
if the specific address space is using address translation.

Signed-off-by: Alejandro Jimenez <alejandro.j.jime...@oracle.com>
---
  hw/i386/amd_iommu.c | 30 ++++++++++++++++++++----------
  1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index ad5869e72fdc..3f9aa2cc8d31 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1529,13 +1529,31 @@ static const MemoryRegionOps amdvi_ir_ops = {
      }
  };
+/*
+ * Toggle between address translation and passthrough modes by 
enabling the
+ * corresponding memory regions.
+ */
+static void amdvi_switch_address_space(AMDVIAddressSpace *amdvi_as)
+{
+    AMDVIState *s = amdvi_as->iommu_state;
+
+    if (s->dma_remap && amdvi_as->addr_translation) {

Hi Alejandro,

I know gnew0 initializes addr_translation to 0. but should we explicitly
initialize it to 0 ? in order to make it more readable.

I am generally in favor of making things more explicit, so I like this 
suggestion, and would also initialize the notifier_flags i.e.

@@ -2152,6 +2152,8 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus 
*bus, void *opaque, int devfn)
         iommu_as[devfn]->devfn = (uint8_t)devfn;
         iommu_as[devfn]->iommu_state = s;
         iommu_as[devfn]->iova_tree = iova_tree_new();
+        iommu_as[devfn]->addr_translation = false;
+        iommu_as[devfn]->notifier_flags = IOMMU_NONE;

         amdvi_dev_as = iommu_as[devfn];

It is possible that others consider these redundant or bad coding style, 
but unless there is pushback I'll include the changes in v2.

Thank you,
Alejandro

Regards
Sairaj Kodilkar

+        /* Enabling DMA region */
+        memory_region_set_enabled(&amdvi_as->iommu_nodma, false);
+        memory_region_set_enabled(MEMORY_REGION(&amdvi_as->iommu), 
true);
+    } else {
+        /* Disabling DMA region, using passthrough */
+        memory_region_set_enabled(MEMORY_REGION(&amdvi_as->iommu), 
false);
+        memory_region_set_enabled(&amdvi_as->iommu_nodma, true);
+    }
+}
+
  static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, 
int devfn)
  {
      char name[128];
      AMDVIState *s = opaque;
      AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
      int bus_num = pci_bus_num(bus);
-    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
      iommu_as = s->address_spaces[bus_num];
@@ -1595,15 +1613,7 @@ static AddressSpace 
*amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
                                              AMDVI_INT_ADDR_FIRST,
                                              &amdvi_dev_as->iommu_ir, 
1);
-        if (!x86_iommu->pt_supported) {
-            memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, 
false);
-            memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as- 
>iommu),
-                                      true);
-        } else {
-            memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as- 
>iommu),
-                                      false);
-            memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, true);
-        }
+        amdvi_switch_address_space(amdvi_dev_as);
      }
      return &iommu_as[devfn]->as;
  }