[v3,05/18] x86/reboot: Disable virtualization during reboot iff callback is registered

  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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
  

On Fri, 2023-05-12 at 16:50 -0700, 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).
> 
> 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 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 92b380e199a3..20f7bdabc52e 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>
> @@ -545,7 +544,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();


IIUC, for cpu_emergency_disable_virtualization() itself, looks it's OK to not
having the pointer check, since it internally will do rcu_dereference() inside
RCU critical section anyway.

But nmi_shootdown_cpus_on_restart() is called after
cpu_emergency_disable_virtualization(), and having the pointer check here can
avoid sending NMI to remote cpus if there's no active hypervisor.

Am I missing something?  If not, is it worth to call this out in changelog?

>  
> -- 
> 2.40.1.606.ga4b1b128d6-goog
>
  

On Mon, May 22, 2023, Kai Huang wrote:
> On Fri, 2023-05-12 at 16:50 -0700, 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).
> > 
> > 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 | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> > index 92b380e199a3..20f7bdabc52e 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>
> > @@ -545,7 +544,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();
> 
> 
> IIUC, for cpu_emergency_disable_virtualization() itself, looks it's OK to not
> having the pointer check, since it internally will do rcu_dereference() inside
> RCU critical section anyway.
> 
> But nmi_shootdown_cpus_on_restart() is called after
> cpu_emergency_disable_virtualization(), and having the pointer check here can
> avoid sending NMI to remote cpus if there's no active hypervisor.
> 
> Am I missing something?  If not, is it worth to call this out in changelog?

No, you're not missing anything.  I agree it's worth a line in the changelog.
Dropping the "spurious" NMI should be a-ok, but explicitly calling out the side
effect could be helpful for debug if something is silently relying on the NMI.
  

On Mon, 2023-05-22 at 10:51 -0700, Sean Christopherson wrote:
> On Mon, May 22, 2023, Kai Huang wrote:
> > On Fri, 2023-05-12 at 16:50 -0700, 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).
> > > 
> > > 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 | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> > > index 92b380e199a3..20f7bdabc52e 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>
> > > @@ -545,7 +544,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();
> > 
> > 
> > IIUC, for cpu_emergency_disable_virtualization() itself, looks it's OK to not
> > having the pointer check, since it internally will do rcu_dereference() inside
> > RCU critical section anyway.
> > 
> > But nmi_shootdown_cpus_on_restart() is called after
> > cpu_emergency_disable_virtualization(), and having the pointer check here can
> > avoid sending NMI to remote cpus if there's no active hypervisor.
> > 
> > Am I missing something?  If not, is it worth to call this out in changelog?
> 
> No, you're not missing anything.  I agree it's worth a line in the changelog.
> Dropping the "spurious" NMI should be a-ok, but explicitly calling out the side
> effect could be helpful for debug if something is silently relying on the NMI.

Yeah my thinking too.  Thanks.