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

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

On 6/6/25 9:42 AM, Igor Mammedov wrote:
> On Thu,  1 May 2025 23:04:56 +0200
> Philippe Mathieu-Daudé <philmd@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-3c8c1b881e40@oracle.com/

[1] 
https://lore.kernel.org/qemu-devel/20250502021605.1795985-1-alejandro.j.jimenez@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)
> 
>