lkml.org 
[lkml]   [2019]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 3/3] x86/umwait: Control umwait maximum time
On Wed, Jan 16, 2019 at 4:13 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
>
> On Wed, Jan 16, 2019 at 04:00:29PM -0800, Andy Lutomirski wrote:
> > On Wed, Jan 16, 2019 at 1:24 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
> > >
> > > IA32_UMWAIT_CONTROL[31:2] determines the maximum time in TSC-quanta
> > > that processor can stay in C0.1 or C0.2.
> > >
> > > The maximum time value in IA32_UMWAIT_CONTROL[31-2] is set as zero which
> > > means there is no global time limit for UMWAIT and TPAUSE instructions.
> > > Each process sets its own umwait maximum time as the instructions operand.
> > >
> > > User can specify global umwait maximum time through interface:
> > > /sys/devices/system/cpu/umwait_control/umwait_max_time
> > > The value in the interface is in decimal in TSC-quanta. Bits[1:0]
> > > are cleared when the value is stored.
> > >
> > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > > ---
> > > arch/x86/include/asm/msr-index.h | 2 ++
> > > arch/x86/power/umwait.c | 42 +++++++++++++++++++++++++++++++-
> > > 2 files changed, 43 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > > index b56bfecae0de..42b9104fc15b 100644
> > > --- a/arch/x86/include/asm/msr-index.h
> > > +++ b/arch/x86/include/asm/msr-index.h
> > > @@ -62,6 +62,8 @@
> > > #define MSR_IA32_UMWAIT_CONTROL 0xe1
> > > #define UMWAIT_CONTROL_C02_BIT 0x0
> > > #define UMWAIT_CONTROL_C02_MASK 0x00000001
> > > +#define UMWAIT_CONTROL_MAX_TIME_BIT 0x2
> > > +#define UMWAIT_CONTROL_MAX_TIME_MASK 0xfffffffc
> > >
> > > #define MSR_PKG_CST_CONFIG_CONTROL 0x000000e2
> > > #define NHM_C3_AUTO_DEMOTE (1UL << 25)
> > > diff --git a/arch/x86/power/umwait.c b/arch/x86/power/umwait.c
> > > index 95b3867aac1e..4a1a507d3bb7 100644
> > > --- a/arch/x86/power/umwait.c
> > > +++ b/arch/x86/power/umwait.c
> > > @@ -10,6 +10,7 @@
> > > #include <asm/msr.h>
> > >
> > > static int umwait_enable_c0_2 = 1; /* 0: disable C0.2. 1: enable C0.2. */
> > > +static u32 umwait_max_time; /* In TSC-quanta. Only bits [31:2] are used. */
> > > static DEFINE_MUTEX(umwait_lock);
> > >
> > > /* Return value that will be used to set umwait control MSR */
> > > @@ -20,7 +21,8 @@ static inline u32 umwait_control_val(void)
> > > * When bit 0 is 1, C0.2 is disabled. Otherwise, C0.2 is enabled.
> > > * So value in bit 0 is opposite of umwait_enable_c0_2.
> > > */
> > > - return ~umwait_enable_c0_2 & UMWAIT_CONTROL_C02_MASK;
> > > + return (~umwait_enable_c0_2 & UMWAIT_CONTROL_C02_MASK) |
> > > + umwait_max_time;
> > > }
> > >
> > > static ssize_t umwait_enable_c0_2_show(struct device *dev,
> > > @@ -61,8 +63,46 @@ static ssize_t umwait_enable_c0_2_store(struct device *dev,
> > >
> > > static DEVICE_ATTR_RW(umwait_enable_c0_2);
> > >
> > > +static ssize_t umwait_max_time_show(struct device *kobj,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + return sprintf(buf, "%u\n", umwait_max_time);
> > > +}
> > > +
> > > +static ssize_t umwait_max_time_store(struct device *kobj,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + u32 msr_val, max_time;
> > > + int cpu, ret;
> > > +
> > > + ret = kstrtou32(buf, 10, &max_time);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mutex_lock(&umwait_lock);
> > > +
> > > + /* Only get max time value from bits [31:2] */
> > > + max_time &= UMWAIT_CONTROL_MAX_TIME_MASK;
> > > + /* Update the max time value in memory */
> > > + umwait_max_time = max_time;
> > > + msr_val = umwait_control_val();
> > > + get_online_cpus();
> > > + /* All CPUs have same umwait max time */
> > > + for_each_online_cpu(cpu)
> > > + wrmsr_on_cpu(cpu, MSR_IA32_UMWAIT_CONTROL, msr_val, 0);
> > > + put_online_cpus();
> > > +
> > > + mutex_unlock(&umwait_lock);
> > > +
> > > + return count;
> > > +}
> > > +
> > > +static DEVICE_ATTR_RW(umwait_max_time);
> > > +
> > > static struct attribute *umwait_attrs[] = {
> > > &dev_attr_umwait_enable_c0_2.attr,
> > > + &dev_attr_umwait_max_time.attr,
> > > NULL
> > > };
> >
> > You need something to make sure that newly onlined CPUs get the right
> > value in the MSR.
>
> Onlined CPU takes the umwait_control value in umwait_cpu_online() in
> patch 2. Please check if it's correct.
>
> > You also need to make sure you restore it on resume
> > from suspend. Something like cpu_init() might be the right place.
> >
> > Also, as previously discussed, I think we should set the default to
> > something quite small, maybe 100 microseconds. IMO the goal is to
> > pick a value that is a high enough multiple of the C0.2 entry+exit
> > latency that we get most of the power and SMT resource savings while
> > being small enough that no one things that UMWAIT is more than a
> > glorified, slightly improved, and far more misleading version of REP
> > NOP.
> >
> > Andrew, would having Linux default to a small value do much to
> > mitigate your concerns that UMWAIT is problematic for hypervisors?
> >
> > Also, can someone who understands the hardware clarify just how
> > dangerous UMWAIT is from a perspective of making speculation attacks
> > more dangerous than they already are? I'm wondering what events will
> > wake up a UMONITOR. I bet that CLFLUSH does. I wonder if a faulting
> > write to a read-only page also does. Or a load from a remote node.
> > Or a speculative store that does not subsequently retire. This
> > instruction seems quite delightful as a tool to create a
> > highish-bandwidth covert channel, and it's possibly quite nice to
> > agument Spectre-like attacks. If it ends up being bad enough, we
> > might need to set the default timeout to the absolute minimum value
> > and possibly ask Intel to give us a way to turn it off entirely.
>
> If CR4.TSD=1 and CPL>0, umwait and tpause generate #GP. So if user thinks
> the instructions are dangerous, he can disable TSC.
>
> Is this the right handling for security concerns?
>

Setting CR4.TSD=1 systemwide would utterly destroy performance of
quite a few workloads. And my argument for setting the value to a
lowish but not minimal value is about functionality, not security.

\
 
 \ /
  Last update: 2019-01-17 01:32    [W:0.121 / U:1.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site