* [PATCH v2] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup
@ 2020-06-08 17:41 Sean Christopherson
2020-06-25 12:06 ` Rafael J. Wysocki
0 siblings, 1 reply; 4+ messages in thread
From: Sean Christopherson @ 2020-06-08 17:41 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
Tony W Wang-oc, Rafael J. Wysocki, Pavel Machek
Cc: H. Peter Anvin, linux-kernel, linux-pm, Brad Campbell,
Liam Merwick, Maxim Levitsky, Paolo Bonzini, kvm,
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>
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>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup
2020-06-08 17:41 [PATCH v2] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup Sean Christopherson
@ 2020-06-25 12:06 ` Rafael J. Wysocki
2020-06-25 12:27 ` Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2020-06-25 12:06 UTC (permalink / raw)
To: Sean Christopherson
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov,
the arch/x86 maintainers, Tony W Wang-oc, Rafael J. Wysocki,
Pavel Machek, H. Peter Anvin, Linux Kernel Mailing List,
Linux PM, Brad Campbell, Liam Merwick, Maxim Levitsky,
Paolo Bonzini, kvm-devel
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup
2020-06-25 12:06 ` Rafael J. Wysocki
@ 2020-06-25 12:27 ` Paolo Bonzini
2020-06-25 13:26 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2020-06-25 12:27 UTC (permalink / raw)
To: Rafael J. Wysocki, Sean Christopherson
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov,
the arch/x86 maintainers, Tony W Wang-oc, Rafael J. Wysocki,
Pavel Machek, H. Peter Anvin, Linux Kernel Mailing List,
Linux PM, Brad Campbell, Liam Merwick, Maxim Levitsky, kvm-devel
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
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup
2020-06-25 12:27 ` Paolo Bonzini
@ 2020-06-25 13:26 ` Borislav Petkov
0 siblings, 0 replies; 4+ messages in thread
From: Borislav Petkov @ 2020-06-25 13:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Rafael J. Wysocki, Sean Christopherson, Thomas Gleixner,
Ingo Molnar, the arch/x86 maintainers, Tony W Wang-oc,
Rafael J. Wysocki, Pavel Machek, H. Peter Anvin,
Linux Kernel Mailing List, Linux PM, Brad Campbell, Liam Merwick,
Maxim Levitsky, kvm-devel
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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-25 13:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-08 17:41 [PATCH v2] x86/cpu: Reinitialize IA32_FEAT_CTL MSR on BSP during wakeup Sean Christopherson
2020-06-25 12:06 ` Rafael J. Wysocki
2020-06-25 12:27 ` Paolo Bonzini
2020-06-25 13:26 ` Borislav Petkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).