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

On 7/23/26 11:23, Vasant Hegde wrote:
> 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) ?

Bascially; yeah.  But one comment 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) &&

I don't think you need to check for pci_upstream_bridge() to be 
non-NULL.  You already checked that it's an endpoint by looking at the 
class.  So an endpoint will be connected to a bridge of some sort 
(either a switch internal to the dGPU or to a root port).

> +           pci_upstream_bridge(pdev)->vendor == PCI_VENDOR_ID_ATI)
> +               return false;
> +

Make sure you leave a comment here about what true means (/* it is the 
GPU in an APU */) so that if we need to add more cases later it's 
obvious without having to dig up this thread again.

> +       return true;
> +}
> +
> 
> -Vasant
>