[PATCH] x86: kvm: add back X86_LOCAL_APIC dependency

[PATCH] x86: kvm: add back X86_LOCAL_APIC dependency

[Arnd Bergmann]

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
 	help
 	  Provides support for KVM on AMD processors equipped with the AMD-V
 	  (SVM) extensions.
-- 
2.39.5

Re: [PATCH] x86: kvm: add back X86_LOCAL_APIC dependency

[Sean Christopherson]

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.

Re: [PATCH] x86: kvm: add back X86_LOCAL_APIC dependency

[H. Peter Anvin]

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

Re: [PATCH] x86: kvm: add back X86_LOCAL_APIC dependency

[Sean Christopherson]

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.

Re: [PATCH] x86: kvm: add back X86_LOCAL_APIC dependency

[Sean Christopherson]

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.

Re: [PATCH] x86: kvm: add back X86_LOCAL_APIC dependency

[Arnd Bergmann]

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