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
