On Sun, Jul 5, 2020 at 1:00 PM David P. Reed <dpr...@deepplum.com> wrote:
>
>
>
> On Sunday, July 5, 2020 2:26pm, "Andy Lutomirski" <l...@kernel.org> said:
>
> > On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <dpr...@deepplum.com> wrote:
> >>
> >> Fix the logic during crash/panic reboot on Intel processors that
> >> can support VMX operation to ensure that all processors are not
> >> in VMX root operation. Prior code made optimistic assumptions
> >> about other cpus that would leave other cpus in VMX root operation
> >> depending on timing of crash/panic reboot.
> >> Builds on cpu_ermergency_vmxoff() and __cpu_emergency_vmxoff() created
> >> in a prior patch.
> >>
> >> Suggested-by: Sean Christopherson <sean.j.christopher...@intel.com>
> >> Signed-off-by: David P. Reed <dpr...@deepplum.com>
> >> ---
> >> arch/x86/kernel/reboot.c | 20 +++++++-------------
> >> 1 file changed, 7 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> >> index 0ec7ced727fe..c8e96ba78efc 100644
> >> --- a/arch/x86/kernel/reboot.c
> >> +++ b/arch/x86/kernel/reboot.c
> >> @@ -543,24 +543,18 @@ static void emergency_vmx_disable_all(void)
> >> * signals when VMX is enabled.
> >> *
> >> * We can't take any locks and we may be on an inconsistent
> >> - * state, so we use NMIs as IPIs to tell the other CPUs to disable
> >> - * VMX and halt.
> >> + * state, so we use NMIs as IPIs to tell the other CPUs to exit
> >> + * VMX root operation and halt.
> >> *
> >> * For safety, we will avoid running the nmi_shootdown_cpus()
> >> * stuff unnecessarily, but we don't have a way to check
> >> - * if other CPUs have VMX enabled. So we will call it only if the
> >> - * CPU we are running on has VMX enabled.
> >> - *
> >> - * We will miss cases where VMX is not enabled on all CPUs. This
> >> - * shouldn't do much harm because KVM always enable VMX on all
> >> - * CPUs anyway. But we can miss it on the small window where KVM
> >> - * is still enabling VMX.
> >> + * if other CPUs might be in VMX root operation.
> >> */
> >> - if (cpu_has_vmx() && cpu_vmx_enabled()) {
> >> - /* Disable VMX on this CPU. */
> >> - cpu_vmxoff();
> >> + if (cpu_has_vmx()) {
> >> + /* Safely force out of VMX root operation on this CPU. */
> >> + __cpu_emergency_vmxoff();
> >>
> >> - /* Halt and disable VMX on the other CPUs */
> >> + /* Halt and exit VMX root operation on the other CPUs */
> >> nmi_shootdown_cpus(vmxoff_nmi);
> >>
> >> }
> >
> > Seems reasonable to me.
> >
> > As a minor caveat, doing cr4_clear_bits() in NMI context is not really
> > okay, but we're about to reboot, so nothing too awful should happen.
> > And this has very little to do with your patch.
>
> I had wondered why the bit is cleared, too. (I assumed it was OK or
> desirable, because it was being cleared in NMI context before). Happy to
> submit a separate patch to eliminate that issue as well, since the point of
> emergency vmxoff is only to get out of VMX root mode - CR4.VMXE's state is
> irrelevant. Of course, clearing it prevents any future emergency vmxoff
> attempts. (there seemed to be some confusion about "enabling" VMX vs. "in VMX
> operation" in the comments) Should I?
I have a vague recollection of some firmwares that got upset if
rebooted with CR4.VMXE set. Sean?
The real issue here is that the percpu CR4 machinery uses IRQ-offness
as a lock, and NMI breaks this.
--Andy
