diff mbox series

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

Message ID 20240522022827.1690416-4-seanjc@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: Register cpuhp/syscore callbacks when enabling virt | expand

Commit Message

Sean Christopherson May 22, 2024, 2:28 a.m. UTC
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.

Enabling virtualizaton during initialization will also allow KVM to setup
the Intel TDX Module, which requires VMX to be fully enabled, without
needing additional APIs to temporarily enable virtualization.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Kai Huang May 22, 2024, 10:27 p.m. UTC | #1
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.

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.
Chao Gao May 23, 2024, 4:23 a.m. UTC | #2
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.
>
Kai Huang May 23, 2024, 11:11 p.m. UTC | #3
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.

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.


>> 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.
>>
Chao Gao May 24, 2024, 2:39 a.m. UTC | #4
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@suse.de>
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.
>> >
Kai Huang May 27, 2024, 10:36 p.m. UTC | #5
On 24/05/2024 2:39 pm, Chao Gao wrote:
> 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).  

Oh, ok.  I was thinking something else.

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

Doing on_each_cpu() explicitly can also meet TDX's purpose.  We just 
explicitly enable virtualization during module loading if we are going 
to enable TDX.  For all other cases, the behaivour remains the same, 
unless they want to change when to enable virtualization, e.g., when 
loading module by default.

For always, or by default enabling virtualization during module loading, 
we somehow discussed before:

https://lore.kernel.org/kvm/ZiKoqMk-wZKdiar9@google.com/

My true comment is introducing a module parameter, which is a userspace 
ABI, to just fix some performance downgrade seems overkill when it can 
be mitigated by the kernel.
Sean Christopherson May 29, 2024, 3:01 p.m. UTC | #6
On Tue, May 28, 2024, Kai Huang wrote:
> On 24/05/2024 2:39 pm, Chao Gao wrote:
> > 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 definitely don't object to making it the default behavior, though I would do so
in a separate patch, e.g. in case enabling virtualization by default somehow
causes problems.

We could also add a Kconfig to control the default behavior, though IMO that'd be
overkill without an actual use case for having virtualization off by default.

> > > 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).
> 
> Oh, ok.  I was thinking something else.
> 
> > > 
> > > The problem of enabling virt during module loading by default is it impacts
> > > all ARCHs.

Pratically speaking, Intel is the only vendor where enabling virtualization is
interesting enough for anyone to care.  On SVM and all other architectures,
enabling virtualization doesn't meaningfully change the functionality of the
current mode.

And impacting all architectures isn't a bad thing.  Yes, it requires getting buy-in
from more people, and maybe additional testing, though in this case we should get
that for "free" by virtue of being in linux-next.  But those are one-time costs,
and not particular high costs.

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

Because it's not radical?  And manually doing on_each_cpu() requires complexity
that we would ideally avoid.

15+ years ago, when VMX and SVM were nascent technologies, there was likely a
good argument from a security perspective for leaving virtualization disabled.
E.g. the ucode flows were new _and_ massive, and thus a potentially juicy attack
surface.

But these days, AFAIK enabling virtualization is not considered to be a security
risk, nor are there performance or stability downsides.  E.g. it's not all that
different than the kernel enabling CR4.PKE even though it's entirely possible
userspace will never actually use protection keys.

> > 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.
> > 
> 
> Doing on_each_cpu() explicitly can also meet TDX's purpose.  We just
> explicitly enable virtualization during module loading if we are going to
> enable TDX.  For all other cases, the behaivour remains the same, unless
> they want to change when to enable virtualization, e.g., when loading module
> by default.
> 
> For always, or by default enabling virtualization during module loading, we
> somehow discussed before:
> 
> https://lore.kernel.org/kvm/ZiKoqMk-wZKdiar9@google.com/
> 
> My true comment is introducing a module parameter, which is a userspace ABI,

Module params aren't strictly ABI, and even if they were, this would only be
problematic if we wanted to remove the module param *and* doing so was a breaking
change.  Enabling virtualization should be entirely transparent to userspace,
at least from a functional perspective; if changing how KVM enables virtualization
breaks userspace then we likely have bigger problems.

> to just fix some performance downgrade seems overkill when it can be
> mitigated by the kernel.

