[v1,1/4] drm/i915/gvt: add one hypercall to clean vfio regions

Submitted by hang.yuan@linux.intel.com on Jan. 10, 2019, 11:04 a.m.

Details

Message ID 1547118288-1001-2-git-send-email-hang.yuan@linux.intel.com
State New
Headers show
Series "drm/i915/gvt: add one VFIO graphics EDID region" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

hang.yuan@linux.intel.com Jan. 10, 2019, 11:04 a.m.
From: Hang Yuan <hang.yuan@linux.intel.com>

Add one hypercall to free VFIO region memory. Also call it in vgpu
destroy.

Signed-off-by: Hang Yuan <hang.yuan@linux.intel.com>
---
 drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 15 +++++++++++++++
 drivers/gpu/drm/i915/gvt/mpt.h       | 14 ++++++++++++++
 drivers/gpu/drm/i915/gvt/vgpu.c      |  1 +
 4 files changed, 31 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
index 5079886..2ab4138 100644
--- a/drivers/gpu/drm/i915/gvt/hypercall.h
+++ b/drivers/gpu/drm/i915/gvt/hypercall.h
@@ -67,6 +67,7 @@  struct intel_gvt_mpt {
 	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
 			     bool map);
 	int (*set_opregion)(void *vgpu);
+	void (*clean_regions)(void *vgpu);
 	int (*get_vfio_device)(void *vgpu);
 	void (*put_vfio_device)(void *vgpu);
 	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index a19e684..8c30dc3 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -493,6 +493,20 @@  static int kvmgt_set_opregion(void *p_vgpu)
 	return ret;
 }
 
