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

[Vasant Hegde <vasant.hegde@amd.com>]

Hi Joao,

[+Baolu to get clarification on intel driver behaviour.]

On 4/24/2025 5:49 PM, Joao Martins wrote:
> On 23/04/2025 07:50, Ankit Soni wrote:
>> Current AMD IOMMU assumes Host Address Translation (HAT) is always
>> supported, and Linux kernel enables this capability by default. However,
>> in case of emulated and virtualized IOMMU, this might not be the case.
> 
> +Alejandro as he is filling that gap
> 
>> For example,current QEMU-emulated AMD vIOMMU does not support host
>> translation for VFIO pass-through device, but the interrupt remapping
>> support is required for x2APIC (i.e. kvm-msi-ext-dest-id is also not
>> supported by the guest OS). This would require the guest kernel to boot
>> with guest kernel option iommu=pt to by-pass the initialization of
>> host (v1) table.
>>
>> The AMD I/O Virtualization Technology (IOMMU) Specification Rev 3.10 [1]>
> introduces a new flag 'HATDis' in the IVHD 11h IOMMU attributes to indicate
>> that HAT is not supported on a particular IOMMU instance.
>>
>> Therefore, modifies the AMD IOMMU driver to detect the new HATDis
>> attributes, and disable host translation and switch to use guest
>> translation if it is available. Otherwise, the driver will disable DMA
>> translation.
>>
>> [1] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/specifications/48882_IOMMU.pdf
>>
>> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Ankit Soni <Ankit.Soni@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu.h       |  1 +
>>  drivers/iommu/amd/amd_iommu_types.h |  6 +++++-
>>  drivers/iommu/amd/init.c            | 23 +++++++++++++++++++++--
>>  drivers/iommu/amd/iommu.c           | 13 +++++++++++++
>>  4 files changed, 40 insertions(+), 3 deletions(-)
>>


.../...

>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index be8761bbef0f..0ebc264726da 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2393,6 +2393,13 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
>>  					     pci_max_pasids(to_pci_dev(dev)));
>>  	}
>>  
>> +	if (amd_iommu_pgtable == PD_MODE_NONE) {
>> +		pr_warn_once("%s: DMA translation not supported by iommu.\n",
>> +			     __func__);
>> +		iommu_dev = ERR_PTR(-ENODEV);
>> +		goto out_err;
>> +	}
>> +
>>  out_err:
>>  
>>  	iommu_completion_wait(iommu);
>> @@ -2480,6 +2487,9 @@ static int pdom_setup_pgtable(struct protection_domain *domain,
>>  	case PD_MODE_V2:
>>  		fmt = AMD_IOMMU_V2;
>>  		break;
>> +	case PD_MODE_NONE:
>> +		WARN_ON_ONCE(1);
>> +		return -EPERM;
>>  	}
>>  
>>  	domain->iop.pgtbl.cfg.amd.nid = dev_to_node(dev);
>> @@ -2501,6 +2511,9 @@ static inline u64 dma_max_address(enum protection_domain_mode pgtable)
>>  
>>  static bool amd_iommu_hd_support(struct amd_iommu *iommu)
>>  {
>> +	if (amd_iommu_hatdis)
>> +		return false;
>> +
>>  	return iommu && (iommu->features & FEATURE_HDSUP);
>>  }
>>  
> 
> It's strange we seem to somehow have host translation disabled, while it
> advertises other translation-related features like the normal case.

I think even if qemu advertises those features we shouldn't hit this code path
as probe() will fail and core shouldn't try to allocate domain, etc.

I was extra cautious and suggested to add this check!

> 
> In any case we should probably follow Intel's example (which does similar thing
> to HATSDis) where we only call invoke IOMMU groups
> iommu_device_register()/iommu_device_sysfs_add() with DMA translation enabled?
> That should simplify most of the patch as those codepaths are not reachable via
> kernel/userspace? Unless I am missing something ofc
> 
> See also commit c40aaaac10 ("iommu/vt-d: Gracefully handle DMAR units with no
> supported address widths"). I am not sure what else is the closest example here
> besides intel-iommu equivalent.

@Baolu,
  Looking into intel driver, my understanding is if DMA is not supported, intel
driver will set `drhd->ignored` to 1 (alloc_iommu()). So iommu_is_dummy() check
will return `true` and hence intel_iommu_probe_device() will fail. But it will
continue to support interrupt remapping. Is that the correct understanding?

-Vasant