Re: [PATCH v5 2/5] x86/umwait: Initialize umwait control values

[Fenghua Yu <fenghua.yu@intel.com>]

On Mon, Jun 24, 2019 at 12:39:05AM +0200, Thomas Gleixner wrote:
> On Wed, 19 Jun 2019, Fenghua Yu wrote:
> >  
> > +#define MSR_IA32_UMWAIT_CONTROL			0xe1
> > +#define MSR_IA32_UMWAIT_CONTROL_C02_DISABLED	BIT(0)
> > +#define MSR_IA32_UMWAIT_CONTROL_MAX_TIME	0xfffffffc
> 
> Errm, no! That's not maxtime, that's the time field mask in the
> MSR. Throughout the code you use that as a mask, which is not really
> obvious.
> 
> > +	(((max_time) & MSR_IA32_UMWAIT_CONTROL_MAX_TIME) |		\
> 
> and later on:
> 
> 	if (max_time & ~MSR_IA32_UMWAIT_CONTROL_MAX_TIME)
> 
> What? How is anyone supposed to understand that?
> 
> 	if (max_time & ~MSR_IA32_UMWAIT_CONTROL_TIME_MASK)
> 
> makes it entirely clear that the value is not allowed to have any bits
> outside of the mask set.
> 
> > +
> > +#define UMWAIT_C02_ENABLED	(0 & MSR_IA32_UMWAIT_CONTROL_C02_DISABLED)
> 
> The AND is there for maximal confusion of the reader?
> 
> > +/*
> > + * On resume, set up IA32_UMWAIT_CONTROL MSR on BP which is the only active
> > + * CPU at this time. Setting up the MSR on APs when they are re-added later
> > + * using CPU hotplug.
> > + * The MSR on BP is supposed not to be changed during suspend and thus it's
> > + * unnecessary to set it again during resume from suspend. But at this point
> > + * we don't know resume is from suspend or hibernation. To simplify the
> > + * situation, just set up the MSR on resume from suspend.
> 
> We also do not trust any firmware by default whatever it is supposed to do.

Thank you very much for pointing out and fixing all the issues and merging
the patches into the tip tree!

I test the tip tree and everything works fine.

-Fenghua