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

[Hang Yuan]

On 1/14/19 11:56 AM, Zhenyu Wang wrote:
> On 2019.01.10 19:04:45 +0800, hang.yuan at linux.intel.com wrote:
>> From: Hang Yuan <hang.yuan at linux.intel.com>
>>
>> Add one hypercall to free VFIO region memory. Also call it in vgpu
>> destroy.
>>
>> Signed-off-by: Hang Yuan <hang.yuan at 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