[v2,07/18] x86/reboot: Disable virtualization during reboot iff callback is registered

Message ID 20230310214232.806108-8-seanjc@google.com
State New
Headers
Series x86/reboot: KVM: Clean up "emergency" virt code |

Commit Message

Sean Christopherson March 10, 2023, 9:42 p.m. UTC
  Attempt to disable virtualization during an emergency reboot if and only
if there is a registered virt callback, i.e. iff a hypervisor (KVM) is
active.  If there's no active hypervisor, then the CPU can't be operating
with VMX or SVM enabled (barring an egregious bug).

Note, IRQs are disabled, which prevents KVM from coming along and enabling
virtualization after the fact.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kernel/reboot.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
  

Comments

Kai Huang March 13, 2023, 12:54 a.m. UTC | #1
On Fri, 2023-03-10 at 13:42 -0800, Sean Christopherson wrote:
> Attempt to disable virtualization during an emergency reboot if and only
> if there is a registered virt callback, i.e. iff a hypervisor (KVM) is
> active.  If there's no active hypervisor, then the CPU can't be operating
> with VMX or SVM enabled (barring an egregious bug).

IIUC, this patch is the final one that you want to achieve how the "disable
virtualization" callback should work in the non-KVM core kernel (the rest
patches are related to moving VMXOFF code to KVM as the core-kernel now just
calls the callback, etc).  

There are middle step patches (2-7) to eventually help to get to this point. 
But to be honest, personally, I am not sure whether those patches are necessary,
i.e. to me they actually cost more time to review since I have to think whether
such intermediate status is reasonable or not.  I am wondering whether we can
just merge those patches together as single one, so it's easy to see what is the
final goal to achieve?

Just my 2cents, of course.

> 
> Note, IRQs are disabled, which prevents KVM from coming along and enabling
> virtualization after the fact.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kernel/reboot.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index cb268ec7ce85..dd7def3d4144 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -22,7 +22,6 @@
>  #include <asm/reboot_fixups.h>
>  #include <asm/reboot.h>
>  #include <asm/pci_x86.h>
> -#include <asm/virtext.h>
>  #include <asm/cpu.h>
>  #include <asm/nmi.h>
>  #include <asm/smp.h>
> @@ -568,7 +567,6 @@ void cpu_emergency_disable_virtualization(void)
>  		callback();
>  	rcu_read_unlock();
>  }
> -#endif /* CONFIG_KVM_INTEL || CONFIG_KVM_AMD */
>  
>  static void emergency_reboot_disable_virtualization(void)
>  {
> @@ -585,7 +583,7 @@ static void emergency_reboot_disable_virtualization(void)
>  	 * Do the NMI shootdown even if virtualization is off on _this_ CPU, as
>  	 * other CPUs may have virtualization enabled.
>  	 */
> -	if (cpu_has_vmx() || cpu_has_svm(NULL)) {
> +	if (rcu_access_pointer(cpu_emergency_virt_callback)) {
>  		/* Safely force _this_ CPU out of VMX/SVM operation. */
>  		cpu_emergency_disable_virtualization();
>  
> @@ -593,6 +591,9 @@ static void emergency_reboot_disable_virtualization(void)
>  		nmi_shootdown_cpus_on_restart();
>  	}
>  }
> +#else
> +static void emergency_reboot_disable_virtualization(void) { }
> +#endif /* CONFIG_KVM_INTEL || CONFIG_KVM_AMD */
>  
>  
>  void __attribute__((weak)) mach_reboot_fixups(void)
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
>
  
Sean Christopherson March 13, 2023, 6:40 p.m. UTC | #2
On Mon, Mar 13, 2023, Huang, Kai wrote:
> On Fri, 2023-03-10 at 13:42 -0800, Sean Christopherson wrote:
> > Attempt to disable virtualization during an emergency reboot if and only
> > if there is a registered virt callback, i.e. iff a hypervisor (KVM) is
> > active.  If there's no active hypervisor, then the CPU can't be operating
> > with VMX or SVM enabled (barring an egregious bug).
> 
> IIUC, this patch is the final one that you want to achieve how the "disable
> virtualization" callback should work in the non-KVM core kernel (the rest
> patches are related to moving VMXOFF code to KVM as the core-kernel now just
> calls the callback, etc). �
> 
> There are middle step patches (2-7) to eventually help to get to this point. 
> But to be honest, personally, I am not sure whether those patches are necessary,
> i.e. to me they actually cost more time to review since I have to think whether
> such intermediate status is reasonable or not.  I am wondering whether we can
> just merge those patches together as single one, so it's easy to see what is the
> final goal to achieve?

