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

Mario,


On 7/23/2026 9:41 PM, Mario Limonciello wrote:
> 
> 
> 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 are right. It got inverted while I was fine tuning the code.

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

Ack.

> 
>>> +
>>> +    /* 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;

Makes sense. And pci_upstream_bridge() return NULL, then return true? (something
like below) ?

+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;
+
+       if (pci_upstream_bridge(pdev) &&
+           pci_upstream_bridge(pdev)->vendor == PCI_VENDOR_ID_ATI)
+               return false;
+
+       return true;
+}
+

-Vasant