[iommufd 0/2] Make mdev driver dma_unmap callback tolerant to unmaps come before device open

[iommufd 0/2] Make mdev driver dma_unmap callback tolerant to unmaps come before device open

[Yi Liu]

Jason's "Connect VFIO to IOMMUFD" introduces vfio iommufd compat mode. Under
this mode, vfio_iommufd_bind() creates an access which has an unmap callback,
which can be called immediately. This means mdev drivers may receive unmap
requests before the mdev is opened. For now, most dma_unmap() callbacks are
tolerant with such unmap requests, except for gvt-g and vfio-ap. This series
tries to enhance the two drivers.

This series is based on Jason's below branch.

https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/log/?h=for-next
(commit: 57f62422b6f0477afaddd2fc77a4bb9b94275f42)

Cc: Tony Krowiak <akrowiak@linux.ibm.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Jason Herne <jjherne@linux.ibm.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>

Regards,
	Yi Liu

Matthew Rosato (1):
  vfio/ap: validate iova during dma_unmap and trigger irq disable

Yi Liu (1):
  i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into
    init

 drivers/gpu/drm/i915/gvt/gvt.h    |  2 ++
 drivers/gpu/drm/i915/gvt/kvmgt.c  |  7 ++-----
 drivers/gpu/drm/i915/gvt/vgpu.c   |  2 ++
 drivers/s390/crypto/vfio_ap_ops.c | 24 +++++++++++++++++++++++-
 4 files changed, 29 insertions(+), 6 deletions(-)

-- 
2.34.1

[iommufd 1/2] i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into init

[Yi Liu]

vfio_iommufd_bind() creates an access which has an unmap callback, which
can be called immediately. So dma_unmap() callback should tolerate the
unmaps that come before the emulated device is opened.

To achieve above, move the protect_table_init and gvt_cache_init into the
init op which is supposed to be triggered prior to the open_device() op.

Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/gpu/drm/i915/gvt/gvt.h   | 2 ++
 drivers/gpu/drm/i915/gvt/kvmgt.c | 7 ++-----
 drivers/gpu/drm/i915/gvt/vgpu.c  | 2 ++
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index dbf8d7470b2c..a3a7e16078ba 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -754,6 +754,8 @@ void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu);
 void intel_gvt_debugfs_init(struct intel_gvt *gvt);
 void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
 
+void gvt_cache_init(struct intel_vgpu *vgpu);
+void kvmgt_protect_table_init(struct intel_vgpu *info);
 int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn);
 int intel_gvt_page_track_remove(struct intel_vgpu *info, u64 gfn);
 int intel_gvt_dma_pin_guest_page(struct intel_vgpu *vgpu, dma_addr_t dma_addr);
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 579b230a0f58..cb21b1ba4162 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -322,7 +322,7 @@ static void gvt_cache_destroy(struct intel_vgpu *vgpu)
 	}
 }
 
-static void gvt_cache_init(struct intel_vgpu *vgpu)
+void gvt_cache_init(struct intel_vgpu *vgpu)
 {
 	vgpu->gfn_cache = RB_ROOT;
 	vgpu->dma_addr_cache = RB_ROOT;
@@ -330,7 +330,7 @@ static void gvt_cache_init(struct intel_vgpu *vgpu)
 	mutex_init(&vgpu->cache_lock);
 }
 
-static void kvmgt_protect_table_init(struct intel_vgpu *info)
+void kvmgt_protect_table_init(struct intel_vgpu *info)
 {
 	hash_init(info->ptable);
 }
@@ -671,9 +671,6 @@ static int intel_vgpu_open_device(struct vfio_device *vfio_dev)
 
 	vgpu->attached = true;
 
