Re: [PATCH v2 3/3] x86/umwait: Control umwait maximum time

[Fenghua Yu]

On Wed, Jan 16, 2019 at 04:30:59PM -0800, Andy Lutomirski wrote:
> On Wed, Jan 16, 2019 at 4:13 PM Fenghua Yu <fenghua.yu@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > >  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.

CR4.TSD can be set up per process by prctl(PR_SET_TSC, PR_TSC_ENABLE)
to enable TSC or prctl(PR_SET_TSC, PR_TSC_SIGSEGV) to disable TSC.

For high performance workloads, user can turn TSC on.

Does it make sense?

Thanks.

-Fenghua