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

From: Alex Williamson <alex.williamson@redhat.com>
To: Keqian Zhu <zhukeqian1@huawei.com>
Cc: <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<iommu@lists.linux-foundation.org>, <kvm@vger.kernel.org>,
	<kvmarm@lists.cs.columbia.edu>, Cornelia Huck <cohuck@redhat.com>,
	Will Deacon <will@kernel.org>, "Marc Zyngier" <maz@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	James Morse <james.morse@arm.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Joerg Roedel <joro@8bytes.org>,
	"Daniel Lezcano" <daniel.lezcano@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Alexios Zavras <alexios.zavras@intel.com>,
	<wanghaibin.wang@huawei.com>, <jiangkunkun@huawei.com>
Subject: Re: [PATCH 2/6] vfio/iommu_type1: Ignore external domain when promote pinned_scope
Date: Fri, 15 Jan 2021 16:23:59 -0700	[thread overview]
Message-ID: <20210115162359.749e8d0d@omen.home.shazbot.org> (raw)
In-Reply-To: <20210107044401.19828-3-zhukeqian1@huawei.com>

On Thu, 7 Jan 2021 12:43:57 +0800
Keqian Zhu <zhukeqian1@huawei.com> 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 <zhukeqian1@huawei.com>
> ---
>  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.