[PATCH v1 1/4] drm/i915/gvt: add one hypercall to clean vfio regions
Tian, Kevin
kevin.tian at intel.com
Mon Jan 14 05:48:13 UTC 2019
> 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 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.
> >>>>
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 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
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
More information about the intel-gvt-dev
mailing list