Messages in this thread | | | Date | Mon, 24 Jun 2019 00:39:05 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v5 2/5] x86/umwait: Initialize umwait control values |
| |
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.
Thanks,
tglx
| |