Re: [PATCH v4] drm/i915/gvt: Change KVMGT as self load module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Zhenyu,
Overall I think the impact to AcrnGT is not big, we can modify our code to adapt
to the new interfaces. 
But I have some comments embedded. 

> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@xxxxxxxxxxxxxxx]
> Sent: Thursday, December 6, 2018 12:28 PM
> To: Yuan, Hang <hang.yuan@xxxxxxxxx>; Wang, Zhi A
> <zhi.a.wang@xxxxxxxxx>; Zhang, Xiong Y <xiong.y.zhang@xxxxxxxxx>; He,
> Min <min.he@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Alex Williamson
> <alex.williamson@xxxxxxxxxx>; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4] drm/i915/gvt: Change KVMGT as self load module
> 
> 
> Ping for any more comments? Or ack?
> 
> Adding Min, for head-up notify, I've moved 'kvmgt' into self load
> module instead of loaded by i915/gvt, in order to clean up the
> dependence, so need to load 'kvmgt.ko' to enable GVT through
> VFIO/mdev. Hypervisor module needs to call new register/unregister
> function to initialize hypervisor specific interface for GVT. If
> AcrnGT rebase on this, it may need to change initial setup too, so
> could you double check?
> 
> thanks
> 
> On 2018.12.04 10:40:28 +0800, Zhenyu Wang wrote:
> > This trys to make 'kvmgt' module as self loadable instead of loading
> > by i915/gvt device model. So hypervisor specific module could be
> > stand-alone, e.g only after loading hypervisor specific module, GVT
> > feature could be enabled via specific hypervisor interface, e.g VFIO/mdev.
> >
> > So this trys to use hypervisor module register/unregister interface
> > for that. Hypervisor module needs to take care of module reference
> > itself when working for hypervisor interface, e.g for VFIO/mdev,
> > hypervisor module would reference counting mdev when open and release.
> >
> > This makes 'kvmgt' module really split from GVT device model. User
> > needs to load 'kvmgt' to enable VFIO/mdev interface.
> >
> > v4:
> > - fix checkpatch warning
> >
> > v3:
> > - Fix module reference handling for device open and release. Unused
> >   mdev devices would be cleaned up in device unregister when module
> unload.
> >
> > v2:
> > - Fix kvmgt order after i915 for built-in case
> >
> > Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Signed-off-by: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/Makefile        |   1 +
> >  drivers/gpu/drm/i915/gvt/Makefile    |   1 -
> >  drivers/gpu/drm/i915/gvt/gvt.c       | 107 +++++++++++----------------
> >  drivers/gpu/drm/i915/gvt/gvt.h       |   6 +-
> >  drivers/gpu/drm/i915/gvt/hypercall.h |   7 +-
> >  drivers/gpu/drm/i915/gvt/kvmgt.c     |  21 +++++-
> >  drivers/gpu/drm/i915/gvt/mpt.h       |   3 +
> >  drivers/gpu/drm/i915/intel_gvt.c     |   9 ---
> >  8 files changed, 72 insertions(+), 83 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile
> b/drivers/gpu/drm/i915/Makefile
> > index 0ff878c994e2..ae0d975a6f34 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -195,3 +195,4 @@ endif
> >  i915-y += intel_lpe_audio.o
> >
> >  obj-$(CONFIG_DRM_I915) += i915.o
> > +obj-$(CONFIG_DRM_I915_GVT_KVMGT) += gvt/kvmgt.o
> > diff --git a/drivers/gpu/drm/i915/gvt/Makefile
> b/drivers/gpu/drm/i915/gvt/Makefile
> > index b016dc753db9..271fb46d4dd0 100644
> > --- a/drivers/gpu/drm/i915/gvt/Makefile
> > +++ b/drivers/gpu/drm/i915/gvt/Makefile
> > @@ -7,4 +7,3 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o
> trace_points.o firmware.o \
> >
> >  ccflags-y				+= -I$(src) -I$(src)/$(GVT_DIR)
> >  i915-y					+= $(addprefix $(GVT_DIR)/,
> $(GVT_SOURCE))
> > -obj-$(CONFIG_DRM_I915_GVT_KVMGT)	+= $(GVT_DIR)/kvmgt.o
> > diff --git a/drivers/gpu/drm/i915/gvt/gvt.c
> b/drivers/gpu/drm/i915/gvt/gvt.c
> > index a5b760b7bc10..e1c9c20918af 100644
> > --- a/drivers/gpu/drm/i915/gvt/gvt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> > @@ -187,52 +187,6 @@ static const struct intel_gvt_ops intel_gvt_ops = {
> >  	.write_protect_handler = intel_vgpu_page_track_handler,
> >  };
> >
> > -/**
> > - * intel_gvt_init_host - Load MPT modules and detect if we're running in
> host
> > - *
> > - * This function is called at the driver loading stage. If failed to find a
> > - * loadable MPT module or detect currently we're running in a VM, then
> GVT-g
> > - * will be disabled
> > - *
> > - * Returns:
> > - * Zero on success, negative error code if failed.
> > - *
> > - */
> > -int intel_gvt_init_host(void)
> > -{
> > -	if (intel_gvt_host.initialized)
> > -		return 0;
> > -
> > -	/* Xen DOM U */
> > -	if (xen_domain() && !xen_initial_domain())
> > -		return -ENODEV;
> > -
> > -	/* Try to load MPT modules for hypervisors */
> > -	if (xen_initial_domain()) {
> > -		/* In Xen dom0 */
> > -		intel_gvt_host.mpt = try_then_request_module(
> > -				symbol_get(xengt_mpt), "xengt");
> > -		intel_gvt_host.hypervisor_type =
> INTEL_GVT_HYPERVISOR_XEN;
> > -	} else {
> > -#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT)
> > -		/* not in Xen. Try KVMGT */
> > -		intel_gvt_host.mpt = try_then_request_module(
> > -				symbol_get(kvmgt_mpt), "kvmgt");
> > -		intel_gvt_host.hypervisor_type =
> INTEL_GVT_HYPERVISOR_KVM;
> > -#endif
> > -	}
> > -
> > -	/* Fail to load MPT modules - bail out */
> > -	if (!intel_gvt_host.mpt)
> > -		return -EINVAL;
> > -
> > -	gvt_dbg_core("Running with hypervisor %s in host mode\n",
> > -
> 	supported_hypervisors[intel_gvt_host.hypervisor_type]);
> > -
> > -	intel_gvt_host.initialized = true;
> > -	return 0;
> > -}
> > -
> >  static void init_device_info(struct intel_gvt *gvt)
> >  {
> >  	struct intel_gvt_device_info *info = &gvt->device_info;
> > @@ -316,7 +270,6 @@ void intel_gvt_clean_device(struct
> drm_i915_private *dev_priv)
> >  		return;
> >
> >  	intel_gvt_destroy_idle_vgpu(gvt->idle_vgpu);
> > -	intel_gvt_hypervisor_host_exit(&dev_priv->drm.pdev->dev);
> >  	intel_gvt_cleanup_vgpu_type_groups(gvt);
> >  	intel_gvt_clean_vgpu_types(gvt);
> >
> > @@ -352,13 +305,6 @@ int intel_gvt_init_device(struct drm_i915_private
> *dev_priv)
> >  	struct intel_vgpu *vgpu;
> >  	int ret;
> >
> > -	/*
> > -	 * Cannot initialize GVT device without intel_gvt_host gets
> > -	 * initialized first.
> > -	 */
> > -	if (WARN_ON(!intel_gvt_host.initialized))
> > -		return -EINVAL;
> > -
> >  	if (WARN_ON(dev_priv->gvt))
> >  		return -EEXIST;
> >
> > @@ -420,13 +366,6 @@ int intel_gvt_init_device(struct drm_i915_private
> *dev_priv)
> >  		goto out_clean_types;
> >  	}
> >
> > -	ret = intel_gvt_hypervisor_host_init(&dev_priv->drm.pdev->dev, gvt,
> > -				&intel_gvt_ops);
> > -	if (ret) {
> > -		gvt_err("failed to register gvt-g host device: %d\n", ret);
> > -		goto out_clean_types;
> > -	}
> > -
> >  	vgpu = intel_gvt_create_idle_vgpu(gvt);
> >  	if (IS_ERR(vgpu)) {
> >  		ret = PTR_ERR(vgpu);
> > @@ -441,6 +380,8 @@ int intel_gvt_init_device(struct drm_i915_private
> *dev_priv)
> >
> >  	gvt_dbg_core("gvt device initialization is done\n");
> >  	dev_priv->gvt = gvt;
> > +	intel_gvt_host.dev = &dev_priv->drm.pdev->dev;
> > +	intel_gvt_host.initialized = true;
> >  	return 0;
> >
> >  out_clean_types:
> > @@ -467,6 +408,44 @@ int intel_gvt_init_device(struct drm_i915_private
> *dev_priv)
> >  	return ret;
> >  }
> >
> > -#if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT)
> > -MODULE_SOFTDEP("pre: kvmgt");
> > -#endif
> > +int
> > +intel_gvt_register_hypervisor(struct intel_gvt_mpt *m)
> > +{
> > +	int ret;
> > +	void *gvt;
> > +
> > +	if (!intel_gvt_host.initialized)
> > +		return -ENODEV;
> > +
> > +	if (m->type != INTEL_GVT_HYPERVISOR_KVM &&
> > +	    m->type != INTEL_GVT_HYPERVISOR_XEN)
> > +		return -EINVAL;
> > +
> > +	/* Get a reference for device model module */
> > +	if (!try_module_get(THIS_MODULE))
> > +		return -ENODEV;
> > +
> > +	intel_gvt_host.mpt = m;
> > +	intel_gvt_host.hypervisor_type = m->type;
> > +	gvt = (void *)kdev_to_i915(intel_gvt_host.dev)->gvt;
> > +
> > +	ret = intel_gvt_hypervisor_host_init(intel_gvt_host.dev, gvt,
> > +					     &intel_gvt_ops);
I think we can remove the host_init and host_exit interfaces, since now
it's mpt modules who proactively call the GVT-g to initialize and un-initialize,
Thus we can return the intel_gvt_ops in intel_gvt_register_hypervisor() and
moduels initialize its rest part. 
Current kvmgt_init-> intel_gvt_register_hypervisor->host_init seems a little bit
redundant.
But it's up to you to make the call to remove it in this patch or other furture
optimization patches.

> > +	if (ret < 0) {
> > +		gvt_err("Failed to init %s hypervisor module\n",
> > +
> 	supported_hypervisors[intel_gvt_host.hypervisor_type]);
> > +		return -ENODEV;
> > +	}
> > +	gvt_dbg_core("Running with hypervisor %s in host mode\n",
> > +		     supported_hypervisors[intel_gvt_host.hypervisor_type]);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(intel_gvt_register_hypervisor);
> > +
> > +void
> > +intel_gvt_unregister_hypervisor(void)
> > +{
> > +	intel_gvt_hypervisor_host_exit(intel_gvt_host.dev);
> > +	module_put(THIS_MODULE);
> > +}
> > +EXPORT_SYMBOL_GPL(intel_gvt_unregister_hypervisor);
> > diff --git a/drivers/gpu/drm/i915/gvt/gvt.h
> b/drivers/gpu/drm/i915/gvt/gvt.h
> > index b4ab1dad0143..8a4cf995d755 100644
> > --- a/drivers/gpu/drm/i915/gvt/gvt.h
> > +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> > @@ -52,12 +52,8 @@
> >
> >  #define GVT_MAX_VGPU 8
> >
> > -enum {
> > -	INTEL_GVT_HYPERVISOR_XEN = 0,
> > -	INTEL_GVT_HYPERVISOR_KVM,
> > -};
> > -
> >  struct intel_gvt_host {
> > +	struct device *dev;
> >  	bool initialized;
> >  	int hypervisor_type;
> >  	struct intel_gvt_mpt *mpt;
> > diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h
> b/drivers/gpu/drm/i915/gvt/hypercall.h
> > index e49a9247ed78..50798868ab15 100644
> > --- a/drivers/gpu/drm/i915/gvt/hypercall.h
> > +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
> > @@ -33,11 +33,17 @@
> >  #ifndef _GVT_HYPERCALL_H_
> >  #define _GVT_HYPERCALL_H_
> >
> > +enum hypervisor_type {
> > +	INTEL_GVT_HYPERVISOR_XEN = 0,
> > +	INTEL_GVT_HYPERVISOR_KVM,
> > +};
> > +
> >  /*
> >   * Specific GVT-g MPT modules function collections. Currently GVT-g
> supports
> >   * both Xen and KVM by providing dedicated hypervisor-related MPT
> modules.
> >   */
> >  struct intel_gvt_mpt {
> > +	enum hypervisor_type type;
> >  	int (*host_init)(struct device *dev, void *gvt, const void *ops);
> >  	void (*host_exit)(struct device *dev);
> >  	int (*attach_vgpu)(void *vgpu, unsigned long *handle);
> > @@ -67,6 +73,5 @@ struct intel_gvt_mpt {
> >  };
> >
> >  extern struct intel_gvt_mpt xengt_mpt;
We can remove this definition, too. 

> > -extern struct intel_gvt_mpt kvmgt_mpt;
> >
> >  #endif /* _GVT_HYPERCALL_H_ */
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index 1bbd04d30c42..ef248d577e49 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -50,6 +50,7 @@
> >  #include "gvt.h"
> >
> >  static const struct intel_gvt_ops *intel_gvt_ops;
> > +static struct intel_gvt *intel_gvt;
This variable intel_gvt seems useless.

> >
> >  /* helper macros copied from vfio-pci */
> >  #define VFIO_PCI_OFFSET_SHIFT   40
> > @@ -627,6 +628,12 @@ static int intel_vgpu_open(struct mdev_device
> *mdev)
> >  		goto undo_iommu;
> >  	}
> >
> > +	/* Take a module reference as mdev core doesn't take
> > +	 * a reference for vendor driver.
> > +	 */
> > +	if (!try_module_get(THIS_MODULE))
> > +		goto undo_group;
> > +
> >  	ret = kvmgt_guest_init(mdev);
> >  	if (ret)
> >  		goto undo_group;
> > @@ -679,6 +686,9 @@ static void __intel_vgpu_release(struct intel_vgpu
> *vgpu)
> >  					&vgpu->vdev.group_notifier);
> >  	WARN(ret, "vfio_unregister_notifier for group failed: %d\n", ret);
> >
> > +	/* dereference module reference taken at open */
> > +	module_put(THIS_MODULE);
> > +
> >  	info = (struct kvmgt_guest_info *)vgpu->handle;
> >  	kvmgt_guest_exit(info);
> >
> > @@ -1459,8 +1469,10 @@ static int kvmgt_host_init(struct device *dev,
> void *gvt, const void *ops)
> >  	struct attribute_group **kvm_vgpu_type_groups;
> >
> >  	intel_gvt_ops = ops;
> > +	intel_gvt = (struct intel_gvt *)gvt;
> > +
> >  	if (!intel_gvt_ops->get_gvt_attrs(&kvm_type_attrs,
> > -			&kvm_vgpu_type_groups))
> > +					  &kvm_vgpu_type_groups))
Unrelated to this patch.

