| Message ID | 20200109171357.115936-5-julian.stecklina@cyberus-technology.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support for out-of-tree hypervisor modules in i915/gvt | expand |
On Thu, Jan 09, 2020 at 07:13:57PM +0200, Julian Stecklina wrote: > Now that the GVT interface to hypervisors does not depend on i915/GVT > internals anymore, we can move the headers to the global include/. > > This makes out-of-tree modules for hypervisor integration possible. What kind of out-of-tree modules do you need/want for this? And why do they somehow have to be out of the tree? We want them in the tree, and so should you, as it will save you time and money if they are. Also, as Christoph said, adding exports for functions that are not used by anything within the kernel tree itself is not ok, that's not how we work. thanks, greg k-h
Hi Greg, Christoph, On Wed, 2020-01-15 at 16:22 +0100, Greg KH wrote: > On Thu, Jan 09, 2020 at 07:13:57PM +0200, Julian Stecklina wrote: > > Now that the GVT interface to hypervisors does not depend on i915/GVT > > internals anymore, we can move the headers to the global include/. > > > > This makes out-of-tree modules for hypervisor integration possible. > > What kind of out-of-tree modules do you need/want for this? The mediated virtualization support in the i915 driver needs a backend to the hypervisor. There is currently one backend for KVM in the tree (drivers/gpu/drm/i915/gvt/kvmgt.c) and at least 3 other hypervisor backends out of tree in various states of development that I know of. We are currently developing one of these. > > Also, as Christoph said, adding exports for functions that are not used > by anything within the kernel tree itself is not ok, that's not how we > work. The exports are used by the KVM hypervisor backend. The patchset I sent basically decouples KVMGT from i915 driver internals. So personally I would count this as a benefit in itself. There is already an indirection in place that looks like it is intended to decouple the hypervisor backends from the i915 driver core: intel_gvt_ops. This is a struct of function pointers that the hypervisor backend uses to talk to the GPU mediator code. Unfortunately, this struct doesn't cover all usecases and the KVM hypervisor backend directly touches the i915 devices' internal state in very few places. My current solution was to wrap these accesses in accessor functions and EXPORT_SYMBOL_GPL them. If the more acceptable solution is to add more function pointers to intel_gvt_ops instead of exporting symbols, I'm happy to go along this route. > And why do they somehow have to be out of the tree? We want them in the > tree, and so should you, as it will save you time and money if they are. I also want these hypervisor backends in the tree, but from a development workflow having the ability to build them as a out-of-tree modules is very convenient. I guess this is also true for the developers working on the other hypervisor backends. When I looked at the status quo in i915/gvt a couple of weeks ago, it seemed like it would be a win for everyone. Let me just clearly say that we have no intention of doing binary blob drivers. :) Thanks, Julian [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/gvt/gvt.h#L555
On Thu, Jan 16, 2020 at 03:13:01PM +0100, Julian Stecklina wrote: > Hi Greg, Christoph, > > On Wed, 2020-01-15 at 16:22 +0100, Greg KH wrote: > > On Thu, Jan 09, 2020 at 07:13:57PM +0200, Julian Stecklina wrote: > > > Now that the GVT interface to hypervisors does not depend on i915/GVT > > > internals anymore, we can move the headers to the global include/. > > > > > > This makes out-of-tree modules for hypervisor integration possible. > > > > What kind of out-of-tree modules do you need/want for this? > > The mediated virtualization support in the i915 driver needs a backend to the > hypervisor. There is currently one backend for KVM in the tree > (drivers/gpu/drm/i915/gvt/kvmgt.c) and at least 3 other hypervisor backends out > of tree in various states of development that I know of. We are currently > developing one of these. Great, then just submit this patch series as part of your patch series when submitting yoru hypervisor code. That's the normal way to export new symbols, we can't do so without an in-kernel user. thanks, greg k-h
Hi Greg, On Thu, 2020-01-16 at 15:23 +0100, Greg KH wrote: > On Thu, Jan 16, 2020 at 03:13:01PM +0100, Julian Stecklina wrote: > > Hi Greg, Christoph, > > > > On Wed, 2020-01-15 at 16:22 +0100, Greg KH wrote: > > > On Thu, Jan 09, 2020 at 07:13:57PM +0200, Julian Stecklina wrote: > > > > Now that the GVT interface to hypervisors does not depend on i915/GVT > > > > internals anymore, we can move the headers to the global include/. > > > > > > > > This makes out-of-tree modules for hypervisor integration possible. > > > > > > What kind of out-of-tree modules do you need/want for this? > > > > The mediated virtualization support in the i915 driver needs a backend to > > the > > hypervisor. There is currently one backend for KVM in the tree > > (drivers/gpu/drm/i915/gvt/kvmgt.c) and at least 3 other hypervisor backends > > out > > of tree in various states of development that I know of. We are currently > > developing one of these. > > Great, then just submit this patch series as part of your patch series > when submitting yoru hypervisor code. That's the normal way to export > new symbols, we can't do so without an in-kernel user. Fair enough. As I already said, the KVMGT code is the in-kernel user. But I guess I can extend the already existing function pointer way of decoupling KVMGT from i915 and be on my way without exporting any symbols. Somewhat independent of the current discussion, I also think that it's valuable to have a defined API (I'm not saying stable API) for the hypervisor backends to define what's okay and not okay for them to do. Thanks, Julian
On Thu, Jan 16, 2020 at 04:05:22PM +0100, Julian Stecklina wrote: > Hi Greg, > > On Thu, 2020-01-16 at 15:23 +0100, Greg KH wrote: > > On Thu, Jan 16, 2020 at 03:13:01PM +0100, Julian Stecklina wrote: > > > Hi Greg, Christoph, > > > > > > On Wed, 2020-01-15 at 16:22 +0100, Greg KH wrote: > > > > On Thu, Jan 09, 2020 at 07:13:57PM +0200, Julian Stecklina wrote: > > > > > Now that the GVT interface to hypervisors does not depend on i915/GVT > > > > > internals anymore, we can move the headers to the global include/. > > > > > > > > > > This makes out-of-tree modules for hypervisor integration possible. > > > > > > > > What kind of out-of-tree modules do you need/want for this? > > > > > > The mediated virtualization support in the i915 driver needs a backend to > > > the > > > hypervisor. There is currently one backend for KVM in the tree > > > (drivers/gpu/drm/i915/gvt/kvmgt.c) and at least 3 other hypervisor backends > > > out > > > of tree in various states of development that I know of. We are currently > > > developing one of these. > > > > Great, then just submit this patch series as part of your patch series > > when submitting yoru hypervisor code. That's the normal way to export > > new symbols, we can't do so without an in-kernel user. > > Fair enough. > > As I already said, the KVMGT code is the in-kernel user. But I guess I can > extend the already existing function pointer way of decoupling KVMGT from i915 > and be on my way without exporting any symbols. > > Somewhat independent of the current discussion, I also think that it's valuable > to have a defined API (I'm not saying stable API) for the hypervisor backends to > define what's okay and not okay for them to do. The only way to get a "good" api is for at least 3 users of them get into the kernel tree. If all you have is one or two, then you go with what you got, and evolve over time as more get added and find better ways to use them. In short, it's just basic evolution, not intelligent design :) thanks, greg k-h
On 2020.01.16 15:13:01 +0100, Julian Stecklina wrote: > Hi Greg, Christoph, > > On Wed, 2020-01-15 at 16:22 +0100, Greg KH wrote: > > On Thu, Jan 09, 2020 at 07:13:57PM +0200, Julian Stecklina wrote: > > > Now that the GVT interface to hypervisors does not depend on i915/GVT > > > internals anymore, we can move the headers to the global include/. > > > > > > This makes out-of-tree modules for hypervisor integration possible. > > > > What kind of out-of-tree modules do you need/want for this? > > The mediated virtualization support in the i915 driver needs a backend to the > hypervisor. There is currently one backend for KVM in the tree > (drivers/gpu/drm/i915/gvt/kvmgt.c) and at least 3 other hypervisor backends out > of tree in various states of development that I know of. We are currently > developing one of these. > > > > > Also, as Christoph said, adding exports for functions that are not used > > by anything within the kernel tree itself is not ok, that's not how we > > work. > > The exports are used by the KVM hypervisor backend. The patchset I sent > basically decouples KVMGT from i915 driver internals. So personally I would > count this as a benefit in itself. > > There is already an indirection in place that looks like it is intended to > decouple the hypervisor backends from the i915 driver core: intel_gvt_ops. This > is a struct of function pointers that the hypervisor backend uses to talk to the > GPU mediator code. > > Unfortunately, this struct doesn't cover all usecases and the KVM hypervisor > backend directly touches the i915 devices' internal state in very few places. My > current solution was to wrap these accesses in accessor functions and > EXPORT_SYMBOL_GPL them. > > If the more acceptable solution is to add more function pointers to > intel_gvt_ops instead of exporting symbols, I'm happy to go along this route. > That depends on the hypervisor requirement and purpose, if it requires gvt device model for some function e.g emulate mmio, we want it to be a general gvt_ops, if it just trys to retrieve some vgpu info, we might see if some common wrapper of internal data would be more easier. > > And why do they somehow have to be out of the tree? We want them in the > > tree, and so should you, as it will save you time and money if they are. > > I also want these hypervisor backends in the tree, but from a development > workflow having the ability to build them as a out-of-tree modules is very > convenient. I guess this is also true for the developers working on the other > hypervisor backends. > > When I looked at the status quo in i915/gvt a couple of weeks ago, it seemed > like it would be a win for everyone. Let me just clearly say that we have no > intention of doing binary blob drivers. :) > yeah, we do like to see more hypervisor support and make more clear interface between core device model with that. thanks
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h index f9693c44e342..d09374aa7710 100644 --- a/drivers/gpu/drm/i915/gvt/gvt.h +++ b/drivers/gpu/drm/i915/gvt/gvt.h @@ -33,9 +33,8 @@ #ifndef _GVT_H_ #define _GVT_H_ -#include "gvt_public.h" +#include <drm/i915_gvt.h> #include "debug.h" -#include "hypercall.h" #include "mmio.h" #include "reg.h" #include "interrupt.h" diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index f5157211d45f..280d69ca964b 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -28,6 +28,7 @@ * Xiaoguang Chen <xiaoguang.chen@intel.com> */ +#include <drm/i915_gvt.h> #include <drm/drm_edid.h> #include <linux/init.h> #include <linux/device.h> @@ -52,7 +53,6 @@ #include <linux/nospec.h> #include "debug.h" -#include "gvt_public.h" static const struct intel_gvt_ops *intel_gvt_ops; diff --git a/drivers/gpu/drm/i915/gvt/gvt_public.h b/include/drm/i915_gvt.h similarity index 97% rename from drivers/gpu/drm/i915/gvt/gvt_public.h rename to include/drm/i915_gvt.h index 23bf1235e1a1..3926ca32f773 100644 --- a/drivers/gpu/drm/i915/gvt/gvt_public.h +++ b/include/drm/i915_gvt.h @@ -21,10 +21,10 @@ * SOFTWARE. */ -#ifndef _GVT_PUBLIC_H_ -#define _GVT_PUBLIC_H_ +#ifndef _I915_GVT_H_ +#define _I915_GVT_H_ -#include "hypercall.h" +#include <drm/i915_gvt_hypercall.h> struct attribute; struct attribute_group; @@ -101,4 +101,4 @@ bool intel_gvt_in_gtt(struct intel_vgpu *vgpu, u64 off); struct dentry *intel_vgpu_debugfs(struct intel_vgpu *vgpu); -#endif /* _GVT_PUBLIC_H_ */ +#endif /* _I915_GVT_H_ */ diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/include/drm/i915_gvt_hypercall.h similarity index 95% rename from drivers/gpu/drm/i915/gvt/hypercall.h rename to include/drm/i915_gvt_hypercall.h index 7ed33e4919a3..c26eef7dbdde 100644 --- a/drivers/gpu/drm/i915/gvt/hypercall.h +++ b/include/drm/i915_gvt_hypercall.h @@ -30,8 +30,12 @@ * */ -#ifndef _GVT_HYPERCALL_H_ -#define _GVT_HYPERCALL_H_ +#ifndef _I915_GVT_HYPERCALL_H_ +#define _I915_GVT_HYPERCALL_H_ + +#include <linux/types.h> + +struct device; #include <linux/types.h> @@ -84,4 +88,4 @@ extern struct intel_gvt_mpt xengt_mpt; int intel_gvt_register_hypervisor(struct intel_gvt_mpt *); void intel_gvt_unregister_hypervisor(void); -#endif /* _GVT_HYPERCALL_H_ */ +#endif /* _I915_GVT_HYPERCALL_H_ */
Now that the GVT interface to hypervisors does not depend on i915/GVT internals anymore, we can move the headers to the global include/. This makes out-of-tree modules for hypervisor integration possible. Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Signed-off-by: Julian Stecklina <julian.stecklina@cyberus-technology.de> --- drivers/gpu/drm/i915/gvt/gvt.h | 3 +-- drivers/gpu/drm/i915/gvt/kvmgt.c | 2 +- .../i915/gvt/gvt_public.h => include/drm/i915_gvt.h | 8 ++++---- .../hypercall.h => include/drm/i915_gvt_hypercall.h | 10 +++++++--- 4 files changed, 13 insertions(+), 10 deletions(-) rename drivers/gpu/drm/i915/gvt/gvt_public.h => include/drm/i915_gvt.h (97%) rename drivers/gpu/drm/i915/gvt/hypercall.h => include/drm/i915_gvt_hypercall.h (95%)