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

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

[Vasant Hegde]

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.

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
-- 
2.31.1

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

[Robin Murphy]

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.

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

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

[Vasant Hegde]

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

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

[Jason Gunthorpe]

On Wed, Apr 24, 2024 at 01:41:57PM +0530, Vasant Hegde wrote:
> 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, 

Then AMD should remove the PASID stuff from def_domain_type and we
need to fix it with properly designed core support for this limitation
before merging the SVA support.

> > and we certainly shouldn't be
> > doing so to bodge around a breakage in how the core code implements said policy.

It isn't breakage. It is evolution of the API.

Hacking in the AMD driver is temprary and Vasant will eventually fix
it. Regressing the core code to go back to allowing drivers to inform
policy is much harder to undo.

Jason

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

[Joerg Roedel]

On Tue, Apr 23, 2024 at 11:17:25AM +0000, Vasant Hegde wrote:
>  drivers/iommu/amd/iommu.c | 4 ++++
>  1 file changed, 4 insertions(+)

Since this fixes a regression I think it makes sense to apply as-is for
v6.9. Things can be done more properly later on-top of this fix.

So: Applied, thanks.