Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5d353fe4-b642-992e-ef42-7d7f8b2edf7e@linux.intel.com>
Date:   Sun, 24 Jul 2022 17:13:35 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Cc:     baolu.lu@linux.intel.com, Joerg Roedel <joro@8bytes.org>,
        Christoph Hellwig <hch@infradead.org>,
        Kevin Tian <kevin.tian@intel.com>,
        Ashok Raj <ashok.raj@intel.com>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Jean-Philippe Brucker <jean-philippe@linaro.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Vinod Koul <vkoul@kernel.org>,
        Eric Auger <eric.auger@redhat.com>,
        Liu Yi L <yi.l.liu@intel.com>,
        Jacob jun Pan <jacob.jun.pan@intel.com>,
        Zhangfei Gao <zhangfei.gao@linaro.org>,
        Zhu Tony <tony.zhu@intel.com>, iommu@lists.linux.dev,
        linux-kernel@vger.kernel.org,
        Jean-Philippe Brucker <jean-philippe@linaro.org>
Subject: Re: [PATCH v10 04/12] iommu: Add attach/detach_dev_pasid iommu
 interface
Content-Language: en-US
To:     Jason Gunthorpe <jgg@nvidia.com>
References: <20220705050710.2887204-1-baolu.lu@linux.intel.com>
 <20220705050710.2887204-5-baolu.lu@linux.intel.com>
 <20220723141118.GD79279@nvidia.com>
From:   Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <20220723141118.GD79279@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/7/23 22:11, Jason Gunthorpe wrote:
>> +void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev,
>> +			       ioasid_t pasid)
>> +{
>> +	struct iommu_group *group = iommu_group_get(dev);
>> +
>> +	mutex_lock(&group->mutex);
>> +	domain->ops->block_dev_pasid(domain, dev, pasid);
> I still really this OP, it is nonsense to invoke 'block_dev_pasid' on
> a domain, it should be on the iommu ops and it should not take in a
> domain parameter. This is why I prefer we write it as
> 
> domain->ops->set_dev_pasid(group->blocking_domain, dev, pasid);
> 

I originally plan to refactor this after both Intel and ARM SMMUv3
drivers have real blocking domain supports. After revisiting this, it
seems that the only difficulty is how to check whether a domain is a
blocking domain. I am going to use below checking code:

+	/*
+	 * Detach the domain if a blocking domain is set. Check the
+	 * right domain type once the IOMMU driver supports a real
+	 * blocking domain.
+	 */
+	if (!domain || domain->type == IOMMU_DOMAIN_UNMANAGED) {

Does this work for you?

The incremental changes look like below:

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 6633c7b040b8..9f8748b51630 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -890,22 +890,23 @@ static int intel_svm_set_dev_pasid(struct 
iommu_domain *domain,
  	int ret = 0;

  	mutex_lock(&pasid_mutex);
-	sva = intel_svm_bind_mm(iommu, dev, mm);
-	if (IS_ERR(sva))
-		ret = PTR_ERR(sva);
+	/*
+	 * Detach the domain if a blocking domain is set. Check the
+	 * right domain type once the IOMMU driver supports a real
+	 * blocking domain.
+	 */
+	if (!domain || domain->type == IOMMU_DOMAIN_UNMANAGED) {
+		intel_svm_unbind_mm(dev, pasid);
+	} else {
+		sva = intel_svm_bind_mm(iommu, dev, mm);
+		if (IS_ERR(sva))
+			ret = PTR_ERR(sva);
+	}
  	mutex_unlock(&pasid_mutex);

  	return ret;
  }

-static void intel_svm_block_dev_pasid(struct iommu_domain *domain,
-				      struct device *dev, ioasid_t pasid)
-{
-	mutex_lock(&pasid_mutex);
-	intel_svm_unbind_mm(dev, pasid);
-	mutex_unlock(&pasid_mutex);
-}
-
  static void intel_svm_domain_free(struct iommu_domain *domain)
  {
  	kfree(to_dmar_domain(domain));
@@ -913,7 +914,6 @@ static void intel_svm_domain_free(struct 
iommu_domain *domain)

  static const struct iommu_domain_ops intel_svm_domain_ops = {
  	.set_dev_pasid		= intel_svm_set_dev_pasid,
-	.block_dev_pasid	= intel_svm_block_dev_pasid,
  	.free			= intel_svm_domain_free,
  };

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index efe6a58eee48..a7f7a611fcce 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3262,7 +3262,7 @@ void iommu_detach_device_pasid(struct iommu_domain 
*domain, struct device *dev,
  	struct iommu_group *group = iommu_group_get(dev);

  	mutex_lock(&group->mutex);
-	domain->ops->block_dev_pasid(domain, dev, pasid);
+	domain->ops->set_dev_pasid(group->blocking_domain, dev, pasid);
  	WARN_ON(xa_erase(&group->pasid_array, pasid) != domain);
  	mutex_unlock(&group->mutex);

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ba6543f4c6a2..c52dccb86460 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -283,7 +283,6 @@ struct iommu_ops {
   * @attach_dev: attach an iommu domain to a device
   * @detach_dev: detach an iommu domain from a device
   * @set_dev_pasid: set an iommu domain to a pasid of device
- * @block_dev_pasid: block pasid of device from using iommu domain
   * @map: map a physically contiguous memory region to an iommu domain
   * @map_pages: map a physically contiguous set of pages of the same 
size to
   *             an iommu domain.
@@ -306,8 +305,6 @@ struct iommu_domain_ops {
  	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
  	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
  			     ioasid_t pasid);
-	void (*block_dev_pasid)(struct iommu_domain *domain, struct device *dev,
-				ioasid_t pasid);

  	int (*map)(struct iommu_domain *domain, unsigned long iova,
  		   phys_addr_t paddr, size_t size, int prot, gfp_t gfp);

Best regards,
baolu