+static void kvmgt_clean_regions(void *p_vgpu)
+{
+	int i;
+	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
+
+	for (i = 0; i < vgpu->vdev.num_regions; i++)
+		if (vgpu->vdev.region[i].ops->release)
+			vgpu->vdev.region[i].ops->release(vgpu,
+					&vgpu->vdev.region[i]);
+	vgpu->vdev.num_regions = 0;
+	kfree(vgpu->vdev.region);
+	vgpu->vdev.region = NULL;
+}
+
 static void kvmgt_put_vfio_device(void *vgpu)
 {
 	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
@@ -1874,6 +1888,7 @@  static struct intel_gvt_mpt kvmgt_mpt = {
 	.dma_map_guest_page = kvmgt_dma_map_guest_page,
 	.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
 	.set_opregion = kvmgt_set_opregion,
+	.clean_regions = kvmgt_clean_regions,
 	.get_vfio_device = kvmgt_get_vfio_device,
 	.put_vfio_device = kvmgt_put_vfio_device,
 	.is_valid_gfn = kvmgt_is_valid_gfn,
diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
index 9b4225d..1a07994 100644
--- a/drivers/gpu/drm/i915/gvt/mpt.h
+++ b/drivers/gpu/drm/i915/gvt/mpt.h
@@ -314,6 +314,20 @@  static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)
 }
 
 /**
+ * intel_gvt_hypervisor_clean_regions - Clean regions for guest
+ * @vgpu: a vGPU
+ *
+ */
+static inline void intel_gvt_hypervisor_clean_regions(struct intel_vgpu *vgpu)
+{
+	if (!intel_gvt_host.mpt->clean_regions)
+		return;
+
+	intel_gvt_host.mpt->clean_regions(vgpu);
+}
+
+
+/**
  * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
  * @vgpu: a vGPU
  *
diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
index e1c860f8..c5eb565 100644
--- a/drivers/gpu/drm/i915/gvt/vgpu.c
+++ b/drivers/gpu/drm/i915/gvt/vgpu.c
@@ -276,6 +276,7 @@  void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu)
 
 	WARN(vgpu->active, "vGPU is still active!\n");
 
+	intel_gvt_hypervisor_clean_regions(vgpu);
 	intel_gvt_debugfs_remove_vgpu(vgpu);
 	intel_vgpu_clean_sched_policy(vgpu);
 	intel_vgpu_clean_submission(vgpu);

Comments

On 2019.01.10 19:04:45 +0800, hang.yuan@linux.intel.com wrote:
> From: Hang Yuan <hang.yuan@linux.intel.com>
> 
> Add one hypercall to free VFIO region memory. Also call it in vgpu
> destroy.
> 
> Signed-off-by: Hang Yuan <hang.yuan@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
>  drivers/gpu/drm/i915/gvt/kvmgt.c     | 15 +++++++++++++++
>  drivers/gpu/drm/i915/gvt/mpt.h       | 14 ++++++++++++++
>  drivers/gpu/drm/i915/gvt/vgpu.c      |  1 +
>  4 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
> index 5079886..2ab4138 100644
> --- a/drivers/gpu/drm/i915/gvt/hypercall.h
> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
> @@ -67,6 +67,7 @@ struct intel_gvt_mpt {
>  	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
>  			     bool map);
>  	int (*set_opregion)(void *vgpu);
> +	void (*clean_regions)(void *vgpu);
>  	int (*get_vfio_device)(void *vgpu);
>  	void (*put_vfio_device)(void *vgpu);
>  	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);

> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index a19e684..8c30dc3 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -493,6 +493,20 @@ static int kvmgt_set_opregion(void *p_vgpu)
>  	return ret;
>  }
>  
> +static void kvmgt_clean_regions(void *p_vgpu)
> +{
> +	int i;
> +	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
> +
> +	for (i = 0; i < vgpu->vdev.num_regions; i++)
> +		if (vgpu->vdev.region[i].ops->release)
> +			vgpu->vdev.region[i].ops->release(vgpu,
> +					&vgpu->vdev.region[i]);
> +	vgpu->vdev.num_regions = 0;
> +	kfree(vgpu->vdev.region);
> +	vgpu->vdev.region = NULL;
> +}

It looks a little overkill to put this requiremnt on all hypervisor,
as VFIO region is KVMGT only thing, e.g might not be valid for other hypervisor.
And looks our region.ops.release is never actually useful, what we need
is to free up vgpu regions, maybe just at detach vgpu time to let hypervisor
do any cleanup required.

> +
>  static void kvmgt_put_vfio_device(void *vgpu)
>  {
>  	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
> @@ -1874,6 +1888,7 @@ static struct intel_gvt_mpt kvmgt_mpt = {
>  	.dma_map_guest_page = kvmgt_dma_map_guest_page,
>  	.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
>  	.set_opregion = kvmgt_set_opregion,
> +	.clean_regions = kvmgt_clean_regions,
>  	.get_vfio_device = kvmgt_get_vfio_device,
>  	.put_vfio_device = kvmgt_put_vfio_device,
>  	.is_valid_gfn = kvmgt_is_valid_gfn,
> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
> index 9b4225d..1a07994 100644
> --- a/drivers/gpu/drm/i915/gvt/mpt.h
> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
> @@ -314,6 +314,20 @@ static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)
>  }
>  
>  /**
> + * intel_gvt_hypervisor_clean_regions - Clean regions for guest
> + * @vgpu: a vGPU
> + *
> + */
> +static inline void intel_gvt_hypervisor_clean_regions(struct intel_vgpu *vgpu)
> +{
> +	if (!intel_gvt_host.mpt->clean_regions)
> +		return;
> +
> +	intel_gvt_host.mpt->clean_regions(vgpu);
> +}
> +
> +
> +/**
>   * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
>   * @vgpu: a vGPU
>   *
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index e1c860f8..c5eb565 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -276,6 +276,7 @@ void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu)
>  
>  	WARN(vgpu->active, "vGPU is still active!\n");
>  
> +	intel_gvt_hypervisor_clean_regions(vgpu);
>  	intel_gvt_debugfs_remove_vgpu(vgpu);
>  	intel_vgpu_clean_sched_policy(vgpu);
>  	intel_vgpu_clean_submission(vgpu);
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
On 1/14/19 11:56 AM, Zhenyu Wang wrote:
> On 2019.01.10 19:04:45 +0800, hang.yuan@linux.intel.com wrote:
>> From: Hang Yuan <hang.yuan@linux.intel.com>
>>
>> Add one hypercall to free VFIO region memory. Also call it in vgpu
>> destroy.
>>
>> Signed-off-by: Hang Yuan <hang.yuan@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
>>   drivers/gpu/drm/i915/gvt/kvmgt.c     | 15 +++++++++++++++
>>   drivers/gpu/drm/i915/gvt/mpt.h       | 14 ++++++++++++++
>>   drivers/gpu/drm/i915/gvt/vgpu.c      |  1 +
>>   4 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
>> index 5079886..2ab4138 100644
>> --- a/drivers/gpu/drm/i915/gvt/hypercall.h
>> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
>> @@ -67,6 +67,7 @@ struct intel_gvt_mpt {
>>   	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
>>   			     bool map);
>>   	int (*set_opregion)(void *vgpu);
>> +	void (*clean_regions)(void *vgpu);
>>   	int (*get_vfio_device)(void *vgpu);
>>   	void (*put_vfio_device)(void *vgpu);
>>   	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
> 
>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> index a19e684..8c30dc3 100644
>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>> @@ -493,6 +493,20 @@ static int kvmgt_set_opregion(void *p_vgpu)
>>   	return ret;
>>   }
>>   
>> +static void kvmgt_clean_regions(void *p_vgpu)
>> +{
>> +	int i;
>> +	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
>> +
>> +	for (i = 0; i < vgpu->vdev.num_regions; i++)
>> +		if (vgpu->vdev.region[i].ops->release)
>> +			vgpu->vdev.region[i].ops->release(vgpu,
>> +					&vgpu->vdev.region[i]);
>> +	vgpu->vdev.num_regions = 0;
>> +	kfree(vgpu->vdev.region);
>> +	vgpu->vdev.region = NULL;
>> +}
> 
> It looks a little overkill to put this requiremnt on all hypervisor,
> as VFIO region is KVMGT only thing, e.g might not be valid for other hypervisor.
> And looks our region.ops.release is never actually useful, what we need
> is to free up vgpu regions, maybe just at detach vgpu time to let hypervisor
> do any cleanup required.
> 
Henry:Since there is already hypercall 'set_opregion', I use hypercall 
as well to do region clean for opregion and edid region to have 
consistent interface between gvt and hypervisor. 'clean_regions' will 
also be called if failure in vgpu create. So I didn't implement it in 
vgpu detach.

>> +
>>   static void kvmgt_put_vfio_device(void *vgpu)
>>   {
>>   	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
>> @@ -1874,6 +1888,7 @@ static struct intel_gvt_mpt kvmgt_mpt = {
>>   	.dma_map_guest_page = kvmgt_dma_map_guest_page,
>>   	.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
>>   	.set_opregion = kvmgt_set_opregion,
>> +	.clean_regions = kvmgt_clean_regions,
>>   	.get_vfio_device = kvmgt_get_vfio_device,
>>   	.put_vfio_device = kvmgt_put_vfio_device,
>>   	.is_valid_gfn = kvmgt_is_valid_gfn,
>> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
>> index 9b4225d..1a07994 100644
>> --- a/drivers/gpu/drm/i915/gvt/mpt.h
>> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
>> @@ -314,6 +314,20 @@ static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)
>>   }
>>   
>>   /**
>> + * intel_gvt_hypervisor_clean_regions - Clean regions for guest
>> + * @vgpu: a vGPU
>> + *
>> + */
>> +static inline void intel_gvt_hypervisor_clean_regions(struct intel_vgpu *vgpu)
>> +{
>> +	if (!intel_gvt_host.mpt->clean_regions)
>> +		return;
>> +
>> +	intel_gvt_host.mpt->clean_regions(vgpu);
>> +}
>> +
>> +
>> +/**
>>    * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
>>    * @vgpu: a vGPU
>>    *
>> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
>> index e1c860f8..c5eb565 100644
>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
>> @@ -276,6 +276,7 @@ void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu)
>>   
>>   	WARN(vgpu->active, "vGPU is still active!\n");
>>   
>> +	intel_gvt_hypervisor_clean_regions(vgpu);
>>   	intel_gvt_debugfs_remove_vgpu(vgpu);
>>   	intel_vgpu_clean_sched_policy(vgpu);
>>   	intel_vgpu_clean_submission(vgpu);
>> -- 
>> 2.7.4
On 2019.01.14 13:02:24 +0800, Hang Yuan wrote:
> On 1/14/19 11:56 AM, Zhenyu Wang wrote:
> > On 2019.01.10 19:04:45 +0800, hang.yuan@linux.intel.com wrote:
> > > From: Hang Yuan <hang.yuan@linux.intel.com>
> > > 
> > > Add one hypercall to free VFIO region memory. Also call it in vgpu
> > > destroy.
> > > 
> > > Signed-off-by: Hang Yuan <hang.yuan@linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
> > >   drivers/gpu/drm/i915/gvt/kvmgt.c     | 15 +++++++++++++++
> > >   drivers/gpu/drm/i915/gvt/mpt.h       | 14 ++++++++++++++
> > >   drivers/gpu/drm/i915/gvt/vgpu.c      |  1 +
> > >   4 files changed, 31 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
> > > index 5079886..2ab4138 100644
> > > --- a/drivers/gpu/drm/i915/gvt/hypercall.h
> > > +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
> > > @@ -67,6 +67,7 @@ struct intel_gvt_mpt {
> > >   	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
> > >   			     bool map);
> > >   	int (*set_opregion)(void *vgpu);
> > > +	void (*clean_regions)(void *vgpu);
> > >   	int (*get_vfio_device)(void *vgpu);
> > >   	void (*put_vfio_device)(void *vgpu);
> > >   	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
> > 
> > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > index a19e684..8c30dc3 100644
> > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > @@ -493,6 +493,20 @@ static int kvmgt_set_opregion(void *p_vgpu)
> > >   	return ret;
> > >   }
> > > +static void kvmgt_clean_regions(void *p_vgpu)
> > > +{
> > > +	int i;
> > > +	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
> > > +
> > > +	for (i = 0; i < vgpu->vdev.num_regions; i++)
> > > +		if (vgpu->vdev.region[i].ops->release)
> > > +			vgpu->vdev.region[i].ops->release(vgpu,
> > > +					&vgpu->vdev.region[i]);
> > > +	vgpu->vdev.num_regions = 0;
> > > +	kfree(vgpu->vdev.region);
> > > +	vgpu->vdev.region = NULL;
> > > +}
> > 
> > It looks a little overkill to put this requiremnt on all hypervisor,
> > as VFIO region is KVMGT only thing, e.g might not be valid for other hypervisor.
> > And looks our region.ops.release is never actually useful, what we need
> > is to free up vgpu regions, maybe just at detach vgpu time to let hypervisor
> > do any cleanup required.
> > 
> Henry:Since there is already hypercall 'set_opregion', I use hypercall as
> well to do region clean for opregion and edid region to have consistent
> interface between gvt and hypervisor. 'clean_regions' will also be called if
> failure in vgpu create. So I didn't implement it in vgpu detach.
>

"set_opregion" hypercall means hypervisor might have different ways to
expose opregion, e.g through vfio region, etc. So that makes sense for a
mpt interface. But here only care about cleanup of VFIO region, and both
opregion and edid region's release function is noop..

> > > +
> > >   static void kvmgt_put_vfio_device(void *vgpu)
> > >   {
> > >   	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
> > > @@ -1874,6 +1888,7 @@ static struct intel_gvt_mpt kvmgt_mpt = {
> > >   	.dma_map_guest_page = kvmgt_dma_map_guest_page,
> > >   	.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
> > >   	.set_opregion = kvmgt_set_opregion,
> > > +	.clean_regions = kvmgt_clean_regions,
> > >   	.get_vfio_device = kvmgt_get_vfio_device,
> > >   	.put_vfio_device = kvmgt_put_vfio_device,
> > >   	.is_valid_gfn = kvmgt_is_valid_gfn,
> > > diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
> > > index 9b4225d..1a07994 100644
> > > --- a/drivers/gpu/drm/i915/gvt/mpt.h
> > > +++ b/drivers/gpu/drm/i915/gvt/mpt.h
> > > @@ -314,6 +314,20 @@ static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)
> > >   }
> > >   /**
> > > + * intel_gvt_hypervisor_clean_regions - Clean regions for guest
> > > + * @vgpu: a vGPU
> > > + *
> > > + */
> > > +static inline void intel_gvt_hypervisor_clean_regions(struct intel_vgpu *vgpu)
> > > +{
> > > +	if (!intel_gvt_host.mpt->clean_regions)
> > > +		return;
> > > +
> > > +	intel_gvt_host.mpt->clean_regions(vgpu);
> > > +}
> > > +
> > > +
> > > +/**
> > >    * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
> > >    * @vgpu: a vGPU
> > >    *
> > > diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> > > index e1c860f8..c5eb565 100644
> > > --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> > > +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> > > @@ -276,6 +276,7 @@ void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu)
> > >   	WARN(vgpu->active, "vGPU is still active!\n");
> > > +	intel_gvt_hypervisor_clean_regions(vgpu);
> > >   	intel_gvt_debugfs_remove_vgpu(vgpu);
> > >   	intel_vgpu_clean_sched_policy(vgpu);
> > >   	intel_vgpu_clean_submission(vgpu);
> > > -- 
> > > 2.7.4
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
On 2019.01.14 13:05:57 +0800, Zhenyu Wang wrote:
> On 2019.01.14 13:02:24 +0800, Hang Yuan wrote:
> > On 1/14/19 11:56 AM, Zhenyu Wang wrote:
> > > On 2019.01.10 19:04:45 +0800, hang.yuan@linux.intel.com wrote:
> > > > From: Hang Yuan <hang.yuan@linux.intel.com>
> > > > 
> > > > Add one hypercall to free VFIO region memory. Also call it in vgpu
> > > > destroy.
> > > > 
> > > > Signed-off-by: Hang Yuan <hang.yuan@linux.intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
> > > >   drivers/gpu/drm/i915/gvt/kvmgt.c     | 15 +++++++++++++++
> > > >   drivers/gpu/drm/i915/gvt/mpt.h       | 14 ++++++++++++++
> > > >   drivers/gpu/drm/i915/gvt/vgpu.c      |  1 +
> > > >   4 files changed, 31 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
> > > > index 5079886..2ab4138 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/hypercall.h
> > > > +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
> > > > @@ -67,6 +67,7 @@ struct intel_gvt_mpt {
> > > >   	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
> > > >   			     bool map);
> > > >   	int (*set_opregion)(void *vgpu);
> > > > +	void (*clean_regions)(void *vgpu);
> > > >   	int (*get_vfio_device)(void *vgpu);
> > > >   	void (*put_vfio_device)(void *vgpu);
> > > >   	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
> > > 
> > > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > > index a19e684..8c30dc3 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > > @@ -493,6 +493,20 @@ static int kvmgt_set_opregion(void *p_vgpu)
> > > >   	return ret;
> > > >   }
> > > > +static void kvmgt_clean_regions(void *p_vgpu)
> > > > +{
> > > > +	int i;
> > > > +	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
> > > > +
> > > > +	for (i = 0; i < vgpu->vdev.num_regions; i++)
> > > > +		if (vgpu->vdev.region[i].ops->release)
> > > > +			vgpu->vdev.region[i].ops->release(vgpu,
> > > > +					&vgpu->vdev.region[i]);
> > > > +	vgpu->vdev.num_regions = 0;
> > > > +	kfree(vgpu->vdev.region);
> > > > +	vgpu->vdev.region = NULL;
> > > > +}
> > > 
> > > It looks a little overkill to put this requiremnt on all hypervisor,
> > > as VFIO region is KVMGT only thing, e.g might not be valid for other hypervisor.
> > > And looks our region.ops.release is never actually useful, what we need
> > > is to free up vgpu regions, maybe just at detach vgpu time to let hypervisor
> > > do any cleanup required.
> > > 
> > Henry:Since there is already hypercall 'set_opregion', I use hypercall as
> > well to do region clean for opregion and edid region to have consistent
> > interface between gvt and hypervisor. 'clean_regions' will also be called if
> > failure in vgpu create. So I didn't implement it in vgpu detach.
> >
> 
> "set_opregion" hypercall means hypervisor might have different ways to
> expose opregion, e.g through vfio region, etc. So that makes sense for a
> mpt interface. But here only care about cleanup of VFIO region, and both
> opregion and edid region's release function is noop..
> 

And as this one is an issue fix patch instead of vfio edid related,
pls split it as a fix one.

thanks
On 1/14/19 1:05 PM, Zhenyu Wang wrote:
> On 2019.01.14 13:02:24 +0800, Hang Yuan wrote:
>> On 1/14/19 11:56 AM, Zhenyu Wang wrote:
>>> On 2019.01.10 19:04:45 +0800, hang.yuan@linux.intel.com wrote:
>>>> From: Hang Yuan <hang.yuan@linux.intel.com>
>>>>
>>>> Add one hypercall to free VFIO region memory. Also call it in vgpu
>>>> destroy.
>>>>
>>>> Signed-off-by: Hang Yuan <hang.yuan@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
>>>>    drivers/gpu/drm/i915/gvt/kvmgt.c     | 15 +++++++++++++++
>>>>    drivers/gpu/drm/i915/gvt/mpt.h       | 14 ++++++++++++++
>>>>    drivers/gpu/drm/i915/gvt/vgpu.c      |  1 +
>>>>    4 files changed, 31 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
>>>> index 5079886..2ab4138 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/hypercall.h
>>>> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
>>>> @@ -67,6 +67,7 @@ struct intel_gvt_mpt {
>>>>    	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
>>>>    			     bool map);
>>>>    	int (*set_opregion)(void *vgpu);
>>>> +	void (*clean_regions)(void *vgpu);
>>>>    	int (*get_vfio_device)(void *vgpu);
>>>>    	void (*put_vfio_device)(void *vgpu);
>>>>    	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
>>>
>>>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>> index a19e684..8c30dc3 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>> @@ -493,6 +493,20 @@ static int kvmgt_set_opregion(void *p_vgpu)
>>>>    	return ret;
>>>>    }
>>>> +static void kvmgt_clean_regions(void *p_vgpu)
>>>> +{
>>>> +	int i;
>>>> +	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
>>>> +
>>>> +	for (i = 0; i < vgpu->vdev.num_regions; i++)
>>>> +		if (vgpu->vdev.region[i].ops->release)
>>>> +			vgpu->vdev.region[i].ops->release(vgpu,
>>>> +					&vgpu->vdev.region[i]);
>>>> +	vgpu->vdev.num_regions = 0;
>>>> +	kfree(vgpu->vdev.region);
>>>> +	vgpu->vdev.region = NULL;
>>>> +}
>>>
>>> It looks a little overkill to put this requiremnt on all hypervisor,
>>> as VFIO region is KVMGT only thing, e.g might not be valid for other hypervisor.
>>> And looks our region.ops.release is never actually useful, what we need
>>> is to free up vgpu regions, maybe just at detach vgpu time to let hypervisor
>>> do any cleanup required.
>>>
>> Henry:Since there is already hypercall 'set_opregion', I use hypercall as
>> well to do region clean for opregion and edid region to have consistent
>> interface between gvt and hypervisor. 'clean_regions' will also be called if
>> failure in vgpu create. So I didn't implement it in vgpu detach.
>>
> 
> "set_opregion" hypercall means hypervisor might have different ways to
> expose opregion, e.g through vfio region, etc. So that makes sense for a
> mpt interface. But here only care about cleanup of VFIO region, and both
> opregion and edid region's release function is noop..
> 
Henry: So do you mean to expose the functions (clean_regions/set_edid) 
in kvmgt instead of hypercall if the function is only valid for KVM?

>>>> +
>>>>    static void kvmgt_put_vfio_device(void *vgpu)
>>>>    {
>>>>    	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))
>>>> @@ -1874,6 +1888,7 @@ static struct intel_gvt_mpt kvmgt_mpt = {
>>>>    	.dma_map_guest_page = kvmgt_dma_map_guest_page,
>>>>    	.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,
>>>>    	.set_opregion = kvmgt_set_opregion,
>>>> +	.clean_regions = kvmgt_clean_regions,
>>>>    	.get_vfio_device = kvmgt_get_vfio_device,
>>>>    	.put_vfio_device = kvmgt_put_vfio_device,
>>>>    	.is_valid_gfn = kvmgt_is_valid_gfn,
>>>> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
>>>> index 9b4225d..1a07994 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/mpt.h
>>>> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
>>>> @@ -314,6 +314,20 @@ static inline int intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)
>>>>    }
>>>>    /**
>>>> + * intel_gvt_hypervisor_clean_regions - Clean regions for guest
>>>> + * @vgpu: a vGPU
>>>> + *
>>>> + */
>>>> +static inline void intel_gvt_hypervisor_clean_regions(struct intel_vgpu *vgpu)
>>>> +{
>>>> +	if (!intel_gvt_host.mpt->clean_regions)
>>>> +		return;
>>>> +
>>>> +	intel_gvt_host.mpt->clean_regions(vgpu);
>>>> +}
>>>> +
>>>> +
>>>> +/**
>>>>     * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref count
>>>>     * @vgpu: a vGPU
>>>>     *
>>>> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> index e1c860f8..c5eb565 100644
>>>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
>>>> @@ -276,6 +276,7 @@ void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu)
>>>>    	WARN(vgpu->active, "vGPU is still active!\n");
>>>> +	intel_gvt_hypervisor_clean_regions(vgpu);
>>>>    	intel_gvt_debugfs_remove_vgpu(vgpu);
>>>>    	intel_vgpu_clean_sched_policy(vgpu);
>>>>    	intel_vgpu_clean_submission(vgpu);
>>>> -- 
>>>> 2.7.4
> From: Hang Yuan

> Sent: Monday, January 14, 2019 1:35 PM

> 

> On 1/14/19 1:05 PM, Zhenyu Wang wrote:

> > On 2019.01.14 13:02:24 +0800, Hang Yuan wrote:

> >> On 1/14/19 11:56 AM, Zhenyu Wang wrote:

> >>> On 2019.01.10 19:04:45 +0800, hang.yuan@linux.intel.com wrote:

> >>>> From: Hang Yuan <hang.yuan@linux.intel.com>

> >>>>

> >>>> Add one hypercall to free VFIO region memory. Also call it in vgpu

> >>>> destroy.

> >>>>


I would not call it as hypercall, as it is just a new function callback.
hypercall specifically means a call from guest to hypervisor.

> >>>> Signed-off-by: Hang Yuan <hang.yuan@linux.intel.com>

> >>>> ---

> >>>>    drivers/gpu/drm/i915/gvt/hypercall.h |  1 +

> >>>>    drivers/gpu/drm/i915/gvt/kvmgt.c     | 15 +++++++++++++++

> >>>>    drivers/gpu/drm/i915/gvt/mpt.h       | 14 ++++++++++++++

> >>>>    drivers/gpu/drm/i915/gvt/vgpu.c      |  1 +

> >>>>    4 files changed, 31 insertions(+)

> >>>>

> >>>> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h

> b/drivers/gpu/drm/i915/gvt/hypercall.h

> >>>> index 5079886..2ab4138 100644

> >>>> --- a/drivers/gpu/drm/i915/gvt/hypercall.h

> >>>> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h

> >>>> @@ -67,6 +67,7 @@ struct intel_gvt_mpt {

> >>>>    	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,

> >>>>    			     bool map);

> >>>>    	int (*set_opregion)(void *vgpu);

> >>>> +	void (*clean_regions)(void *vgpu);

> >>>>    	int (*get_vfio_device)(void *vgpu);

> >>>>    	void (*put_vfio_device)(void *vgpu);

> >>>>    	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);

> >>>

> >>>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c

> b/drivers/gpu/drm/i915/gvt/kvmgt.c

> >>>> index a19e684..8c30dc3 100644

> >>>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c

> >>>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c

> >>>> @@ -493,6 +493,20 @@ static int kvmgt_set_opregion(void *p_vgpu)

> >>>>    	return ret;

> >>>>    }

> >>>> +static void kvmgt_clean_regions(void *p_vgpu)

> >>>> +{

> >>>> +	int i;

> >>>> +	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;

> >>>> +

> >>>> +	for (i = 0; i < vgpu->vdev.num_regions; i++)

> >>>> +		if (vgpu->vdev.region[i].ops->release)

> >>>> +			vgpu->vdev.region[i].ops->release(vgpu,

> >>>> +					&vgpu->vdev.region[i]);

> >>>> +	vgpu->vdev.num_regions = 0;

> >>>> +	kfree(vgpu->vdev.region);

> >>>> +	vgpu->vdev.region = NULL;

> >>>> +}

> >>>

> >>> It looks a little overkill to put this requiremnt on all hypervisor,

> >>> as VFIO region is KVMGT only thing, e.g might not be valid for other

> hypervisor.

> >>> And looks our region.ops.release is never actually useful, what we

> need

> >>> is to free up vgpu regions, maybe just at detach vgpu time to let

> hypervisor

> >>> do any cleanup required.

> >>>

> >> Henry:Since there is already hypercall 'set_opregion', I use hypercall as

> >> well to do region clean for opregion and edid region to have consistent

> >> interface between gvt and hypervisor. 'clean_regions' will also be called

> if

> >> failure in vgpu create. So I didn't implement it in vgpu detach.

> >>

> >

> > "set_opregion" hypercall means hypervisor might have different ways to

> > expose opregion, e.g through vfio region, etc. So that makes sense for a

> > mpt interface. But here only care about cleanup of VFIO region, and both

> > opregion and edid region's release function is noop..

> >

> Henry: So do you mean to expose the functions (clean_regions/set_edid)

> in kvmgt instead of hypercall if the function is only valid for KVM?

> 

> >>>> +

> >>>>    static void kvmgt_put_vfio_device(void *vgpu)

> >>>>    {

> >>>>    	if (WARN_ON(!((struct intel_vgpu *)vgpu)->vdev.vfio_device))

> >>>> @@ -1874,6 +1888,7 @@ static struct intel_gvt_mpt kvmgt_mpt = {

> >>>>    	.dma_map_guest_page = kvmgt_dma_map_guest_page,

> >>>>    	.dma_unmap_guest_page = kvmgt_dma_unmap_guest_page,

> >>>>    	.set_opregion = kvmgt_set_opregion,

> >>>> +	.clean_regions = kvmgt_clean_regions,

> >>>>    	.get_vfio_device = kvmgt_get_vfio_device,

> >>>>    	.put_vfio_device = kvmgt_put_vfio_device,

> >>>>    	.is_valid_gfn = kvmgt_is_valid_gfn,

> >>>> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h

> b/drivers/gpu/drm/i915/gvt/mpt.h

> >>>> index 9b4225d..1a07994 100644

> >>>> --- a/drivers/gpu/drm/i915/gvt/mpt.h

> >>>> +++ b/drivers/gpu/drm/i915/gvt/mpt.h

> >>>> @@ -314,6 +314,20 @@ static inline int

> intel_gvt_hypervisor_set_opregion(struct intel_vgpu *vgpu)

> >>>>    }

> >>>>    /**

> >>>> + * intel_gvt_hypervisor_clean_regions - Clean regions for guest

> >>>> + * @vgpu: a vGPU

> >>>> + *

> >>>> + */