Performance is secondary for me, the primary motivation is simplifying the overall
KVM code base.  Yes, we _could_ use on_each_cpu() and enable virtualization
on-demand for TDX, but as above, it's extra complexity without any meaningful
benefit, at least AFAICT.
Kai Huang May 29, 2024, 10:45 p.m. UTC | #7
On Wed, 2024-05-29 at 08:01 -0700, Sean Christopherson wrote:
> On Tue, May 28, 2024, Kai Huang wrote:
> > On 24/05/2024 2:39 pm, Chao Gao wrote:
> > > 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 definitely don't object to making it the default behavior, though I would do so
> in a separate patch, e.g. in case enabling virtualization by default somehow
> causes problems.
> 
> We could also add a Kconfig to control the default behavior, though IMO that'd be
> overkill without an actual use case for having virtualization off by default.
> 
> > > > 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).
> > 
> > Oh, ok.  I was thinking something else.
> > 
> > > > 
> > > > The problem of enabling virt during module loading by default is it impacts
> > > > all ARCHs.
> 
> Pratically speaking, Intel is the only vendor where enabling virtualization is
> interesting enough for anyone to care.  On SVM and all other architectures,
> enabling virtualization doesn't meaningfully change the functionality of the
> current mode.
> 
> And impacting all architectures isn't a bad thing.  Yes, it requires getting buy-in
> from more people, and maybe additional testing, though in this case we should get
> that for "free" by virtue of being in linux-next.  But those are one-time costs,
> and not particular high costs.
> 
> > > > 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.
> 
> Because it's not radical?  And manually doing on_each_cpu() requires complexity
> that we would ideally avoid.
> 
> 15+ years ago, when VMX and SVM were nascent technologies, there was likely a
> good argument from a security perspective for leaving virtualization disabled.
> E.g. the ucode flows were new _and_ massive, and thus a potentially juicy attack
> surface.
> 
> But these days, AFAIK enabling virtualization is not considered to be a security
> risk, nor are there performance or stability downsides.  E.g. it's not all that
> different than the kernel enabling CR4.PKE even though it's entirely possible
> userspace will never actually use protection keys.

Agree this is not a security risk.  If all other ARCHs are fine to enable
on module loading then I don't see any real problem.

> 
> > > 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.
> > > 
> > 
> > Doing on_each_cpu() explicitly can also meet TDX's purpose.  We just
> > explicitly enable virtualization during module loading if we are going to
> > enable TDX.  For all other cases, the behaivour remains the same, unless
> > they want to change when to enable virtualization, e.g., when loading module
> > by default.
> > 
> > For always, or by default enabling virtualization during module loading, we
> > somehow discussed before:
> > 
> > https://lore.kernel.org/kvm/ZiKoqMk-wZKdiar9@google.com/
> > 
> > My true comment is introducing a module parameter, which is a userspace ABI,
> 
> Module params aren't strictly ABI, and even if they were, this would only be
> problematic if we wanted to remove the module param *and* doing so was a breaking
> change.  
> 

Yes.  The concern is once introduced I don't think we can easily remove it
even it becomes useless.


> Enabling virtualization should be entirely transparent to userspace,
> at least from a functional perspective; if changing how KVM enables virtualization
> breaks userspace then we likely have bigger problems.

I am not sure how should I interpret this?

"having a module param" doesn't necessarily mean "entirely transparent to
userspace", right? :-)

> 
> > to just fix some performance downgrade seems overkill when it can be
> > mitigated by the kernel.
> 
> Performance is secondary for me, the primary motivation is simplifying the overall
> KVM code base.  Yes, we _could_ use on_each_cpu() and enable virtualization
> on-demand for TDX, but as above, it's extra complexity without any meaningful
> benefit, at least AFAICT.

Either way works for me.

I just think using a module param to resolve some problem while there can
be solution completely in the kernel seems overkill :-)
Sean Christopherson May 29, 2024, 11:07 p.m. UTC | #8
On Wed, May 29, 2024, Kai Huang wrote:
> On Wed, 2024-05-29 at 08:01 -0700, Sean Christopherson wrote:
> > Enabling virtualization should be entirely transparent to userspace,
> > at least from a functional perspective; if changing how KVM enables virtualization
> > breaks userspace then we likely have bigger problems.
> 
> I am not sure how should I interpret this?
> 
> "having a module param" doesn't necessarily mean "entirely transparent to
> userspace", right? :-)

Ah, sorry, that was unclear.  By "transparent to userspace" I meant the
functionality of userspace VMMs wouldn't be affected if we add (or delete) a
module param.  E.g. QEMU should work exactly the same regardless of when KVM
enables virtualization.

> > Performance is secondary for me, the primary motivation is simplifying the overall
> > KVM code base.  Yes, we _could_ use on_each_cpu() and enable virtualization
> > on-demand for TDX, but as above, it's extra complexity without any meaningful
> > benefit, at least AFAICT.
> 
> Either way works for me.
> 
> I just think using a module param to resolve some problem while there can
> be solution completely in the kernel seems overkill :-)

