[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

On Tue, Sep 10, 2019 at 07:32:24PM -0700, speck for Pawan Gupta wrote:
> On Tue, Sep 10, 2019 at 11:33:34PM +0100, speck for Greg KH wrote:
> > On Tue, Sep 10, 2019 at 11:42:24AM -0700, Pawan Gupta wrote:
> > > On Fri, Sep 06, 2019 at 11:27:27AM +0200, speck for Greg KH wrote:
> > > > On Fri, Sep 06, 2019 at 12:28:35AM -0700, speck for Pawan Gupta wrote:
> > > > > > > +static int __init tsx_sysfs_init(void)
> > > > > > > +{
> > > > > > > +	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
> > > > > > > +		return -ENODEV;
> > > > > > > +
> > > > > > > +	return sysfs_create_file(&cpu_subsys.dev_root->kobj,
> > > > > > > +				 &dev_attr_tsx.attr);
> > > > > > 
> > > > > > Huge hint, if you have "driver" code calling a sysfs_* function,
> > > > > > something is really wrong.
> > > > > > 
> > > > > > This does not feel right, isn't there some attribute group that this
> > > > > > file should be assigned to instead of this "one off" file creation?
> > > > > 
> > > > > Yes there is an attribute group cpu_root_attrs (in drivers/base/cpu.c).
> > > > > Putting an arch specific attribute and supporting functions in a common
> > > > > file doesn't seem right either.
> > > > 
> > > > Then perhaps this isn't correct either as you are putting an
> > > > arch-specific attribute in a generic location :)
> > > 
> > > Firstly, sorry for taking your time.  If it makes your life easier I
> > > will be more than happy to contribute to backporting and testing this
> > > stuff.
> > 
> > That would be most appreciated.
> > 
> > > Adding tsx to cpu_subsys ends up at:
> > > /sys/devices/system/cpu/tsx
> > > 
> > > Which is where /sys/devices/system/cpu/vulnerabilities are.
> > > 
> > > I couldn't find a better place to put this.
> > 
> > $ ls /sys/devices/cpu/
> > allow_tsx_force_abort  format                      power      type
> > caps                   freeze_on_smi               rdpmc      uevent
> > events                 perf_event_mux_interval_ms  subsystem
> > 
> > Oh look, a tsx-specific cpu sysfs file on my laptop...
> 
> Yes, but these are PMU attributes.
> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/events/core.c#L2211
> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/events/core.c#L1831

Ick, ok, then no, you are back to putting it in /sys/devices/system/cpu/
then.

But that "feels" wrong given there really isn't anything else in that
directory for cpu features like this, they are all buried in the
individual cpuX directories, right?

Is there any other cpu feature you can turn on/off like this today in
the system that is controlled in sysfs?  If so, where is it at?

thanks,

greg k-h