kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] vfio/iommu_type1: some fixes
@ 2021-01-15  9:26 Keqian Zhu
  2021-01-15  9:26 ` [PATCH v2 1/2] vfio/iommu_type1: Populate full dirty when detach non-pinned group Keqian Zhu
  2021-01-15  9:26 ` [PATCH v2 2/2] vfio/iommu_type1: Sanity check pfn_list when remove vfio_dma Keqian Zhu
  0 siblings, 2 replies; 9+ messages in thread
From: Keqian Zhu @ 2021-01-15  9:26 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, iommu, kvm, kvmarm,
	Alex Williamson, Kirti Wankhede, Cornelia Huck, Will Deacon,
	Marc Zyngier, Catalin Marinas
  Cc: Mark Rutland, James Morse, Robin Murphy, Joerg Roedel,
	Daniel Lezcano, Thomas Gleixner, Suzuki K Poulose,
	Julien Thierry, Andrew Morton, Alexios Zavras, wanghaibin.wang,
	jiangkunkun

changelog:

v2:
 - Address suggestions from Alex.
 - Remove unnecessary patches.

Keqian Zhu (2):
  vfio/iommu_type1: Populate full dirty when detach non-pinned group
  vfio/iommu_type1: Sanity check pfn_list when remove vfio_dma

 drivers/vfio/vfio_iommu_type1.c | 42 +++++++++++++++++----------------
 1 file changed, 22 insertions(+), 20 deletions(-)

-- 
2.19.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] vfio/iommu_type1: Populate full dirty when detach non-pinned group
  2021-01-15  9:26 [PATCH v2 0/2] vfio/iommu_type1: some fixes Keqian Zhu
@ 2021-01-15  9:26 ` Keqian Zhu
  2021-01-15 18:01   ` Alex Williamson
  2021-01-15  9:26 ` [PATCH v2 2/2] vfio/iommu_type1: Sanity check pfn_list when remove vfio_dma Keqian Zhu
  1 sibling, 1 reply; 9+ messages in thread
From: Keqian Zhu @ 2021-01-15  9:26 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, iommu, kvm, kvmarm,
	Alex Williamson, Kirti Wankhede, Cornelia Huck, Will Deacon,
	Marc Zyngier, Catalin Marinas
  Cc: Mark Rutland, James Morse, Robin Murphy, Joerg Roedel,
	Daniel Lezcano, Thomas Gleixner, Suzuki K Poulose,
	Julien Thierry, Andrew Morton, Alexios Zavras, wanghaibin.wang,
	jiangkunkun

If a group with non-pinned-page dirty scope is detached with dirty
logging enabled, we should fully populate the dirty bitmaps at the
time it's removed since we don't know the extent of its previous DMA,
nor will the group be present to trigger the full bitmap when the user
retrieves the dirty bitmap.

Fixes: d6a4c185660c ("vfio iommu: Implementation of ioctl for dirty pages tracking")
Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0b4dedaa9128..4e82b9a3440f 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -236,6 +236,19 @@ static void vfio_dma_populate_bitmap(struct vfio_dma *dma, size_t pgsize)
 	}
 }
 
+static void vfio_iommu_populate_bitmap_full(struct vfio_iommu *iommu)
+{
+	struct rb_node *n;
+	unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
+
+	for (n = rb_first(&iommu->dma_list); n; n = rb_next(n)) {
+		struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
+
+		if (dma->iommu_mapped)
+			bitmap_set(dma->bitmap, 0, dma->size >> pgshift);
+	}
+}
+
 static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
 {
 	struct rb_node *n;
@@ -2415,8 +2428,11 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 	 * Removal of a group without dirty tracking may allow the iommu scope
 	 * to be promoted.
 	 */
-	if (update_dirty_scope)
+	if (update_dirty_scope) {
 		update_pinned_page_dirty_scope(iommu);
+		if (iommu->dirty_page_tracking)
+			vfio_iommu_populate_bitmap_full(iommu);
+	}
 	mutex_unlock(&iommu->lock);
 }
 
-- 
2.19.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] vfio/iommu_type1: Sanity check pfn_list when remove vfio_dma
  2021-01-15  9:26 [PATCH v2 0/2] vfio/iommu_type1: some fixes Keqian Zhu
  2021-01-15  9:26 ` [PATCH v2 1/2] vfio/iommu_type1: Populate full dirty when detach non-pinned group Keqian Zhu
