Re: [PATCH 06/29] drm/i915/gvt: move the gvt code into kvmgt.ko

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

+ Thomas, Maarten and Matt

(Also, Zhi and Zhenyu, please see down)

Quoting Christoph Hellwig (2021-11-02 09:05:38)
> Instead of having an option to build the gvt code into the main i915
> module, just move it into the kvmgt.ko module.  This only requires
> a new struct with three entries that the KVMGT modules needs to register
> with the main i915 module, and a proper list of GVT-enabled devices
> instead of global device pointer.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

<SNIP>

> +/*
> + * Exported here so that the exports only get created when GVT support is
> + * actually enabled.
> + */
> +EXPORT_SYMBOL_NS_GPL(i915_gem_object_alloc, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_object_create_shmem, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_object_init, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_object_ggtt_pin_ww, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_object_pin_map, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_object_set_to_cpu_domain, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(__i915_gem_object_flush_map, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(__i915_gem_object_set_pages, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_gtt_insert, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_prime_export, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_ww_ctx_init, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_ww_ctx_backoff, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_gem_ww_ctx_fini, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_ppgtt_create, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_request_add, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_request_create, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_request_wait, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_reserve_fence, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_unreserve_fence, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_vm_release, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(i915_vma_move_to_active, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(intel_context_create, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(__intel_context_do_pin, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(__intel_context_do_unpin, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(intel_ring_begin, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(intel_runtime_pm_get, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(intel_runtime_pm_put_unchecked, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(intel_uncore_forcewake_for_reg, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(intel_uncore_forcewake_get, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(intel_uncore_forcewake_put, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(shmem_pin_map, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(shmem_unpin_map, I915_GVT);
> +EXPORT_SYMBOL_NS_GPL(__px_dma, I915_GVT);

This is where the module conversion got stuck last time.

Conditionally exporting is kind of a partial remedy. Previously the
intention for the rewrite was to define a clear interface between the
two modules which would be well defined enough that we could avoid frequent
clashes and hopefully get GVT into drm-tip, too.

The absolute minimum requirement is not to have any of the double
underscore symbols in this list. As that convention is specifically used
to mark functions which are expected to have reduced error checking
because of the exact point they are being called from. With that done
we should be where we can enable the exports by default and modprobe
would be all that is required.

I think Zhenyu and Zhi took an AR back in time to see how small they
could shrink this list. Would we have some followup patches available
to shrink this interface?

Also, the golden MMIO state capture remains something that leaks the
hypervisor state into the guests. For example the fact which bug W/A
are applied in hypervisor, and I would rather have that code path
isolated before enabling by default.

Regards, Joonas