Messages in this thread | | | From | Andrew Cooper <> | Subject | Re: [PATCH] x86/cpu: Avoid writing MSR_IA32_TSX_CTRL when writing it is not supported | Date | Thu, 8 Sep 2022 13:46:34 +0000 |
| |
On 08/09/2022 02:03, Pawan Gupta wrote: > On Tue, Sep 06, 2022 at 11:00:08PM +0000, Andrew Cooper wrote: >> On 06/09/2022 22:00, Peter Zijlstra wrote: >>> On Tue, Sep 06, 2022 at 10:56:47PM +0200, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 9/6/22 22:43, Peter Zijlstra wrote: >>>>> On Tue, Sep 06, 2022 at 10:17:43PM +0200, Hans de Goede wrote: >>>>>> On an Intel Atom N2600 (and presumable other Cedar Trail models) >>>>>> MSR_IA32_TSX_CTRL can be read, causing saved_msr.valid to be set for it >>>>>> by msr_build_context(). >>>>>> >>>>>> This causes restore_processor_state() to try and restore it, but writing >>>>>> this MSR is not allowed on the Intel Atom N2600 leading to: >>>>> FWIW, virt tends to do this same thing a lot. They'll allow reading >>>>> random MSRs and only fail on write. >>>> Right. So I guess I should send a v2 with an updated commit >>>> message mentioning this ? >>> Nah, just saying this is a somewhat common pattern with MSRs. >>> >>> The best ones are the one where writing the value read is invalid :/ or >>> those who also silently eat a 0 write just for giggles. Luckily that >>> doesn't happen often. >> Several comments. First of all, MSR_TSX_CTRL is a fully read/write >> MSR. If virt is doing this wrong, fix the hypervisor. But this doesn't >> look virt related? >> >> More importantly, MSR_TSX_CTRL does not plausibly exist on an Atom >> N2600, as it is more than a decade old. >> >> MSR_TSX_CTRL was retrofitted in microcode to the MDS_NO, TAA-vulnerable >> CPUs which is a very narrow range from about 1 quarter of 2019 which >> includes Cascade Lake, and then included architecturally on subsequent >> parts which support TSX. >> >> pm_save_spec_msr() is totally broken. It's poking MSRs blindly without >> checking the enumeration of the capability first. > pm_save_spec_msr() relies on valid-msr-check in build_msr_context(), but > obviously it is not working in this particular case. > > Does adding the enumeration check as below looks okay: > > (I am not sure if I got the enumeration right for MSR_AMD64_LS_CFG).
family >= 0x10 && family <= 0x18
> > --- > diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h > index 8cbf623f0ecf..a750c1a1964b 100644 > --- a/arch/x86/include/asm/cpu.h > +++ b/arch/x86/include/asm/cpu.h > @@ -76,6 +76,8 @@ static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c) {} > > extern __noendbr void cet_disable(void); > > +extern bool spec_msr_valid(u32 msr_id); > + > struct ucode_cpu_info; > > int intel_cpu_collect_info(struct ucode_cpu_info *uci); > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index 3e508f239098..7430a36fd7ae 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -1278,6 +1278,26 @@ static bool __init cpu_matches(const struct x86_cpu_id *table, unsigned long whi > return m && !!(m->driver_data & which); > } > > +bool spec_msr_valid(u32 msr_id) > +{ > + u64 ia32_cap = x86_read_arch_cap_msr(); > + > + switch (msr_id) { > + case MSR_IA32_SPEC_CTRL: > + return boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL); > + case MSR_IA32_TSX_CTRL: > + return !!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR); > + case MSR_TSX_FORCE_ABORT: > + return boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT); > + case MSR_IA32_MCU_OPT_CTRL: > + return boot_cpu_has(X86_FEATURE_SRBDS_CTRL); > + case MSR_AMD64_LS_CFG: > + return boot_cpu_has(X86_FEATURE_LS_CFG_SSBD); > + } > + > + return false; > +} > + > u64 x86_read_arch_cap_msr(void) > { > u64 ia32_cap = 0; > diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c > index bb176c72891c..8db73f7982c7 100644 > --- a/arch/x86/power/cpu.c > +++ b/arch/x86/power/cpu.c > @@ -520,8 +520,12 @@ static void pm_save_spec_msr(void) > MSR_IA32_MCU_OPT_CTRL, > MSR_AMD64_LS_CFG,
Checking the enumerations is definitely an improvement, but this wants to become a tuple list of { msr, flag } so it can't get out of sync.
Except two of the options aren't simple bits. The contents of MSR_ARCH_CAPS ought to become feature bits because it's a CPUID feature leaf in disguise.
AMD LS_CFG is more complicated, because the dispatch serialising bit needs setting unilaterally (families 0x10, 0x12 thru 0x18), but the SSBD control ought to resolve on the next context switch.
~Andrew
| |