Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] Fix undefined operation VMXOFF during reboot and crash
To:     "David P. Reed" <dpreed@deepplum.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        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>,
        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>,
        linux-kernel@vger.kernel.org
References: <20200610181254.2142-1-dpreed@deepplum.com>
From:   Randy Dunlap <rdunlap@infradead.org>
Message-ID: <e62c3c33-b3c4-c885-f588-577899d1d602@infradead.org>
Date:   Wed, 10 Jun 2020 12:36:13 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <20200610181254.2142-1-dpreed@deepplum.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi David,

If you happen to make a v2 of this patch, there are a few comments
that begin with "/**" but they are not kernel-doc comments, so they
should instead begin with just "/*". Please see below.

(and you did not introduce this comment style here.)

On 6/10/20 11:12 AM, David P. Reed wrote:
> 
> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> ---
>  arch/x86/include/asm/virtext.h | 24 ++++++++++++----
>  arch/x86/kernel/reboot.c       | 13 ++-------
>  arch/x86/kernel/traps.c        | 52 ++++++++++++++++++++++++++++++++--
>  3 files changed, 71 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> index 9aad0e0876fb..ea2d67191684 100644
> --- a/arch/x86/include/asm/virtext.h
> +++ b/arch/x86/include/asm/virtext.h
> @@ -13,12 +13,16 @@
>  #ifndef _ASM_X86_VIRTEX_H
>  #define _ASM_X86_VIRTEX_H
>  
> +#include <linux/percpu.h>
> +
>  #include <asm/processor.h>
>  
>  #include <asm/vmx.h>
>  #include <asm/svm.h>
>  #include <asm/tlbflush.h>
>  
> +DECLARE_PER_CPU_READ_MOSTLY(int, doing_emergency_vmxoff);
> +
>  /*
>   * VMX functions:
>   */
> @@ -33,8 +37,8 @@ static inline int cpu_has_vmx(void)
>  /** Disable VMX on the current CPU

just
   /* Disable VMX on the current CPU

>   *
>   * vmxoff causes a undefined-opcode exception if vmxon was not run
> - * on the CPU previously. Only call this function if you know VMX
> - * is enabled.
> + * on the CPU previously. Only call this function directly if you know VMX
> + * is enabled *and* CPU is in VMX root operation.
>   */
>  static inline void cpu_vmxoff(void)
>  {
> @@ -47,17 +51,25 @@ static inline int cpu_vmx_enabled(void)
>  	return __read_cr4() & X86_CR4_VMXE;
>  }
>  
> -/** Disable VMX if it is enabled on the current CPU
> +/** Force disable VMX if it is enabled on the current CPU.

just
   /* Force disable VMX if it is enabled on the current CPU.

> + * Note that if CPU is not in VMX root operation this
> + * VMXOFF will fault an undefined operation fault.
> + * So the 'doing_emergency_vmxoff' percpu flag is set,
> + * the trap handler for just restarts execution after
> + * the VMXOFF instruction.
>   *
> - * You shouldn't call this if cpu_has_vmx() returns 0.
> + * You shouldn't call this directly if cpu_has_vmx() returns 0.
>   */
>  static inline void __cpu_emergency_vmxoff(void)
>  {
> -	if (cpu_vmx_enabled())
> +	if (cpu_vmx_enabled()) {
> +		this_cpu_write(doing_emergency_vmxoff, 1);
>  		cpu_vmxoff();
> +		this_cpu_write(doing_emergency_vmxoff, 0);
> +	}
>  }
>  
> -/** Disable VMX if it is supported and enabled on the current CPU
> +/** Force disable VMX if it is supported and enabled on the current CPU

ditto.


>   */
>  static inline void cpu_emergency_vmxoff(void)
>  {


thanks.
-- 
~Randy