Re: [PATCH 18/18] hw/i386/x86-iommu: Remove X86IOMMUState::pt_supported field

[Alejandro Jimenez]

On 6/6/25 9:42 AM, Igor Mammedov wrote:
On Thu,  1 May 2025 23:04:56 +0200
Philippe Mathieu-Daudé <phi...@linaro.org> wrote:

@@ -1486,15 +1485,8 @@ 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);
-        }
+        memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
+        memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu), true);

given default is 'true', this hunk seems to be backwards,
shouldn't it be 'else' branch code


I believe this is trying to preserve the change recently introduced in:
31753d5a336f ("hw/i386/amd_iommu: Fix device setup failure when PT is on.")

However, that doesn't explain the logic of enabling those specific 
memory regions, which I think is also prone to cause confusion. I tried 
to explain the "trick" and argued against it here[0].

I am ultimately rewriting that code in a series to add DMA remap 
support[1], which hopefully makes it easier to follow which memory 
regions are active under specific configurations.

Thank you,
Alejandro

[0] 
https://lore.kernel.org/qemu-devel/914314b3-611d-4da3-9050-3c8c1b881...@oracle.com/

[1] 
https://lore.kernel.org/qemu-devel/20250502021605.1795985-1-alejandro.j.jime...@oracle.com/


      }
      return &iommu_as[devfn]->as;
  }
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c980cecb4ee..cc08dc41441 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1066,6 +1066,7 @@ static inline bool vtd_ce_type_check(X86IOMMUState 
*x86_iommu,
  {
      switch (vtd_ce_get_type(ce)) {
      case VTD_CONTEXT_TT_MULTI_LEVEL:
+    case VTD_CONTEXT_TT_PASS_THROUGH:
          /* Always supported */
          break;
      case VTD_CONTEXT_TT_DEV_IOTLB:
@@ -1074,12 +1075,6 @@ static inline bool vtd_ce_type_check(X86IOMMUState 
*x86_iommu,
              return false;
          }
          break;
-    case VTD_CONTEXT_TT_PASS_THROUGH:
-        if (!x86_iommu->pt_supported) {
-            error_report_once("%s: PT specified but not supported", __func__);
-            return false;
-        }
-        break;
      default:
          /* Unknown type */
          error_report_once("%s: unknown ce type: %"PRIu32, __func__,
@@ -4520,7 +4515,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
  {
      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
  
-    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
+    s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_ECAP_PT |
               VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS |
               VTD_CAP_MGAW(s->aw_bits);
      if (s->dma_drain) {
@@ -4548,10 +4543,6 @@ static void vtd_cap_init(IntelIOMMUState *s)
          s->ecap |= VTD_ECAP_DT;
      }
  
-    if (x86_iommu->pt_supported) {
-        s->ecap |= VTD_ECAP_PT;
-    }
-
      if (s->caching_mode) {
          s->cap |= VTD_CAP_CM;
      }
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index d34a6849f4a..ca7cd953e98 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -129,7 +129,6 @@ static const Property x86_iommu_properties[] = {
      DEFINE_PROP_ON_OFF_AUTO("intremap", X86IOMMUState,
                              intr_supported, ON_OFF_AUTO_AUTO),
      DEFINE_PROP_BOOL("device-iotlb", X86IOMMUState, dt_supported, false),
-    DEFINE_PROP_BOOL("pt", X86IOMMUState, pt_supported, true),
  };
  
  static void x86_iommu_class_init(ObjectClass *klass, const void *data)