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

[Hang Yuan]

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 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.
>>>>
>>>
>>> "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!