Re: [PATCH v2 3/6] KVM: Add a module param to allow enabling virtualization when KVM is loaded

[Chao Gao <chao.gao@intel.com>]

On Thu, May 23, 2024 at 10:27:53AM +1200, Huang, Kai wrote:
>
>
>On 22/05/2024 2:28 pm, Sean Christopherson wrote:
>> Add an off-by-default module param, enable_virt_at_load, to let userspace
>> force virtualization to be enabled in hardware when KVM is initialized,
>> i.e. just before /dev/kvm is exposed to userspace.  Enabling virtualization
>> during KVM initialization allows userspace to avoid the additional latency
>> when creating/destroying the first/last VM.  Now that KVM uses the cpuhp
>> framework to do per-CPU enabling, the latency could be non-trivial as the
>> cpuhup bringup/teardown is serialized across CPUs, e.g. the latency could
>> be problematic for use case that need to spin up VMs quickly.
>
>How about we defer this until there's a real complain that this isn't
>acceptable?  To me it doesn't sound "latency of creating the first VM"
>matters a lot in the real CSP deployments.

I suspect kselftest and kvm-unit-tests will be impacted a lot because
hundreds of tests are run serially. And it looks clumsy to reload KVM
module to set enable_virt_at_load to make tests run faster. I think the
test slowdown is a more realistic problem than running an off-tree
hypervisor, so I vote to make enabling virtualization at load time the
default behavior and if we really want to support an off-tree hypervisor,
we can add a new module param to opt in enabling virtualization at runtime.

>
>The concern of adding a new module param is once we add it, we need to
>maintain it even it is no longer needed in the future for backward
>compatibility.  Especially this param is in kvm.ko, and for all ARCHs.
>
>E.g., I think _IF_ the core cpuhp code is enhanced to call those callbacks in
>parallel in cpuhp_setup_state(), then this issue could be mitigated to an
>unnoticeable level.
>
>Or we just still do:
>
>	cpus_read_lock();
>	on_each_cpu(hardware_enable_nolock, ...);
>	cpuhp_setup_state_nocalls_cpuslocked(...);
>	cpus_read_unlock();
>
>I think the main benefit of series is to put all virtualization enabling
>related things into one single function.  Whether using cpuhp_setup_state()
>or using on_each_cpu() shouldn't be the main point.
>