@ 2021-01-15  9:26 ` Keqian Zhu
  2021-01-15 19:14   ` Alex Williamson
  1 sibling, 1 reply; 9+ messages in thread
From: Keqian Zhu @ 2021-01-15  9:26 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, iommu, kvm, kvmarm,
	Alex Williamson, Kirti Wankhede, Cornelia Huck, Will Deacon,
	Marc Zyngier, Catalin Marinas
  Cc: Mark Rutland, James Morse, Robin Murphy, Joerg Roedel,
	Daniel Lezcano, Thomas Gleixner, Suzuki K Poulose,
	Julien Thierry, Andrew Morton, Alexios Zavras, wanghaibin.wang,
	jiangkunkun

vfio_sanity_check_pfn_list() is used to check whether pfn_list of
vfio_dma is empty when remove the external domain, so it makes a
wrong assumption that only external domain will add pfn to dma pfn_list.

Now we apply this check when remove a specific vfio_dma and extract
the notifier check just for external domain.

Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices")
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 4e82b9a3440f..a9bc15e84a4e 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -958,6 +958,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
 
 static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
 {
+	WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list);
 	vfio_unmap_unpin(iommu, dma, true);
 	vfio_unlink_dma(iommu, dma);
 	put_task_struct(dma->task);
@@ -2251,23 +2252,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
 	}
 }
 
-static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu)
-{
-	struct rb_node *n;
-
-	n = rb_first(&iommu->dma_list);
-	for (; n; n = rb_next(n)) {
-		struct vfio_dma *dma;
-
-		dma = rb_entry(n, struct vfio_dma, node);
-
-		if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)))
-			break;
-	}
-	/* mdev vendor driver must unregister notifier */
-	WARN_ON(iommu->notifier.head);
-}
-
 /*
  * Called when a domain is removed in detach. It is possible that
  * the removed domain decided the iova aperture window. Modify the
@@ -2367,7 +2351,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			kfree(group);
 
 			if (list_empty(&iommu->external_domain->group_list)) {
-				vfio_sanity_check_pfn_list(iommu);
+				/* mdev vendor driver must unregister notifier */
+				WARN_ON(iommu->notifier.head);
 
 				if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
 					vfio_iommu_unmap_unpin_all(iommu);
@@ -2491,7 +2476,8 @@ static void vfio_iommu_type1_release(void *iommu_data)
 
 	if (iommu->external_domain) {
 		vfio_release_domain(iommu->external_domain, true);
-		vfio_sanity_check_pfn_list(iommu);
+		/* mdev vendor driver must unregister notifier */
+		WARN_ON(iommu->notifier.head);
 		kfree(iommu->external_domain);
 	}
 
