Re: [Intel-gfx] [PATCH v4] drm/i915/gvt: Change KVMGT as self load module
On 2018.12.07 07:47:59 +, He, Min wrote:
> 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.
>
Good!
> > > @@ -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,
> > > + _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.
>
I'd like to keep as in current approach, as it can keep hypervisor init
function in one place instead of passing gvt host info to hypervisor module.
And calling submodule's init function is fine process I think.
> > > @@ -67,6 +73,5 @@ struct intel_gvt_mpt {
> > > };
> > >
> > > extern struct intel_gvt_mpt xengt_mpt;
> We can remove this definition, too.
>
yeah, but maybe in another xengt cleanup patch.
> > > 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.
>
Thanks for pointing this out, was trying to use for exit path,
but looks I missed to remove it finally.
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] drm/i915/gvt: Change KVMGT as self load module
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:zhen...@linux.intel.com]
> Sent: Thursday, December 6, 2018 12:28 PM
> To: Yuan, Hang ; Wang, Zhi A
> ; Zhang, Xiong Y ; He,
> Min
> Cc: intel-gfx@lists.freedesktop.org; Alex Williamson
> ; intel-gvt-...@lists.freedesktop.org
> 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
> > Signed-off-by: Zhenyu Wang
> > ---
> > 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(
> > -
Re: [Intel-gfx] [PATCH v4] drm/i915/gvt: Change KVMGT as self load module
> -Original Message-
> From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> Behalf Of Zhenyu Wang
> Sent: Tuesday, December 4, 2018 10:40 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Alex Williamson ; intel-gvt-
> d...@lists.freedesktop.org
> Subject: [PATCH v4] drm/i915/gvt: Change KVMGT as self load module
>
> 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
> Signed-off-by: Zhenyu Wang
> ---
> 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 = >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(_priv->drm.pdev->dev);
>
Re: [Intel-gfx] [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
> Signed-off-by: Zhenyu Wang
> ---
> 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 = >device_info;
> @@ -316,7 +270,6 @@ void intel_gvt_clean_device(struct
