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

[Zhenyu Wang <zhenyuw@linux.intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2761 bytes --]

On 2018.12.07 07:47:59 +0000, 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,
> > > +					     &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.
>

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx