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

On Mon, Sep 09, 2019 at 08:42:54AM -0700, speck for Dave Hansen wrote:
> >>> +static void tsx_update_this_cpu(void *arg)
> >>> +{
> >>> +	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> >>> +	unsigned long enable = (unsigned long)arg;
> >>> +
> >>> +	if (enable) {
> >>> +		tsx_enable();
> >>> +		set_cpu_cap(c, X86_FEATURE_RTM);
> >>> +	} else {
> >>> +		tsx_disable();
> >>> +		clear_cpu_cap(c, X86_FEATURE_RTM);
> >>> +	}
> >>> +}
> >>
> >> This makes me really nervous.
> >>
> >> Do we have any other *runtime* (after early boot) manipulation of
> >> X86_FEATURE_* flags?
> > 
> > I don't think we have an option.  TSX_CTRL MSR write would change the
> > CPUID anyways.  It is probably better to have X86_FEATURE_RTM reflect
> > the correct state.
> 
> We have a choice.
> 
> If we don't have the infrastructure to do something right, we don't do
> it.  Poking at the bits and praying nothing breaks is not my preferred
> approach.  Could you please take a look and try to answer my question:
> 
> 	Do we have any other *runtime* (after early boot) manipulation
> 	of X86_FEATURE_* flags?
> 
> If the answer is no, then there's some homework that needs to be done to
> see what the implications are here and to make sure that nothing will
> break.  From the changelog and comments, it appears that research hasn't
> been done.

Right.  This patch feels dangerous and fragile...

And what's the point anyway?  There's hardly any justification in the
patch description.  Is it a theoretical problem or is there a real-world
production use case?

Please drop this patch, or at least, put it at the very end of the set.

-- 
Josh