> >>>> +static inline void intel_gvt_hypervisor_clean_regions(struct

> intel_vgpu *vgpu)

> >>>> +{

> >>>> +	if (!intel_gvt_host.mpt->clean_regions)

> >>>> +		return;

> >>>> +

> >>>> +	intel_gvt_host.mpt->clean_regions(vgpu);

> >>>> +}

> >>>> +

> >>>> +

> >>>> +/**

> >>>>     * intel_gvt_hypervisor_get_vfio_device - increase vfio device ref

> count

> >>>>     * @vgpu: a vGPU

> >>>>     *

> >>>> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c

> b/drivers/gpu/drm/i915/gvt/vgpu.c

> >>>> index e1c860f8..c5eb565 100644

> >>>> --- a/drivers/gpu/drm/i915/gvt/vgpu.c

> >>>> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c

> >>>> @@ -276,6 +276,7 @@ void intel_gvt_destroy_vgpu(struct intel_vgpu

> *vgpu)

> >>>>    	WARN(vgpu->active, "vGPU is still active!\n");

> >>>> +	intel_gvt_hypervisor_clean_regions(vgpu);

> >>>>    	intel_gvt_debugfs_remove_vgpu(vgpu);

> >>>>    	intel_vgpu_clean_sched_policy(vgpu);

> >>>>    	intel_vgpu_clean_submission(vgpu);

> >>>> --

> >>>> 2.7.4

> _______________________________________________

> intel-gvt-dev mailing list

> intel-gvt-dev@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
On 2019.01.14 13:34:33 +0800, Hang Yuan wrote:
> On 1/14/19 1:05 PM, Zhenyu Wang wrote:
> > On 2019.01.14 13:02:24 +0800, Hang Yuan wrote:
> > > On 1/14/19 11:56 AM, Zhenyu Wang wrote:
> > > > On 2019.01.10 19:04:45 +0800, hang.yuan@linux.intel.com wrote:
> > > > > From: Hang Yuan <hang.yuan@linux.intel.com>
> > > > > 
> > > > > Add one hypercall to free VFIO region memory. Also call it in vgpu
> > > > > destroy.
> > > > > 
> > > > > Signed-off-by: Hang Yuan <hang.yuan@linux.intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
> > > > >    drivers/gpu/drm/i915/gvt/kvmgt.c     | 15 +++++++++++++++
> > > > >    drivers/gpu/drm/i915/gvt/mpt.h       | 14 ++++++++++++++
> > > > >    drivers/gpu/drm/i915/gvt/vgpu.c      |  1 +
> > > > >    4 files changed, 31 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
> > > > > index 5079886..2ab4138 100644
> > > > > --- a/drivers/gpu/drm/i915/gvt/hypercall.h
> > > > > +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
> > > > > @@ -67,6 +67,7 @@ struct intel_gvt_mpt {
> > > > >    	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
> > > > >    			     bool map);
> > > > >    	int (*set_opregion)(void *vgpu);
> > > > > +	void (*clean_regions)(void *vgpu);
> > > > >    	int (*get_vfio_device)(void *vgpu);
> > > > >    	void (*put_vfio_device)(void *vgpu);
> > > > >    	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
> > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > > > index a19e684..8c30dc3 100644
> > > > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > > > > @@ -493,6 +493,20 @@ static int kvmgt_set_opregion(void *p_vgpu)
> > > > >    	return ret;
> > > > >    }
> > > > > +static void kvmgt_clean_regions(void *p_vgpu)
> > > > > +{
> > > > > +	int i;
> > > > > +	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
> > > > > +
> > > > > +	for (i = 0; i < vgpu->vdev.num_regions; i++)
> > > > > +		if (vgpu->vdev.region[i].ops->release)
> > > > > +			vgpu->vdev.region[i].ops->release(vgpu,
> > > > > +					&vgpu->vdev.region[i]);
> > > > > +	vgpu->vdev.num_regions = 0;
> > > > > +	kfree(vgpu->vdev.region);
> > > > > +	vgpu->vdev.region = NULL;
> > > > > +}
> > > > 
> > > > It looks a little overkill to put this requiremnt on all hypervisor,
> > > > as VFIO region is KVMGT only thing, e.g might not be valid for other hypervisor.
> > > > And looks our region.ops.release is never actually useful, what we need
> > > > is to free up vgpu regions, maybe just at detach vgpu time to let hypervisor
> > > > do any cleanup required.
> > > > 
> > > Henry:Since there is already hypercall 'set_opregion', I use hypercall as
> > > well to do region clean for opregion and edid region to have consistent
> > > interface between gvt and hypervisor. 'clean_regions' will also be called if
> > > failure in vgpu create. So I didn't implement it in vgpu detach.
> > > 
> > 
> > "set_opregion" hypercall means hypervisor might have different ways to
> > expose opregion, e.g through vfio region, etc. So that makes sense for a
> > mpt interface. But here only care about cleanup of VFIO region, and both
> > opregion and edid region's release function is noop..
> > 
> Henry: So do you mean to expose the functions (clean_regions/set_edid) in
> kvmgt instead of hypercall if the function is only valid for KVM?
> 

For clean region here, for me it looks we just need to fix it by clean
vgpu regions in detach call, and for set_edid one mpt interface is ok
and region cleanup in detach would also work in error handling path.
On 1/14/19 2:09 PM, Zhenyu Wang wrote:
> On 2019.01.14 13:34:33 +0800, Hang Yuan wrote:
>> On 1/14/19 1:05 PM, Zhenyu Wang wrote:
>>> On 2019.01.14 13:02:24 +0800, Hang Yuan wrote:
>>>> On 1/14/19 11:56 AM, Zhenyu Wang wrote:
>>>>> On 2019.01.10 19:04:45 +0800, hang.yuan@linux.intel.com wrote:
>>>>>> From: Hang Yuan <hang.yuan@linux.intel.com>
>>>>>>
>>>>>> Add one hypercall to free VFIO region memory. Also call it in vgpu
>>>>>> destroy.
>>>>>>
>>>>>> Signed-off-by: Hang Yuan <hang.yuan@linux.intel.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/gvt/hypercall.h |  1 +
>>>>>>     drivers/gpu/drm/i915/gvt/kvmgt.c     | 15 +++++++++++++++
>>>>>>     drivers/gpu/drm/i915/gvt/mpt.h       | 14 ++++++++++++++
>>>>>>     drivers/gpu/drm/i915/gvt/vgpu.c      |  1 +
>>>>>>     4 files changed, 31 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
>>>>>> index 5079886..2ab4138 100644
>>>>>> --- a/drivers/gpu/drm/i915/gvt/hypercall.h
>>>>>> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
>>>>>> @@ -67,6 +67,7 @@ struct intel_gvt_mpt {
>>>>>>     	int (*set_trap_area)(unsigned long handle, u64 start, u64 end,
>>>>>>     			     bool map);
>>>>>>     	int (*set_opregion)(void *vgpu);
>>>>>> +	void (*clean_regions)(void *vgpu);
>>>>>>     	int (*get_vfio_device)(void *vgpu);
>>>>>>     	void (*put_vfio_device)(void *vgpu);
>>>>>>     	bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn);
>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>>>> index a19e684..8c30dc3 100644
>>>>>> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>>>> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
>>>>>> @@ -493,6 +493,20 @@ static int kvmgt_set_opregion(void *p_vgpu)
>>>>>>     	return ret;
>>>>>>     }
>>>>>> +static void kvmgt_clean_regions(void *p_vgpu)
>>>>>> +{
>>>>>> +	int i;
>>>>>> +	struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu;
>>>>>> +
>>>>>> +	for (i = 0; i < vgpu->vdev.num_regions; i++)
>>>>>> +		if (vgpu->vdev.region[i].ops->release)
>>>>>> +			vgpu->vdev.region[i].ops->release(vgpu,
>>>>>> +					&vgpu->vdev.region[i]);
>>>>>> +	vgpu->vdev.num_regions = 0;
>>>>>> +	kfree(vgpu->vdev.region);
>>>>>> +	vgpu->vdev.region = NULL;
>>>>>> +}
>>>>>
>>>>> It looks a little overkill to put this requiremnt on all hypervisor,
>>>>> as VFIO region is KVMGT only thing, e.g might not be valid for other hypervisor.
>>>>> And looks our region.ops.release is never actually useful, what we need
>>>>> is to free up vgpu regions, maybe just at detach vgpu time to let hypervisor
>>>>> do any cleanup required.
>>>>>
>>>> Henry:Since there is already hypercall 'set_opregion', I use hypercall as
>>>> well to do region clean for opregion and edid region to have consistent
>>>> interface between gvt and hypervisor. 'clean_regions' will also be called if
>>>> failure in vgpu create. So I didn't implement it in vgpu detach.
>>>>
>>>
>>> "set_opregion" hypercall means hypervisor might have different ways to
>>> expose opregion, e.g through vfio region, etc. So that makes sense for a
>>> mpt interface. But here only care about cleanup of VFIO region, and both
>>> opregion and edid region's release function is noop..
>>>
>> Henry: So do you mean to expose the functions (clean_regions/set_edid) in
>> kvmgt instead of hypercall if the function is only valid for KVM?
>>
> 
> For clean region here, for me it looks we just need to fix it by clean
> vgpu regions in detach call, and for set_edid one mpt interface is ok
> and region cleanup in detach would also work in error handling path.
> 
Got it. Thanks for the comments!