From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <jpoimboe@redhat.com>
Received: from mail.linutronix.de (193.142.43.55:993) by
  crypto-ml.lab.linutronix.de with IMAP4-SSL for <speck@linutronix.de>; 06 Apr
  2020 22:07:24 -0000
Received: from us-smtp-1.mimecast.com ([205.139.110.61]
 helo=us-smtp-delivery-1.mimecast.com)	by Galois.linutronix.de with esmtps
 (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256)	(Exim 4.80)	(envelope-from
 <jpoimboe@redhat.com>)	id 1jLZtq-0001mc-KU	for speck@linutronix.de; Tue, 07
 Apr 2020 00:07:23 +0200
Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com
 [10.5.11.14])	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))	(No
 client certificate requested)	by mimecast-mx01.redhat.com (Postfix) with
 ESMTPS id 711AC107ACC4	for <speck@linutronix.de>; Mon,  6 Apr 2020 22:07:17
 +0000 (UTC)
Received: from treble (ovpn-116-24.rdu2.redhat.com [10.10.116.24])
	by smtp.corp.redhat.com (Postfix) with ESMTPS id 2124A5D9C5
	for <speck@linutronix.de>; Mon,  6 Apr 2020 22:07:16 +0000 (UTC)
Date: Mon, 6 Apr 2020 17:07:14 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [MODERATED] Re: [PATCH 2/3] V5 more sampling fun 2
Message-ID: <20200406220714.q2jzjftqlxqnhco7@treble>
References: <cover.1586195554.git.mgross@linux.intel.com> =?utf-8?q?=3C64dda?=
 =?utf-8?q?4916925a38de932137b0cc6ed301137d13f=2E1586195554=2Egit=2Emgro?=
 =?utf-8?q?ss=40linux=2Eintel=2Ecom=3E?=
MIME-Version: 1.0
In-Reply-To: =?utf-8?q?=3C64dda4916925a38de932137b0cc6ed301137d13f=2E15861?=
 =?utf-8?q?95554=2Egit=2Emgross=40linux=2Eintel=2Ecom=3E?=
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
To: speck@linutronix.de
List-ID: <speck.linutronix.de>

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