[PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[Sean Christopherson]

Reinitialize IA32_FEAT_CTL on the BSP during wakeup to handle the case
where firmware doesn't initialize or save/restore across S3.  This fixes
a bug where IA32_FEAT_CTL is left uninitialized and results in VMXON
taking a #GP due to VMX not being fully enabled, i.e. breaks KVM.

Use init_ia32_feat_ctl() to "restore" IA32_FEAT_CTL as it already deals
with the case where the MSR is locked, and because APs already redo
init_ia32_feat_ctl() during suspend by virtue of the SMP boot flow being
used to reinitialize APs upon wakeup.  Do the call in the early wakeup
flow to avoid dependencies in the syscore_ops chain, e.g. simply adding
a resume hook is not guaranteed to work, as KVM does VMXON in its own
resume hook, kvm_resume(), when KVM has active guests.

Reported-by: Brad Campbell <lists2009@fnarfbargle.com>
Cc: Maxim Levitsky <mlevitsk@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm@vger.kernel.org
Fixes: 21bd3467a58e ("KVM: VMX: Drop initialization of IA32_FEAT_CTL MSR")
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/cpu.h | 5 +++++
 arch/x86/kernel/cpu/cpu.h  | 4 ----
 arch/x86/power/cpu.c       | 6 ++++++
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index dd17c2da1af5..da78ccbd493b 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -58,4 +58,9 @@ static inline bool handle_guest_split_lock(unsigned long ip)
 	return false;
 }
 #endif
+#ifdef CONFIG_IA32_FEAT_CTL
+void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
+#else
+static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c) {}
+#endif
 #endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
index 37fdefd14f28..38ab6e115eac 100644
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -80,8 +80,4 @@ extern void x86_spec_ctrl_setup_ap(void);
 
 extern u64 x86_read_arch_cap_msr(void);
 
-#ifdef CONFIG_IA32_FEAT_CTL
-void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
-#endif
-
 #endif /* ARCH_X86_CPU_H */
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index aaff9ed7ff45..b0d3c5ca6d80 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -193,6 +193,8 @@ static void fix_processor_context(void)
  */
 static void notrace __restore_processor_state(struct saved_context *ctxt)
 {
+	struct cpuinfo_x86 *c;
+
 	if (ctxt->misc_enable_saved)
 		wrmsrl(MSR_IA32_MISC_ENABLE, ctxt->misc_enable);
 	/*
@@ -263,6 +265,10 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	mtrr_bp_restore();
 	perf_restore_debug_store();
 	msr_restore_context(ctxt);
+
+	c = &cpu_data(smp_processor_id());
+	if (cpu_has(c, X86_FEATURE_MSR_IA32_FEAT_CTL))
+		init_ia32_feat_ctl(c);
 }
 
 /* Needed by apm.c */
-- 
2.26.0

Re: [PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2459 bytes --]

Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/x86/core]
[also build test ERROR on tip/auto-latest v5.7 next-20200605]
[cannot apply to bp/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Sean-Christopherson/x86-cpu-Reinitialize-IA32_FEAT_CTL-MSR-on-BSP-during-wakeup/20200606-040921
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 9cb1fd0efd195590b828b9b865421ad345a4a145
config: i386-tinyconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

arch/x86/kernel/cpu/centaur.c: In function 'init_centaur':
>> arch/x86/kernel/cpu/centaur.c:219:2: error: implicit declaration of function 'init_ia32_feat_ctl' [-Werror=implicit-function-declaration]
219 |  init_ia32_feat_ctl(c);
|  ^~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
arch/x86/kernel/cpu/zhaoxin.c: In function 'init_zhaoxin':
>> arch/x86/kernel/cpu/zhaoxin.c:110:2: error: implicit declaration of function 'init_ia32_feat_ctl' [-Werror=implicit-function-declaration]
110 |  init_ia32_feat_ctl(c);
|  ^~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

vim +/init_ia32_feat_ctl +219 arch/x86/kernel/cpu/centaur.c

60882cc159e141 arch/x86/kernel/cpu/centaur.c  David Wang          2018-04-20  218  
501444905fcb41 arch/x86/kernel/cpu/centaur.c  Sean Christopherson 2019-12-20 @219  	init_ia32_feat_ctl(c);
^1da177e4c3f41 arch/i386/kernel/cpu/centaur.c Linus Torvalds      2005-04-16  220  }
^1da177e4c3f41 arch/i386/kernel/cpu/centaur.c Linus Torvalds      2005-04-16  221  

