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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190624221224.GA245468@romley-ivt3.sc.intel.com>
Date:   Mon, 24 Jun 2019 15:12:24 -0700
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ashok Raj <ashok.raj@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v5 2/5] x86/umwait: Initialize umwait control values

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