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 ; 16 Apr 2020 22:55:07 -0000 Received: from mga09.intel.com ([134.134.136.24]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1jPDPR-0007qS-L2 for speck@linutronix.de; Fri, 17 Apr 2020 00:55:03 +0200 Date: Thu, 16 Apr 2020 15:54:56 -0700 From: mark gross Subject: [MODERATED] Re: [PATCH 3/4] V8 more sampling fun 3 Message-ID: <20200416225455.GC2583@u1904> Reply-To: mgross@linux.intel.com References: <20200416171723.zk3lzznvslmtt4zf@treble> MIME-Version: 1.0 In-Reply-To: <20200416171723.zk3lzznvslmtt4zf@treble> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Thu, Apr 16, 2020 at 12:17:23PM -0500, speck for Josh Poimboeuf wrote: > On Thu, Jan 16, 2020 at 02:16:07PM -0800, speck for mark gross wrote: > > From: mark gross > > Subject: [PATCH 3/4] x86/speculation: Special Register Buffer Data Sampling > > (SRBDS) mitigation control. > > Subjects don't need periods. > > > +static enum srbds_mitigations srbds_mitigation __ro_after_init = SRBDS_MITIGATION_FULL; > > +static const char * const srbds_strings[] = { > > + [SRBDS_MITIGATION_OFF] = "Vulnerable", > > + [SRBDS_MITIGATION_UCODE_NEEDED] = "Vulnerable: No microcode", > > + [SRBDS_MITIGATION_FULL] = "Mitigated: Microcode", > > FWIW, this is at least the third time I've made this comment... I'm very sorry I miss this multiple times. > s/Mitigated/Mitigation/ > > > + [SRBDS_MITIGATION_TSX_OFF] = "Mitigated: TSX disabled", > > s/Mitigated/Mitigation > > > @@ -1142,6 +1177,26 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c) > > (ia32_cap & ARCH_CAP_TSX_CTRL_MSR))) > > setup_force_cpu_bug(X86_BUG_TAA); > > > > + /* > > + * Some parts on the list don't have RDRAND or RDSEED. Make sure > > + * they show as "Not affected". > > + */ > > + if (cpu_has(c, X86_FEATURE_RDRAND) || cpu_has(c, X86_FEATURE_RDSEED)) { > > + if (cpu_matches(SRBDS, cpu_vuln_blacklist)) { > > + /* > > + * Parts in the blacklist that enumerate MDS_NO are > > + * only vulnerable if TSX can be used. To handle cases > > + * where TSX gets fused off check to see if TSX is > > + * fused off and thus not affected. > > + */ > > + if ((ia32_cap & ARCH_CAP_MDS_NO) && tsx_fused_off(c)) > > + goto srbds_not_affected; > > + > > + setup_force_cpu_bug(X86_BUG_SRBDS); > > + } > > + } > > + > > +srbds_not_affected: > > if (cpu_matches(NO_MELTDOWN, cpu_vuln_whitelist)) > > return; > > When nitpicking the whitespace before, I think I completely missed the > fact that this goto is extremely ugly. And there are a lot of > unnecessary nested ifs. And the comment is redundant. > > How about something like this: > > /* > * Parts in the SRBDS blacklist that enumerate MDS_NO are only > * vulnerable if TSX isn't fused off. > */ > if (cpu_matches(SRBDS, cpu_vuln_blacklist) && > (cpu_has(c, X86_FEATURE_RDRAND) || cpu_has(c, X86_FEATURE_RDSEED)) && > (!(ia32_cap & ARCH_CAP_MDS_NO) || !tsx_fused_off(c))) > setup_force_cpu_bug(X86_BUG_SRBDS); I had this in an unreleased version but, I thought the redability of the conditional too much for me so I created the tsx_fused_off funtion to. I also considerd the contropositive of what I did have if (!(ia32_cap & ARCH_CAP_MDS_NO) || !tsx_fused_off(c)) goto srbds_not_affected; also hard to read / parse finally I thought of: if ((ia32_cap & ARCH_CAP_MDS_NO) && tsx_fused_off(c)) continue; else setup_force_cpu_bug(X86_BUG_SRBDS); and thought that looked pretty dumb too. But, Like I said I'm not going to argue. --mark