Re: [PATCH v3 4/5] x86/umwait: Add sysfs interface to control umwait maximum time

[Andy Lutomirski]

On Fri, May 24, 2019 at 5:05 PM Fenghua Yu <fenghua.yu@xxxxxxxxx> wrote:
>
> IA32_UMWAIT_CONTROL[31:2] determines the maximum time in TSC-quanta
> that processor can stay in C0.1 or C0.2. A zero value means no maximum
> time.
>
> Each instruction sets its own deadline in the instruction's implicit
> input EDX:EAX value. The instruction wakes up if the time-stamp counter
> reaches or exceeds the specified deadline, or the umwait maximum time
> expires, or a store happens in the monitored address range in umwait.
>
> Users can write an unsigned 32-bit number to
> /sys/devices/system/cpu/umwait_control/max_time to change the default
> value. Note that a value of zero means there is no limit. Low order
> two bits are ignored.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Reviewed-by: Ashok Raj <ashok.raj@xxxxxxxxx>
> Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
>  arch/x86/power/umwait.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/arch/x86/power/umwait.c b/arch/x86/power/umwait.c
> index cf5de7e1cc24..61076aad7138 100644
> --- a/arch/x86/power/umwait.c
> +++ b/arch/x86/power/umwait.c
> @@ -103,8 +103,45 @@ static ssize_t enable_c0_2_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(enable_c0_2);
>
> +static ssize_t
> +max_time_show(struct device *kobj, struct device_attribute *attr, char *buf)
> +{
> +       return sprintf(buf, "%u\n", umwait_max_time);
> +}
> +
> +static ssize_t max_time_store(struct device *kobj,
> +                             struct device_attribute *attr,
> +                             const char *buf, size_t count)
> +{
> +       u32 max_time;
> +       int ret;
> +
> +       ret = kstrtou32(buf, 0, &max_time);
> +       if (ret)
> +               return ret;
> +
> +       mutex_lock(&umwait_lock);
> +
> +       /* Only get max time value from bits[31:2] */
> +       max_time &= MSR_IA32_UMWAIT_CONTROL_MAX_TIME;

I think you should error out if high bits are set.  I'm okay with
masking off low bits, except that an input of 1 should not turn into
0, since 0 is special IIRC.