-	kvmgt_protect_table_init(vgpu);
-	gvt_cache_init(vgpu);
-
 	vgpu->track_node.track_write = kvmgt_page_track_write;
 	vgpu->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
 	kvm_page_track_register_notifier(vgpu->vfio_device.kvm,
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index 56c71474008a..036e1a72a26b 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -382,6 +382,8 @@ int intel_gvt_create_vgpu(struct intel_vgpu *vgpu,
 
 	intel_gvt_update_reg_whitelist(vgpu);
 	mutex_unlock(&gvt->lock);
+	kvmgt_protect_table_init(vgpu);
+	gvt_cache_init(vgpu);
 	return 0;
 
 out_clean_sched_policy:
-- 
2.34.1

[iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

[Yi Liu]

From: Matthew Rosato <mjrosato@linux.ibm.com>

vfio_iommufd_bind() creates an access which has an unmap callback, which
can be called immediately. So dma_unmap() callback should tolerate the
unmaps that come before the emulated device is opened.

To achieve above, vfio_ap_mdev_dma_unmap() needs to validate that unmap
request matches with one or more of these stashed values before
attempting unpins.

Currently, each mapped iova is stashed in its associated vfio_ap_queue;
Each stashed iova represents IRQ that was enabled for a queue. Therefore,
if a match is found, trigger IRQ disable for this queue to ensure that
underlying firmware will no longer try to use the associated pfn after
the page is unpinned. IRQ disable will also handle the associated unpin.

Cc: Tony Krowiak <akrowiak@linux.ibm.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Jason Herne <jjherne@linux.ibm.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index bb7776d20792..62bfca2bbe6d 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1535,13 +1535,35 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 	return 0;
 }
 
+static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova, u64 length)
+{
+	struct ap_queue_table *qtable = &matrix_mdev->qtable;
+	u64 iova_pfn_end = (iova + length - 1) >> PAGE_SHIFT;
+	u64 iova_pfn_start = iova >> PAGE_SHIFT;
+	struct vfio_ap_queue *q;
+	int loop_cursor;
+	u64 pfn;
+
+	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
+		pfn = q->saved_iova >> PAGE_SHIFT;
+		if (pfn >= iova_pfn_start && pfn <= iova_pfn_end) {
+			vfio_ap_irq_disable(q);
+			break;
+		}
+	}
+}
+
 static void vfio_ap_mdev_dma_unmap(struct vfio_device *vdev, u64 iova,
 				   u64 length)
 {
 	struct ap_matrix_mdev *matrix_mdev =
 		container_of(vdev, struct ap_matrix_mdev, vdev);
 
-	vfio_unpin_pages(&matrix_mdev->vdev, iova, 1);
+	mutex_lock(&matrix_dev->mdevs_lock);
+
+	unmap_iova(matrix_mdev, iova, length);
+
+	mutex_unlock(&matrix_dev->mdevs_lock);
 }
 
 /**
-- 
2.34.1

RE: [iommufd 0/2] Make mdev driver dma_unmap callback tolerant to unmaps come before device open

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 23, 2022 9:49 PM
> 
> Jason's "Connect VFIO to IOMMUFD" introduces vfio iommufd compat mode.
> Under
> this mode, vfio_iommufd_bind() creates an access which has an unmap
> callback,
> which can be called immediately. This means mdev drivers may receive
> unmap
> requests before the mdev is opened. For now, most dma_unmap() callbacks
> are
> tolerant with such unmap requests, except for gvt-g and vfio-ap. This series
> tries to enhance the two drivers.
> 

there are only three drivers (gvt, vfio-ap and vfio-ccw) providing
dma_unmap(). 

I'd not call the situation where only one driver is OK as "most
dma_unmap() callbacks are tolerant". 

RE: [iommufd 1/2] i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into init

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 23, 2022 9:49 PM
> 
> vfio_iommufd_bind() creates an access which has an unmap callback, which
> can be called immediately. So dma_unmap() callback should tolerate the
> unmaps that come before the emulated device is opened.

this should first talk about how it works today and then why iommufd changes
it.

> 
> To achieve above, move the protect_table_init and gvt_cache_init into the
> init op which is supposed to be triggered prior to the open_device() op.

what about below?
--
vfio container registers .dma_unmap() callback after the device is opened.
So it's fine for mdev drivers to initialize internal mapping cache in
.open_device(). See vfio_device_container_register().

Now with iommufd an access ops with an unmap callback is registered
when the device is bound to iommufd which is before .open_device()
is called. This implies gvt's .dma_unmap() could be called before its
internal mapping cache is initialized.

The fix is moving gvt mapping cache initialization to vGPU creation.
While at it also move ptable initialization together.

> 
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/gvt.h   | 2 ++
>  drivers/gpu/drm/i915/gvt/kvmgt.c | 7 ++-----
>  drivers/gpu/drm/i915/gvt/vgpu.c  | 2 ++
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index dbf8d7470b2c..a3a7e16078ba 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -754,6 +754,8 @@ void intel_gvt_debugfs_remove_vgpu(struct
> intel_vgpu *vgpu);
>  void intel_gvt_debugfs_init(struct intel_gvt *gvt);
>  void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
> 
> +void gvt_cache_init(struct intel_vgpu *vgpu);
> +void kvmgt_protect_table_init(struct intel_vgpu *info);
>  int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn);
>  int intel_gvt_page_track_remove(struct intel_vgpu *info, u64 gfn);
>  int intel_gvt_dma_pin_guest_page(struct intel_vgpu *vgpu, dma_addr_t
> dma_addr);
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 579b230a0f58..cb21b1ba4162 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -322,7 +322,7 @@ static void gvt_cache_destroy(struct intel_vgpu *vgpu)
>  	}
>  }
> 
> -static void gvt_cache_init(struct intel_vgpu *vgpu)
> +void gvt_cache_init(struct intel_vgpu *vgpu)

those are local functions. Just move to vgpu.c.

or you can remove the function wrap and directly put the internal lines
in intel_gvt_create_vgpu()

>  {
>  	vgpu->gfn_cache = RB_ROOT;
>  	vgpu->dma_addr_cache = RB_ROOT;
> @@ -330,7 +330,7 @@ static void gvt_cache_init(struct intel_vgpu *vgpu)
>  	mutex_init(&vgpu->cache_lock);
>  }
> 
> -static void kvmgt_protect_table_init(struct intel_vgpu *info)
> +void kvmgt_protect_table_init(struct intel_vgpu *info)
>  {
>  	hash_init(info->ptable);
>  }
> @@ -671,9 +671,6 @@ static int intel_vgpu_open_device(struct vfio_device
> *vfio_dev)
> 
>  	vgpu->attached = true;
> 
> -	kvmgt_protect_table_init(vgpu);
> -	gvt_cache_init(vgpu);
> -
>  	vgpu->track_node.track_write = kvmgt_page_track_write;
>  	vgpu->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
>  	kvm_page_track_register_notifier(vgpu->vfio_device.kvm,
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c
> b/drivers/gpu/drm/i915/gvt/vgpu.c
> index 56c71474008a..036e1a72a26b 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -382,6 +382,8 @@ int intel_gvt_create_vgpu(struct intel_vgpu *vgpu,
> 
>  	intel_gvt_update_reg_whitelist(vgpu);
>  	mutex_unlock(&gvt->lock);
> +	kvmgt_protect_table_init(vgpu);
> +	gvt_cache_init(vgpu);
>  	return 0;
> 
>  out_clean_sched_policy:
> --
> 2.34.1

RE: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 23, 2022 9:49 PM
> 
> From: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> vfio_iommufd_bind() creates an access which has an unmap callback, which
> can be called immediately. So dma_unmap() callback should tolerate the
> unmaps that come before the emulated device is opened.
> 
> To achieve above, vfio_ap_mdev_dma_unmap() needs to validate that
> unmap
> request matches with one or more of these stashed values before
> attempting unpins.
> 
> Currently, each mapped iova is stashed in its associated vfio_ap_queue;
> Each stashed iova represents IRQ that was enabled for a queue. Therefore,
> if a match is found, trigger IRQ disable for this queue to ensure that
> underlying firmware will no longer try to use the associated pfn after
> the page is unpinned. IRQ disable will also handle the associated unpin.
> 
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Jason Herne <jjherne@linux.ibm.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> b/drivers/s390/crypto/vfio_ap_ops.c
> index bb7776d20792..62bfca2bbe6d 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1535,13 +1535,35 @@ static int vfio_ap_mdev_set_kvm(struct
> ap_matrix_mdev *matrix_mdev,
>  	return 0;
>  }
> 
> +static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova,
> u64 length)
> +{
> +	struct ap_queue_table *qtable = &matrix_mdev->qtable;
> +	u64 iova_pfn_end = (iova + length - 1) >> PAGE_SHIFT;
> +	u64 iova_pfn_start = iova >> PAGE_SHIFT;
> +	struct vfio_ap_queue *q;
> +	int loop_cursor;
> +	u64 pfn;
> +
> +	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
> +		pfn = q->saved_iova >> PAGE_SHIFT;
> +		if (pfn >= iova_pfn_start && pfn <= iova_pfn_end) {
> +			vfio_ap_irq_disable(q);
> +			break;

does this need a WARN_ON if the length is more than one page?

> +		}
> +	}
> +}
> +
>  static void vfio_ap_mdev_dma_unmap(struct vfio_device *vdev, u64 iova,
>  				   u64 length)
>  {
>  	struct ap_matrix_mdev *matrix_mdev =
>  		container_of(vdev, struct ap_matrix_mdev, vdev);
> 
> -	vfio_unpin_pages(&matrix_mdev->vdev, iova, 1);
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +
> +	unmap_iova(matrix_mdev, iova, length);
> +
> +	mutex_unlock(&matrix_dev->mdevs_lock);
>  }
> 
>  /**
> --
> 2.34.1

Re: [iommufd 1/2] i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into init

[Yi Liu]

On 2022/11/24 15:07, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, November 23, 2022 9:49 PM
>>
>> vfio_iommufd_bind() creates an access which has an unmap callback, which
>> can be called immediately. So dma_unmap() callback should tolerate the
>> unmaps that come before the emulated device is opened.
> 
> this should first talk about how it works today and then why iommufd changes
> it.
> 
>>
>> To achieve above, move the protect_table_init and gvt_cache_init into the
>> init op which is supposed to be triggered prior to the open_device() op.
> 
> what about below?
> --
> vfio container registers .dma_unmap() callback after the device is opened.
> So it's fine for mdev drivers to initialize internal mapping cache in
> .open_device(). See vfio_device_container_register().
> 
> Now with iommufd an access ops with an unmap callback is registered
> when the device is bound to iommufd which is before .open_device()
> is called. This implies gvt's .dma_unmap() could be called before its
> internal mapping cache is initialized.
> 
> The fix is moving gvt mapping cache initialization to vGPU creation.
> While at it also move ptable initialization together.

much clearer :-)

>>
>> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
>> Cc: Zhi Wang <zhi.a.wang@intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/gvt.h   | 2 ++
>>   drivers/gpu/drm/i915/gvt/kvmgt.c | 7 ++-----
>>   drivers/gpu/drm/i915/gvt/vgpu.c  | 2 ++
>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>> index dbf8d7470b2c..a3a7e16078ba 100644
>> --- a/drivers/gpu/drm/i915/gvt/gvt.h
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>> @@ -754,6 +754,8 @@ void intel_gvt_debugfs_remove_vgpu(struct
>> intel_vgpu *vgpu);
>>   void intel_gvt_debugfs_init(struct intel_gvt *gvt);
>>   void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
>>
>> +void gvt_cache_init(struct intel_vgpu *vgpu);
>> +void kvmgt_protect_table_init(struct intel_vgpu *info);
>>   int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn);
>>   int intel_gvt_page_track_remove(struct intel_vgpu *info, u64 gfn);
>>   int intel_gvt_dma_pin_guest_page(struct intel_vgpu *vgpu, dma_addr_t
>> dma_addr);
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index 579b230a0f58..cb21b1ba4162 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -322,7 +322,7 @@ static void gvt_cache_destroy(struct intel_vgpu *vgpu)
>>   	}
>>   }
>>
>> -static void gvt_cache_init(struct intel_vgpu *vgpu)
>> +void gvt_cache_init(struct intel_vgpu *vgpu)
> 
> those are local functions. Just move to vgpu.c.
> 
> or you can remove the function wrap and directly put the internal lines
> in intel_gvt_create_vgpu()

yes. maybe see Zhenyu and Zhi's input. which way is preferred by them.

>>   {
>>   	vgpu->gfn_cache = RB_ROOT;
>>   	vgpu->dma_addr_cache = RB_ROOT;
>> @@ -330,7 +330,7 @@ static void gvt_cache_init(struct intel_vgpu *vgpu)
>>   	mutex_init(&vgpu->cache_lock);
>>   }
>>
>> -static void kvmgt_protect_table_init(struct intel_vgpu *info)
>> +void kvmgt_protect_table_init(struct intel_vgpu *info)
>>   {
>>   	hash_init(info->ptable);
>>   }
>> @@ -671,9 +671,6 @@ static int intel_vgpu_open_device(struct vfio_device
>> *vfio_dev)
>>
>>   	vgpu->attached = true;
>>
>> -	kvmgt_protect_table_init(vgpu);
>> -	gvt_cache_init(vgpu);
>> -
>>   	vgpu->track_node.track_write = kvmgt_page_track_write;
>>   	vgpu->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
>>   	kvm_page_track_register_notifier(vgpu->vfio_device.kvm,
>> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c
>> b/drivers/gpu/drm/i915/gvt/vgpu.c
>> index 56c71474008a..036e1a72a26b 100644
>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
>> @@ -382,6 +382,8 @@ int intel_gvt_create_vgpu(struct intel_vgpu *vgpu,
>>
>>   	intel_gvt_update_reg_whitelist(vgpu);
>>   	mutex_unlock(&gvt->lock);
>> +	kvmgt_protect_table_init(vgpu);
>> +	gvt_cache_init(vgpu);
>>   	return 0;
>>
>>   out_clean_sched_policy:
>> --
>> 2.34.1
> 

-- 
Regards,
Yi Liu

Re: [iommufd 0/2] Make mdev driver dma_unmap callback tolerant to unmaps come before device open

[Yi Liu]

On 2022/11/24 14:50, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, November 23, 2022 9:49 PM
>>
>> Jason's "Connect VFIO to IOMMUFD" introduces vfio iommufd compat mode.
>> Under
>> this mode, vfio_iommufd_bind() creates an access which has an unmap
>> callback,
>> which can be called immediately. This means mdev drivers may receive
>> unmap
>> requests before the mdev is opened. For now, most dma_unmap() callbacks
>> are
>> tolerant with such unmap requests, except for gvt-g and vfio-ap. This series
>> tries to enhance the two drivers.
>>
> 
> there are only three drivers (gvt, vfio-ap and vfio-ccw) providing
> dma_unmap().
> 
> I'd not call the situation where only one driver is OK as "most
> dma_unmap() callbacks are tolerant".

oh, yes. will refine it.

-- 
Regards,
Yi Liu

Re: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

[Jason Gunthorpe]

On Thu, Nov 24, 2022 at 07:08:06AM +0000, Tian, Kevin wrote:
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Wednesday, November 23, 2022 9:49 PM
> > 
> > From: Matthew Rosato <mjrosato@linux.ibm.com>
> > 
> > vfio_iommufd_bind() creates an access which has an unmap callback, which
> > can be called immediately. So dma_unmap() callback should tolerate the
> > unmaps that come before the emulated device is opened.
> > 
> > To achieve above, vfio_ap_mdev_dma_unmap() needs to validate that
> > unmap
> > request matches with one or more of these stashed values before
> > attempting unpins.
> > 
> > Currently, each mapped iova is stashed in its associated vfio_ap_queue;
> > Each stashed iova represents IRQ that was enabled for a queue. Therefore,
> > if a match is found, trigger IRQ disable for this queue to ensure that
> > underlying firmware will no longer try to use the associated pfn after
> > the page is unpinned. IRQ disable will also handle the associated unpin.
> > 
> > Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> > Cc: Halil Pasic <pasic@linux.ibm.com>
> > Cc: Jason Herne <jjherne@linux.ibm.com>
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >  drivers/s390/crypto/vfio_ap_ops.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> > b/drivers/s390/crypto/vfio_ap_ops.c
> > index bb7776d20792..62bfca2bbe6d 100644
> > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > @@ -1535,13 +1535,35 @@ static int vfio_ap_mdev_set_kvm(struct
> > ap_matrix_mdev *matrix_mdev,
> >  	return 0;
> >  }
> > 
> > +static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova,
> > u64 length)
> > +{
> > +	struct ap_queue_table *qtable = &matrix_mdev->qtable;
> > +	u64 iova_pfn_end = (iova + length - 1) >> PAGE_SHIFT;
> > +	u64 iova_pfn_start = iova >> PAGE_SHIFT;
> > +	struct vfio_ap_queue *q;
> > +	int loop_cursor;
> > +	u64 pfn;
> > +
> > +	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
> > +		pfn = q->saved_iova >> PAGE_SHIFT;
> > +		if (pfn >= iova_pfn_start && pfn <= iova_pfn_end) {
> > +			vfio_ap_irq_disable(q);
> > +			break;
> 
> does this need a WARN_ON if the length is more than one page?

The iova and length are the range being invalidated, the driver has no
control over them and length is probably multiple pages.

But this test doesn't look right?

   if (iova > q->saved_iova && q->saved_iova < iova + length)

Since the page was pinned we can assume iova and length are already
PAGE_SIZE aligned.

Jason

Re: [iommufd 1/2] i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into init

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 4997 bytes --]

On 2022.11.24 17:15:12 +0800, Yi Liu wrote:
> On 2022/11/24 15:07, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Wednesday, November 23, 2022 9:49 PM
> > > 
> > > vfio_iommufd_bind() creates an access which has an unmap callback, which
> > > can be called immediately. So dma_unmap() callback should tolerate the
> > > unmaps that come before the emulated device is opened.
> > 
> > this should first talk about how it works today and then why iommufd changes
> > it.
> > 
> > > 
> > > To achieve above, move the protect_table_init and gvt_cache_init into the
> > > init op which is supposed to be triggered prior to the open_device() op.
> > 
> > what about below?
> > --
> > vfio container registers .dma_unmap() callback after the device is opened.
> > So it's fine for mdev drivers to initialize internal mapping cache in
> > .open_device(). See vfio_device_container_register().
> > 
> > Now with iommufd an access ops with an unmap callback is registered
> > when the device is bound to iommufd which is before .open_device()
> > is called. This implies gvt's .dma_unmap() could be called before its
> > internal mapping cache is initialized.
> > 
> > The fix is moving gvt mapping cache initialization to vGPU creation.
> > While at it also move ptable initialization together.
> 
> much clearer :-)
>

Current gvt internal cache is handled with .open_device() and .close_device() pair,
so those internal cache is now re-initialized for each device session, how is that
handled for iommufd? Looks that's missed in this patch..

> > > 
> > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > Cc: Zhi Wang <zhi.a.wang@intel.com>
> > > Cc: Kevin Tian <kevin.tian@intel.com>
> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gvt/gvt.h   | 2 ++
> > >   drivers/gpu/drm/i915/gvt/kvmgt.c | 7 ++-----
> > >   drivers/gpu/drm/i915/gvt/vgpu.c  | 2 ++
> > >   3 files changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> > > index dbf8d7470b2c..a3a7e16078ba 100644
> > > --- a/drivers/gpu/drm/i915/gvt/gvt.h
> > > +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> > > @@ -754,6 +754,8 @@ void intel_gvt_debugfs_remove_vgpu(struct
> > > intel_vgpu *vgpu);
> > >   void intel_gvt_debugfs_init(struct intel_gvt *gvt);
> > >   void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
> > > 
> > > +void gvt_cache_init(struct intel_vgpu *vgpu);
> > > +void kvmgt_protect_table_init(struct intel_vgpu *info);
> > >   int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn);
> > >   int intel_gvt_page_track_remove(struct intel_vgpu *info, u64 gfn);
> > >   int intel_gvt_dma_pin_guest_page(struct intel_vgpu *vgpu, dma_addr_t
> > > dma_addr);
> > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > index 579b230a0f58..cb21b1ba4162 100644
> > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > @@ -322,7 +322,7 @@ static void gvt_cache_destroy(struct intel_vgpu *vgpu)
> > >   	}
> > >   }
> > > 
> > > -static void gvt_cache_init(struct intel_vgpu *vgpu)
> > > +void gvt_cache_init(struct intel_vgpu *vgpu)
> > 
> > those are local functions. Just move to vgpu.c.
> > 
> > or you can remove the function wrap and directly put the internal lines
> > in intel_gvt_create_vgpu()
> 
> yes. maybe see Zhenyu and Zhi's input. which way is preferred by them.
> 
> > >   {
> > >   	vgpu->gfn_cache = RB_ROOT;
> > >   	vgpu->dma_addr_cache = RB_ROOT;
> > > @@ -330,7 +330,7 @@ static void gvt_cache_init(struct intel_vgpu *vgpu)
> > >   	mutex_init(&vgpu->cache_lock);
> > >   }
> > > 
> > > -static void kvmgt_protect_table_init(struct intel_vgpu *info)
> > > +void kvmgt_protect_table_init(struct intel_vgpu *info)
> > >   {
> > >   	hash_init(info->ptable);
> > >   }
> > > @@ -671,9 +671,6 @@ static int intel_vgpu_open_device(struct vfio_device
> > > *vfio_dev)
> > > 
> > >   	vgpu->attached = true;
> > > 
> > > -	kvmgt_protect_table_init(vgpu);
> > > -	gvt_cache_init(vgpu);
> > > -
> > >   	vgpu->track_node.track_write = kvmgt_page_track_write;
> > >   	vgpu->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
> > >   	kvm_page_track_register_notifier(vgpu->vfio_device.kvm,
> > > diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c
> > > b/drivers/gpu/drm/i915/gvt/vgpu.c
> > > index 56c71474008a..036e1a72a26b 100644
> > > --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> > > +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> > > @@ -382,6 +382,8 @@ int intel_gvt_create_vgpu(struct intel_vgpu *vgpu,
> > > 
> > >   	intel_gvt_update_reg_whitelist(vgpu);
> > >   	mutex_unlock(&gvt->lock);
> > > +	kvmgt_protect_table_init(vgpu);
> > > +	gvt_cache_init(vgpu);
> > >   	return 0;
> > > 
> > >   out_clean_sched_policy:
> > > --
> > > 2.34.1
> > 
> 
> -- 
> Regards,
> Yi Liu

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [iommufd 0/2] Make mdev driver dma_unmap callback tolerant to unmaps come before device open

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

On 2022.11.23 05:48:30 -0800, Yi Liu wrote:
> Jason's "Connect VFIO to IOMMUFD" introduces vfio iommufd compat mode. Under
> this mode, vfio_iommufd_bind() creates an access which has an unmap callback,
> which can be called immediately. This means mdev drivers may receive unmap
> requests before the mdev is opened. For now, most dma_unmap() callbacks are
> tolerant with such unmap requests, except for gvt-g and vfio-ap. This series
> tries to enhance the two drivers.
> 
> This series is based on Jason's below branch.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/log/?h=for-next
> (commit: 57f62422b6f0477afaddd2fc77a4bb9b94275f42)
> 
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Jason Herne <jjherne@linux.ibm.com>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> 
> Regards,
> 	Yi Liu
> 
> Matthew Rosato (1):
>   vfio/ap: validate iova during dma_unmap and trigger irq disable
> 
> Yi Liu (1):
>   i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into
>     init

btw, for gvt change, pls at least add intel-gvt-dev@lists.freedesktop.org in cc.

thanks

> 
>  drivers/gpu/drm/i915/gvt/gvt.h    |  2 ++
>  drivers/gpu/drm/i915/gvt/kvmgt.c  |  7 ++-----
>  drivers/gpu/drm/i915/gvt/vgpu.c   |  2 ++
>  drivers/s390/crypto/vfio_ap_ops.c | 24 +++++++++++++++++++++++-
>  4 files changed, 29 insertions(+), 6 deletions(-)
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [iommufd 0/2] Make mdev driver dma_unmap callback tolerant to unmaps come before device open

[Yi Liu]

On 2022/11/25 14:06, Zhenyu Wang wrote:
> On 2022.11.23 05:48:30 -0800, Yi Liu wrote:
>> Jason's "Connect VFIO to IOMMUFD" introduces vfio iommufd compat mode. Under
>> this mode, vfio_iommufd_bind() creates an access which has an unmap callback,
>> which can be called immediately. This means mdev drivers may receive unmap
>> requests before the mdev is opened. For now, most dma_unmap() callbacks are
>> tolerant with such unmap requests, except for gvt-g and vfio-ap. This series
>> tries to enhance the two drivers.
>>
>> This series is based on Jason's below branch.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/log/?h=for-next
>> (commit: 57f62422b6f0477afaddd2fc77a4bb9b94275f42)
>>
>> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
>> Cc: Halil Pasic <pasic@linux.ibm.com>
>> Cc: Jason Herne <jjherne@linux.ibm.com>
>> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
>> Cc: Zhi Wang <zhi.a.wang@intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>>
>> Regards,
>> 	Yi Liu
>>
>> Matthew Rosato (1):
>>    vfio/ap: validate iova during dma_unmap and trigger irq disable
>>
>> Yi Liu (1):
>>    i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into
>>      init
> 
> btw, for gvt change, pls at least add intel-gvt-dev@lists.freedesktop.org in cc.

sure. :-)

-- 
Regards,
Yi Liu

Re: [iommufd 1/2] i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into init

[Yi Liu]

On 2022/11/25 14:04, Zhenyu Wang wrote:
> On 2022.11.24 17:15:12 +0800, Yi Liu wrote:
>> On 2022/11/24 15:07, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Wednesday, November 23, 2022 9:49 PM
>>>>
>>>> vfio_iommufd_bind() creates an access which has an unmap callback, which
>>>> can be called immediately. So dma_unmap() callback should tolerate the
>>>> unmaps that come before the emulated device is opened.
>>>
>>> this should first talk about how it works today and then why iommufd changes
>>> it.
>>>
>>>>
>>>> To achieve above, move the protect_table_init and gvt_cache_init into the
>>>> init op which is supposed to be triggered prior to the open_device() op.
>>>
>>> what about below?
>>> --
>>> vfio container registers .dma_unmap() callback after the device is opened.
>>> So it's fine for mdev drivers to initialize internal mapping cache in
>>> .open_device(). See vfio_device_container_register().
>>>
>>> Now with iommufd an access ops with an unmap callback is registered
>>> when the device is bound to iommufd which is before .open_device()
>>> is called. This implies gvt's .dma_unmap() could be called before its
>>> internal mapping cache is initialized.
>>>
>>> The fix is moving gvt mapping cache initialization to vGPU creation.
>>> While at it also move ptable initialization together.
>>
>> much clearer :-)
>>
> 
> Current gvt internal cache is handled with .open_device() and .close_device() pair,
> so those internal cache is now re-initialized for each device session, how is that
> handled for iommufd? Looks that's missed in this patch..

you are right. I noticed below two helpers are used to destroy. However, 
the code seems to be more clear the internal cache. So seems no need to
re-initialize. I'm no expert here. :)

gvt_cache_destroy()
kvmgt_protect_table_destroy()

>>>>
>>>> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
>>>> Cc: Zhi Wang <zhi.a.wang@intel.com>
>>>> Cc: Kevin Tian <kevin.tian@intel.com>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gvt/gvt.h   | 2 ++
>>>>    drivers/gpu/drm/i915/gvt/kvmgt.c | 7 ++-----
>>>>    drivers/gpu/drm/i915/gvt/vgpu.c  | 2 ++
>>>>    3 files changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>>>> index dbf8d7470b2c..a3a7e16078ba 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/gvt.h
>>>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>>>> @@ -754,6 +754,8 @@ void intel_gvt_debugfs_remove_vgpu(struct
>>>> intel_vgpu *vgpu);
>>>>    void intel_gvt_debugfs_init(struct intel_gvt *gvt);
>>>>    void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
>>>>
>>>> +void gvt_cache_init(struct intel_vgpu *vgpu);
>>>> +void kvmgt_protect_table_init(struct intel_vgpu *info);
>>>>    int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn);
>>>>    int intel_gvt_page_track_remove(struct intel_vgpu *info, u64 gfn);
>>>>    int intel_gvt_dma_pin_guest_page(struct intel_vgpu *vgpu, dma_addr_t
>>>> dma_addr);
>>>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>> b/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>> index 579b230a0f58..cb21b1ba4162 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>> @@ -322,7 +322,7 @@ static void gvt_cache_destroy(struct intel_vgpu *vgpu)
>>>>    	}
>>>>    }
>>>>
>>>> -static void gvt_cache_init(struct intel_vgpu *vgpu)
>>>> +void gvt_cache_init(struct intel_vgpu *vgpu)
>>>
>>> those are local functions. Just move to vgpu.c.
>>>
>>> or you can remove the function wrap and directly put the internal lines
>>> in intel_gvt_create_vgpu()
>>
>> yes. maybe see Zhenyu and Zhi's input. which way is preferred by them.
>>
>>>>    {
>>>>    	vgpu->gfn_cache = RB_ROOT;
>>>>    	vgpu->dma_addr_cache = RB_ROOT;
>>>> @@ -330,7 +330,7 @@ static void gvt_cache_init(struct intel_vgpu *vgpu)
>>>>    	mutex_init(&vgpu->cache_lock);
>>>>    }
>>>>
>>>> -static void kvmgt_protect_table_init(struct intel_vgpu *info)
>>>> +void kvmgt_protect_table_init(struct intel_vgpu *info)
>>>>    {
>>>>    	hash_init(info->ptable);
>>>>    }
>>>> @@ -671,9 +671,6 @@ static int intel_vgpu_open_device(struct vfio_device
>>>> *vfio_dev)
>>>>
>>>>    	vgpu->attached = true;
>>>>
>>>> -	kvmgt_protect_table_init(vgpu);
>>>> -	gvt_cache_init(vgpu);
>>>> -
>>>>    	vgpu->track_node.track_write = kvmgt_page_track_write;
>>>>    	vgpu->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
>>>>    	kvm_page_track_register_notifier(vgpu->vfio_device.kvm,
>>>> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> b/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> index 56c71474008a..036e1a72a26b 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> @@ -382,6 +382,8 @@ int intel_gvt_create_vgpu(struct intel_vgpu *vgpu,
>>>>
>>>>    	intel_gvt_update_reg_whitelist(vgpu);
>>>>    	mutex_unlock(&gvt->lock);
>>>> +	kvmgt_protect_table_init(vgpu);
>>>> +	gvt_cache_init(vgpu);
>>>>    	return 0;
>>>>
>>>>    out_clean_sched_policy:
>>>> --
>>>> 2.34.1
>>>
>>
>> -- 
>> Regards,
>> Yi Liu

-- 
Regards,
Yi Liu

RE: [iommufd 1/2] i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into init

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, November 25, 2022 5:07 PM
> >
> > Current gvt internal cache is handled with .open_device()
> and .close_device() pair,
> > so those internal cache is now re-initialized for each device session, how is
> that
> > handled for iommufd? Looks that's missed in this patch..
> 
> you are right. I noticed below two helpers are used to destroy. However,
> the code seems to be more clear the internal cache. So seems no need to
> re-initialize. I'm no expert here. :)
> 
> gvt_cache_destroy()
> kvmgt_protect_table_destroy()
> 

It's common practice to have init in the probe path and then do the
housekeeping in the close path.

Re-init in every open is unnecessary.

RE: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, November 24, 2022 8:59 PM
> 
> On Thu, Nov 24, 2022 at 07:08:06AM +0000, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Wednesday, November 23, 2022 9:49 PM
> > > +static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova,
> > > u64 length)
> > > +{
> > > +	struct ap_queue_table *qtable = &matrix_mdev->qtable;
> > > +	u64 iova_pfn_end = (iova + length - 1) >> PAGE_SHIFT;
> > > +	u64 iova_pfn_start = iova >> PAGE_SHIFT;
> > > +	struct vfio_ap_queue *q;
> > > +	int loop_cursor;
> > > +	u64 pfn;
> > > +
> > > +	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
> > > +		pfn = q->saved_iova >> PAGE_SHIFT;
> > > +		if (pfn >= iova_pfn_start && pfn <= iova_pfn_end) {
> > > +			vfio_ap_irq_disable(q);
> > > +			break;
> >
> > does this need a WARN_ON if the length is more than one page?
> 
> The iova and length are the range being invalidated, the driver has no
> control over them and length is probably multiple pages.

Yes. I'm misled by the 'break'. Presumably all queues covered by
the unmapped range should have interrupt disabled while above only
disables interrupt for the first covered queue.

Re: [iommufd 1/2] i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into init

[Wang, Zhi A]

On 11/23/22 13:48, Liu, Yi L wrote:
> vfio_iommufd_bind() creates an access which has an unmap callback, which
> can be called immediately. So dma_unmap() callback should tolerate the
> unmaps that come before the emulated device is opened.

After reading code on Jason's tree, I think it would be nice to document

the requirements for a vfio driver somewhere, like what they must do and

must not do in the callbacks of vfio_device_ops. maybe in include/linux/vfio.h?

> To achieve above, move the protect_table_init and gvt_cache_init into the
> init op which is supposed to be triggered prior to the open_device() op.
>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/gvt.h   | 2 ++
>  drivers/gpu/drm/i915/gvt/kvmgt.c | 7 ++-----
>  drivers/gpu/drm/i915/gvt/vgpu.c  | 2 ++
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index dbf8d7470b2c..a3a7e16078ba 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -754,6 +754,8 @@ void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu);
>  void intel_gvt_debugfs_init(struct intel_gvt *gvt);
>  void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
>  
> +void gvt_cache_init(struct intel_vgpu *vgpu);
> +void kvmgt_protect_table_init(struct intel_vgpu *info);
>  int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn);
>  int intel_gvt_page_track_remove(struct intel_vgpu *info, u64 gfn);
>  int intel_gvt_dma_pin_guest_page(struct intel_vgpu *vgpu, dma_addr_t dma_addr);
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 579b230a0f58..cb21b1ba4162 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -322,7 +322,7 @@ static void gvt_cache_destroy(struct intel_vgpu *vgpu)
>  	}
>  }
>  
> -static void gvt_cache_init(struct intel_vgpu *vgpu)
> +void gvt_cache_init(struct intel_vgpu *vgpu)
>  {
>  	vgpu->gfn_cache = RB_ROOT;
>  	vgpu->dma_addr_cache = RB_ROOT;
> @@ -330,7 +330,7 @@ static void gvt_cache_init(struct intel_vgpu *vgpu)
>  	mutex_init(&vgpu->cache_lock);
>  }
>  
> -static void kvmgt_protect_table_init(struct intel_vgpu *info)
> +void kvmgt_protect_table_init(struct intel_vgpu *info)
>  {
>  	hash_init(info->ptable);
>  }
> @@ -671,9 +671,6 @@ static int intel_vgpu_open_device(struct vfio_device *vfio_dev)
>  
>  	vgpu->attached = true;
>  
> -	kvmgt_protect_table_init(vgpu);
> -	gvt_cache_init(vgpu);
> -
>  	vgpu->track_node.track_write = kvmgt_page_track_write;
>  	vgpu->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
>  	kvm_page_track_register_notifier(vgpu->vfio_device.kvm,
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index 56c71474008a..036e1a72a26b 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -382,6 +382,8 @@ int intel_gvt_create_vgpu(struct intel_vgpu *vgpu,
>  
>  	intel_gvt_update_reg_whitelist(vgpu);
>  	mutex_unlock(&gvt->lock);
> +	kvmgt_protect_table_init(vgpu);
> +	gvt_cache_init(vgpu);

It would be nice that you can still keep the initialization in the kvmgt.c as they are

kvmgt related.

With the changes above: Reviewed-by: Zhi Wang <zhi.a.wang@intel.com>

>  	return 0;
>  
>  out_clean_sched_policy:

Re: [iommufd 1/2] i915/gvt: Move kvmgt_protect_table_init() and gvt_cache_init() into init

[Yi Liu]

On 2022/11/28 18:22, Wang, Zhi A wrote:
> On 11/23/22 13:48, Liu, Yi L wrote:
>> vfio_iommufd_bind() creates an access which has an unmap callback, which
>> can be called immediately. So dma_unmap() callback should tolerate the
>> unmaps that come before the emulated device is opened.
> 
> After reading code on Jason's tree, I think it would be nice to document
> 
> the requirements for a vfio driver somewhere, like what they must do and
> 
> must not do in the callbacks of vfio_device_ops. maybe in include/linux/vfio.h?

yes. maybe vfio.rst. Altough I found it is out of date. I've got one fix
and may send out later.

>> To achieve above, move the protect_table_init and gvt_cache_init into the
>> init op which is supposed to be triggered prior to the open_device() op.
>>
>> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
>> Cc: Zhi Wang <zhi.a.wang@intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/gvt.h   | 2 ++
>>   drivers/gpu/drm/i915/gvt/kvmgt.c | 7 ++-----
>>   drivers/gpu/drm/i915/gvt/vgpu.c  | 2 ++
>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
>> index dbf8d7470b2c..a3a7e16078ba 100644
>> --- a/drivers/gpu/drm/i915/gvt/gvt.h
>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>> @@ -754,6 +754,8 @@ void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu);
>>   void intel_gvt_debugfs_init(struct intel_gvt *gvt);
>>   void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
>>   
>> +void gvt_cache_init(struct intel_vgpu *vgpu);
>> +void kvmgt_protect_table_init(struct intel_vgpu *info);
>>   int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn);
>>   int intel_gvt_page_track_remove(struct intel_vgpu *info, u64 gfn);
>>   int intel_gvt_dma_pin_guest_page(struct intel_vgpu *vgpu, dma_addr_t dma_addr);
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index 579b230a0f58..cb21b1ba4162 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -322,7 +322,7 @@ static void gvt_cache_destroy(struct intel_vgpu *vgpu)
>>   	}
>>   }
>>   
>> -static void gvt_cache_init(struct intel_vgpu *vgpu)
>> +void gvt_cache_init(struct intel_vgpu *vgpu)
>>   {
>>   	vgpu->gfn_cache = RB_ROOT;
>>   	vgpu->dma_addr_cache = RB_ROOT;
>> @@ -330,7 +330,7 @@ static void gvt_cache_init(struct intel_vgpu *vgpu)
>>   	mutex_init(&vgpu->cache_lock);
>>   }
>>   
>> -static void kvmgt_protect_table_init(struct intel_vgpu *info)
>> +void kvmgt_protect_table_init(struct intel_vgpu *info)
>>   {
>>   	hash_init(info->ptable);
>>   }
>> @@ -671,9 +671,6 @@ static int intel_vgpu_open_device(struct vfio_device *vfio_dev)
>>   
>>   	vgpu->attached = true;
>>   
>> -	kvmgt_protect_table_init(vgpu);
>> -	gvt_cache_init(vgpu);
>> -
>>   	vgpu->track_node.track_write = kvmgt_page_track_write;
>>   	vgpu->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
>>   	kvm_page_track_register_notifier(vgpu->vfio_device.kvm,
>> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
>> index 56c71474008a..036e1a72a26b 100644
>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
>> @@ -382,6 +382,8 @@ int intel_gvt_create_vgpu(struct intel_vgpu *vgpu,
>>   
>>   	intel_gvt_update_reg_whitelist(vgpu);
>>   	mutex_unlock(&gvt->lock);
>> +	kvmgt_protect_table_init(vgpu);
>> +	gvt_cache_init(vgpu);
> 
> It would be nice that you can still keep the initialization in the kvmgt.c as they are

ok, so I'll leave the function implementation in kvmgt.c.

> kvmgt related.
> 
> With the changes above: Reviewed-by: Zhi Wang <zhi.a.wang@intel.com>

thanks. :-)

>>   	return 0;
>>   
>>   out_clean_sched_policy:

-- 
Regards,
Yi Liu

Re: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

[Matthew Rosato]

On 11/28/22 1:31 AM, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Thursday, November 24, 2022 8:59 PM
>>
>> On Thu, Nov 24, 2022 at 07:08:06AM +0000, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Wednesday, November 23, 2022 9:49 PM
>>>> +static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova,
>>>> u64 length)
>>>> +{
>>>> +	struct ap_queue_table *qtable = &matrix_mdev->qtable;
>>>> +	u64 iova_pfn_end = (iova + length - 1) >> PAGE_SHIFT;
>>>> +	u64 iova_pfn_start = iova >> PAGE_SHIFT;
>>>> +	struct vfio_ap_queue *q;
>>>> +	int loop_cursor;
>>>> +	u64 pfn;
>>>> +
>>>> +	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
>>>> +		pfn = q->saved_iova >> PAGE_SHIFT;
>>>> +		if (pfn >= iova_pfn_start && pfn <= iova_pfn_end) {
>>>> +			vfio_ap_irq_disable(q);
>>>> +			break;
>>>
>>> does this need a WARN_ON if the length is more than one page?
>>
>> The iova and length are the range being invalidated, the driver has no
>> control over them and length is probably multiple pages.
> 
> Yes. I'm misled by the 'break'. Presumably all queues covered by
> the unmapped range should have interrupt disabled while above only
> disables interrupt for the first covered queue.

Oops, yeah the break shouldn't be there; we want to disable any queue in the table that falls within the iova range.

Re: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

[Matthew Rosato]

On 11/24/22 7:59 AM, Jason Gunthorpe wrote:
> On Thu, Nov 24, 2022 at 07:08:06AM +0000, Tian, Kevin wrote:
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Wednesday, November 23, 2022 9:49 PM
>>>
>>> From: Matthew Rosato <mjrosato@linux.ibm.com>
>>>
>>> vfio_iommufd_bind() creates an access which has an unmap callback, which
>>> can be called immediately. So dma_unmap() callback should tolerate the
>>> unmaps that come before the emulated device is opened.
>>>
>>> To achieve above, vfio_ap_mdev_dma_unmap() needs to validate that
>>> unmap
>>> request matches with one or more of these stashed values before
>>> attempting unpins.
>>>
>>> Currently, each mapped iova is stashed in its associated vfio_ap_queue;
>>> Each stashed iova represents IRQ that was enabled for a queue. Therefore,
>>> if a match is found, trigger IRQ disable for this queue to ensure that
>>> underlying firmware will no longer try to use the associated pfn after
>>> the page is unpinned. IRQ disable will also handle the associated unpin.
>>>
>>> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
>>> Cc: Halil Pasic <pasic@linux.ibm.com>
>>> Cc: Jason Herne <jjherne@linux.ibm.com>
>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> ---
>>>  drivers/s390/crypto/vfio_ap_ops.c | 24 +++++++++++++++++++++++-
>>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>> index bb7776d20792..62bfca2bbe6d 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -1535,13 +1535,35 @@ static int vfio_ap_mdev_set_kvm(struct
>>> ap_matrix_mdev *matrix_mdev,
>>>  	return 0;
>>>  }
>>>
>>> +static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova,
>>> u64 length)
>>> +{
>>> +	struct ap_queue_table *qtable = &matrix_mdev->qtable;
>>> +	u64 iova_pfn_end = (iova + length - 1) >> PAGE_SHIFT;
>>> +	u64 iova_pfn_start = iova >> PAGE_SHIFT;
>>> +	struct vfio_ap_queue *q;
>>> +	int loop_cursor;
>>> +	u64 pfn;
>>> +
>>> +	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
>>> +		pfn = q->saved_iova >> PAGE_SHIFT;
>>> +		if (pfn >= iova_pfn_start && pfn <= iova_pfn_end) {
>>> +			vfio_ap_irq_disable(q);
>>> +			break;
>>
>> does this need a WARN_ON if the length is more than one page?
> 
> The iova and length are the range being invalidated, the driver has no
> control over them and length is probably multiple pages.
> 
> But this test doesn't look right?
> 
>    if (iova > q->saved_iova && q->saved_iova < iova + length)> 
> Since the page was pinned we can assume iova and length are already
> PAGE_SIZE aligned.

Yeah, I think that would be fine with a minor tweak to pick up q->saved_iova at the very start of the iova range:

   if (iova >= q->saved_iova && q->saved_iova < iova + length)

Re: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 10:40:48AM -0500, Matthew Rosato wrote:
> On 11/24/22 7:59 AM, Jason Gunthorpe wrote:
> > The iova and length are the range being invalidated, the driver has no
> > control over them and length is probably multiple pages.
> > 
> > But this test doesn't look right?
> > 
> >    if (iova > q->saved_iova && q->saved_iova < iova + length)> 
> > Since the page was pinned we can assume iova and length are already
> > PAGE_SIZE aligned.
> 
> Yeah, I think that would be fine with a minor tweak to pick up q->saved_iova at the very start of the iova range:
> 
>    if (iova >= q->saved_iova && q->saved_iova < iova + length)
> 

Yi can you update and repost this please?

I don't know if we will get a rc8, but we must be prepared with a
final branch by Friday in case not.

Thanks,
Jason

RE: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, November 29, 2022 4:28 AM
> 
> On Mon, Nov 28, 2022 at 10:40:48AM -0500, Matthew Rosato wrote:
> > On 11/24/22 7:59 AM, Jason Gunthorpe wrote:
> > > The iova and length are the range being invalidated, the driver has no
> > > control over them and length is probably multiple pages.
> > >
> > > But this test doesn't look right?
> > >
> > >    if (iova > q->saved_iova && q->saved_iova < iova + length)>
> > > Since the page was pinned we can assume iova and length are already
> > > PAGE_SIZE aligned.
> >
> > Yeah, I think that would be fine with a minor tweak to pick up q-
> >saved_iova at the very start of the iova range:
> >
> >    if (iova >= q->saved_iova && q->saved_iova < iova + length)
> >
> 
> Yi can you update and repost this please?
> 
> I don't know if we will get a rc8, but we must be prepared with a
> final branch by Friday in case not.
> 

Just in case, Linus already said:
--
As a result, I'm now pretty sure that this is going to be one of those
"we'll have an extra week and I'll make an rc8" releases.
--

Re: [iommufd 2/2] vfio/ap: validate iova during dma_unmap and trigger irq disable

[Yi Liu]

On 2022/11/29 04:28, Jason Gunthorpe wrote:
> On Mon, Nov 28, 2022 at 10:40:48AM -0500, Matthew Rosato wrote:
>> On 11/24/22 7:59 AM, Jason Gunthorpe wrote:
>>> The iova and length are the range being invalidated, the driver has no
>>> control over them and length is probably multiple pages.
>>>
>>> But this test doesn't look right?
>>>
>>>     if (iova > q->saved_iova && q->saved_iova < iova + length)>
>>> Since the page was pinned we can assume iova and length are already
>>> PAGE_SIZE aligned.
>>
>> Yeah, I think that would be fine with a minor tweak to pick up q->saved_iova at the very start of the iova range:
>>
>>     if (iova >= q->saved_iova && q->saved_iova < iova + length)
>>
> 
> Yi can you update and repost this please?
> 
> I don't know if we will get a rc8, but we must be prepared with a
> final branch by Friday in case not.

done.

https://lore.kernel.org/kvm/20221129093535.359357-1-yi.l.liu@intel.com/T/#m261c22a7d9c9b2fe4258c5156f53ea6da97a27c9

-- 
Regards,
Yi Liu