[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

On Fri, Sep 06, 2019 at 12:46:45AM -0700, speck for Pawan Gupta wrote:
> On Wed, Sep 04, 2019 at 05:19:43AM -0700, speck for Dave Hansen wrote:
> > > diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> > > index e7b1fe929cab..cdfc6c3d11d1 100644
> > > --- a/arch/x86/kernel/cpu/tsx.c
> > > +++ b/arch/x86/kernel/cpu/tsx.c
> > > @@ -29,6 +29,18 @@ static void tsx_disable(void)
> > >  	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
> > >  }
> > >  
> > > +static void tsx_enable(void)
> > > +{
> > > +	u64 tsx;
> > > +
> > > +	rdmsrl(MSR_IA32_TSX_CTRL, tsx);
> > > +
> > > +	tsx &= ~MSR_TSX_CTRL_RTM_DISABLE;
> > > +	tsx &= ~MSR_TSX_CTRL_CPUID_CLEAR;
> > > +
> > > +	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
> > > +}
> > 
> > OK, so in the last patch we went through all the steps to enumerate this
> > sucker.  Is that still being respected here?
> 
> This doesn't affect the enumeration, only adds support for tsx=on case.
> 
> > 
> > Also, how would these bits have gotten set so that we need to clear them
> > here?  kexec?
> 
> Yes.

So you're saying we need to support the case where we booted with
tsx=off, but then want to kexec with tsx=on?

I don't know, that sounds a little esoteric to me.  Do we actually
support that type of scenario for other cmdline options?

> > > +static int __init tsx_cmdline(char *str)
> > > +{
> > > +	if (!str)
> > > +		return -EINVAL;
> > > +
> > > +	if (!strcmp(str, "on"))
> > > +		tsx_ctrl_state = TSX_CTRL_ENABLE;
> > > +	else if (!strcmp(str, "off"))
> > > +		tsx_ctrl_state = TSX_CTRL_DISABLE;
> > > +
> > > +	return 0;
> > > +}
> > > +early_param("tsx", tsx_cmdline);
> > 
> > Hmm...  Let's say I have a non-TSX system.  I specify tsx=on.  This will
> > set tsx_ctrl_state=TSX_CTRL_ENABLE, then tsx_ctrl_check_support() will
> > set it to tsx_ctrl_state=TSX_CTRL_NOT_SUPPORTED.
> > 
> > I *think* this all works, but I'd love a comment or two about it.  Maybe
> > even something that makes it clear that tsx_en/disable() are both only
> > called when TSX *AND* TSX_CTRL_MSR are supported.
> > 
> > This is all kinda weird and confusing because we're doing this all
> > without the X86_FEATURE bits for TSX.
> 
> If we add another X86_FEATURE bit it will have exact same state as
> X86_FEATURE_RTM.  Do we really need this?

How about moving the tsx_init() call to early_init_intel()?  Then you'd
be able to check the feature bit above.  And that might make the
ordering a little easier to follow.

-- 
Josh