| Message ID | 20241112065415.3974321-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: kvm: add back X86_LOCAL_APIC dependency | expand |
On Tue, Nov 12, 2024, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Enabling KVM now causes a build failure on x86-32 if X86_LOCAL_APIC > is disabled: > > arch/x86/kvm/svm/svm.c: In function 'svm_emergency_disable_virtualization_cpu': > arch/x86/kvm/svm/svm.c:597:9: error: 'kvm_rebooting' undeclared (first use in this function); did you mean 'kvm_irq_routing'? > 597 | kvm_rebooting = true; > | ^~~~~~~~~~~~~ > | kvm_irq_routing > arch/x86/kvm/svm/svm.c:597:9: note: each undeclared identifier is reported only once for each function it appears in > make[6]: *** [scripts/Makefile.build:221: arch/x86/kvm/svm/svm.o] Error 1 > In file included from include/linux/rculist.h:11, > from include/linux/hashtable.h:14, > from arch/x86/kvm/svm/avic.c:18: > arch/x86/kvm/svm/avic.c: In function 'avic_pi_update_irte': > arch/x86/kvm/svm/avic.c:909:38: error: 'struct kvm' has no member named 'irq_routing' > 909 | irq_rt = srcu_dereference(kvm->irq_routing, &kvm->irq_srcu); > | ^~ > include/linux/rcupdate.h:538:17: note: in definition of macro '__rcu_dereference_check' > 538 | typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > > Move the dependency to the same place as before. > > Fixes: ea4290d77bda ("KVM: x86: leave kvm.ko out of the build if no vendor module is requested") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202410060426.e9Xsnkvi-lkp@intel.com/ > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Question: is there actually any point in keeping KVM support for 32-bit host > processors? Nope. We need _a_ 32-bit KVM build to run as a nested (L1) hypervisor for testing purposes, but AFAIK there's zero need to keep 32-bit KVM up-to-date. > From what I can tell, the only 32-bit CPUs that support this are > the rare Atom E6xx and Z5xx models and the even older Yonah/Sossaman "Core > Duo", everything else is presumably better off just running a 64-bit kernel > even for 32-bit guests? Yep. I am 99.9% certain there are no users of 32-bit KVM. There have been multiple instances in the past few years where 32-bit KVM was quite broken, for several kernel releases, and no one complained. Paolo, should we go ahead and start the process of removing 32-bit support in x86? I forget who it was, but someone from the QEMU world mentioned that dropping 32-bit support in KVM would allow dropping a rather large pile of code in QEMU too.
On 11/12/24 10:05, Sean Christopherson wrote: >> >> Fixes: ea4290d77bda ("KVM: x86: leave kvm.ko out of the build if no vendor module is requested") >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: https://lore.kernel.org/oe-kbuild-all/202410060426.e9Xsnkvi-lkp@intel.com/ >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> Question: is there actually any point in keeping KVM support for 32-bit host >> processors? > > Nope. We need _a_ 32-bit KVM build to run as a nested (L1) hypervisor for testing > purposes, but AFAIK there's zero need to keep 32-bit KVM up-to-date. > What do you mean here? Running an old kernel with the 32-bit KVM in a VM for testing the L0 hypervisor? -hpa
On Tue, Nov 12, 2024, H. Peter Anvin wrote: > On 11/12/24 10:05, Sean Christopherson wrote: > > > > > > Fixes: ea4290d77bda ("KVM: x86: leave kvm.ko out of the build if no vendor module is requested") > > > Reported-by: kernel test robot <lkp@intel.com> > > > Closes: https://lore.kernel.org/oe-kbuild-all/202410060426.e9Xsnkvi-lkp@intel.com/ > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > --- > > > Question: is there actually any point in keeping KVM support for 32-bit host > > > processors? > > > > Nope. We need _a_ 32-bit KVM build to run as a nested (L1) hypervisor for testing > > purposes, but AFAIK there's zero need to keep 32-bit KVM up-to-date. > > What do you mean here? Running an old kernel with the 32-bit KVM in a VM for > testing the L0 hypervisor? Yep, to validate nested NPT (NPT is AMD/SVM's stage-2 paging mechanism). Unlike EPT, which is completely disassociated from the host's CPU mode, NPT is tightly coupled to the host mode and uses/supports all of the flavors of stage-1 paging, i.e. legacy 32-bit, PSE, PAE, 4-level, and 5-level. Because there's no architectural way to prevent L1 from using 32-bit or PAE NPT, KVM needs to support shadowing such NPT tables. And so to validate that KVM (L0) correctly shadows L1's NPT tables, we need a 32-bit hypervisor to run in L1. We briefly considered writing dedicated tests, but the effort required is absurd, relatively to the coverage provided. It's quite annoying, because I highly doubt anyone actually uses 32-bit hypervisors of any flavor, but nested NPT allows for some truly unique setups, e.g. where KVM is using 5-level NPT to shadow legacy 3-level 32-bit page tables. As a result, KVM has paths are only reachable with a 32-bit L1 KVM, and at the very least we need to ensure they aren't juicy attack vectors.
On Tue, Nov 12, 2024, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Enabling KVM now causes a build failure on x86-32 if X86_LOCAL_APIC > is disabled: > > arch/x86/kvm/svm/svm.c: In function 'svm_emergency_disable_virtualization_cpu': > arch/x86/kvm/svm/svm.c:597:9: error: 'kvm_rebooting' undeclared (first use in this function); did you mean 'kvm_irq_routing'? > 597 | kvm_rebooting = true; > | ^~~~~~~~~~~~~ > | kvm_irq_routing > arch/x86/kvm/svm/svm.c:597:9: note: each undeclared identifier is reported only once for each function it appears in > make[6]: *** [scripts/Makefile.build:221: arch/x86/kvm/svm/svm.o] Error 1 > In file included from include/linux/rculist.h:11, > from include/linux/hashtable.h:14, > from arch/x86/kvm/svm/avic.c:18: > arch/x86/kvm/svm/avic.c: In function 'avic_pi_update_irte': > arch/x86/kvm/svm/avic.c:909:38: error: 'struct kvm' has no member named 'irq_routing' > 909 | irq_rt = srcu_dereference(kvm->irq_routing, &kvm->irq_srcu); > | ^~ > include/linux/rcupdate.h:538:17: note: in definition of macro '__rcu_dereference_check' > 538 | typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > > Move the dependency to the same place as before. > > Fixes: ea4290d77bda ("KVM: x86: leave kvm.ko out of the build if no vendor module is requested") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202410060426.e9Xsnkvi-lkp@intel.com/ > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Question: is there actually any point in keeping KVM support for 32-bit host > processors? From what I can tell, the only 32-bit CPUs that support this are > the rare Atom E6xx and Z5xx models and the even older Yonah/Sossaman "Core > Duo", everything else is presumably better off just running a 64-bit kernel > even for 32-bit guests? > --- > arch/x86/kvm/Kconfig | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig > index 1ed1e4f5d51c..849a03f3ba95 100644 > --- a/arch/x86/kvm/Kconfig > +++ b/arch/x86/kvm/Kconfig > @@ -19,7 +19,6 @@ if VIRTUALIZATION > > config KVM_X86 > def_tristate KVM if KVM_INTEL || KVM_AMD > - depends on X86_LOCAL_APIC > select KVM_COMMON > select KVM_GENERIC_MMU_NOTIFIER > select KVM_ELIDE_TLB_FLUSH_IF_YOUNG > @@ -93,6 +92,7 @@ config KVM_SW_PROTECTED_VM > config KVM_INTEL > tristate "KVM for Intel (and compatible) processors support" > depends on KVM && IA32_FEAT_CTL > + depends on X86_LOCAL_APIC > help > Provides support for KVM on processors equipped with Intel's VT > extensions, a.k.a. Virtual Machine Extensions (VMX). > @@ -130,6 +130,7 @@ config X86_SGX_KVM > config KVM_AMD > tristate "KVM for AMD processors support" > depends on KVM && (CPU_SUP_AMD || CPU_SUP_HYGON) > + depends on X86_LOCAL_APIC The dependency can and should go on "config KVM", not on the vendor modules. The net effect on the build is the same, but preventing the user from selecting KVM will provide a slightly better experience.
On Wed, Nov 13, 2024, at 15:42, Sean Christopherson wrote: > > The dependency can and should go on "config KVM", not on the vendor > modules. The net effect on the build is the same, but preventing > the user from selecting KVM will provide a slightly better experience. Makes sense, sending v2 now. Arnd
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig index 1ed1e4f5d51c..849a03f3ba95 100644 --- a/arch/x86/kvm/Kconfig +++ b/arch/x86/kvm/Kconfig @@ -19,7 +19,6 @@ if VIRTUALIZATION config KVM_X86 def_tristate KVM if KVM_INTEL || KVM_AMD - depends on X86_LOCAL_APIC select KVM_COMMON select KVM_GENERIC_MMU_NOTIFIER select KVM_ELIDE_TLB_FLUSH_IF_YOUNG @@ -93,6 +92,7 @@ config KVM_SW_PROTECTED_VM config KVM_INTEL tristate "KVM for Intel (and compatible) processors support" depends on KVM && IA32_FEAT_CTL + depends on X86_LOCAL_APIC help Provides support for KVM on processors equipped with Intel's VT extensions, a.k.a. Virtual Machine Extensions (VMX). @@ -130,6 +130,7 @@ config X86_SGX_KVM config KVM_AMD tristate "KVM for AMD processors support" depends on KVM && (CPU_SUP_AMD || CPU_SUP_HYGON) + depends on X86_LOCAL_APIC help Provides support for KVM on AMD processors equipped with the AMD-V (SVM) extensions.