-- 
2.19.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] vfio/iommu_type1: Populate full dirty when detach non-pinned group
  2021-01-15  9:26 ` [PATCH v2 1/2] vfio/iommu_type1: Populate full dirty when detach non-pinned group Keqian Zhu
@ 2021-01-15 18:01   ` Alex Williamson
  2021-01-18 12:25     ` Keqian Zhu
  0 siblings, 1 reply; 9+ messages in thread
From: Alex Williamson @ 2021-01-15 18:01 UTC (permalink / raw)
  To: Keqian Zhu
  Cc: linux-kernel, linux-arm-kernel, iommu, kvm, kvmarm,
	Kirti Wankhede, Cornelia Huck, Will Deacon, Marc Zyngier,
	Catalin Marinas, Mark Rutland, James Morse, Robin Murphy,
	Joerg Roedel, Daniel Lezcano, Thomas Gleixner, Suzuki K Poulose,
	Julien Thierry, Andrew Morton, Alexios Zavras, wanghaibin.wang,
	jiangkunkun

On Fri, 15 Jan 2021 17:26:42 +0800
Keqian Zhu <zhukeqian1@huawei.com> wrote:

> If a group with non-pinned-page dirty scope is detached with dirty
> logging enabled, we should fully populate the dirty bitmaps at the
> time it's removed since we don't know the extent of its previous DMA,
> nor will the group be present to trigger the full bitmap when the user
> retrieves the dirty bitmap.
> 
> Fixes: d6a4c185660c ("vfio iommu: Implementation of ioctl for dirty pages tracking")
> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 0b4dedaa9128..4e82b9a3440f 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -236,6 +236,19 @@ static void vfio_dma_populate_bitmap(struct vfio_dma *dma, size_t pgsize)
>  	}
>  }
>  
> +static void vfio_iommu_populate_bitmap_full(struct vfio_iommu *iommu)
> +{
> +	struct rb_node *n;
> +	unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
> +
> +	for (n = rb_first(&iommu->dma_list); n; n = rb_next(n)) {
> +		struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
> +
> +		if (dma->iommu_mapped)
> +			bitmap_set(dma->bitmap, 0, dma->size >> pgshift);
> +	}
> +}
> +
>  static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
>  {
>  	struct rb_node *n;
> @@ -2415,8 +2428,11 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
>  	 * Removal of a group without dirty tracking may allow the iommu scope
>  	 * to be promoted.
>  	 */
> -	if (update_dirty_scope)
> +	if (update_dirty_scope) {
>  		update_pinned_page_dirty_scope(iommu);
> +		if (iommu->dirty_page_tracking)
> +			vfio_iommu_populate_bitmap_full(iommu);
> +	}
>  	mutex_unlock(&iommu->lock);
>  }
>  

This doesn't do the right thing.  This marks the bitmap dirty if:

 * The detached group dirty scope was not limited to pinned pages

 AND

 * Dirty tracking is enabled

 AND

 * The vfio_dma is *currently* (ie. after the detach) iommu_mapped

We need to mark the bitmap dirty based on whether the vfio_dma *was*
iommu_mapped by the group that is now detached.  Thanks,

Alex


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] vfio/iommu_type1: Sanity check pfn_list when remove vfio_dma
  2021-01-15  9:26 ` [PATCH v2 2/2] vfio/iommu_type1: Sanity check pfn_list when remove vfio_dma Keqian Zhu
@ 2021-01-15 19:14   ` Alex Williamson
  2021-01-18 13:16     ` Keqian Zhu
  0 siblings, 1 reply; 9+ messages in thread
From: Alex Williamson @ 2021-01-15 19:14 UTC (permalink / raw)
  To: Keqian Zhu
  Cc: linux-kernel, linux-arm-kernel, iommu, kvm, kvmarm,
	Kirti Wankhede, Cornelia Huck, Will Deacon, Marc Zyngier,
	Catalin Marinas, Mark Rutland, James Morse, Robin Murphy,
	Joerg Roedel, Daniel Lezcano, Thomas Gleixner, Suzuki K Poulose,
	Julien Thierry, Andrew Morton, Alexios Zavras, wanghaibin.wang,
	jiangkunkun

On Fri, 15 Jan 2021 17:26:43 +0800
Keqian Zhu <zhukeqian1@huawei.com> wrote:

> vfio_sanity_check_pfn_list() is used to check whether pfn_list of
> vfio_dma is empty when remove the external domain, so it makes a
> wrong assumption that only external domain will add pfn to dma pfn_list.
> 
> Now we apply this check when remove a specific vfio_dma and extract
> the notifier check just for external domain.

The page pinning interface is gated by having a notifier registered for
unmaps, therefore non-external domains would also need to register a
notifier.  There's currently no other way to add entries to the
pfn_list.  So if we allow pinning for such domains, then it's wrong to
WARN_ON() when the notifier list is not-empty when removing an external
domain.  Long term we should probably extend page {un}pinning for the
caller to pass their notifier to be validated against the notifier list
rather than just allowing page pinning if *any* notifier is registered.
Thanks,

Alex
 
> Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices")
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 4e82b9a3440f..a9bc15e84a4e 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -958,6 +958,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
>  
>  static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
>  {
> +	WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list);
>  	vfio_unmap_unpin(iommu, dma, true);
>  	vfio_unlink_dma(iommu, dma);
>  	put_task_struct(dma->task);
> @@ -2251,23 +2252,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
>  	}
>  }
>  
> -static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu)
> -{
> -	struct rb_node *n;
> -
> -	n = rb_first(&iommu->dma_list);
> -	for (; n; n = rb_next(n)) {
> -		struct vfio_dma *dma;
> -
> -		dma = rb_entry(n, struct vfio_dma, node);
> -
> -		if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)))
> -			break;
> -	}
> -	/* mdev vendor driver must unregister notifier */
> -	WARN_ON(iommu->notifier.head);
> -}
> -
>  /*
>   * Called when a domain is removed in detach. It is possible that
>   * the removed domain decided the iova aperture window. Modify the
> @@ -2367,7 +2351,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
>  			kfree(group);
>  
>  			if (list_empty(&iommu->external_domain->group_list)) {
> -				vfio_sanity_check_pfn_list(iommu);
> +				/* mdev vendor driver must unregister notifier */
> +				WARN_ON(iommu->notifier.head);
>  
>  				if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
>  					vfio_iommu_unmap_unpin_all(iommu);
> @@ -2491,7 +2476,8 @@ static void vfio_iommu_type1_release(void *iommu_data)
>  
>  	if (iommu->external_domain) {
>  		vfio_release_domain(iommu->external_domain, true);
> -		vfio_sanity_check_pfn_list(iommu);
> +		/* mdev vendor driver must unregister notifier */
> +		WARN_ON(iommu->notifier.head);
>  		kfree(iommu->external_domain);
>  	}
>  


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] vfio/iommu_type1: Populate full dirty when detach non-pinned group
  2021-01-15 18:01   ` Alex Williamson
@ 2021-01-18 12:25     ` Keqian Zhu
  2021-01-21 18:05       ` Alex Williamson
  0 siblings, 1 reply; 9+ messages in thread
From: Keqian Zhu @ 2021-01-18 12:25 UTC (permalink / raw)
  To: Alex Williamson
  Cc: linux-kernel, linux-arm-kernel, iommu, kvm, kvmarm,
	Kirti Wankhede, Cornelia Huck, Will Deacon, Marc Zyngier,
	Catalin Marinas, Mark Rutland, James Morse, Robin Murphy,
	Joerg Roedel, Daniel Lezcano, Thomas Gleixner, Suzuki K Poulose,
	Julien Thierry, Andrew Morton, Alexios Zavras, wanghaibin.wang,
	jiangkunkun



On 2021/1/16 2:01, Alex Williamson wrote:
> On Fri, 15 Jan 2021 17:26:42 +0800
> Keqian Zhu <zhukeqian1@huawei.com> wrote:
> 
>> If a group with non-pinned-page dirty scope is detached with dirty
>> logging enabled, we should fully populate the dirty bitmaps at the
>> time it's removed since we don't know the extent of its previous DMA,
>> nor will the group be present to trigger the full bitmap when the user
>> retrieves the dirty bitmap.
>>
>> Fixes: d6a4c185660c ("vfio iommu: Implementation of ioctl for dirty pages tracking")
>> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 0b4dedaa9128..4e82b9a3440f 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -236,6 +236,19 @@ static void vfio_dma_populate_bitmap(struct vfio_dma *dma, size_t pgsize)
>>  	}
>>  }
>>  
>> +static void vfio_iommu_populate_bitmap_full(struct vfio_iommu *iommu)
>> +{
>> +	struct rb_node *n;
>> +	unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
>> +
>> +	for (n = rb_first(&iommu->dma_list); n; n = rb_next(n)) {
>> +		struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
>> +
>> +		if (dma->iommu_mapped)
>> +			bitmap_set(dma->bitmap, 0, dma->size >> pgshift);
>> +	}
>> +}
>> +
>>  static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
>>  {
>>  	struct rb_node *n;
>> @@ -2415,8 +2428,11 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
>>  	 * Removal of a group without dirty tracking may allow the iommu scope
>>  	 * to be promoted.
>>  	 */
>> -	if (update_dirty_scope)
>> +	if (update_dirty_scope) {
>>  		update_pinned_page_dirty_scope(iommu);
>> +		if (iommu->dirty_page_tracking)
>> +			vfio_iommu_populate_bitmap_full(iommu);
>> +	}
>>  	mutex_unlock(&iommu->lock);
>>  }
>>  
> 
> This doesn't do the right thing.  This marks the bitmap dirty if:
> 
>  * The detached group dirty scope was not limited to pinned pages
> 
>  AND
> 
>  * Dirty tracking is enabled
> 
>  AND
> 
>  * The vfio_dma is *currently* (ie. after the detach) iommu_mapped
> 
> We need to mark the bitmap dirty based on whether the vfio_dma *was*
> iommu_mapped by the group that is now detached.  Thanks,
> 
> Alex
> 
Hi Alex,

Yes, I missed this point again :-(. The update_dirty_scope means we detached
an iommu backed group, and that means the vfio_dma *was* iommu_mapped by this
group, so we can populate full bitmap unconditionally, right?

Thanks,
Keqian


> .
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] vfio/iommu_type1: Sanity check pfn_list when remove vfio_dma
  2021-01-15 19:14   ` Alex Williamson
