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

[Chao Gao]

On Fri, May 24, 2024 at 11:11:37AM +1200, Huang, Kai wrote:
>
>
>On 23/05/2024 4:23 pm, Chao Gao wrote:
>> 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.
>
>I am not following why off-tree hypervisor is ever related to this.

Enabling virtualization at runtime was added to support an off-tree hypervisor
(see the commit below).  To me, supporting an off-tree hypervisor while KVM is
autoloaded is a niche usage. so, my preference is to make enabling
virtualization at runtime opt-in rather than the default.

commit 10474ae8945ce08622fd1f3464e55bd817bf2376
Author: Alexander Graf <agraf@xxxxxxx>
Date:   Tue Sep 15 11:37:46 2009 +0200

    KVM: Activate Virtualization On Demand

    X86 CPUs need to have some magic happening to enable the virtualization
    extensions on them. This magic can result in unpleasant results for
    users, like blocking other VMMs from working (vmx) or using invalid TLB
    entries (svm).

    Currently KVM activates virtualization when the respective kernel module
    is loaded. This blocks us from autoloading KVM modules without breaking
    other VMMs.

    To circumvent this problem at least a bit, this patch introduces on
    demand activation of virtualization. This means, that instead
    virtualization is enabled on creation of the first virtual machine
    and disabled on destruction of the last one.

    So using this, KVM can be easily autoloaded, while keeping other
    hypervisors usable.

>
>Could you elaborate?
>
>The problem of enabling virt during module loading by default is it impacts
>all ARCHs. Given this performance downgrade (if we care) can be resolved by
>explicitly doing on_each_cpu() below, I am not sure why we want to choose
>this radical approach.

IIUC, we plan to set up TDX module at KVM load time; we need to enable virt
at load time at least for TDX. Definitely, on_each_cpu() can solve the perf
concern. But a solution which can also satisfy TDX's need is better to me.

>
>
>> > 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.
>> >