[MODERATED] Re: [PATCH v4 02/10] TAAv4 2

[Pawan Gupta <pawan.kumar.gupta@linux.intel.com>]

On Wed, Sep 25, 2019 at 05:30:14PM -0500, speck for Josh Poimboeuf wrote:
> On Tue, Sep 03, 2019 at 02:12:32PM -0700, speck for Pawan Gupta wrote:
> > From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > Subject: [PATCH v4 02/10] x86/tsx: Add TSX control initialization
> > 
> > Add a new file to host TSX feature controls.  TSX control has three
> 
> "Add a new file" makes it sound like a sysfs file rather than a .c file.
> 
> And it would probably be better to lead off with a description of the
> *functional* change of the patch, rather than the not-so-interesting
> aspect of adding a .c file.  For example, this patch disables TSX by
> default.  That should also be reflected in the patch subject.
> 
> (This is of course pending the other discussion about what defaults make
> sense.)
> 
> > valid states ENABLE, DISABLE and NOT_SUPPORTED.  TSX may be used on
> > certain processors as part of a speculative side channel attack.
> 
> How is that relevant to this patch?

This is why we are defaulting to off.

> 
> > Set the boot time default to DISABLE or NOT_SUPPORTED based on the
> > presence of IA32_TSX_CTRL MSR.
> 
> This should say *why* it's changing the default to disabled, assuming
> that's what we decide to do.

Ok. Updating commit log to below:

    x86/tsx: Disable TSX by default
    
    Disable TSX by default on bootup. If IA32_TSX_CTRL MSR is not present,
    TSX state stays the compile time default which is NOT_SUPPORTED,
    otherwise change TSX state to DISABLE.  This is because on certain
    processsors TSX may be used as a part of a speculative side channel
    attack.

> 
> > +void tsx_init(struct cpuinfo_x86 *c)
> > +{
> > +	tsx_ctrl_check_support(c);
> > +
> > +	switch (tsx_ctrl_state) {
> > +	case TSX_CTRL_DISABLE:
> > +		tsx_disable();
> > +		clear_cpu_cap(c, X86_FEATURE_RTM);
> > +		setup_clear_cpu_cap(X86_FEATURE_RTM);
> 
> Can you clarify why setup_clear_cpu_cap() is needed?
> 
> AFAICT, setup_clear_cpu_cap() would be used for forcing a feature to be
> cleared on all CPUs, but this function already calls clear_cpu_cap() for
> every CPU anyway.

clear_cpu_cap() would update per-cpu capability bit for a particular CPU (used
by this_cpu_has()).  setup_clear_cpu_cap() updates the common (boot_cpu_data)
capability bit so that boot_cpu_has() tests are evaluated correctly. We need to
update both data structures.

Thanks,
Pawan