Re: [PATCH] iommu: iommu_group_claim_dma_owner() must always assign a domain

From: Nicolin Chen via iommu <iommu@lists.linux-foundation.org>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Will Deacon <will@kernel.org>,
	Qian Cai <quic_qiancai@quicinc.com>,
	iommu@lists.linux-foundation.org,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH] iommu: iommu_group_claim_dma_owner() must always assign a domain
Date: Wed, 4 May 2022 01:22:00 -0700	[thread overview]
Message-ID: <YnI3qKBIBR1RlN1A@Asurada-Nvidia> (raw)
In-Reply-To: <0-v1-6e9d2d0a759d+11b-iommu_dma_block_jgg@nvidia.com>

On Tue, May 03, 2022 at 09:11:02PM -0300, Jason Gunthorpe wrote:

> This is based on Robins draft here:
> 
> https://lore.kernel.org/linux-iommu/18831161-473f-e04f-4a81-1c7062ad192d@arm.com/
> 
> With some rework. I re-organized the call chains instead of introducing
> iommu_group_user_attached(), fixed a recursive locking for
> iommu_group_get_purgatory(), and made a proper commit message.
> 
> Still only compile tested, so RFCish.
> 
> Nicolin/Lu? What do you think, can you check it?

I am able to repro the issue on ARM64 and give this a quick try.
But the patch seems to need to include the following change too.

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 94d99768023c..9bb108d01baa 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2040,7 +2040,8 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 {
 	int ret;
 
-	if (group->domain && group->domain != group->default_domain)
+	if (group->domain && group->domain != group->default_domain &&
+	    group->domain != group->blocking_domain)
 		return -EBUSY;
 
 	ret = __iommu_group_for_each_dev(group, domain,

> @@ -2072,38 +2072,66 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>  	return 0;
>  }
>  
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group)
> +static int __iommu_group_attach_domain(struct iommu_group *group,
> +				       struct iommu_domain *new_domain)
>  {
>  	int ret;
>  
> +	if (group->domain == new_domain)
> +		return 0;
> +
>  	/*
> -	 * If the group has been claimed already, do not re-attach the default
> -	 * domain.
> +	 * A NULL domain means to call the detach_dev() op. New drivers should
> +	 * use a IOMMU_DOMAIN_IDENTITY domain instead of a NULL default_domain

an IOMMU_DOMAIN_IDENTITY?

Just a nit here. I will take a closer look at the change tomorrow.

Thanks
Nic
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