@ 2021-01-18 13:16     ` Keqian Zhu
  2021-01-21 18:14       ` Alex Williamson
  0 siblings, 1 reply; 9+ messages in thread
From: Keqian Zhu @ 2021-01-18 13:16 UTC (permalink / raw)
  To: Alex Williamson
  Cc: linux-kernel, linux-arm-kernel, iommu, kvm, kvmarm,
	Kirti Wankhede, Cornelia Huck, Will Deacon, Marc Zyngier,
	Catalin Marinas, Mark Rutland, James Morse, Robin Murphy,
	Joerg Roedel, Daniel Lezcano, Thomas Gleixner, Suzuki K Poulose,
	Julien Thierry, Andrew Morton, Alexios Zavras, wanghaibin.wang,
	jiangkunkun



On 2021/1/16 3:14, Alex Williamson wrote:
> On Fri, 15 Jan 2021 17:26:43 +0800
> Keqian Zhu <zhukeqian1@huawei.com> wrote:
> 
>> vfio_sanity_check_pfn_list() is used to check whether pfn_list of
>> vfio_dma is empty when remove the external domain, so it makes a
>> wrong assumption that only external domain will add pfn to dma pfn_list.
>>
>> Now we apply this check when remove a specific vfio_dma and extract
>> the notifier check just for external domain.
> 
> The page pinning interface is gated by having a notifier registered for
> unmaps, therefore non-external domains would also need to register a
> notifier.  There's currently no other way to add entries to the
> pfn_list.  So if we allow pinning for such domains, then it's wrong to
> WARN_ON() when the notifier list is not-empty when removing an external
> domain.  Long term we should probably extend page {un}pinning for the
> caller to pass their notifier to be validated against the notifier list
> rather than just allowing page pinning if *any* notifier is registered.
> Thanks,
I was misled by the code comments. So when the commit a54eb55045ae is added, the only
user of pin interface is mdev vendor driver, but now we also allow iommu backed group
to use this interface to constraint dirty scope. Is vfio_iommu_unmap_unpin_all() a
proper place to put this WARN()?

Thanks,
Keqian

> 
> Alex
>  
>> Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices")
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 24 +++++-------------------
>>  1 file changed, 5 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 4e82b9a3440f..a9bc15e84a4e 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -958,6 +958,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
>>  
>>  static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
>>  {
>> +	WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list);
>>  	vfio_unmap_unpin(iommu, dma, true);
>>  	vfio_unlink_dma(iommu, dma);
>>  	put_task_struct(dma->task);
>> @@ -2251,23 +2252,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
>>  	}
>>  }
>>  
>> -static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu)
>> -{
>> -	struct rb_node *n;
>> -
>> -	n = rb_first(&iommu->dma_list);
>> -	for (; n; n = rb_next(n)) {
>> -		struct vfio_dma *dma;
>> -
>> -		dma = rb_entry(n, struct vfio_dma, node);
>> -
>> -		if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)))
>> -			break;
>> -	}
>> -	/* mdev vendor driver must unregister notifier */
>> -	WARN_ON(iommu->notifier.head);
>> -}
>> -
>>  /*
>>   * Called when a domain is removed in detach. It is possible that
>>   * the removed domain decided the iova aperture window. Modify the
>> @@ -2367,7 +2351,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
>>  			kfree(group);
>>  
>>  			if (list_empty(&iommu->external_domain->group_list)) {
>> -				vfio_sanity_check_pfn_list(iommu);
>> +				/* mdev vendor driver must unregister notifier */
>> +				WARN_ON(iommu->notifier.head);
>>  
>>  				if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
>>  					vfio_iommu_unmap_unpin_all(iommu);
>> @@ -2491,7 +2476,8 @@ static void vfio_iommu_type1_release(void *iommu_data)
>>  
>>  	if (iommu->external_domain) {
>>  		vfio_release_domain(iommu->external_domain, true);
>> -		vfio_sanity_check_pfn_list(iommu);
>> +		/* mdev vendor driver must unregister notifier */
>> +		WARN_ON(iommu->notifier.head);
>>  		kfree(iommu->external_domain);
>>  	}
>>  
> 
> .
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] vfio/iommu_type1: Populate full dirty when detach non-pinned group
  2021-01-18 12:25     ` Keqian Zhu
@ 2021-01-21 18:05       ` Alex Williamson
  0 siblings, 0 replies; 9+ messages in thread
