lkml.org 
[lkml]   [2019]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2019-06-24 05:03    [W:0.165 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site