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 <[email protected]>
Signed-off-by: David P. Reed <[email protected]>
---
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);
}
--
2.26.2
On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <[email protected]> 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 <[email protected]>
> Signed-off-by: David P. Reed <[email protected]>
> ---
> 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.
Reviewed-by: Andy Lutomirski <[email protected]>
On Sunday, July 5, 2020 2:26pm, "Andy Lutomirski" <[email protected]> said:
> On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <[email protected]> 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 <[email protected]>
>> Signed-off-by: David P. Reed <[email protected]>
>> ---
>> 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?
>
> Reviewed-by: Andy Lutomirski <[email protected]>
>
On Sun, Jul 5, 2020 at 1:00 PM David P. Reed <[email protected]> wrote:
>
>
>
> On Sunday, July 5, 2020 2:26pm, "Andy Lutomirski" <[email protected]> said:
>
> > On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <[email protected]> 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 <[email protected]>
> >> Signed-off-by: David P. Reed <[email protected]>
> >> ---
> >> 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
On Sun, Jul 05, 2020 at 01:53:39PM -0700, Andy Lutomirski wrote:
> On Sun, Jul 5, 2020 at 1:00 PM David P. Reed <[email protected]> wrote:
> >
> > On Sunday, July 5, 2020 2:26pm, "Andy Lutomirski" <[email protected]> said:
> > > 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?
Hmm, rebooting with CR4.VMXE=1 shouldn't be a problem. VMXON does all the
special stuff that causes problems with reboot, e.g. blocks INIT, prevents
disabling paging, etc...
That being said, I think it makes sense to keep the clearing of CR4.VMXE out
of paranoia as BIOS will be BIOS, and there is no real downside. Not only
is the system about to reboot, but the CPUs that call cr4_clear_bits() from
NMI context are also being put into an infinite loop by crash_nmi_callback(),
i.e. they never leave NMI context. And we rely on that behavior, otherwise
KVM could set CR4.VMXE and do VMXON after the NMI and the whole thing would
be for naught.
> The real issue here is that the percpu CR4 machinery uses IRQ-offness as a
> lock, and NMI breaks this.