1. Patchew
  2. linux
  3. [PATCH 0/5] Simplify vfio_iommu_type1 attach/detach routine
  4. View patch

[PATCH 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()

Nicolin Chen posted 5 patches 3 years, 8 months ago
There is a newer version of this series
[PATCH 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()
Posted by Nicolin Chen 3 years, 8 months ago 
All devices in emulated_iommu_groups have pinned_page_dirty_scope
set, so the update_dirty_scope in the first list_for_each_entry
is always false. Clean it up, and move the "if update_dirty_scope"
part from the detach_group_done routine to the domain_list part.

Rename the "detach_group_done" goto label accordingly.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index f4e3b423a453..b45b1cc118ef 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2463,14 +2463,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_domain *domain;
 	struct vfio_iommu_group *group;
-	bool update_dirty_scope = false;
 	LIST_HEAD(iova_copy);
 
 	mutex_lock(&iommu->lock);
 	list_for_each_entry(group, &iommu->emulated_iommu_groups, next) {
 		if (group->iommu_group != iommu_group)
 			continue;
-		update_dirty_scope = !group->pinned_page_dirty_scope;
 		list_del(&group->next);
 		kfree(group);
 
@@ -2479,7 +2477,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			WARN_ON(iommu->notifier.head);
 			vfio_iommu_unmap_unpin_all(iommu);
 		}
-		goto detach_group_done;
+		goto out_unlock;
 	}
 
 	/*
@@ -2495,9 +2493,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			continue;
 
 		iommu_detach_group(domain->domain, group->iommu_group);
-		update_dirty_scope = !group->pinned_page_dirty_scope;
 		list_del(&group->next);
-		kfree(group);
 		/*
 		 * Group ownership provides privilege, if the group list is
 		 * empty, the domain goes away. If it's the last domain with
@@ -2519,7 +2515,17 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			kfree(domain);
 			vfio_iommu_aper_expand(iommu, &iova_copy);
 			vfio_update_pgsize_bitmap(iommu);
+			/*
+			 * Removal of a group without dirty tracking may allow
+			 * the iommu scope to be promoted.
+			 */
+			if (!group->pinned_page_dirty_scope) {
+				iommu->num_non_pinned_groups--;
+				if (iommu->dirty_page_tracking)
+					vfio_iommu_populate_bitmap_full(iommu);
+			}
 		}
+		kfree(group);
 		break;
 	}
 
@@ -2528,16 +2534,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 	else
 		vfio_iommu_iova_free(&iova_copy);
 
-detach_group_done:
-	/*
-	 * Removal of a group without dirty tracking may allow the iommu scope
-	 * to be promoted.
-	 */
-	if (update_dirty_scope) {
-		iommu->num_non_pinned_groups--;
-		if (iommu->dirty_page_tracking)
-			vfio_iommu_populate_bitmap_full(iommu);
-	}
+out_unlock:
 	mutex_unlock(&iommu->lock);
 }
 
-- 
2.17.1
RE: [PATCH 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()
Posted by Tian, Kevin 3 years, 8 months ago 
> From: Nicolin Chen
> Sent: Monday, June 6, 2022 2:19 PM
> 
> All devices in emulated_iommu_groups have pinned_page_dirty_scope
> set, so the update_dirty_scope in the first list_for_each_entry
> is always false. Clean it up, and move the "if update_dirty_scope"
> part from the detach_group_done routine to the domain_list part.
> 
> Rename the "detach_group_done" goto label accordingly.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c
> index f4e3b423a453..b45b1cc118ef 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2463,14 +2463,12 @@ static void
> vfio_iommu_type1_detach_group(void *iommu_data,
>  	struct vfio_iommu *iommu = iommu_data;
>  	struct vfio_domain *domain;
>  	struct vfio_iommu_group *group;
> -	bool update_dirty_scope = false;
>  	LIST_HEAD(iova_copy);
> 
>  	mutex_lock(&iommu->lock);
>  	list_for_each_entry(group, &iommu->emulated_iommu_groups,
> next) {
>  		if (group->iommu_group != iommu_group)
>  			continue;
> -		update_dirty_scope = !group->pinned_page_dirty_scope;
>  		list_del(&group->next);
>  		kfree(group);
> 
> @@ -2479,7 +2477,7 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
>  			WARN_ON(iommu->notifier.head);
>  			vfio_iommu_unmap_unpin_all(iommu);
>  		}
> -		goto detach_group_done;
> +		goto out_unlock;
>  	}
> 
>  	/*
> @@ -2495,9 +2493,7 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
>  			continue;
> 
>  		iommu_detach_group(domain->domain, group-
> >iommu_group);
> -		update_dirty_scope = !group->pinned_page_dirty_scope;
>  		list_del(&group->next);
> -		kfree(group);
>  		/*
>  		 * Group ownership provides privilege, if the group list is
>  		 * empty, the domain goes away. If it's the last domain with
> @@ -2519,7 +2515,17 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
>  			kfree(domain);
>  			vfio_iommu_aper_expand(iommu, &iova_copy);
>  			vfio_update_pgsize_bitmap(iommu);
> +			/*
> +			 * Removal of a group without dirty tracking may
> allow
> +			 * the iommu scope to be promoted.
> +			 */
> +			if (!group->pinned_page_dirty_scope) {
> +				iommu->num_non_pinned_groups--;
> +				if (iommu->dirty_page_tracking)
> +
> 	vfio_iommu_populate_bitmap_full(iommu);

This doesn't look correct. The old code decrements
num_non_pinned_groups for every detach group without dirty
tracking. But now it's only done when the domain is about to
be released...

> +			}
>  		}
> +		kfree(group);
>  		break;
>  	}
> 
> @@ -2528,16 +2534,7 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
>  	else
>  		vfio_iommu_iova_free(&iova_copy);
> 
> -detach_group_done:
> -	/*
> -	 * Removal of a group without dirty tracking may allow the iommu
> scope
> -	 * to be promoted.
> -	 */
> -	if (update_dirty_scope) {
> -		iommu->num_non_pinned_groups--;
> -		if (iommu->dirty_page_tracking)
> -			vfio_iommu_populate_bitmap_full(iommu);
> -	}
> +out_unlock:
>  	mutex_unlock(&iommu->lock);
>  }
> 
> --
> 2.17.1
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/5] vfio/iommu_type1: Clean up update_dirty_scope in detach_group()
Posted by Nicolin Chen 3 years, 8 months ago 
On Wed, Jun 08, 2022 at 08:35:47AM +0000, Tian, Kevin wrote:

> > @@ -2519,7 +2515,17 @@ static void vfio_iommu_type1_detach_group(void
> > *iommu_data,
> >                       kfree(domain);
> >                       vfio_iommu_aper_expand(iommu, &iova_copy);
> >                       vfio_update_pgsize_bitmap(iommu);
> > +                     /*
> > +                      * Removal of a group without dirty tracking may
> > allow
> > +                      * the iommu scope to be promoted.
> > +                      */
> > +                     if (!group->pinned_page_dirty_scope) {
> > +                             iommu->num_non_pinned_groups--;
> > +                             if (iommu->dirty_page_tracking)
> > +
> >       vfio_iommu_populate_bitmap_full(iommu);
> 
> This doesn't look correct. The old code decrements
> num_non_pinned_groups for every detach group without dirty
> tracking. But now it's only done when the domain is about to
> be released...

Hmm..you are right. It should be placed outside:
		if (list_empty(&domain->group_list)) {
			...
		}
+		if (!group->pinned_page_dirty_scope) {
+			...
+		}

Will fix this and the same problem in PATCH-5 too.

Thanks!

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.