Re: [PATCH 2/2] iommu/amd: Force identity mode for selected GPUs only

On 7/23/26 10:59, Bjorn Helgaas wrote:
> On Thu, Jul 23, 2026 at 06:15:48AM +0000, Vasant Hegde wrote:
>> Certain AMD GPU's must always be in identity mode. Currently its enforced
>> using PASID check. It worked fine as most GPU's has PASID feature. But
>> this means, identity mode enforcement is done for all PASID capable devices.
> 
> I think it would be useful to know something about *why* these devices
> require identity mode.  And what happens without identity mode, i.e., is
> there a user-visible symptom that happens when the wrong mode is used?
> 
> Since the code doesn't test any feature bits, I assume it's because these
> devices have some hardware defect?
> 
> s/GPU's/GPUs/ (twice)
> s/Currently its/Currently it's/
> s/has PASID/have PASID/
> 
>> Previously it made sense as domain allocation API
>> (iommu_ops->domain_alloc()) was just passing domain type. So it couldn't
>> check device capability and decide best suited page table type (v1 or
>> v2). With recent enhancement to driver code, it uses
>> domain_alloc_paging_flags() ops for all paging mode domain allocation.
>> This can check device/flags and allocate best suited page table (v1 or v2).
>> Hence fix amd_iommu_def_domain_type() to force identity mapping for selected
>> GPUs only.
>>
>> With this change system booted with DMA translation mode will select:
>>   * Guest (v2) page table for PASID capable device
>>   * Host (v1) page table for non-PASID capable device
>>
>> Cc: Alex Deucher <[email protected]>
>> Cc: Mario Limonciello <[email protected]>
>> Signed-off-by: Vasant Hegde <[email protected]>
>> Tested-by: Amandeep Kaur Longia <[email protected]>
>> ---
>>   drivers/iommu/amd/iommu.c | 56 +++++++++++++++++++++++++++++++--------
>>   1 file changed, 45 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 563f9c2672d5..fe642e33c87d 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -3112,6 +3112,32 @@ static bool amd_iommu_is_attach_deferred(struct device *dev)
>>   	return dev_data->defer_attach;
>>   }
>>   
>> +static bool quirks_force_identity_mapping(struct pci_dev *pdev)
>> +{
>> +	struct pci_dev *root_port;
>> +	int class = pdev->class >> 8;
>> +
>> +	/* AMD GPU vendor ID */
>> +	if (pdev->vendor != PCI_VENDOR_ID_ATI)
>> +		return false;
>> +
>> +	/* GPU class */
>> +	if (class != PCI_CLASS_DISPLAY_VGA &&
>> +	    class != PCI_CLASS_DISPLAY_OTHER)
>> +		return false;
>> +
>> +	root_port = pcie_find_root_port(pdev);
>> +	if (!root_port)
>> +		return false;
>> +
>> +	/* If bridge vendor is not ATI then its APU and force IDENTITY mode */
> 
> s/its/it's/  "it's" == "it is"; "its" shows ownership
> 
>> +	if (root_port->vendor != PCI_VENDOR_ID_ATI)
>> +		return true;

This logic I believe is wrong.  You're trying to look at the parent of 
the display device (which is an internal PCIe switch for a dGPU).

You basically want a similar implementation to 
amdgpu_device_find_parent() which figures out first device outside of 
the dGPU.

>> +
>> +	/* Rest all are dGPUs and works fine with DMA mode */
>> +	return false;

Mostly for code flow, I think it would make sense the 
force_identity_mapping fallback is purely for APU.

IE something like this:

if (vendor != PCI_VENDOR_ATI)
	return false;
if (class != display)
	return false;
if (pci_upstream_bridge()->vendor == PCI_VENDOR_ATI)
	return false;

/* rest are APUs, force identity */
return true;

>> +}
>> +
>>   static int amd_iommu_def_domain_type(struct device *dev)
>>   {
>>   	struct iommu_dev_data *dev_data;
>> @@ -3120,20 +3146,28 @@ static int amd_iommu_def_domain_type(struct device *dev)
>>   	if (!dev_data)
>>   		return 0;
>>   
>> +	if (!dev_is_pci(dev))
>> +		return 0;
>> +
>>   	/* Always use DMA domain for untrusted device */
>> -	if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
>> +	if (to_pci_dev(dev)->untrusted)
>>   		return IOMMU_DOMAIN_DMA;
> 
> This seems to apply a different rule to untrusted (e.g., removable) devices
> based on the topology, not the device itself.  What if one of these devices
> is used as an eGPU and thus uses IOMMU_DOMAIN_DMA when it really needs
> IOMMU_DOMAIN_IDENTITY?

I believe the identity domain mapping is only needed for APUs, which 
can't be put into an eGPU enclosure.

> 
>> -	/*
>> -	 * Do not identity map IOMMUv2 capable devices when:
>> -	 *  - memory encryption is active, because some of those devices
>> -	 *    (AMD GPUs) don't have the encryption bit in their DMA-mask
>> -	 *    and require remapping.
>> -	 *  - SNP is enabled, because it prohibits DTE[Mode]=0.
>> -	 */
>> -	if (pdev_pasid_supported(dev_data) &&
>> -	    !cc_platform_has(CC_ATTR_MEM_ENCRYPT) &&
>> -	    !amd_iommu_snp_en) {
>> +	/* Apply device specific quirks */
>> +	if (quirks_force_identity_mapping(to_pci_dev(dev))) {
>> +
>> +		/*
>> +		 * When memory encryption is active, some of these devices
>> +		 * don't have the encryption bit in their DMA-mask and
>> +		 * require remapping.
>> +		 */
>> +		if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
>> +			return 0;
>> +
>> +		/* DTE[Mode]=0 is prohibited when SNP is enabled */
>> +		if (amd_iommu_snp_en)
>> +			return 0;
>> +
>>   		return IOMMU_DOMAIN_IDENTITY;
>>   	}
>>   
>> -- 
>> 2.31.1
>>