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