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

[Robin Murphy <robin.murphy@arm.com>]

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