:::::: The code at line 219 was first introduced by commit
:::::: 501444905fcb4166589fda99497c273ac5efc65e x86/centaur: Use common IA32_FEAT_CTL MSR initialization

:::::: TO: Sean Christopherson <sean.j.christopherson@intel.com>
:::::: CC: Borislav Petkov <bp@suse.de>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7244 bytes --]

Re: [PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/x86/core]
[also build test ERROR on tip/auto-latest v5.7 next-20200605]
[cannot apply to bp/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Sean-Christopherson/x86-cpu-Reinitialize-IA32_FEAT_CTL-MSR-on-BSP-during-wakeup/20200606-040921
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 9cb1fd0efd195590b828b9b865421ad345a4a145
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 6dd738e2f0609f7d3313b574a1d471263d2d3ba1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> arch/x86/kernel/cpu/centaur.c:219:2: error: implicit declaration of function 'init_ia32_feat_ctl' [-Werror,-Wimplicit-function-declaration]
init_ia32_feat_ctl(c);
^
1 error generated.
--
>> arch/x86/kernel/cpu/zhaoxin.c:110:2: error: implicit declaration of function 'init_ia32_feat_ctl' [-Werror,-Wimplicit-function-declaration]
init_ia32_feat_ctl(c);
^
1 error generated.

vim +/init_ia32_feat_ctl +219 arch/x86/kernel/cpu/centaur.c

60882cc159e141 arch/x86/kernel/cpu/centaur.c  David Wang          2018-04-20  218  
501444905fcb41 arch/x86/kernel/cpu/centaur.c  Sean Christopherson 2019-12-20 @219  	init_ia32_feat_ctl(c);
^1da177e4c3f41 arch/i386/kernel/cpu/centaur.c Linus Torvalds      2005-04-16  220  }
^1da177e4c3f41 arch/i386/kernel/cpu/centaur.c Linus Torvalds      2005-04-16  221  

:::::: The code at line 219 was first introduced by commit
:::::: 501444905fcb4166589fda99497c273ac5efc65e x86/centaur: Use common IA32_FEAT_CTL MSR initialization

:::::: TO: Sean Christopherson <sean.j.christopherson@intel.com>
:::::: CC: Borislav Petkov <bp@suse.de>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 73441 bytes --]

