[MODERATED] Re: [PATCH 2/3] V5 more sampling fun 2

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 <[email protected]>
> > 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)
I've been working addressing this issue.  It is common for Intel to fuse off
features on low end SKU's so this could happen.  After looking into how to
disambiguate a CFL part (Family 6, Model 158 stepping 13) with TSX fused off
from a CLF part where TSX is disabled using TSX_CNTRL MSR.  I'd like to side
step the problem.

The solution is code for common.c that I have a hard time parsing even though I
wrote it.  It makes my eyes bleed.  I wonder if it would be better to simply
reword the string used for sysfs in the SRBDS_NOT_AFFECTED_TSX_OFF case to
report simply "Not affected" i.e. drop the "(TSX disbled)" part to hide the
ambiguous case?

FWIW this is what I'm thinking of adding to common.c if we choose to not hide
this ambiguity.  We are checking with the uCode folks to confirm but
ARCH_CAP_TSX_CTRL_MSR will not be set if the part has TSX fused off and we
think we can use that to disambiguate:

1166        if (cpu_matches(SRBDS, cpu_vuln_blacklist)) {
1167               /*
1168                * Some low-end SKUs on the affected list do not support
1169                * RDRAND or RDSEED. Make sure they show as "Not affected".
1170                */
1171               if (cpu_has(c, X86_FEATURE_RDRAND) ||
1172                   cpu_has(c, X86_FEATURE_RDSEED)) {
1173                      /* Some low end SKU's of parts with F/M/S in the
1174                       * blacklist that normally are only affected if TSX is
1175                       * enabled could have TSX fused off.  To avoid reporting 
1176                       * "Not affected (TSX disabled)" check !MDS_NO or available
1177                       * TSX_CTRL_MSR as a check for parts not fused
1178                       * off.
1179                       */
1180                      if (!(ia32_cap & ARCH_CAP_MDS_NO) ||.
1181                          (ia32_cap & ARCH_CAP_TSX_CTRL_MSR))
1182                             setup_force_cpu_bug(X86_BUG_SRBDS);
1183 
1184               }
1185        }

Another option is to add back the SRBDS_MITIGATION_NOT_AFFECTED and do this TSX
fused off check of ARCH_CAP_TSX_CTRL_MSR in bugs.c when checking for
X86_FEATURE_RTM.

What do you or others think?

--mark