LKML Archive on lore.kernel.org
 help / color / Atom feed
From: "David P. Reed" <dpreed@deepplum.com>
To: "Andy Lutomirski" <luto@kernel.org>
Cc: "Sean Christopherson" <sean.j.christopherson@intel.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Borislav Petkov" <bp@alien8.de>, "X86 ML" <x86@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"Allison Randal" <allison@lohutok.net>,
	"Enrico Weigelt" <info@metux.net>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Kate Stewart" <kstewart@linuxfoundation.org>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>,
	"Randy Dunlap" <rdunlap@infradead.org>,
	"Martin Molnar" <martin.molnar.programming@gmail.com>,
	"Andy Lutomirski" <luto@kernel.org>,
	"Alexandre Chartre" <alexandre.chartre@oracle.com>,
	"Jann Horn" <jannh@google.com>,
	"Dave Hansen" <dave.hansen@linux.intel.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably
Date: Sun, 5 Jul 2020 16:00:33 -0400 (EDT)
Message-ID: <1593979233.22877148@apps.rackspace.com> (raw)
In-Reply-To: <CALCETrWzh03fefGuSTM9hr9QY9_=xqbHg3hQ-_vo2PLUXuZ4wg@mail.gmail.com>



On Sunday, July 5, 2020 2:26pm, "Andy Lutomirski" <luto@kernel.org> said:

> On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <dpreed@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.christopherson@intel.com>
>> Signed-off-by: David P. Reed <dpreed@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?

> 
> Reviewed-by: Andy Lutomirski <luto@kernel.org>
> 



  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-25 14:45 [PATCH v2] Fix undefined operation VMXOFF during reboot and crash David P. Reed
2020-06-25 14:59 ` David P. Reed
2020-06-29 20:54   ` David P. Reed
2020-06-29 21:22     ` Andy Lutomirski
2020-06-29 21:49       ` Sean Christopherson
2020-06-29 22:46         ` David P. Reed
2020-07-04 20:38         ` [PATCH v3 0/3] " David P. Reed
2020-07-04 20:38           ` [PATCH v3 1/3] Correct asm VMXOFF side effects David P. Reed
2020-07-05  5:46             ` Randy Dunlap
2020-07-04 20:38           ` [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic David P. Reed
2020-07-05 18:22             ` Andy Lutomirski
2020-07-05 19:52               ` David P. Reed
2020-07-05 20:55                 ` Andy Lutomirski
2020-07-05 22:07                   ` David P. Reed
2020-07-07  5:09             ` Sean Christopherson
2020-07-07 19:09               ` David P. Reed
2020-07-07 19:24                 ` Sean Christopherson
2020-07-07 19:52                   ` David P. Reed
2020-07-04 20:38           ` [PATCH v3 3/3] Force all cpus to exit VMX root operation on crash/panic reliably David P. Reed
2020-07-05 18:26             ` Andy Lutomirski
2020-07-05 20:00               ` David P. Reed [this message]
2020-07-05 20:53                 ` Andy Lutomirski
2020-07-07  5:29                   ` Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1593979233.22877148@apps.rackspace.com \
    --to=dpreed@deepplum.com \
    --cc=alexandre.chartre@oracle.com \
    --cc=allison@lohutok.net \
    --cc=bp@alien8.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hpa@zytor.com \
    --cc=info@metux.net \
    --cc=jannh@google.com \
    --cc=kstewart@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=martin.molnar.programming@gmail.com \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rdunlap@infradead.org \
    --cc=sean.j.christopherson@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git