Re: [PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[Sean Christopherson]

On Sat, Jun 06, 2020 at 05:08:38AM +0800, kernel test robot wrote:
 arch/x86/kernel/cpu/centaur.c: In function 'init_centaur':
> >> arch/x86/kernel/cpu/centaur.c:219:2: error: implicit declaration of function 'init_ia32_feat_ctl' [-Werror=implicit-function-declaration]
> 219 |  init_ia32_feat_ctl(c);
> |  ^~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
> --
> arch/x86/kernel/cpu/zhaoxin.c: In function 'init_zhaoxin':
> >> arch/x86/kernel/cpu/zhaoxin.c:110:2: error: implicit declaration of function 'init_ia32_feat_ctl' [-Werror=implicit-function-declaration]
> 110 |  init_ia32_feat_ctl(c);
> |  ^~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors

Blech, zhaoxin.c an centaur.c don't include asm/cpu.h, and I (obviously)
don't have them enabled in my configs.  I'll wait a day or two more before
sending v2.

diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
index 426792565d86..c5cf336e5077 100644
--- a/arch/x86/kernel/cpu/centaur.c
+++ b/arch/x86/kernel/cpu/centaur.c
@@ -3,6 +3,7 @@
 #include <linux/sched.h>
 #include <linux/sched/clock.h>

+#include <asm/cpu.h>
 #include <asm/cpufeature.h>
 #include <asm/e820/api.h>
 #include <asm/mtrr.h>
diff --git a/arch/x86/kernel/cpu/zhaoxin.c b/arch/x86/kernel/cpu/zhaoxin.c
index df1358ba622b..05fa4ef63490 100644
--- a/arch/x86/kernel/cpu/zhaoxin.c
+++ b/arch/x86/kernel/cpu/zhaoxin.c
@@ -2,6 +2,7 @@
 #include <linux/sched.h>
 #include <linux/sched/clock.h>

+#include <asm/cpu.h>
 #include <asm/cpufeature.h>

 #include "cpu.h"

Re: [PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[Liam Merwick]

On 05/06/2020 21:07, Sean Christopherson wrote:
> Reinitialize IA32_FEAT_CTL on the BSP during wakeup to handle the case
> where firmware doesn't initialize or save/restore across S3.  This fixes
> a bug where IA32_FEAT_CTL is left uninitialized and results in VMXON
> taking a #GP due to VMX not being fully enabled, i.e. breaks KVM.
> 
> Use init_ia32_feat_ctl() to "restore" IA32_FEAT_CTL as it already deals
> with the case where the MSR is locked, and because APs already redo
> init_ia32_feat_ctl() during suspend by virtue of the SMP boot flow being
> used to reinitialize APs upon wakeup.  Do the call in the early wakeup
> flow to avoid dependencies in the syscore_ops chain, e.g. simply adding
> a resume hook is not guaranteed to work, as KVM does VMXON in its own
> resume hook, kvm_resume(), when KVM has active guests.
> 
> Reported-by: Brad Campbell <lists2009@fnarfbargle.com>
> Cc: Maxim Levitsky <mlevitsk@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: kvm@vger.kernel.org

Should it have the following tag since it fixes a commit introduced in 5.6?
Cc: stable@vger.kernel.org # v5.6

> Fixes: 21bd3467a58e ("KVM: VMX: Drop initialization of IA32_FEAT_CTL MSR")
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Reviewed-by: Liam Merwick <liam.merwick@oracle.com>

> ---
>   arch/x86/include/asm/cpu.h | 5 +++++
>   arch/x86/kernel/cpu/cpu.h  | 4 ----
>   arch/x86/power/cpu.c       | 6 ++++++
>   3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
> index dd17c2da1af5..da78ccbd493b 100644
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -58,4 +58,9 @@ static inline bool handle_guest_split_lock(unsigned long ip)
>   	return false;
>   }
>   #endif
> +#ifdef CONFIG_IA32_FEAT_CTL
> +void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
> +#else
> +static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c) {}
> +#endif
>   #endif /* _ASM_X86_CPU_H */
> diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
> index 37fdefd14f28..38ab6e115eac 100644
> --- a/arch/x86/kernel/cpu/cpu.h
> +++ b/arch/x86/kernel/cpu/cpu.h
> @@ -80,8 +80,4 @@ extern void x86_spec_ctrl_setup_ap(void);
>   
>   extern u64 x86_read_arch_cap_msr(void);
>   
> -#ifdef CONFIG_IA32_FEAT_CTL
> -void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
> -#endif
> -
>   #endif /* ARCH_X86_CPU_H */
> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
> index aaff9ed7ff45..b0d3c5ca6d80 100644
> --- a/arch/x86/power/cpu.c
> +++ b/arch/x86/power/cpu.c
> @@ -193,6 +193,8 @@ static void fix_processor_context(void)
>    */
>   static void notrace __restore_processor_state(struct saved_context *ctxt)
>   {
> +	struct cpuinfo_x86 *c;
> +
>   	if (ctxt->misc_enable_saved)
>   		wrmsrl(MSR_IA32_MISC_ENABLE, ctxt->misc_enable);
>   	/*
> @@ -263,6 +265,10 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
>   	mtrr_bp_restore();
>   	perf_restore_debug_store();
>   	msr_restore_context(ctxt);
> +
> +	c = &cpu_data(smp_processor_id());
> +	if (cpu_has(c, X86_FEATURE_MSR_IA32_FEAT_CTL))
> +		init_ia32_feat_ctl(c);
>   }
>   
>   /* Needed by apm.c */
> 

Re: [PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[Brad Campbell]

G'day Sean,

With the addition of this patch on a vanilla v5.7 :
Tested-by: Brad Campbell <lists2009@fnarfbargle.com>

On 8/6/20 12:34 am, Sean Christopherson wrote:
> On Sat, Jun 06, 2020 at 05:08:38AM +0800, kernel test robot wrote:
>   arch/x86/kernel/cpu/centaur.c: In function 'init_centaur':
>>>> arch/x86/kernel/cpu/centaur.c:219:2: error: implicit declaration of function 'init_ia32_feat_ctl' [-Werror=implicit-function-declaration]
>> 219 |  init_ia32_feat_ctl(c);
>> |  ^~~~~~~~~~~~~~~~~~
>> cc1: some warnings being treated as errors
>> --
>> arch/x86/kernel/cpu/zhaoxin.c: In function 'init_zhaoxin':
>>>> arch/x86/kernel/cpu/zhaoxin.c:110:2: error: implicit declaration of function 'init_ia32_feat_ctl' [-Werror=implicit-function-declaration]
>> 110 |  init_ia32_feat_ctl(c);
>> |  ^~~~~~~~~~~~~~~~~~
>> cc1: some warnings being treated as errors
> 
> Blech, zhaoxin.c an centaur.c don't include asm/cpu.h, and I (obviously)
> don't have them enabled in my configs.  I'll wait a day or two more before
> sending v2.
> diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
> index 426792565d86..c5cf336e5077 100644
> --- a/arch/x86/kernel/cpu/centaur.c
> +++ b/arch/x86/kernel/cpu/centaur.c
> @@ -3,6 +3,7 @@
>   #include <linux/sched.h>
>   #include <linux/sched/clock.h>
> 
> +#include <asm/cpu.h>
>   #include <asm/cpufeature.h>
>   #include <asm/e820/api.h>
>   #include <asm/mtrr.h>
> diff --git a/arch/x86/kernel/cpu/zhaoxin.c b/arch/x86/kernel/cpu/zhaoxin.c
> index df1358ba622b..05fa4ef63490 100644
> --- a/arch/x86/kernel/cpu/zhaoxin.c
> +++ b/arch/x86/kernel/cpu/zhaoxin.c
> @@ -2,6 +2,7 @@
>   #include <linux/sched.h>
>   #include <linux/sched/clock.h>
> 
> +#include <asm/cpu.h>
>   #include <asm/cpufeature.h>
> 
>   #include "cpu.h"
> 
> 

Re: [PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[mlevitsk]

On Fri, 2020-06-05 at 13:07 -0700, Sean Christopherson wrote:
> Reinitialize IA32_FEAT_CTL on the BSP during wakeup to handle the
> case
> where firmware doesn't initialize or save/restore across S3.  This
> fixes
> a bug where IA32_FEAT_CTL is left uninitialized and results in VMXON
> taking a #GP due to VMX not being fully enabled, i.e. breaks KVM.
> 
> Use init_ia32_feat_ctl() to "restore" IA32_FEAT_CTL as it already
> deals
> with the case where the MSR is locked, and because APs already redo
> init_ia32_feat_ctl() during suspend by virtue of the SMP boot flow
> being
> used to reinitialize APs upon wakeup.  Do the call in the early
> wakeup
> flow to avoid dependencies in the syscore_ops chain, e.g. simply
> adding
> a resume hook is not guaranteed to work, as KVM does VMXON in its own
> resume hook, kvm_resume(), when KVM has active guests.
> 
> Reported-by: Brad Campbell <lists2009@fnarfbargle.com>
> Cc: Maxim Levitsky <mlevitsk@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: kvm@vger.kernel.org
> Fixes: 21bd3467a58e ("KVM: VMX: Drop initialization of IA32_FEAT_CTL
> MSR")
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/include/asm/cpu.h | 5 +++++
>  arch/x86/kernel/cpu/cpu.h  | 4 ----
>  arch/x86/power/cpu.c       | 6 ++++++
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
> index dd17c2da1af5..da78ccbd493b 100644
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -58,4 +58,9 @@ static inline bool handle_guest_split_lock(unsigned
> long ip)
>  	return false;
>  }
>  #endif
> +#ifdef CONFIG_IA32_FEAT_CTL
> +void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
> +#else
> +static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c) {}
> +#endif
>  #endif /* _ASM_X86_CPU_H */
> diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
> index 37fdefd14f28..38ab6e115eac 100644
> --- a/arch/x86/kernel/cpu/cpu.h
> +++ b/arch/x86/kernel/cpu/cpu.h
> @@ -80,8 +80,4 @@ extern void x86_spec_ctrl_setup_ap(void);
>  
>  extern u64 x86_read_arch_cap_msr(void);
>  
> -#ifdef CONFIG_IA32_FEAT_CTL
> -void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
> -#endif
> -
>  #endif /* ARCH_X86_CPU_H */
> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
> index aaff9ed7ff45..b0d3c5ca6d80 100644
> --- a/arch/x86/power/cpu.c
> +++ b/arch/x86/power/cpu.c
> @@ -193,6 +193,8 @@ static void fix_processor_context(void)
>   */
>  static void notrace __restore_processor_state(struct saved_context
> *ctxt)
>  {
> +	struct cpuinfo_x86 *c;
> +
>  	if (ctxt->misc_enable_saved)
>  		wrmsrl(MSR_IA32_MISC_ENABLE, ctxt->misc_enable);
>  	/*
> @@ -263,6 +265,10 @@ static void notrace
> __restore_processor_state(struct saved_context *ctxt)
>  	mtrr_bp_restore();
>  	perf_restore_debug_store();
>  	msr_restore_context(ctxt);
> +
> +	c = &cpu_data(smp_processor_id());
> +	if (cpu_has(c, X86_FEATURE_MSR_IA32_FEAT_CTL))
> +		init_ia32_feat_ctl(c);
>  }
>  
>  /* Needed by apm.c */


I don't have currently an active VMX system to test this on,
but from the code and from my knowelege of this area this looks all
right.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[Sean Christopherson]

On Mon, Jun 08, 2020 at 11:12:35AM +0100, Liam Merwick wrote:
> On 05/06/2020 21:07, Sean Christopherson wrote:
> >Reinitialize IA32_FEAT_CTL on the BSP during wakeup to handle the case
> >where firmware doesn't initialize or save/restore across S3.  This fixes
> >a bug where IA32_FEAT_CTL is left uninitialized and results in VMXON
> >taking a #GP due to VMX not being fully enabled, i.e. breaks KVM.
> >
> >Use init_ia32_feat_ctl() to "restore" IA32_FEAT_CTL as it already deals
> >with the case where the MSR is locked, and because APs already redo
> >init_ia32_feat_ctl() during suspend by virtue of the SMP boot flow being
> >used to reinitialize APs upon wakeup.  Do the call in the early wakeup
> >flow to avoid dependencies in the syscore_ops chain, e.g. simply adding
> >a resume hook is not guaranteed to work, as KVM does VMXON in its own
> >resume hook, kvm_resume(), when KVM has active guests.
> >
> >Reported-by: Brad Campbell <lists2009@fnarfbargle.com>
> >Cc: Maxim Levitsky <mlevitsk@redhat.com>
> >Cc: Paolo Bonzini <pbonzini@redhat.com>
> >Cc: kvm@vger.kernel.org
> 
> Should it have the following tag since it fixes a commit introduced in 5.6?
> Cc: stable@vger.kernel.org # v5.6

It definitely warrants a backport to v5.6.  I didn't include a Cc to stable
because I swear I had seen an email fly by that stated an explicit Cc is
unnecessary/unwanted for tip-tree patches, but per a recent statement from
Boris it looks like I'm simply confused[*].  I'll add the Cc in v2.

[*] https://lkml.kernel.org/r/20200417164752.GF7322@zn.tnic

> >Fixes: 21bd3467a58e ("KVM: VMX: Drop initialization of IA32_FEAT_CTL MSR")
> >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Reviewed-by: Liam Merwick <liam.merwick@oracle.com>

Re: [PATCH] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup

[Borislav Petkov]

On Mon, Jun 08, 2020 at 10:29:21AM -0700, Sean Christopherson wrote:
> On Mon, Jun 08, 2020 at 11:12:35AM +0100, Liam Merwick wrote:
> > On 05/06/2020 21:07, Sean Christopherson wrote:
> > >Reinitialize IA32_FEAT_CTL on the BSP during wakeup to handle the case
> > >where firmware doesn't initialize or save/restore across S3.  This fixes
> > >a bug where IA32_FEAT_CTL is left uninitialized and results in VMXON
> > >taking a #GP due to VMX not being fully enabled, i.e. breaks KVM.
> > >
> > >Use init_ia32_feat_ctl() to "restore" IA32_FEAT_CTL as it already deals
> > >with the case where the MSR is locked, and because APs already redo
> > >init_ia32_feat_ctl() during suspend by virtue of the SMP boot flow being
> > >used to reinitialize APs upon wakeup.  Do the call in the early wakeup
> > >flow to avoid dependencies in the syscore_ops chain, e.g. simply adding
> > >a resume hook is not guaranteed to work, as KVM does VMXON in its own
> > >resume hook, kvm_resume(), when KVM has active guests.
> > >
> > >Reported-by: Brad Campbell <lists2009@fnarfbargle.com>
> > >Cc: Maxim Levitsky <mlevitsk@redhat.com>
> > >Cc: Paolo Bonzini <pbonzini@redhat.com>
> > >Cc: kvm@vger.kernel.org
> > 
> > Should it have the following tag since it fixes a commit introduced in 5.6?
> > Cc: stable@vger.kernel.org # v5.6
> 
> It definitely warrants a backport to v5.6.  I didn't include a Cc to stable
> because I swear I had seen an email fly by that stated an explicit Cc is
> unnecessary/unwanted for tip-tree patches, but per a recent statement from
> Boris it looks like I'm simply confused[*].  I'll add the Cc in v2.
> 
> [*] https://lkml.kernel.org/r/20200417164752.GF7322@zn.tnic

Yeah, I was simply parroting what Greg has told me. Maybe he should
finally do that script. :-P

Also, I believe Sasha's Skynet machine already does that...

CCed both.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette