2021-01-07 04:48:18

by Keqian Zhu

Subject: [PATCH 2/6] vfio/iommu_type1: Ignore external domain when promote pinned_scope

The pinned_scope of external domain's groups are always true, that's
to say we can safely ignore external domain when promote pinned_scope
status of vfio_iommu.

Signed-off-by: Keqian Zhu <[email protected]>
---
drivers/vfio/vfio_iommu_type1.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 334a8240e1da..110ada24ee91 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1637,14 +1637,7 @@ static void promote_pinned_page_dirty_scope(struct vfio_iommu *iommu)
}
}

- if (iommu->external_domain) {
- domain = iommu->external_domain;
- list_for_each_entry(group, &domain->group_list, next) {
- if (!group->pinned_page_dirty_scope)
- return;
- }
- }
-
+ /* The external domain always passes check */
iommu->pinned_page_dirty_scope = true;
}

@@ -2347,7 +2340,6 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
if (iommu->external_domain) {
group = find_iommu_group(iommu->external_domain, iommu_group);
if (group) {
- promote_dirty_scope = !group->pinned_page_dirty_scope;
list_del(&group->next);
kfree(group);

@@ -2360,7 +2352,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
kfree(iommu->external_domain);
iommu->external_domain = NULL;
}
- goto detach_group_done;
+ mutex_unlock(&iommu->lock);
+ return;
}
}

@@ -2408,7 +2401,6 @@ 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.
--
2.19.1

2021-01-15 23:28:50

by Alex Williamson

Subject: Re: [PATCH 2/6] vfio/iommu_type1: Ignore external domain when promote pinned_scope

On Thu, 7 Jan 2021 12:43:57 +0800
Keqian Zhu <[email protected]> wrote:

> The pinned_scope of external domain's groups are always true, that's
> to say we can safely ignore external domain when promote pinned_scope
> status of vfio_iommu.
>
> Signed-off-by: Keqian Zhu <[email protected]>
> ---
> drivers/vfio/vfio_iommu_type1.c | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 334a8240e1da..110ada24ee91 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1637,14 +1637,7 @@ static void promote_pinned_page_dirty_scope(struct vfio_iommu *iommu)
> }
> }
>
> - if (iommu->external_domain) {
> - domain = iommu->external_domain;
> - list_for_each_entry(group, &domain->group_list, next) {
> - if (!group->pinned_page_dirty_scope)
> - return;
> - }
> - }
> -
> + /* The external domain always passes check */
> iommu->pinned_page_dirty_scope = true;
> }
>
> @@ -2347,7 +2340,6 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
> if (iommu->external_domain) {
> group = find_iommu_group(iommu->external_domain, iommu_group);
> if (group) {
> - promote_dirty_scope = !group->pinned_page_dirty_scope;


With this, vfio_group.pinned_page_dirty_scope is effectively a dead
field on the struct for groups on the external_domain group list and
handled specially. That's not great.

If you actually want to make more than a trivial improvement to scope
tracking, what about making a counter on our struct vfio_iommu for all
the non-pinned-page (ie. all-dma) scope groups attached to the
container. Groups on the external domain would still set their group
dirty scope to pinned pages, groups making use of an iommu domain would
have an all-dma scope initially and increment that counter when
attached. Groups that still have an all-dma scope on detach would
decrement the counter. If a group changes from all-dma to pinned-page
scope, the counter is also decremented. We'd never need to search
across group lists. Thanks,

Alex

> list_del(&group->next);
> kfree(group);
>
> @@ -2360,7 +2352,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
> kfree(iommu->external_domain);
> iommu->external_domain = NULL;
> }
> - goto detach_group_done;
> + mutex_unlock(&iommu->lock);
> + return;
> }
> }
>
> @@ -2408,7 +2401,6 @@ 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.