On Tue, Mar 14, 2023, Huang, Kai wrote:
> 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 that idea, it also makes a few other patches a bit cleaner.
> 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?
I'd prefer not to squash the two. I agree it's ugly, but I dislike converting
VMX and SVM at the same time. I'm not totally opposed to moving everything in
one fell swoop, but my preference is to keep them separate.
