historical-speck.lore.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: speck@linutronix.de
Subject: [MODERATED] Re: [PATCH 2/3] V5 more sampling fun 2
Date: Mon, 6 Apr 2020 17:07:14 -0500	[thread overview]
Message-ID: <20200406220714.q2jzjftqlxqnhco7@treble> (raw)
In-Reply-To: =?utf-8?q?=3C64dda4916925a38de932137b0cc6ed301137d13f=2E15861?= =?utf-8?q?95554=2Egit=2Emgross=40linux=2Eintel=2Ecom=3E?=

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_".

> +};
> +
> +enum srbds_mitigations srbds_mitigation __ro_after_init = SRBDS_MITIGATION_FULL;

This can be static.

> +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"

> +	[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"

> +	[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)

> +	[SRBDS_HYPERVISOR]		= "Unknown",

A little more detail would be helpful:

  "Unknown: Dependent on hypervisor status"

> +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.

> +
> +	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.

> +
> +	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'.

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

out:

> --- 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?

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

> +
> +/*
> + * List affected CPU's for issues that cannot be enumerated.
> + */

I don't understand the comment, SRBDS seems to be enumerated above.

> +static const struct x86_cpu_id affected_cpus[] __initconst = {

cpu_vuln_blacklist?

> +	VULNHW_INTEL(IVYBRIDGE,		SRBDS),

VULNBL?

> +	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 */
	{}
};

> +	/*
> +	 * 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.

> 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.

-- 
Josh

  parent reply	other threads:[~2020-04-06 22:07 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 ` Josh Poimboeuf [this message]
2020-04-07  0:34   ` [MODERATED] Re: [PATCH 2/3] V5 more sampling fun 2 mark gross
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=20200406220714.q2jzjftqlxqnhco7@treble \
    --to=jpoimboe@redhat.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).