Re: [PATCH v2 05/18] x86/reboot: KVM: Disable SVM during reboot via virt/KVM reboot callback

[Huang, Kai]

On Mon, 2023-03-13 at 10:18 -0700, Sean Christopherson wrote:
> On Mon, Mar 13, 2023, Huang, Kai wrote:
> > On Fri, 2023-03-10 at 13:42 -0800, Sean Christopherson wrote:
> > > Use the virt callback to disable SVM (and set GIF=1) during an emergency
> > > instead of blindly attempting to disable SVM.� Like the VMX case, if KVM
> > > (or an out-of-tree hypervisor) isn't loaded/active, SVM can't be in use.
> > > 
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > 
> > [...]
> > 
> > > -#if IS_ENABLED(CONFIG_KVM_INTEL)
> > > +#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
> > > �/* RCU-protected callback to disable virtualization prior to reboot. */
> > > �static cpu_emergency_virt_cb __rcu *cpu_emergency_virt_callback;
> > > �
> > > @@ -821,7 +821,7 @@ int crashing_cpu = -1;
> > > � */
> > > �void cpu_emergency_disable_virtualization(void)
> > > �{
> > > -#if IS_ENABLED(CONFIG_KVM_INTEL)
> > > +#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
> > > �	cpu_emergency_virt_cb *callback;
> > > �
> > > �	rcu_read_lock();
> > > @@ -830,8 +830,6 @@ void cpu_emergency_disable_virtualization(void)
> > > �		callback();
> > > �	rcu_read_unlock();
> > > �#endif
> > > -	/* KVM_AMD doesn't yet utilize the common callback. */
> > > -	cpu_emergency_svm_disable();
> > > �}
> > 
> > Shouldn't the callback be always present since you want to consider 'out-of-
> > tree' hypervisor case?
> 
> No?  The kernel doesn't provide any guarantees for out-of-tree code.  I don't have
> a super strong preference, though I do like the effective documentation the checks
> provide.  Buy more importantly, my understanding is that the x86 maintainers want
> to limit the exposure for these types of interfaces, e.g. `git grep IS_ENABLED\(CONFIG_KVM`
> for a variety of hooks that are enabled iff KVM is enabled in the kernel config.

How about doing the embracing the code to do the emergency virt callback as the
last step?

I like the "cleanup" work in this series regardless whether we should guard the
emergency virt callback with CONFIG_KVM_INTEL || CONFIG_KVM_AMD.  If we do the
actual "cleanup" work first, and put the CONFIG_KVM_INTEL || CONFIG_KVM_AMD as
the last step, it is also easier to review I guess, because it's kinda a
separate logic independent to the actual "cleanup" work.

Also, personally I don't particularly like the middle state in patch 04:

 void cpu_emergency_disable_virtualization(void)
 {
 #if IS_ENABLED(CONFIG_KVM_INTEL)
-	cpu_crash_vmclear_loaded_vmcss();
-#endif
+	cpu_emergency_virt_cb *callback;
 
-	cpu_emergency_vmxoff();
+	rcu_read_lock();
+	callback = rcu_dereference(cpu_emergency_virt_callback);
+	if (callback)
+		callback();
+	rcu_read_unlock();
+#endif
+	/* KVM_AMD doesn't yet utilize the common callback. */
 	cpu_emergency_svm_disable();
 }

Which eventually got fixed up in patch 05:

 void cpu_emergency_disable_virtualization(void)
 {
-#if IS_ENABLED(CONFIG_KVM_INTEL)
+#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
 	cpu_emergency_virt_cb *callback;
 
 	rcu_read_lock();
@@ -830,8 +830,6 @@ void cpu_emergency_disable_virtualization(void)
 		callback();
 	rcu_read_unlock();
 #endif
-	/* KVM_AMD doesn't yet utilize the common callback. */
-	cpu_emergency_svm_disable();
 }
 
Could we just merge the two patches together?