> >  		return -EFAULT;
> >  	intel_vgpu_ops.supported_type_groups = kvm_vgpu_type_groups;
> >
> > @@ -1849,7 +1861,8 @@ static bool kvmgt_is_valid_gfn(unsigned long
> handle, unsigned long gfn)
> >  	return ret;
> >  }
> >
> > -struct intel_gvt_mpt kvmgt_mpt = {
> > +static struct intel_gvt_mpt kvmgt_mpt = {
> > +	.type = INTEL_GVT_HYPERVISOR_KVM,
> >  	.host_init = kvmgt_host_init,
> >  	.host_exit = kvmgt_host_exit,
> >  	.attach_vgpu = kvmgt_attach_vgpu,
> > @@ -1868,15 +1881,17 @@ struct intel_gvt_mpt kvmgt_mpt = {
> >  	.put_vfio_device = kvmgt_put_vfio_device,
> >  	.is_valid_gfn = kvmgt_is_valid_gfn,
> >  };
> > -EXPORT_SYMBOL_GPL(kvmgt_mpt);
> >
> >  static int __init kvmgt_init(void)
> >  {
> > +	if (intel_gvt_register_hypervisor(&kvmgt_mpt) < 0)
> > +		return -ENODEV;
> >  	return 0;
> >  }
> >
> >  static void __exit kvmgt_exit(void)
> >  {
> > +	intel_gvt_unregister_hypervisor();
> >  }
> >
> >  module_init(kvmgt_init);
> > diff --git a/drivers/gpu/drm/i915/gvt/mpt.h
> b/drivers/gpu/drm/i915/gvt/mpt.h
> > index c95ef77da62c..9b4225d44243 100644
> > --- a/drivers/gpu/drm/i915/gvt/mpt.h
> > +++ b/drivers/gpu/drm/i915/gvt/mpt.h
> > @@ -360,4 +360,7 @@ static inline bool
> intel_gvt_hypervisor_is_valid_gfn(
> >  	return intel_gvt_host.mpt->is_valid_gfn(vgpu->handle, gfn);
> >  }
> >
> > +int intel_gvt_register_hypervisor(struct intel_gvt_mpt *);
> > +void intel_gvt_unregister_hypervisor(void);
> > +
> >  #endif /* _GVT_MPT_H_ */
> > diff --git a/drivers/gpu/drm/i915/intel_gvt.c
> b/drivers/gpu/drm/i915/intel_gvt.c
> > index c22b3e18a0f5..d74e59e22c9d 100644
> > --- a/drivers/gpu/drm/i915/intel_gvt.c
> > +++ b/drivers/gpu/drm/i915/intel_gvt.c
> > @@ -105,15 +105,6 @@ int intel_gvt_init(struct drm_i915_private
> *dev_priv)
> >  		return -EIO;
> >  	}
> >
> > -	/*
> > -	 * We're not in host or fail to find a MPT module, disable GVT-g
> > -	 */
> > -	ret = intel_gvt_init_host();
> > -	if (ret) {
> > -		DRM_DEBUG_DRIVER("Not in host or MPT modules not
> found\n");
> > -		goto bail;
> > -	}
> > -
> >  	ret = intel_gvt_init_device(dev_priv);
> >  	if (ret) {
> >  		DRM_DEBUG_DRIVER("Fail to init GVT device\n");
> > --
> > 2.19.1
> >
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux