perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from NMI context

Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI context is problematic since the only way to change the various unrelated bits in there is:

  debugctl = get_debugctlmsr()
  /* frob flags in debugctl */
  update_debugctlmsr(debugctl);

Which is entirely unsafe if we prod at the MSR from NMI context.

In particular the path that is responsible is:

  x86_pmu_handle_irq() (NMI handler)
    x86_pmu_stop()
      x86_pmu.disable -> intel_pmu_disable_event()
        intel_pmu_lbr_disable()
          __intel_pmu_lbr_disable()
            wrmsrl(MSR_IA32_DEBUGCTLMSR,... );

So remove intel_pmu_lbr_{en,dis}able() from intel_pmu_{en,dis}able_event() and stick them in intel_{get,put}_event_constraints() which are only ever called from regular contexts.

We combine intel_pmu_needs_lbr_smpl(), which tells us if the events wants LBR data, with event->hw.branch_reg.alloc, which given intel_shared_regs_constraints() is set if our event got the shared resource, to give us a correctly balanced condition in {get,put}_event_constraints() for intel_pmu_lbr_{en,dis}able().

Also change core_pmu to only use x86_get_event_constraints since it doesn't support any of the fancy DS (BTS,PEBS), LBR and OFFCORE features that make up intel_{get,put}_event_constraints.

Signed-off-by Peter Zijlstra <[email protected]>

---
 arch/x86/kernel/cpu/perf_event_intel.c | 48 ++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 19 deletions(-)

arch/x86/kernel/cpu/perf_event_intel.c +29 -19

index 0d3d63a..ef4cd36 100644
@@ -821,6 +821,11 @@ static inline bool intel_pmu_needs_lbr_smpl(struct perf_event *event)
        return false;
 }
 
+static inline bool intel_pmu_has_lbr(struct perf_event *event)
+{
+       return intel_pmu_needs_lbr_smpl(event) && event->hw.branch_reg.alloc;
+}
+
 static void intel_pmu_disable_all(void)
 {
        struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);

@@ -975,13 +980,6 @@ static void intel_pmu_disable_event(struct perf_event *event)
        cpuc->intel_ctrl_guest_mask &= ~(1ull << hwc->idx);
        cpuc->intel_ctrl_host_mask &= ~(1ull << hwc->idx);
 
-       /*
-        * must disable before any actual event
-        * because any event may be combined with LBR
-        */
-       if (intel_pmu_needs_lbr_smpl(event))
-               intel_pmu_lbr_disable(event);
-
        if (unlikely(hwc->config_base == MSR_ARCH_PERFMON_FIXED_CTR_CTRL)) {
                intel_pmu_disable_fixed(hwc);
                return;

@@ -1036,12 +1034,6 @@ static void intel_pmu_enable_event(struct perf_event *event)
                intel_pmu_enable_bts(hwc->config);
                return;
        }
-       /*
-        * must enabled before any actual event
-        * because any event may be combined with LBR
-        */
-       if (intel_pmu_needs_lbr_smpl(event))
-               intel_pmu_lbr_enable(event);
 
        if (event->attr.exclude_host)
                cpuc->intel_ctrl_guest_mask |= (1ull << hwc->idx);

@@ -1382,17 +1374,28 @@ intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event
 
        c = intel_bts_constraints(event);
        if (c)
-               return c;
+               goto got_constraint;
 
        c = intel_pebs_constraints(event);
        if (c)
-               return c;
+               goto got_constraint;
 
        c = intel_shared_regs_constraints(cpuc, event);
        if (c)
-               return c;
+               goto got_constraint;
+
+       c = x86_get_event_constraints(cpuc, event);
+
+got_constraint:
 
-       return x86_get_event_constraints(cpuc, event);
+       /*
+        * Must enabled before any actual event because any event may be
+        * combined with LBR.
+        */
+       if (intel_pmu_has_lbr(event))
+               intel_pmu_lbr_enable(event);
+
+       return c;
 }
 
 static void

@@ -1413,6 +1416,14 @@ intel_put_shared_regs_event_constraints(struct cpu_hw_events *cpuc,
 static void intel_put_event_constraints(struct cpu_hw_events *cpuc,
                                        struct perf_event *event)
 {
+       /*
+        * Must disabled after any actual event because any event may be
+        * combined with LBR, but must be done before releasing the shared
+        * regs resource since that protects the LBR state.
+        */
+       if (intel_pmu_has_lbr(event))
+               intel_pmu_lbr_disable(event);
+
        intel_put_shared_regs_event_constraints(cpuc, event);
 }
 

@@ -1623,8 +1634,7 @@ static __initconst const struct x86_pmu core_pmu = {
         * the generic event period:
         */
        .max_period             = (1ULL << 31) - 1,
-       .get_event_constraints  = intel_get_event_constraints,
-       .put_event_constraints  = intel_put_event_constraints,
+       .get_event_constraints  = x86_get_event_constraints,
        .event_constraints      = intel_core_event_constraints,
        .guest_get_msrs         = core_guest_get_msrs,
        .format_attrs           = intel_arch_formats_attr,

We use FREEZE_LBR_ON_PMI to sync LBR data with counter overflows. That means, LBR is already frozen by the time we get to the handler. But that means we need to re-enable LBR when we leave the handler. I don't think EOI is going to magically re-enable it. So we need to touch DEBUGCTL in the irq handler.

Ah, so I do think EIO will re-enable LBR,

also the handler is wrapped in
x86_pmu::{dis,en}able_all() which does end up calling intel_pmu_lbr_{dis,en}able_all().

However that leaves the MSR in the
exact same state on exit as it was on enter, so that's not a problem for
the: read-modify-write change.

Quoted 1 line · Peter Zijlstra

Ah, so I do think EIO will re-enable LBR,

OK, it does not, but the:

Quoted 3 lines

also the handler is wrapped in
x86_pmu::{dis,en}able_all() which does end up calling intel_pmu_lbr_{dis,en}able_all().

Thing does the re-enable for us,

Quoted 3 lines

However that leaves the MSR in the
exact same state on exit as it was on enter, so that's not a problem for
the: read-modify-write change.

in a safe way.

Quoted 11 lines · Peter Zijlstra

Quoted 1 line · Peter Zijlstra

Ah, so I do think EIO will re-enable LBR,

OK, it does not, but the:

Quoted 3 lines

also the handler is wrapped in
x86_pmu::{dis,en}able_all() which does end up calling intel_pmu_lbr_{dis,en}able_all().

Thing does the re-enable for us,

Quoted 5 lines

Quoted 3 lines

However that leaves the MSR in the
exact same state on exit as it was on enter, so that's not a problem for
the: read-modify-write change.

in a safe way.

Well, I think it does even when we have to stop events (x86_pmu_stop) because the buffer is full. Looks like we always re-enable lbr.

So looks like the handler is a wash for debugctl.

Quoted 17 lines · Stephane Eranian

Quoted 16 lines · Peter Zijlstra

Quoted 1 line · Peter Zijlstra

Ah, so I do think EIO will re-enable LBR,

OK, it does not, but the:

Quoted 3 lines

also the handler is wrapped in
x86_pmu::{dis,en}able_all() which does end up calling intel_pmu_lbr_{dis,en}able_all().

Thing does the re-enable for us,

Quoted 3 lines

However that leaves the MSR in the
exact same state on exit as it was on enter, so that's not a problem for
the: read-modify-write change.

Quoted 3 lines

Quoted 1 line

in a safe way.

Well, I think it does even when we have to stop events (x86_pmu_stop) because the buffer is full. Looks like we always re-enable lbr.

How so, without the proposed patch, the intel_pmu_disable_event() can do intel_pmu_lbr_disable() which decrements cpuc->lbr_users, so the final intel_pmu_enable_all()->intel_pmu_lbr_enable_all() will be a NOP, leaving LBR disabled, where we entered the NMI with LBR enabled.

Quoted 1 line

So looks like the handler is a wash for debugctl.

As for BTS, it looks like we don't throttle the thing at all, so we shouldn't ever get to the asymmetric thing, right?

Quoted 2 lines

As for BTS, it looks like we don't throttle the thing at all, so we shouldn't ever get to the asymmetric thing, right?

No you do, in the same function:
static void intel_pmu_disable_event(struct perf_event *event)

{
        struct hw_perf_event *hwc = &event->hw;
        struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);

        if (unlikely(hwc->idx == INTEL_PMC_IDX_FIXED_BTS)) {
                intel_pmu_disable_bts();
                intel_pmu_drain_bts_buffer();
                return;
        }

Quoted 4 lines · Stephane Eranian

Quoted 2 lines

As for BTS, it looks like we don't throttle the thing at all, so we shouldn't ever get to the asymmetric thing, right?

No you do, in the same function:
static void intel_pmu_disable_event(struct perf_event *event)

Quoted 9 lines

{
        struct hw_perf_event *hwc = &event->hw;
        struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);

        if (unlikely(hwc->idx == INTEL_PMC_IDX_FIXED_BTS)) {
                intel_pmu_disable_bts();
                intel_pmu_drain_bts_buffer();
                return;
        }

Right, but the main event loop in intel_pmu_handle_irq() is over the MSR_CORE_PERF_GLOBAL_STATUS status bits, BTS is not included in those, so we'd never end up calling x86_pmu_stop() on the associated event.

Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI context is problematic since the only way to change the various unrelated bits in there is:

  debugctl = get_debugctlmsr()
  /* frob flags in debugctl */
  update_debugctlmsr(debugctl);

Which is entirely unsafe if we prod at the MSR from NMI context.

In particular the path that is responsible is:

  x86_pmu_handle_irq() (NMI handler)
    x86_pmu_stop()
      x86_pmu.disable -> intel_pmu_disable_event()
        intel_pmu_lbr_disable()
          __intel_pmu_lbr_disable()
            wrmsrl(MSR_IA32_DEBUGCTLMSR,... );

Quoted 11 lines · Peter Zijlstra

Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI context is problematic since the only way to change the various unrelated bits in there is:

  debugctl = get_debugctlmsr()
  /* frob flags in debugctl */
  update_debugctlmsr(debugctl);

Which is entirely unsafe if we prod at the MSR from NMI context.

Quoted 8 lines

In particular the path that is responsible is:

  x86_pmu_handle_irq() (NMI handler)
    x86_pmu_stop()
      x86_pmu.disable -> intel_pmu_disable_event()
        intel_pmu_lbr_disable()
          __intel_pmu_lbr_disable()
            wrmsrl(MSR_IA32_DEBUGCTLMSR,... );

Not only.

x86_pmu_handle_irq() does intel_pmu_disable_all() and intel_pmu_enable_all(), this leads to intel_pmu_enable_bts() and intel_pmu_disable_bts().

And those intel_pmu_*_bts() are also called by intel_pmu_disable_event() and intel_pmu_enable_event(), the latter is probably fine.

Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI context is problematic since the only way to change the various unrelated bits in there is:

  debugctl = get_debugctlmsr()
  /* frob flags in debugctl */
  update_debugctlmsr(debugctl);

Which is entirely unsafe if we prod at the MSR from NMI context.

In particular the path that is responsible is:

  x86_pmu_handle_irq() (NMI handler)
    x86_pmu_stop()
      x86_pmu.disable -> intel_pmu_disable_event()
        intel_pmu_lbr_disable()
          __intel_pmu_lbr_disable()
            wrmsrl(MSR_IA32_DEBUGCTLMSR,... );

So remove intel_pmu_lbr_{en,dis}able() from intel_pmu_{en,dis}able_event() and stick them in intel_{get,put}_event_constraints() which are only ever called from regular contexts.

We combine intel_pmu_needs_lbr_smpl(), which tells us if the events wants LBR data, with event->hw.branch_reg.alloc, which given intel_shared_regs_constraints() is set if our event got the shared resource, to give us a correctly balanced condition in {get,put}_event_constraints() for intel_pmu_lbr_{en,dis}able().

Also change core_pmu to only use x86_get_event_constraints since it doesn't support any of the fancy DS (BTS,PEBS), LBR and OFFCORE features that make up intel_{get,put}_event_constraints.

Quoted 30 lines · Peter Zijlstra

Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI context is problematic since the only way to change the various unrelated bits in there is:

  debugctl = get_debugctlmsr()
  /* frob flags in debugctl */
  update_debugctlmsr(debugctl);

Which is entirely unsafe if we prod at the MSR from NMI context.

In particular the path that is responsible is:

  x86_pmu_handle_irq() (NMI handler)
    x86_pmu_stop()
      x86_pmu.disable -> intel_pmu_disable_event()
        intel_pmu_lbr_disable()
          __intel_pmu_lbr_disable()
            wrmsrl(MSR_IA32_DEBUGCTLMSR,... );

So remove intel_pmu_lbr_{en,dis}able() from intel_pmu_{en,dis}able_event() and stick them in intel_{get,put}_event_constraints() which are only ever called from regular contexts.

We combine intel_pmu_needs_lbr_smpl(), which tells us if the events wants LBR data, with event->hw.branch_reg.alloc, which given intel_shared_regs_constraints() is set if our event got the shared resource, to give us a correctly balanced condition in {get,put}_event_constraints() for intel_pmu_lbr_{en,dis}able().

Quoted 3 lines

Also change core_pmu to only use x86_get_event_constraints since it doesn't support any of the fancy DS (BTS,PEBS), LBR and OFFCORE features that make up intel_{get,put}_event_constraints.

OK, so with the added stipulation that touching the MSR from the NMI isn't a problem as long as we ensure we leave the MSR in the same state that we found it in, and the above is the only path found so far that could cause this to be violated, the patch is fine?

In particular we could note that both LBR and BTS use the DEBUGCTL MSR, but BTS doesn't throttle, so the LBR code is the only thing needing attention as per the above description.

Should be, though it is pretty ugly to stash all of this in the put/get constraints.

I will run some tests.

I wonder what this does when you come in to get/put with a fake cpuc. You don't want to perturb the local lbr which may be in use.