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 ; 27 Mar 2020 17:37:18 -0000 Received: from us-smtp-delivery-74.mimecast.com ([216.205.24.74]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1jHsuz-00036q-9f for speck@linutronix.de; Fri, 27 Mar 2020 18:37:17 +0100 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C71551926DA0 for ; Fri, 27 Mar 2020 17:37:12 +0000 (UTC) Received: from treble (ovpn-117-12.rdu2.redhat.com [10.10.117.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 72A1A60BF4 for ; Fri, 27 Mar 2020 17:37:12 +0000 (UTC) Date: Fri, 27 Mar 2020 12:37:10 -0500 From: Josh Poimboeuf Subject: [MODERATED] Re: [PATCH 2/3] v4 more sampling fun 2 Message-ID: <20200327173710.c6gblbcvb4ynxdjz@treble> References: <20200326031923.d7hwuq65bb7ymw6g@treble> <20200327162041.GA2707@mtg-dev.jf.intel.com> MIME-Version: 1.0 In-Reply-To: <20200327162041.GA2707@mtg-dev.jf.intel.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Fri, Mar 27, 2020 at 09:20:41AM -0700, speck for mark gross wrote: > > > + > > > + off: disable mitigation and remove performance > > > + impact to rdrand and rdseed > > > + > > > srcutree.counter_wrap_check [KNL] > > > Specifies how frequently to check for > > > grace-period sequence counter wrap for the > > > @@ -397,6 +399,103 @@ static int __init tsx_async_abort_parse_cmdline(char *str) > > > } > > > early_param("tsx_async_abort", tsx_async_abort_parse_cmdline); > > > > > > +#undef pr_fmt > > > +#define pr_fmt(fmt) "SRBDS: " fmt > > > + > > > +enum srbds_mitigations srbds_mitigation __ro_after_init = SRBDS_MITIGATION_FULL; > > > +static const char * const srbds_strings[] = { > > > + [SRBDS_NOT_AFFECTED] = "Not affected", > > > > I don't think the SRBDS_NOT_AFFECTED enum is needed. > > > > The "Not affected" string is already printed by cpu_show_common(). > > > > And srbds_configure_mitigation() can just check for > > boot_cpu_has_bug(X86_BUG_SRBDS) instead of this enum value. > > True. I prefer having the complete state machine represented by the > enumeration but, if you feel strongly I'll try to remove it. If the state isn't actually used by the sysfs printing code then it just serves to add confusion for a reader of the code. And notice none of the other mitigations have this. It's a subtle inconsistency, which is a recipe for future bugs. > > > + [SRBDS_MITIGATION_OFF] = "Vulnerable", > > > + [SRBDS_MITIGATION_UCODE_NEEDED] = "Vulnerable: no microcode", > > > + [SRBDS_MITIGATION_FULL] = "Mitigated", > > > + [SRBDS_NOT_AFFECTED_TSX_OFF] = "Not affected (TSX disabled)", > > > + [SRBDS_HYPERVISOR] = "Unknown", > > > +}; > > > + > > > +static bool srbds_off; > > > + > > > +void srbds_configure_mitigation(void) > > > > To me, "configure" sounds like it could be a software thing. > We are configuring the meditation by honoring the kernel command line in this > function. > > > > > How about update_srbds_msr(), which is also more consistent with some of > > the other naming in the file. > > If you like but, I was mimicking the TAA implementation when choosing the name > of this function. Hm, which TAA function name were you mimicking? > > > +static void __init srbds_select_mitigation(void) > > > +{ > > > + u64 ia32_cap; > > > + > > > + if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) { > > > + srbds_mitigation = SRBDS_HYPERVISOR; > > > + return; > > > + } > > > > It's confusing that this comes before the X86_BUG_SRBDS check. Is that > > on purpose? > VM vendors on keybase wanted it so. > > > > > If so, I presume it doesn't have the intended effect, because if booting > > in a guest on a non-vulnerable CPU, srbds_mitigation will get set to > > SRBDS_HYPERVISOR, but the shown state will still be "Not affected" > > because of how cpu_show_common() checks for the bug at the beginning. > > The intended effect is to report "Unknown" for the vulnerability status of > SRBDS for all guest VM's running this code and to never touch the MSR > controlling the mitigation. > > I think this is what happens. I'll double check and confirm with testing. Maybe I'm blind, but given how cpu_show_common() works, I don't see how that could possibly happen when boot_cpu_has_bug(X86_BUG_SRBDS) is false. If I'm wrong please enlighten me. > > > + > > > + if (!boot_cpu_has_bug(X86_BUG_SRBDS)) { > > > + srbds_mitigation = SRBDS_NOT_AFFECTED; > > > + return; > > > + } > > > + > > > + if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL)) { > > > + srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED; > > > + return; > > > + } > > > + > > > + if (boot_cpu_has_bug(X86_BUG_SRBDS)) { > > > + srbds_mitigation = SRBDS_MITIGATION_FULL; > > > + /* > > > + * Check to see if this is one of the MDS_NO systems supporting > > > + * TSX that are only exposed to SRBDS when TSX is enabled. > > > + */ > > > + ia32_cap = x86_read_arch_cap_msr(); > > > + if (ia32_cap & ARCH_CAP_MDS_NO) { > > > + if (!boot_cpu_has(X86_FEATURE_RTM)) > > > + srbds_mitigation = SRBDS_NOT_AFFECTED_TSX_OFF; > > > + } > > > > I wonder if this would be equivalent? > > > > if (!boot_cpu_has_bug(X86_BUG_MDS) && boot_cpu_has(X86_BUG_TAA) && > > !boot_cpu_has(X86_FEATURE_RTM)) > > srbds_mitigation = SRBDS_NOT_AFFECTED_TSX_OFF; > I have no clue if its equivalent but, I find your version much harder to parse > and my version matches more closely to what the white paper states. Ok. > > > + } > > > + > > > + if (cpu_mitigations_off() || srbds_off) { > > > + if (srbds_mitigation != SRBDS_NOT_AFFECTED_TSX_OFF) > > > + srbds_mitigation = SRBDS_MITIGATION_OFF; > > > + } > > > + > > > + srbds_configure_mitigation(); > > > +} > > > > The other mitigations do a printk with the mitigation status here. > > Should we not do the same here? > Perhaps but, I was attempting to limit the extra chatter in the kernel logs. > If you feel it adds value to the code I'll put it in. FWIW I think the chatter > from the other places should be removed. It should at least be consistent with the others. > > > +ssize_t cpu_show_special_register_data_sampling(struct device *dev, struct device_attribute *attr, char *buf) > > > +{ > > > > This function name can be renamed to match whatever the filename turns > > out to be: > > > > cpu_show_srbds() > > > > or > > > > cpu_show_special_register_data_buffer_sampling() > > I got some early review feedback to not use the shorter name "srbds" for the > sysfs inodes so I was trying to avoid the full spelled out the full names. > > "srbds" and "special_register_data_buffer_sampling" do not imply HWRNG security > risk so IMO I'd like to just use srbds. What do others think? +1 for "srbds" for both file name and function name. > > > +void srbds_configure_mitigation(void); > > > + > > > #ifdef CONFIG_CPU_SUP_INTEL > > > + > > > enum tsx_ctrl_states { > > > TSX_CTRL_ENABLE, > > > TSX_CTRL_DISABLE, > > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > > > index be82cd5841c3..1b083a2a415b 100644 > > > --- a/arch/x86/kernel/cpu/intel.c > > > +++ b/arch/x86/kernel/cpu/intel.c > > > @@ -684,6 +684,8 @@ static void init_intel(struct cpuinfo_x86 *c) > > > tsx_enable(); > > > if (tsx_ctrl_state == TSX_CTRL_DISABLE) > > > tsx_disable(); > > > + > > > + srbds_configure_mitigation(); > > > } > > > > I'm not sure this is the right place to call this. On the boot CPU, > > this gets called *before* the mitigation is selected, and then again > > from srbds_select_mitigation() itself. > > > > Maybe it should instead be called from identify_secondary_cpu(), while > > keeping the existing call from srbds_select_mitigation() so the boot CPU > > also gets updated. > The SRBDS MSR needs to be poked from each CPU thread if you want to disable the > mitigation. Does identify_secondary_cpu get called per HW thread? identify_secondary_cpu() gets called for every HW thread, other than the boot one. The boot one already does it in srbds_select_mitigation(). > I felt it would be nice to keep the call close to other mitigation setup > logic, in this case the TAA logic. That's not the TAA logic, but rather the TSX logic (though yes I realize they're related). And anyway SRBDS and TSX have different requirements. > What do you think it will be more readable if I move it to > identify_secondary_cpu? As I said - it gets called *twice* on the boot CPU. The first time is before srbds_mitigation has even been configured. That's confusing. -- Josh