Message ID | 20200605200728.10145-1-sean.j.christopherson@intel.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup | expand |
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
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"
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 */ >
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" > >
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
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>
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.
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 */
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(-)