Date:   Wed, 16 Jan 2019 16:30:59 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Fenghua Yu <fenghua.yu@...el.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>,
        H Peter Anvin <hpa@...or.com>,
        Andrew Cooper <andrew.cooper3@...rix.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v2 3/3] x86/umwait: Control umwait maximum time

On Wed, Jan 16, 2019 at 4:13 PM Fenghua Yu <fenghua.yu@...el.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@...el.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@...el.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.

Powered by blists - more mailing lists