I agree that the fine granularity makes it difficult to see the final form, but
from a bisection perspective I really, really want each change to be isolated as
much as possible.  This code is extremely difficult, if not practically impossible,
to exhaustively test due to multiple points of entry from "this should never happen!"
types of flows.  If any of these changes breaks someones deployment, I want to
make it as easy as possible for that someone to determine exactly what broke.
  
Kai Huang March 14, 2023, 12:50 a.m. UTC | #3
On Mon, 2023-03-13 at 11:40 -0700, Sean Christopherson wrote:
> On Mon, Mar 13, 2023, Huang, Kai wrote:
> > On Fri, 2023-03-10 at 13:42 -0800, Sean Christopherson wrote:
> > > Attempt to disable virtualization during an emergency reboot if and only
> > > if there is a registered virt callback, i.e. iff a hypervisor (KVM) is
> > > active.  If there's no active hypervisor, then the CPU can't be operating
> > > with VMX or SVM enabled (barring an egregious bug).
> > 
> > IIUC, this patch is the final one that you want to achieve how the "disable
> > virtualization" callback should work in the non-KVM core kernel (the rest
> > patches are related to moving VMXOFF code to KVM as the core-kernel now just
> > calls the callback, etc). �
> > 
> > There are middle step patches (2-7) to eventually help to get to this point. 
> > But to be honest, personally, I am not sure whether those patches are necessary,
> > i.e. to me they actually cost more time to review since I have to think whether
> > such intermediate status is reasonable or not.  I am wondering whether we can
> > just merge those patches together as single one, so it's easy to see what is the
> > final goal to achieve?
> 
> I agree that the fine granularity makes it difficult to see the final form, but
> from a bisection perspective I really, really want each change to be isolated as
> much as possible.  This code is extremely difficult, if not practically impossible,
> to exhaustively test due to multiple points of entry from "this should never happen!"
> types of flows.  If any of these changes breaks someones deployment, I want to
> make it as easy as possible for that someone to determine exactly what broke.

Yeah sure.

Yes in general I agree we should make bisection easy to pinpoint the exact code
which breaks something, but I think over-splitting is also unnecessary
especially when code change is small ;)
  

Patch

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index cb268ec7ce85..dd7def3d4144 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -22,7 +22,6 @@ 
 #include <asm/reboot_fixups.h>
 #include <asm/reboot.h>
 #include <asm/pci_x86.h>
-#include <asm/virtext.h>
 #include <asm/cpu.h>
 #include <asm/nmi.h>
 #include <asm/smp.h>
@@ -568,7 +567,6 @@  void cpu_emergency_disable_virtualization(void)
 		callback();
 	rcu_read_unlock();
 }
-#endif /* CONFIG_KVM_INTEL || CONFIG_KVM_AMD */
 
 static void emergency_reboot_disable_virtualization(void)
 {
@@ -585,7 +583,7 @@  static void emergency_reboot_disable_virtualization(void)
 	 * Do the NMI shootdown even if virtualization is off on _this_ CPU, as
 	 * other CPUs may have virtualization enabled.
 	 */
-	if (cpu_has_vmx() || cpu_has_svm(NULL)) {
+	if (rcu_access_pointer(cpu_emergency_virt_callback)) {
 		/* Safely force _this_ CPU out of VMX/SVM operation. */
 		cpu_emergency_disable_virtualization();
 
@@ -593,6 +591,9 @@  static void emergency_reboot_disable_virtualization(void)
 		nmi_shootdown_cpus_on_restart();
 	}
 }
+#else
+static void emergency_reboot_disable_virtualization(void) { }
+#endif /* CONFIG_KVM_INTEL || CONFIG_KVM_AMD */
 
 
 void __attribute__((weak)) mach_reboot_fixups(void)