| Message ID | 20220606061927.26049-5-nicolinc@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Simplify vfio_iommu_type1 attach/detach routine | expand |
> 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
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!
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); }
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(-)