From: Alex Williamson @ 2021-01-21 18:05 UTC (permalink / raw)
  To: Keqian Zhu
  Cc: linux-kernel, linux-arm-kernel, iommu, kvm, kvmarm,
	Kirti Wankhede, Cornelia Huck, Will Deacon, Marc Zyngier,
	Catalin Marinas, Mark Rutland, James Morse, Robin Murphy,
	Joerg Roedel, Daniel Lezcano, Thomas Gleixner, Suzuki K Poulose,
	Julien Thierry, Andrew Morton, Alexios Zavras, wanghaibin.wang,
	jiangkunkun

On Mon, 18 Jan 2021 20:25:09 +0800
Keqian Zhu <zhukeqian1@huawei.com> wrote:

> On 2021/1/16 2:01, Alex Williamson wrote:
> > On Fri, 15 Jan 2021 17:26:42 +0800
> > Keqian Zhu <zhukeqian1@huawei.com> wrote:
> >   
> >> If a group with non-pinned-page dirty scope is detached with dirty
> >> logging enabled, we should fully populate the dirty bitmaps at the
> >> time it's removed since we don't know the extent of its previous DMA,
> >> nor will the group be present to trigger the full bitmap when the user
> >> retrieves the dirty bitmap.
> >>
> >> Fixes: d6a4c185660c ("vfio iommu: Implementation of ioctl for dirty pages tracking")
> >> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> >> ---
> >>  drivers/vfio/vfio_iommu_type1.c | 18 +++++++++++++++++-
> >>  1 file changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 0b4dedaa9128..4e82b9a3440f 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -236,6 +236,19 @@ static void vfio_dma_populate_bitmap(struct vfio_dma *dma, size_t pgsize)
> >>  	}
> >>  }
> >>  
> >> +static void vfio_iommu_populate_bitmap_full(struct vfio_iommu *iommu)
> >> +{
> >> +	struct rb_node *n;
> >> +	unsigned long pgshift = __ffs(iommu->pgsize_bitmap);
> >> +
> >> +	for (n = rb_first(&iommu->dma_list); n; n = rb_next(n)) {
> >> +		struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
> >> +
> >> +		if (dma->iommu_mapped)
> >> +			bitmap_set(dma->bitmap, 0, dma->size >> pgshift);
> >> +	}
> >> +}
> >> +
> >>  static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
> >>  {
> >>  	struct rb_node *n;
> >> @@ -2415,8 +2428,11 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
> >>  	 * Removal of a group without dirty tracking may allow the iommu scope
> >>  	 * to be promoted.
> >>  	 */
> >> -	if (update_dirty_scope)
> >> +	if (update_dirty_scope) {
> >>  		update_pinned_page_dirty_scope(iommu);
> >> +		if (iommu->dirty_page_tracking)
> >> +			vfio_iommu_populate_bitmap_full(iommu);
> >> +	}
> >>  	mutex_unlock(&iommu->lock);
> >>  }
> >>    
> > 
> > This doesn't do the right thing.  This marks the bitmap dirty if:
> > 
> >  * The detached group dirty scope was not limited to pinned pages
> > 
> >  AND
> > 
> >  * Dirty tracking is enabled
> > 
> >  AND
> > 
> >  * The vfio_dma is *currently* (ie. after the detach) iommu_mapped
> > 
> > We need to mark the bitmap dirty based on whether the vfio_dma *was*
> > iommu_mapped by the group that is now detached.  Thanks,
> > 
> > Alex
> >   
> Hi Alex,
> 
> Yes, I missed this point again :-(. The update_dirty_scope means we
> detached an iommu backed group, and that means the vfio_dma *was*
> iommu_mapped by this group, so we can populate full bitmap
> unconditionally, right?

To do it unconditionally, the assumption would be that all current
vfio_dmas are iommu_mapped.  It seems like it's deterministic that a
non-pinned-page scope group implies all vfio_dmas are iommu_mapped.  I
can't currently think of an exception.  Thanks,

Alex


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] vfio/iommu_type1: Sanity check pfn_list when remove vfio_dma
  2021-01-18 13:16     ` Keqian Zhu
@ 2021-01-21 18:14       ` Alex Williamson
  0 siblings, 0 replies; 9+ messages in thread
