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

From: Baolu Lu <baolu.lu@linux.intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>,
	iommu@lists.linux-foundation.org,
	Nicolin Chen <nicolinc@nvidia.com>, Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Qian Cai <quic_qiancai@quicinc.com>
Subject: Re: [PATCH] iommu: iommu_group_claim_dma_owner() must always assign a domain
Date: Wed, 4 May 2022 19:48:58 +0800	[thread overview]
Message-ID: <f213ba6f-2cbc-e4b2-c510-16ef8ef0a337@linux.intel.com> (raw)
In-Reply-To: <0-v1-6e9d2d0a759d+11b-iommu_dma_block_jgg@nvidia.com>

Hi Jason,

On 2022/5/4 08:11, Jason Gunthorpe wrote:
> +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
> +	 * and detatch_dev().
>   	 */
> -	if (!group->default_domain || group->owner) {
> -		__iommu_group_for_each_dev(group, domain,
> +	if (!new_domain) {
> +		WARN_ON(!group->domain->ops->detach_dev);
> +		__iommu_group_for_each_dev(group, group->domain,
>   					   iommu_group_do_detach_device);
>   		group->domain = NULL;
> -		return;
> +		return 0;
>   	}
>   
> -	if (group->domain == group->default_domain)
> -		return;
> -
> -	/* Detach by re-attaching to the default domain */
> +	/*
> +	 * New drivers do not implement detach_dev, so changing the domain is
> +	 * done by calling attach on the new domain. Drivers should implement
> +	 * this so that DMA is always translated by either the new, old, or a
> +	 * blocking domain. DMA should never become untranslated.
> +	 *
> +	 * Note that this is called in error unwind paths, attaching to a
> +	 * domain that has already been attached cannot fail.
> +	 */
>   	ret = __iommu_group_for_each_dev(group, group->default_domain,
						^^^^^^^^^^^^^^^^^^^^^^

I suppose this should be @new_domain, right?

>   					 iommu_group_do_attach_device);
> -	if (ret != 0)
> -		WARN_ON(1);
> +	if (ret)
> +		return ret;
> +	group->domain = new_domain;
> +	return 0;
> +}

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu