Re: [PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Yi Liu <yi.l.liu@intel.com>]

On 2024/10/21 20:33, Jason Gunthorpe wrote:
> On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote:
>> On 2024/10/18 22:39, Jason Gunthorpe wrote:
>>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote:
>>>> The iommu drivers are on the way to drop the remove_dev_pasid op by
>>>> extending the blocked_domain to support PASID. However, this cannot be
>>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
>>>> supported it, while the AMD iommu driver has not yet. During this
>>>> transition, the IOMMU core needs to support both ways to destroy the
>>>> attachment of device/PASID and domain.
>>>
>>> Let's just fix AMD?
>>
>> cool.
> 
> You could probably do better on this and fixup
> amd_iommu_remove_dev_pasid() to have the right signature directly,
> like the other drivers did

It might make sense to move the amd_iommu_remove_dev_pasid() to the
drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid().


diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index b11b014fa82d..55ac1ad10fb3 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -54,8 +54,8 @@ void amd_iommu_domain_free(struct iommu_domain *dom);
  int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
  			    struct device *dev, ioasid_t pasid,
  			    struct iommu_domain *old);
-void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-				struct iommu_domain *domain);
+void remove_pdom_dev_pasid(struct protection_domain *pdom,
+			   struct device *dev, ioasid_t pasid);

  /* SVA/PASID */
  bool amd_iommu_pasid_supported(void);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8364cd6fa47d..f807c4956a75 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2437,6 +2437,30 @@ static int blocked_domain_attach_device(struct 
iommu_domain *domain,
  	return 0;
  }

+static int blocked_domain_set_dev_pasid(struct iommu_domain *domain,
+					struct device *dev, ioasid_t pasid,
+					struct iommu_domain *old)
+{
+	struct protection_domain *pdom = to_pdomain(old);
+	unsigned long flags;
+
+	if (old->type != IOMMU_DOMAIN_SVA)
+		return -EINVAL;
+
+	if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
+		return 0;
+
+	pdom = to_pdomain(domain);
+
+	spin_lock_irqsave(&pdom->lock, flags);
+
+	/* Remove PASID from dev_data_list */
+	remove_pdom_dev_pasid(pdom, dev, pasid);
+
+	spin_unlock_irqrestore(&pdom->lock, flags);
+	return 0;
+}
+
  static struct iommu_domain blocked_domain = {
  	.type = IOMMU_DOMAIN_BLOCKED,
  	.ops = &(const struct iommu_domain_ops) {
diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
index 8c73a30c2800..c43c7286c872 100644
--- a/drivers/iommu/amd/pasid.c
+++ b/drivers/iommu/amd/pasid.c
@@ -39,8 +39,8 @@ static void remove_dev_pasid(struct pdom_dev_data 
*pdom_dev_data)
  }

  /* Clear PASID from device GCR3 table and remove pdom_dev_data from list */
-static void remove_pdom_dev_pasid(struct protection_domain *pdom,
-				  struct device *dev, ioasid_t pasid)
+void remove_pdom_dev_pasid(struct protection_domain *pdom,
+			   struct device *dev, ioasid_t pasid)
  {
  	struct pdom_dev_data *pdom_dev_data;
  	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
@@ -145,25 +145,6 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
  	return ret;
  }

-void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-				struct iommu_domain *domain)
-{
-	struct protection_domain *sva_pdom;
-	unsigned long flags;
-
-	if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
-		return;
-
-	sva_pdom = to_pdomain(domain);
-
-	spin_lock_irqsave(&sva_pdom->lock, flags);
-
-	/* Remove PASID from dev_data_list */
-	remove_pdom_dev_pasid(sva_pdom, dev, pasid);
-
-	spin_unlock_irqrestore(&sva_pdom->lock, flags);
-}
-
  static void iommu_sva_domain_free(struct iommu_domain *domain)
  {
  	struct protection_domain *sva_pdom = to_pdomain(domain);


-- 
Regards,
Yi Liu