historical-speck.lore.kernel.org archive mirror
 help / color / mirror / Atom feed
From: mark gross <mgross@linux.intel.com>
To: speck@linutronix.de
Subject: [MODERATED] Re: [PATCH 2/3] V5 more sampling fun 2
Date: Mon, 6 Apr 2020 17:34:56 -0700	[thread overview]
Message-ID: <20200407003456.GA58233@mtg-dev.jf.intel.com> (raw)
In-Reply-To: <20200406220714.q2jzjftqlxqnhco7@treble>

On Mon, Apr 06, 2020 at 05:07:14PM -0500, speck for Josh Poimboeuf wrote:
> On Thu, Jan 16, 2020 at 02:16:07PM -0800, speck for mark gross wrote:
> > From: mark gross <mgross@linux.intel.com>
> > Subject: [PATCH 2/3] x86/speculation: Special Register Buffer Data Sampling
> >  (SRBDS) mitigation control.
> > +enum srbds_mitigations {
> > +	SRBDS_MITIGATION_OFF,
> > +	SRBDS_MITIGATION_UCODE_NEEDED,
> > +	SRBDS_MITIGATION_FULL,
> > +	SRBDS_NOT_AFFECTED_TSX_OFF,
> > +	SRBDS_HYPERVISOR,
> 
> These should all be prefixed with "SRBDS_MITIGATION_".
fixed.

> > +};
> > +
> > +enum srbds_mitigations srbds_mitigation __ro_after_init = SRBDS_MITIGATION_FULL;
> 
> This can be static.
its static now.
> 
> > +static const char * const srbds_strings[] = {
> > +	[SRBDS_MITIGATION_OFF]		= "Vulnerable",
> > +	[SRBDS_MITIGATION_UCODE_NEEDED]	= "Vulnerable: no microcode",
> 
> "no microcode" should be capitalized:
> 
>   "Vulnerable: No microcode"
Done

> > +	[SRBDS_MITIGATION_FULL]		= "Mitigated",
> 
> All the other mitigations say "Mitigation: <description of mitigation>".
> 
> So to be consistent, and to not break dumb scripts which might rely on
> that, how about:
> 
>   "Mitigation: Microcode"
done

> > +	[SRBDS_NOT_AFFECTED_TSX_OFF]	= "Not affected (TSX disabled)",
> 
> Is this actually two distinct states?
> 
> 1) MDS_NO parts which support TSX, where the user has disabled TSX.  In
>    which case it should say:
> 
>    "Mitigation: TSX disabled"
> 
> 2) MDS_NO CPUs which *don't* support TSX, which should say:
> 
>   "Not affected"
> 
>   (I presume the bug bit doesn't need to be set in this case)

The white paper algorithm is simply:
Check boot CPU is in the list of affected processors then check for MDS_NO and
TSX status and special case those only if TSX is off and MDS_NO.


The situation of an MDS_NO part that has no TSX capability (not tsx is disable)
is not handled. Unless such parts don't exist or such parts are not included in
the affected processor list in the white paper.

I'll check with the chip folks to see if we are exposed to this corner case.



> > +	[SRBDS_HYPERVISOR]		= "Unknown",
> 
> A little more detail would be helpful:
> 
>   "Unknown: Dependent on hypervisor status"
done

> > +static void __init srbds_select_mitigation(void)
> > +{
> > +	u64 ia32_cap;
> > +
> > +	/*
> > +	 * This test relies on the CPUID values of vendor, family, model,
> > +	 * stepping which might not reflect the real hardware when we are
> > +	 * running as a guest. But VMM vendors have asked that we do this
> > +	 * before the X86_FEATURE_HYPERVISOR test since this provides better
> > +	 * guidance to users in most real situations.
> > +	 */
> 
> I wonder if this comment is really needed.  All the other mitigation
> code also checks for the bug bit first, so there's nothing unusual going
> on here.  But if you still think it's needed, it's fine with me.
I'm happy to drop the comment.

> > +
> > +	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
> > +		return;
> 
> No newline needed between the above comment and the above check.
> 
> Instead, put a newline here, before the next comment.
> 
> > +	/*
> > +	 * 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;
> > +	}
> 
> A 'goto out' would be helpful here; then the TSX_OFF checks below aren't
> needed and the flow is simplified.
a goto out would mess up the hypervisor check but, I'll add the goto for the
mid function returns that set the srbds_mitigation value.

> > +
> > +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> > +		if (srbds_mitigation != SRBDS_NOT_AFFECTED_TSX_OFF)
> > +			srbds_mitigation = SRBDS_HYPERVISOR;
> > +		return;
> > +	}
> > +
> > +	if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL)) {
> > +		srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED;
> > +		return;
> > +	}
> 
> This status is important and should be printed, via 'goto out'.
done

> > +
> > +	if (cpu_mitigations_off() || srbds_off) {
> > +		if (srbds_mitigation != SRBDS_NOT_AFFECTED_TSX_OFF)
> > +			srbds_mitigation = SRBDS_MITIGATION_OFF;
> > +	}
> > +
> 
> out:
done

> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -1075,9 +1075,34 @@ static const __initconst struct x86_cpu_id cpu_vuln_whitelist[] = {
> >  	{}
> >  };
> >  
> > -static bool __init cpu_matches(unsigned long which)
> > +#define VULNHW_INTEL_STEPPING(model, steppings, issues)			   \
> > +	X86_MATCH_VENDOR_FAM_MODEL_STEPPING_FEATURE(INTEL, 6,		   \
> > +					    INTEL_FAM6_##model, steppings, \
> > +					    X86_FEATURE_ANY, issues)
> > +
> > +#define VULNHW_INTEL(model, issues) X86_MATCH_INTEL_FAM6_MODEL(model, issues)
> > +#define SRBDS			BIT(1)
> 
> Why not start at bit 0?
no good reason,  its zero now.

> Also I don't think tabs are needed here since the value isn't being
> vertically aligned with anything AFAICT.
extra tabs gone.

> > +
> > +/*
> > + * List affected CPU's for issues that cannot be enumerated.
> > + */
> 
> I don't understand the comment, SRBDS seems to be enumerated above.
hmm, how about I remove the comment? 

> > +static const struct x86_cpu_id affected_cpus[] __initconst = {
> 
> cpu_vuln_blacklist?
I was trying to avoid using the string "blacklist" based on some diversity and
inclusion guidance but, I'm ok with changing the variable name to what you
recommend.

Changed to cpu_vuln_blacklist

.
> 
> > +	VULNHW_INTEL(IVYBRIDGE,		SRBDS),
> 
> VULNBL?
Why not... done

> 
> > +	VULNHW_INTEL(HASWELL,		SRBDS),
> > +	VULNHW_INTEL(HASWELL_L,		SRBDS),
> > +	VULNHW_INTEL(HASWELL_G,		SRBDS),
> > +	VULNHW_INTEL(BROADWELL_G,	SRBDS),
> > +	VULNHW_INTEL(BROADWELL,		SRBDS),
> > +	VULNHW_INTEL(SKYLAKE_L,		SRBDS),
> > +	VULNHW_INTEL(SKYLAKE,		SRBDS),
> > +	VULNHW_INTEL_STEPPING(KABYLAKE_L, GENMASK(0xC, 0), SRBDS), /*06_8E steppings <=C*/
> > +	VULNHW_INTEL_STEPPING(KABYLAKE, GENMASK(0xD, 0),   SRBDS), /*06_9E steppings <=D*/
> > +	{}
> 
> This can be more legible, with aligned columns and readable comments:
> 
> static const struct x86_cpu_id affected_cpus[] __initconst = {
> 	VULNHW_INTEL_STEPPING(IVYBRIDGE,	X86_STEPPING_ANY,	SRBDS),
> 	VULNHW_INTEL_STEPPING(HASWELL,		X86_STEPPING_ANY,	SRBDS),
> 	VULNHW_INTEL_STEPPING(HASWELL_L,	X86_STEPPING_ANY,	SRBDS),
> 	VULNHW_INTEL_STEPPING(HASWELL_G,	X86_STEPPING_ANY,	SRBDS),
> 	VULNHW_INTEL_STEPPING(BROADWELL_G,	X86_STEPPING_ANY,	SRBDS),
> 	VULNHW_INTEL_STEPPING(BROADWELL,	X86_STEPPING_ANY,	SRBDS),
> 	VULNHW_INTEL_STEPPING(SKYLAKE_L,	X86_STEPPING_ANY,	SRBDS),
> 	VULNHW_INTEL_STEPPING(SKYLAKE,		X86_STEPPING_ANY,	SRBDS),
> 	VULNHW_INTEL_STEPPING(KABYLAKE_L,	GENMASK(0xC, 0),	SRBDS), /* 06_8E steppings <= 0xC */
> 	VULNHW_INTEL_STEPPING(KABYLAKE,		GENMASK(0xD, 0),	SRBDS),	/* 06_9E steppings <= 0xD */
> 	{}
> };
done


> > +	/*
> > +	 * Some low-end SKUs on the affected list do not support
> > +	 * RDRAND or RDSEED. Make sure they show as "Not affected".
> > +	 */
> > +	if (cpu_matches(SRBDS, affected_cpus) &&
> > +	    (cpu_has(c, X86_FEATURE_RDRAND) || cpu_has(c, X86_FEATURE_RDSEED)))
> > +		setup_force_cpu_bug(X86_BUG_SRBDS);
> > +
> 
> Might as well put this after the TAA check so it's more chronological.
done

> > diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
> > index 37fdefd14f28..9188ad5be4ed 100644
> > --- a/arch/x86/kernel/cpu/cpu.h
> > +++ b/arch/x86/kernel/cpu/cpu.h
> > @@ -44,7 +44,10 @@ struct _tlb_table {
> >  extern const struct cpu_dev *const __x86_cpu_dev_start[],
> >  			    *const __x86_cpu_dev_end[];
> >  
> > +void update_srbds_msr(void);
> > +
> >  #ifdef CONFIG_CPU_SUP_INTEL
> > +
> >  enum tsx_ctrl_states {
> 
> Unnecessary newline after the ifdef.
newline removed.

--mark

  reply	other threads:[~2020-04-07  0:35 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-06 17:52 [MODERATED] [PATCH 0/3] V5 more sampling fun 0 mark gross
2020-01-16 22:16 ` [MODERATED] [PATCH 2/3] V5 more sampling fun 2 mark gross
2020-01-30 19:12 ` [MODERATED] [PATCH 3/3] V5 more sampling fun 3 mark gross
2020-03-17  0:56 ` [MODERATED] [PATCH 1/3] V5 more sampling fun 1 mark gross
     [not found] ` <5e8b7166.1c69fb81.4c99a.3619SMTPIN_ADDED_BROKEN@mx.google.com>
2020-04-06 18:31   ` [MODERATED] " Kees Cook
     [not found] ` <5e8b71d8.1c69fb81.64075.43abSMTPIN_ADDED_BROKEN@mx.google.com>
2020-04-06 18:34   ` [MODERATED] Re: [PATCH 2/3] V5 more sampling fun 2 Kees Cook
2020-04-06 18:37     ` Greg KH
2020-04-06 20:56       ` mark gross
2020-04-06 22:14       ` Luck, Tony
2020-04-07  7:51         ` Greg KH
2020-04-06 18:52     ` mark gross
     [not found] ` <5e8b71af.1c69fb81.d8b8.ac6bSMTPIN_ADDED_BROKEN@mx.google.com>
2020-04-06 18:34   ` [MODERATED] Re: [PATCH 3/3] V5 more sampling fun 3 Kees Cook
2020-04-06 22:07 ` [MODERATED] Re: [PATCH 2/3] V5 more sampling fun 2 Josh Poimboeuf
2020-04-07  0:34   ` mark gross [this message]
2020-04-07 12:39     ` Josh Poimboeuf
2020-04-08 20:26       ` mark gross
2020-04-08 22:14   ` mark gross
2020-04-08 22:58     ` mark gross
2020-04-07 15:17 ` Thomas Gleixner
2020-04-08 20:33   ` [MODERATED] " mark gross
2020-04-08 23:21     ` Thomas Gleixner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200407003456.GA58233@mtg-dev.jf.intel.com \
    --to=mgross@linux.intel.com \
    --cc=speck@linutronix.de \
    --subject='[MODERATED] Re: [PATCH 2/3] V5 more sampling fun 2' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).