From: Alex Williamson @ 2021-01-21 18:14 UTC (permalink / raw)
  To: Keqian Zhu
  Cc: linux-kernel, linux-arm-kernel, iommu, kvm, kvmarm,
	Kirti Wankhede, Cornelia Huck, Will Deacon, Marc Zyngier,
	Catalin Marinas, Mark Rutland, James Morse, Robin Murphy,
	Joerg Roedel, Daniel Lezcano, Thomas Gleixner, Suzuki K Poulose,
	Julien Thierry, Andrew Morton, Alexios Zavras, wanghaibin.wang,
	jiangkunkun

On Mon, 18 Jan 2021 21:16:08 +0800
Keqian Zhu <zhukeqian1@huawei.com> wrote:

> On 2021/1/16 3:14, Alex Williamson wrote:
> > On Fri, 15 Jan 2021 17:26:43 +0800
> > Keqian Zhu <zhukeqian1@huawei.com> wrote:
> >   
> >> vfio_sanity_check_pfn_list() is used to check whether pfn_list of
> >> vfio_dma is empty when remove the external domain, so it makes a
> >> wrong assumption that only external domain will add pfn to dma pfn_list.
> >>
> >> Now we apply this check when remove a specific vfio_dma and extract
> >> the notifier check just for external domain.  
> > 
> > The page pinning interface is gated by having a notifier registered for
> > unmaps, therefore non-external domains would also need to register a
> > notifier.  There's currently no other way to add entries to the
> > pfn_list.  So if we allow pinning for such domains, then it's wrong to
> > WARN_ON() when the notifier list is not-empty when removing an external
> > domain.  Long term we should probably extend page {un}pinning for the
> > caller to pass their notifier to be validated against the notifier list
> > rather than just allowing page pinning if *any* notifier is registered.
> > Thanks,  
> I was misled by the code comments. So when the commit a54eb55045ae is
> added, the only user of pin interface is mdev vendor driver, but now
> we also allow iommu backed group to use this interface to constraint
> dirty scope. Is vfio_iommu_unmap_unpin_all() a proper place to put
> this WARN()?

vfio_iommu_unmap_unpin_all() deals with removing vfio_dmas, it's
logically unrelated to whether any driver is registered to receive
unmap notifications.  Thanks,

Alex


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-01-21 18:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-15  9:26 [PATCH v2 0/2] vfio/iommu_type1: some fixes Keqian Zhu
2021-01-15  9:26 ` [PATCH v2 1/2] vfio/iommu_type1: Populate full dirty when detach non-pinned group Keqian Zhu
2021-01-15 18:01   ` Alex Williamson
2021-01-18 12:25     ` Keqian Zhu
2021-01-21 18:05       ` Alex Williamson
2021-01-15  9:26 ` [PATCH v2 2/2] vfio/iommu_type1: Sanity check pfn_list when remove vfio_dma Keqian Zhu
2021-01-15 19:14   ` Alex Williamson
2021-01-18 13:16     ` Keqian Zhu
2021-01-21 18:14       ` Alex Williamson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).