Re: [PATCH v2] iommu/amd: Enhance def_domain_type to handle untrusted device

[Vasant Hegde <vashegde@amd.com>]

Hi Robin,


On 4/23/2024 4:56 PM, Robin Murphy wrote:
> On 23/04/2024 12:17 pm, Vasant Hegde wrote:
>> Previously, IOMMU core layer was forcing IOMMU_DOMAIN_DMA domain for
>> untrusted device. This always took precedence over driver's
>> def_domain_type(). Commit 59ddce4418da ("iommu: Reorganize
>> iommu_get_default_domain_type() to respect def_domain_type()") changed
>> the behaviour. Current code calls def_domain_type() but if it doesn't
>> return IOMMU_DOMAIN_DMA for untrusted device it throws error. This
>> results in IOMMU group (and potentially IOMMU itself) in undetermined
>> state.
>>
>> This patch adds untrusted check in AMD IOMMU driver code. So that it
>> allows eGPUs behind Thunderbolt work again.
> 
> Sorry, but I firmly disagree with this approach - regardless of what the AMD
> driver is currently doing and how appropriate it may or may not be, we should
> not be duplicating core code policy in drivers, and we certainly shouldn't be
> doing so to bodge around a breakage in how the core code implements said policy.


Ack. I still think fixing in core layer (what your patch does) is the right fix.


@Joerg,
  This is alternative to Robin's original patch [1]. If everyone agrees with
that fix then we don't need this patch.

[1]
https://lore.kernel.org/linux-iommu/fa14583e94cbf540b60a6be94b41bb24d0037e75.1713272443.git.robin.murphy@arm.com/


-Vasant


> 
> Thanks,
> Robin.
> 
>> Fine tuning amd_iommu_def_domain_type() will be done later.
>>
>> Reported-by: Eric Wagner <ewagner12@gmail.com>
>> Link:
>> https://lore.kernel.org/linux-iommu/CAHudX3zLH6CsRmLE-yb+gRjhh-v4bU5_1jW_xCcxOo_oUUZKYg@mail.gmail.com
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3182
>> Fixes: 59ddce4418da ("iommu: Reorganize iommu_get_default_domain_type() to
>> respect def_domain_type()")
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Jason Gunthorpe <jgg@ziepe.ca>
>> Cc: stable@kernel.org # v6.7+
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> Changes in v2:
>>    - Added dev_is_pci() check - Thanks Lu Baolu
>>
>> -Vasant
>>
>>   drivers/iommu/amd/iommu.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index e692217fcb28..fb727f5b0b82 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2754,6 +2754,10 @@ static int amd_iommu_def_domain_type(struct device *dev)
>>       if (!dev_data)
>>           return 0;
>>   +    /* Always use DMA domain for untrusted device */
>> +    if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
>> +        return IOMMU_DOMAIN_DMA;
>> +
>>       /*
>>        * Do not identity map IOMMUv2 capable devices when:
>>        *  - memory encryption is active, because some of those devices