| Message ID | 20200608174134.11157-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup | expand |
On Mon, Jun 8, 2020 at 7:49 PM Sean Christopherson <sean.j.christopherson@intel.com> 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> > Tested-by: Brad Campbell <lists2009@fnarfbargle.com> > Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: kvm@vger.kernel.org > 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> Given the regression fix nature of this patch, is it being taken care of by anyone (tip in particular) already? > --- > > v2: > - Collect Reviewed/Tested tags. [Brad, Liam, Maxim]. > - Include asm/cpu.h to fix Zhaoxin and Centaur builds. [Brad, LKP] > - Add Cc to stable. [Liam] > > arch/x86/include/asm/cpu.h | 5 +++++ > arch/x86/kernel/cpu/centaur.c | 1 + > arch/x86/kernel/cpu/cpu.h | 4 ---- > arch/x86/kernel/cpu/zhaoxin.c | 1 + > arch/x86/power/cpu.c | 6 ++++++ > 5 files changed, 13 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/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/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/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" > 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 >
On 25/06/20 14:06, Rafael J. Wysocki wrote: > On Mon, Jun 8, 2020 at 7:49 PM Sean Christopherson > <sean.j.christopherson@intel.com> 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> >> Tested-by: Brad Campbell <lists2009@fnarfbargle.com> >> Reviewed-by: Liam Merwick <liam.merwick@oracle.com> >> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: kvm@vger.kernel.org >> 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> > > Given the regression fix nature of this patch, is it being taken care > of by anyone (tip in particular) already? I was waiting for tip to pick it up, but I can as well with an Acked-by (KVM is broken by the patch but there's nothing KVM specific in it). Paolo >> --- >> >> v2: >> - Collect Reviewed/Tested tags. [Brad, Liam, Maxim]. >> - Include asm/cpu.h to fix Zhaoxin and Centaur builds. [Brad, LKP] >> - Add Cc to stable. [Liam] >> >> arch/x86/include/asm/cpu.h | 5 +++++ >> arch/x86/kernel/cpu/centaur.c | 1 + >> arch/x86/kernel/cpu/cpu.h | 4 ---- >> arch/x86/kernel/cpu/zhaoxin.c | 1 + >> arch/x86/power/cpu.c | 6 ++++++ >> 5 files changed, 13 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/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/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/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" >> 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 >> >
On Thu, Jun 25, 2020 at 02:27:46PM +0200, Paolo Bonzini wrote: > > Given the regression fix nature of this patch, is it being taken care > > of by anyone (tip in particular) already? > > I was waiting for tip to pick it up, but I can as well with an Acked-by > (KVM is broken by the patch but there's nothing KVM specific in it). https://git.kernel.org/tip/5d5103595e9e53048bb7e70ee2673c897ab38300 will be in -rc3, most likely.
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/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/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/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" 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 */