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

On Tue, Apr 07, 2020 at 07:39:55AM -0500, speck for Josh Poimboeuf wrote:
> On Mon, Apr 06, 2020 at 05:34:56PM -0700, speck for mark gross wrote:
> > On Mon, Apr 06, 2020 at 05:07:14PM -0500, speck for Josh Poimboeuf wrote:
> > > > +	/*
> > > > +	 * 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.
> 
> Just to clarify, I was thinking something like:
> 
> 	ia32_cap = x86_read_arch_cap_msr();
> 	if ((ia32_cap & ARCH_CAP_MDS_NO) && !boot_cpu_has(X86_FEATURE_RTM)) {
> 		srbds_mitigation = SRBDS_NOT_AFFECTED_TSX_OFF;
> 		goto out;
> 	}
> 
> As far as I can tell, that doesn't mess up the hypervisor check, since
> it only sets SRBDS_HYPERVISOR if TSX_OFF isn't set.
after looking again I agree and using the goto allows simplification of the
following if block.

> > 
> > > > +
> > > > +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> > > > +		if (srbds_mitigation != SRBDS_NOT_AFFECTED_TSX_OFF)
> > > > +			srbds_mitigation = SRBDS_HYPERVISOR;
> > > > +		return;
with your goto out aboe I can simplify this if block by removing the nested if.

Thanks!

--mark

> > > > +	}
> > > > +
> > > > +/*
> > > > + * 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? 
> 
> Sounds good to me.
> 
> -- 
> Josh