The module param doesn't solve the problem, e.g. we could solve this entirely
in-kernel simply by having KVM unconditionally enable virtualization during
initialization.  The module param is mostly there to continue playing nice with
out-of-tree hypervisors, and to a lesser extent to give us a "break in case of
fire" knob.
Kai Huang May 30, 2024, 12:06 a.m. UTC | #9
On Wed, 2024-05-29 at 16:07 -0700, Sean Christopherson wrote:
> On Wed, May 29, 2024, Kai Huang wrote:
> > On Wed, 2024-05-29 at 08:01 -0700, Sean Christopherson wrote:
> > > Enabling virtualization should be entirely transparent to userspace,
> > > at least from a functional perspective; if changing how KVM enables virtualization
> > > breaks userspace then we likely have bigger problems.
> > 
> > I am not sure how should I interpret this?
> > 
> > "having a module param" doesn't necessarily mean "entirely transparent to
> > userspace", right? :-)
> 
> Ah, sorry, that was unclear.  By "transparent to userspace" I meant the
> functionality of userspace VMMs wouldn't be affected if we add (or delete) a
> module param.  E.g. QEMU should work exactly the same regardless of when KVM
> enables virtualization.
> 
> > > Performance is secondary for me, the primary motivation is simplifying the overall
> > > KVM code base.  Yes, we _could_ use on_each_cpu() and enable virtualization
> > > on-demand for TDX, but as above, it's extra complexity without any meaningful
> > > benefit, at least AFAICT.
> > 
> > Either way works for me.
> > 
> > I just think using a module param to resolve some problem while there can
> > be solution completely in the kernel seems overkill :-)
> 
> The module param doesn't solve the problem, e.g. we could solve this entirely
> in-kernel simply by having KVM unconditionally enable virtualization during
> initialization.  The module param is mostly there to continue playing nice with
> out-of-tree hypervisors, and to a lesser extent to give us a "break in case of
> fire" knob.

OK.  Now I understand you want to make enabling virtualization at loading
time as default behaviour, but make the module param as a way to defer to
enable virt when creating the first VM.
diff mbox series

Patch

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8ba2861e7788..8d331ab3aef7 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -5495,6 +5495,9 @@  static struct miscdevice kvm_dev = {
 };
 
 #ifdef CONFIG_KVM_GENERIC_HARDWARE_ENABLING
+static bool enable_virt_at_load;
+module_param(enable_virt_at_load, bool, 0444);
+
 __visible bool kvm_rebooting;
 EXPORT_SYMBOL_GPL(kvm_rebooting);
 
@@ -5645,15 +5648,41 @@  static void kvm_disable_virtualization(void)
 	unregister_syscore_ops(&kvm_syscore_ops);
 	cpuhp_remove_state(CPUHP_AP_KVM_ONLINE);
 }
+
+static int kvm_init_virtualization(void)
+{
+	if (enable_virt_at_load)
+		return kvm_enable_virtualization();
+
+	return 0;
+}
+
+static void kvm_uninit_virtualization(void)
+{
+	if (enable_virt_at_load)
+		kvm_disable_virtualization();
+
+	WARN_ON(kvm_usage_count);
+}
 #else /* CONFIG_KVM_GENERIC_HARDWARE_ENABLING */
 static int kvm_enable_virtualization(void)
 {
 	return 0;
 }
 
+static int kvm_init_virtualization(void)
+{
+	return 0;
+}
+
 static void kvm_disable_virtualization(void)
 {
 
+}
+
+static void kvm_uninit_virtualization(void)
+{
+
 }
 #endif /* CONFIG_KVM_GENERIC_HARDWARE_ENABLING */
 
@@ -6395,6 +6424,10 @@  int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
 
 	kvm_gmem_init(module);
 
+	r = kvm_init_virtualization();
+	if (r)
+		goto err_virt;
+
 	/*
 	 * Registration _must_ be the very last thing done, as this exposes
 	 * /dev/kvm to userspace, i.e. all infrastructure must be setup!
@@ -6408,6 +6441,8 @@  int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
 	return 0;
 
 err_register:
+	kvm_uninit_virtualization();
+err_virt:
 	kvm_vfio_ops_exit();
 err_vfio:
 	kvm_async_pf_deinit();
@@ -6433,6 +6468,8 @@  void kvm_exit(void)
 	 */
 	misc_deregister(&kvm_dev);
 
+	kvm_uninit_virtualization();
+
 	debugfs_remove_recursive(kvm_debugfs_dir);
 	for_each_possible_cpu(cpu)
 		free_cpumask_var(per_cpu(cpu_kick_mask, cpu));