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

[Hang Yuan]

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?

>>>> +
>>>>    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