On Thu, Sep 01, 2022 at 01:00:29PM +0200, Peter Zijlstra wrote: > Since I'm not feeling too well I figured I'd do something trivial and > whipped up the below patch. > > > --- > arch/x86/include/asm/cpufeatures.h | 3 ++ > arch/x86/include/asm/msr-index.h | 4 +++ > arch/x86/kernel/cpu/common.c | 69 ++++++++++++++++++++++++++++++-------- > arch/x86/kernel/cpu/scattered.c | 1 + > 4 files changed, 63 insertions(+), 14 deletions(-) We still need to do something about this. As this thread died out, I'll revive it by reviewing this patch. (I'm not an expert in arch/x86/ stuff, though!) > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h > index 333d94394516..9b92f4e5e80a 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -305,6 +305,7 @@ > #define X86_FEATURE_USE_IBPB_FW (11*32+16) /* "" Use IBPB during runtime firmware calls */ > #define X86_FEATURE_RSB_VMEXIT_LITE (11*32+17) /* "" Fill RSB on VM exit when EIBRS is enabled */ > #define X86_FEATURE_CALL_DEPTH (11*32+18) /* "" Call depth tracking for RSB stuffing */ > +#define X86_FEATURE_MCDT_NO (11*32+19) /* Not affected by MCDT */ Some of the other CPU feature flags have comments beginning with "", which apparently results in the feature not being listed in /proc/cpuinfo. (This header file is run through a shell script that looks at these comments and generates C code...) Should this "feature" be listed in /proc/cpuinfo? Looking for examples of other "feature" flags that mean that a CPU is not vulnerable to something, I found X86_FEATURE_AMD_SSB_NO and X86_FEATURE_BTC_NO. Those aren't listed in /proc/cpuinfo. Maybe this should be the same? Side note: maybe the comment should spell out "MXCSR Configuration Dependent Timing"? Acronyms can be hard to read. > +#define X86_BUG_DOIT X86_BUG(28) Maybe it should be X86_BUG_DODT? The bug is data operand *dependent* timing. Data operand *independent* timing is the desired behavior and the fix. > +#define X86_BUG_MCDT X86_BUG(29) According to Intel's documentation (https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html), MCDT is a separate bug which requires a separate mitigation. So I think any MCDT related stuff should be in a separate patch from DOITM. But more importantly, this patch doesn't actually implement any mitigation for MCDT. Should we be doing that? Intel recommends writing a certain value to MXCSR to mitigate MCDT. Is that feasible? > > #endif /* _ASM_X86_CPUFEATURES_H */ > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > index 6674bdb096f3..08b4e0c2f7d3 100644 > --- a/arch/x86/include/asm/msr-index.h > +++ b/arch/x86/include/asm/msr-index.h > @@ -119,6 +119,7 @@ > * Not susceptible to > * TSX Async Abort (TAA) vulnerabilities. > */ > +#define ARCH_CAP_DOIT BIT(12) /* Data Operand Independent Timing */ > #define ARCH_CAP_SBDR_SSDP_NO BIT(13) /* > * Not susceptible to SBDR and SSDP > * variants of Processor MMIO stale data > @@ -155,6 +156,9 @@ > * Return Stack Buffer Predictions. > */ > > +#define MSR_IA32_UARCH_MISC_CTL 0x00001b01 > +#define UARCH_MISC_DOIT BIT(0) /* Enable DOIT */ The Intel documentation calls this bit "DOITM" (Data Operand Independent Timing Mode), not "DOIT". > +static __always_inline void setup_doit(struct cpuinfo_x86 *c) > +{ > + u64 msr = 0; > + > + if (!cpu_has(c, X86_BUG_DOIT)) > + return; > + > + if (!doit_disabled) > + return; This is backwards; it needs to be 'if (doit_disabled)'. > diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c > index fd44b54c90d5..5063f8046554 100644 > --- a/arch/x86/kernel/cpu/scattered.c > +++ b/arch/x86/kernel/cpu/scattered.c > @@ -27,6 +27,7 @@ static const struct cpuid_bit cpuid_bits[] = { > { X86_FEATURE_APERFMPERF, CPUID_ECX, 0, 0x00000006, 0 }, > { X86_FEATURE_EPB, CPUID_ECX, 3, 0x00000006, 0 }, > { X86_FEATURE_INTEL_PPIN, CPUID_EBX, 0, 0x00000007, 1 }, > + { X86_FEATURE_MCDT_NO, CPUID_ECX, 5, 0x00000007, 2 }, The Intel documentation says this bit is CPUID.(EAX=7H,ECX=2):EDX[5]=1. So CPUID_ECX is wrong; it needs to be CPUID_EDX. - Eric
