From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (193.142.43.55:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 22 Oct 2019 17:51:06 -0000 Received: from mga07.intel.com ([134.134.136.100]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1iMyJE-0003yN-UA for speck@linutronix.de; Tue, 22 Oct 2019 19:51:05 +0200 Date: Tue, 22 Oct 2019 10:44:52 -0700 From: Pawan Gupta Subject: [MODERATED] Re: [PATCH v7 04/10] TAAv7 4 Message-ID: <20191022174452.GE29216@guptapadev.amr> References: <20191022165112.GK31458@zn.tnic> MIME-Version: 1.0 In-Reply-To: <20191022165112.GK31458@zn.tnic> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Tue, Oct 22, 2019 at 06:51:12PM +0200, speck for Borislav Petkov wrote: > On Mon, Oct 21, 2019 at 01:26:02PM -0700, speck for Pawan Gupta wrote: > > @@ -268,6 +270,110 @@ static int __init mds_cmdline(char *str) > > } > > early_param("mds", mds_cmdline); > > > > +#undef pr_fmt > > +#define pr_fmt(fmt) "TAA: " fmt > > + > > +/* Default mitigation for TAA-affected CPUs */ > > +static enum taa_mitigations taa_mitigation __ro_after_init = TAA_MITIGATION_VERW; > > +static bool taa_nosmt __ro_after_init; > > + > > +static const char * const taa_strings[] = { > > + [TAA_MITIGATION_OFF] = "Vulnerable", > > + [TAA_MITIGATION_UCODE_NEEDED] = "Vulnerable: Clear CPU buffers attempted, no microcode", > > + [TAA_MITIGATION_VERW] = "Mitigation: Clear CPU buffers", > > + [TAA_MITIGATION_TSX_DISABLE] = "Mitigation: TSX disabled", > > +}; > > + > > +static void __init taa_select_mitigation(void) > > +{ > > + u64 ia32_cap = x86_read_arch_cap_msr(); > > Do that MSR access... > > > + > > + if (!boot_cpu_has_bug(X86_BUG_TAA)) { > > + taa_mitigation = TAA_MITIGATION_OFF; > > + return; > > + } > > ... here. ok. > > + > > + /* > > + * As X86_BUG_TAA=1, TSX feature is supported by the hardware. If > > + * TSX was disabled (X86_FEATURE_RTM=0) earlier during tsx_init(). > > That sentence is having trouble saying whatever it is trying to say. :) I will fix it. > > + if (!boot_cpu_has(X86_FEATURE_RTM)) { > > + taa_mitigation = TAA_MITIGATION_TSX_DISABLE; > > + pr_info("%s\n", taa_strings[taa_mitigation]); > > + return; > > + } > > + > > + /* All mitigations turned off from cmdline (mitigations=off) */ > > + if (cpu_mitigations_off()) { > > + taa_mitigation = TAA_MITIGATION_OFF; > > + return; > > + } > > + > > + /* TAA mitigation is turned off from cmdline (tsx_async_abort=off) */ > > + if (taa_mitigation == TAA_MITIGATION_OFF) { > > goto out; > > and you slap an out label above the pr_info at the end of this function. > > To show better what I mean, here's the whole function fixed up: > > static void __init taa_select_mitigation(void) > { > u64 ia32_cap; > > if (!boot_cpu_has(X86_FEATURE_RTM)) { > taa_mitigation = TAA_MITIGATION_TSX_DISABLE; > goto out; > } There is a small problem with this, it will set taa_mitigation = TAA_MITIGATION_TSX_DISABLE; when there is no X86_BUG_TAA. > > if (!boot_cpu_has_bug(X86_BUG_TAA) || cpu_mitigations_off()) { > taa_mitigation = TAA_MITIGATION_OFF; > goto out; > } goto will cause it to differ from other mitigations that don't print individual mitigation status when mitigations are turned off globally (when cpu_mitigations_off() is true). > > + > > +static int __init tsx_async_abort_cmdline(char *str) > > That function name needs a verb: tsx_async_abort_parse_cmdline() Ok. > > > +{ > > + if (!boot_cpu_has_bug(X86_BUG_TAA)) > > + return 0; > > + > > + if (!str) > > + return -EINVAL; > > + > > + if (!strcmp(str, "off")) { > > + taa_mitigation = TAA_MITIGATION_OFF; > > + } else if (!strcmp(str, "full")) { > > + taa_mitigation = TAA_MITIGATION_VERW; > > + } else if (!strcmp(str, "full,nosmt")) { > > + taa_mitigation = TAA_MITIGATION_VERW; > > + taa_nosmt = true; > > + } > > + > > + return 0; > > +} > > +early_param("tsx_async_abort", tsx_async_abort_cmdline); > > Say what now?! > > The previous patch added this: > > tsx= [X86] Control Transactional Synchronization > Extensions (TSX) feature in Intel processors that > support TSX control. > > This parameter controls the TSX feature. The options are: > > on - Enable TSX on the system. > off - Disable TSX on the system. > > So what is the final name of command line option now? We support two cmdline parameters: tsx= To control TSX feature. tsx_async_abort= To control the TAA mitigation. > > > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > > index 885d4ac2111a..86f22c1e5912 100644 > > --- a/arch/x86/kernel/cpu/common.c > > +++ b/arch/x86/kernel/cpu/common.c > > @@ -1128,6 +1128,21 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c) > > if (!cpu_matches(NO_SWAPGS)) > > setup_force_cpu_bug(X86_BUG_SWAPGS); > > > > + /* > > + * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when: > > + * - TSX is supported or > > + * - TSX_CTRL is supported > > s/supported/present/ Ok. > > > + * > > + * TSX_CTRL check is needed for cases when TSX could be disabled before > > + * the kernel boot e.g. kexec > > + * TSX_CTRL check alone is not sufficient for cases when the microcode > > + * update is not present or running as guest that don't get TSX_CTRL. > > + */ > > + if (!(ia32_cap & ARCH_CAP_TAA_NO) && > > + (boot_cpu_has(X86_FEATURE_RTM) || > > You have a @c parameter passed in, use it: > > cpu_has(c, X86_FEATURE_RTM) Ok. Thanks, Pawan