CR4 handling improvements

arch/x86/include/asm/tlbflush.h +9 -0

+/* Set in this cpu's CR4. */
+static inline void cr4_set(unsigned long mask)
+{
+	unsigned long cr4;
+
+	cr4 = read_cr4();
+	cr4 |= mask;
+	write_cr4(cr4);
+}

Quoted 9 lines · Andy Lutomirski

arch/x86/include/asm/tlbflush.h +9 -0

+/* Set in this cpu's CR4. */
+static inline void cr4_set(unsigned long mask)
+{
+	unsigned long cr4;
+
+	cr4 = read_cr4();
+	cr4 |= mask;
+	write_cr4(cr4);
+}

arch/x86/include/asm/tlbflush.h line 36

Quoted 10 lines

arch/x86/include/asm/tlbflush.h +10 -0

+
+/* Clear in this cpu's CR4. */
+static inline void cr4_clear(unsigned long mask)
+{
+	unsigned long cr4;
+
+	cr4 = read_cr4();
+	cr4 &= ~mask;
+	write_cr4(cr4);
+}

I would have called these cr4_or() and cr4_nand(). When first reading this I expected cr4_set() to be a pure write_cr4() and cr4_clear() to do write_cr4(0) -- which obviously doesn't make too much sense.

cr4_{set,clear}_mask() might maybe be clearer but is more typing, and the logical ops as suggested should have unambiguous meaning.

Quoted 12 lines · Peter Zijlstra

Quoted 11 lines · Andy Lutomirski

arch/x86/include/asm/tlbflush.h +11 -0

+/* Set in this cpu's CR4. */
+static inline void cr4_set(unsigned long mask)
+{
+	unsigned long cr4;
+
+	cr4 = read_cr4();
+	cr4 |= mask;
+	write_cr4(cr4);
+}
+
+/* Clear in this cpu's CR4. */

Quoted 8 lines

arch/x86/include/asm/tlbflush.h line 36

Quoted 8 lines

arch/x86/include/asm/tlbflush.h +8 -0

+static inline void cr4_clear(unsigned long mask)
+{
+	unsigned long cr4;
+
+	cr4 = read_cr4();
+	cr4 &= ~mask;
+	write_cr4(cr4);
+}

Quoted 13 lines · Borislav Petkov

Quoted 12 lines · Peter Zijlstra

Quoted 11 lines · Andy Lutomirski

arch/x86/include/asm/tlbflush.h +11 -0

+/* Set in this cpu's CR4. */
+static inline void cr4_set(unsigned long mask)
+{
+ unsigned long cr4;
+
+ cr4 = read_cr4();
+ cr4 |= mask;
+ write_cr4(cr4);
+}
+
+/* Clear in this cpu's CR4. */

Quoted 8 lines

Quoted 8 lines

arch/x86/include/asm/tlbflush.h line 36

Quoted 8 lines

arch/x86/include/asm/tlbflush.h +8 -0

+static inline void cr4_clear(unsigned long mask)
+{
+ unsigned long cr4;
+
+ cr4 = read_cr4();
+ cr4 &= ~mask;
+ write_cr4(cr4);
+}

Btw, on a related note, we probably should check whether the bits we want to set/clear are already set/cleared and then save us the CR4 write.

It probably won't be the case all that much but still...

--
Regards/Gruss,
Boris.
--

That's in patch 2. I can move it here, though.

Context switches and TLB flushes can change individual bits of CR4. CR4 reads take several cycles, so store a shadow copy of CR4 in a per-cpu variable.

To avoid wasting a cache line, I added the CR4 shadow to cpu_tlbstate, which is already touched during context switches.

From a quick look, only switch_mm() pokes at tlb, switch_to() does not.

Context switches and TLB flushes can change individual bits of CR4. CR4 reads take several cycles, so store a shadow copy of CR4 in a per-cpu variable.

To avoid wasting a cache line, I added the CR4 shadow to cpu_tlbstate, which is already touched during context switches.

Quoted 6 lines · Andy Lutomirski

Context switches and TLB flushes can change individual bits of CR4. CR4 reads take several cycles, so store a shadow copy of CR4 in a per-cpu variable.

To avoid wasting a cache line, I added the CR4 shadow to cpu_tlbstate, which is already touched during context switches.

So does this even show in any workloads as any improvement?

Also, what's the rule with reading the shadow CR4? kvm only? Because svm_set_cr4() in svm.c reads the host CR4 too.

Should we make all code access the shadow CR4 maybe...

--
Regards/Gruss,
Boris.
--

The code is correct, but only for a rather subtle reason. This confused me for quite a while when I read switch_mm, so clarify the code to avoid confusing other people, too.

TBH, I wouldn't be surprised if this code was only correct by accident.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/mmu_context.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

arch/x86/include/asm/mmu_context.h +0 -0

index 166af2a8e865..04478103df37 100644

arch/x86/include/asm/mmu_context.h +4 -0

+		 * if prev->context.ldt is non-null, since we never free
+		 * an LDT.  But LDTs can't be shared across mms, so
+		 * prev->context.ldt won't be equal to next->context.ldt.
+		 */
 		if (unlikely(prev->context.ldt != next->context.ldt))
 			load_LDT_nolock(&next->context);
 	}

Signature · 7 lines

1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 20 lines · Andy Lutomirski

The code is correct, but only for a rather subtle reason. This confused me for quite a while when I read switch_mm, so clarify the code to avoid confusing other people, too.

TBH, I wouldn't be surprised if this code was only correct by accident.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/mmu_context.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

arch/x86/include/asm/mmu_context.h +0 -0

index 166af2a8e865..04478103df37 100644
@@ -53,7 +53,16 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
              /* Stop flush ipis for the previous mm */
              cpumask_clear_cpu(cpu, mm_cpumask(prev));
 

arch/x86/include/asm/mmu_context.h static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next, · line 61

Quoted 7 lines

arch/x86/include/asm/mmu_context.h +6 -1

-             /* Load the LDT, if the LDT is different: */
+             /*
+              * Load the LDT, if the LDT is different.
+              *
+              * It's possible leave_mm(prev) has been called.  If so,
+              * then prev->context.ldt could be out of sync with the
+              * LDT descriptor or the LDT register.  This can only happen

I'm staring at the code and trying to figure out where on the leave_mm() path this could happen. Got any code pointers?

:-)

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +4 -1

+              * if prev->context.ldt is non-null, since we never free
+              * an LDT.  But LDTs can't be shared across mms, so
+              * prev->context.ldt won't be equal to next->context.ldt.
+              */
              if (unlikely(prev->context.ldt != next->context.ldt))
                      load_LDT_nolock(&next->context);
      }
--

1.9.3

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
Regards/Gruss,
Boris.
--

I think it's the same as in the other case in switch_mm. leave_mm does cpumask_clear_cpu(cpu, mm_cpumask(active_mm)), and, once that has happened, modify_ldt won't send an IPI to this CPU. So, if leave_mm runs, and then another CPU calls modify_ldt on the mm that is in lazy mode here, it won't update our LDT register, so the LDT register and prev->context.ldt might not match.

Quoted 6 lines · Andy Lutomirski

I think it's the same as in the other case in switch_mm. leave_mm does cpumask_clear_cpu(cpu, mm_cpumask(active_mm)), and, once that has happened, modify_ldt won't send an IPI to this CPU. So, if leave_mm runs, and then another CPU calls modify_ldt on the mm that is in lazy mode here, it won't update our LDT register, so the LDT register and prev->context.ldt might not match.

Ok, let me see if I can follow with an example:

We call leave_mm() on, say, cpu 3 and mm_cpumask(active_mm) has cpu 3 and 4 set. Then, on cpu 4 we call modify_ldt on that same mm and there in alloc_ldt() we have this:

                if (!cpumask_equal(mm_cpumask(current->mm),
                                   cpumask_of(smp_processor_id())))
                        smp_call_function(flush_ldt, current->mm, 1);

and since we've cleared cpu 3 from the cpumask, we don't flush_ldt() on it and there you have the difference.

Am I close?

You're exactly correct, or at least you seem to understand it the way I do :)

We currently allow any process to use rdpmc. This significantly weakens the protection offered by PR_TSC_DISABLED, and it could be helpful to users attempting to exploit timing attacks.

Since we can't enable access to individual counters, use a very coarse heuristic to limit access to rdpmc: allow access only when a perf_event is mmapped. This protects seccomp sandboxes.

There is plenty of room to further tighen these restrictions. For example, on x86, *all* perf_event mappings set cap_user_rdpmc. This should probably be changed to only apply to perf_events that are accessible using rdpmc.

arch/x86/kernel/cpu/perf_event.c +12 -2

@@ -1852,10 +1865,26 @@ static ssize_t set_attr_rdpmc(struct device *cdev,
 	if (x86_pmu.attr_rdpmc_broken)
 		return -ENOTSUPP;
 
+	mutex_lock(&rdpmc_enable_mutex);
 	if (!!val != !!x86_pmu.attr_rdpmc) {
-		x86_pmu.attr_rdpmc = !!val;
-		on_each_cpu(change_rdpmc, (void *)val, 1);
+		if (val) {
+			static_key_slow_inc(&rdpmc_enabled);
+			on_each_cpu(refresh_pce, NULL, 1);
+			smp_wmb();
+			x86_pmu.attr_rdpmc = 1;
+		} else {
+			/*
+			 * This direction can race against existing
+			 * rdpmc-capable mappings.  Try our best regardless.
+			 */
+			x86_pmu.attr_rdpmc = 0;

Quoted 8 lines · Andy Lutomirski

We currently allow any process to use rdpmc. This significantly weakens the protection offered by PR_TSC_DISABLED, and it could be helpful to users attempting to exploit timing attacks.

Since we can't enable access to individual counters, use a very coarse heuristic to limit access to rdpmc: allow access only when a perf_event is mmapped. This protects seccomp sandboxes.

Quoted 4 lines

There is plenty of room to further tighen these restrictions. For example, on x86, *all* perf_event mappings set cap_user_rdpmc. This should probably be changed to only apply to perf_events that are accessible using rdpmc.

So I suppose this patch is a little over engineered,

Quoted 28 lines

arch/x86/kernel/cpu/perf_event.c +18 -2

@@ -1852,10 +1865,26 @@ static ssize_t set_attr_rdpmc(struct device *cdev,
      if (x86_pmu.attr_rdpmc_broken)
              return -ENOTSUPP;
 
+     mutex_lock(&rdpmc_enable_mutex);
      if (!!val != !!x86_pmu.attr_rdpmc) {
-             x86_pmu.attr_rdpmc = !!val;
-             on_each_cpu(change_rdpmc, (void *)val, 1);
+             if (val) {
+                     static_key_slow_inc(&rdpmc_enabled);
+                     on_each_cpu(refresh_pce, NULL, 1);
+                     smp_wmb();
+                     x86_pmu.attr_rdpmc = 1;
+             } else {
+                     /*
+                      * This direction can race against existing
+                      * rdpmc-capable mappings.  Try our best regardless.
+                      */
+                     x86_pmu.attr_rdpmc = 0;
+                     smp_wmb();
+                     static_key_slow_dec(&rdpmc_enabled);
+                     WARN_ON(static_key_true(&rdpmc_enabled));
+                     on_each_cpu(refresh_pce, NULL, 1);
+             }
      }
+     mutex_unlock(&rdpmc_enable_mutex);
 

arch/x86/kernel/cpu/perf_event.c static ssize_t set_attr_rdpmc(struct device *cdev, · line 1890

Quoted 2 lines

arch/x86/kernel/cpu/perf_event.c +0 -0

      return count;
 }

why do you care about that rdpmc_enabled static key thing? Also you should not expose static key control to userspace like this, they can totally wreck the system. At the very least it should be static_key_slow_dec_deferred() -- gawd I hate the static_key API.

gawd I hate the static_key API.

The current cap_user_rdpmc code seems rather confused to me. On x86, *all* events set cap_user_rdpmc if the global rdpmc control is set. But only x86_pmu events define .event_idx, so amd uncore events won't actually expose their rdpmc index to userspace.

Would it make more sense to add a flag PERF_X86_EVENT_RDPMC_PERMITTED that gets set on all events created while rdpmc == 1, to change x86_pmu_event_idx to do something like:

if (event->hw.flags & PERF_X86_EVENT_RDPMC_PERMITTED)
return event->hw.event_base_rdpmc + 1;
else
return 0;

and to change arch_perf_update_userpage cap_user_rdpmc to match PERF_X86_EVENT_RDPMC_PERMITTED?

Then we could ditch the static key and greatly simplify writes to the rdpmc flag by just counting PERF_X86_EVENT_RDPMC_PERMITTED events.

This would be a user-visible change on AMD, and I can't test it.

On a semi-related note: would this all be nicer if there were vdso function __u64 __vdso_perf_event__read_count(int fd, void *userpage)? This is very easy to do nowadays. If we got *really* fancy, it would be possible to have an rdpmc_safe in the vdso, which has some benefits, although it would be a bit evil and wouldn't work if userspace tracers like pin are in use.

Quoted 23 lines · Andy Lutomirski

The current cap_user_rdpmc code seems rather confused to me. On x86, *all* events set cap_user_rdpmc if the global rdpmc control is set. But only x86_pmu events define .event_idx, so amd uncore events won't actually expose their rdpmc index to userspace.

Would it make more sense to add a flag PERF_X86_EVENT_RDPMC_PERMITTED that gets set on all events created while rdpmc == 1, to change x86_pmu_event_idx to do something like:

if (event->hw.flags & PERF_X86_EVENT_RDPMC_PERMITTED)
return event->hw.event_base_rdpmc + 1;
else
return 0;

and to change arch_perf_update_userpage cap_user_rdpmc to match PERF_X86_EVENT_RDPMC_PERMITTED?

Then we could ditch the static key and greatly simplify writes to the rdpmc flag by just counting PERF_X86_EVENT_RDPMC_PERMITTED events.

This would be a user-visible change on AMD, and I can't test it.

Quoted 7 lines

On a semi-related note: would this all be nicer if there were vdso function __u64 __vdso_perf_event__read_count(int fd, void *userpage)? This is very easy to do nowadays. If we got *really* fancy, it would be possible to have an rdpmc_safe in the vdso, which has some benefits, although it would be a bit evil and wouldn't work if userspace tracers like pin are in use.

Also, I don't understand the purpose of cap_user_time. Wouldn't it be easier to just record the last CLOCK_MONOTONIC time and let the user call __vdso_clock_gettime if they need an updated time?

Quoted 32 lines · Andy Lutomirski

Quoted 23 lines · Andy Lutomirski

The current cap_user_rdpmc code seems rather confused to me. On x86, *all* events set cap_user_rdpmc if the global rdpmc control is set. But only x86_pmu events define .event_idx, so amd uncore events won't actually expose their rdpmc index to userspace.

Would it make more sense to add a flag PERF_X86_EVENT_RDPMC_PERMITTED that gets set on all events created while rdpmc == 1, to change x86_pmu_event_idx to do something like:

if (event->hw.flags & PERF_X86_EVENT_RDPMC_PERMITTED)
return event->hw.event_base_rdpmc + 1;
else
return 0;

and to change arch_perf_update_userpage cap_user_rdpmc to match PERF_X86_EVENT_RDPMC_PERMITTED?

Then we could ditch the static key and greatly simplify writes to the rdpmc flag by just counting PERF_X86_EVENT_RDPMC_PERMITTED events.

This would be a user-visible change on AMD, and I can't test it.

Quoted 7 lines

On a semi-related note: would this all be nicer if there were vdso function __u64 __vdso_perf_event__read_count(int fd, void *userpage)? This is very easy to do nowadays. If we got *really* fancy, it would be possible to have an rdpmc_safe in the vdso, which has some benefits, although it would be a bit evil and wouldn't work if userspace tracers like pin are in use.

Quoted 3 lines

Also, I don't understand the purpose of cap_user_time. Wouldn't it be easier to just record the last CLOCK_MONOTONIC time and let the user call __vdso_clock_gettime if they need an updated time?

Because perf doesn't use CLOCK_MONOTONIC. Due to performance considerations we used the sched_clock stuff, which tries its best to make the best of the TSC without reverting to HPET and the like.

Not to mention that CLOCK_MONOTONIC was not available from NMI context until very recently.

Also, things like c73deb6aecda ("perf/x86: Add ability to calculate TSC from perf sample timestamps") seem to suggest people actually use TSC for things as well.

Now we might change to using the new NMI safe CLOCK_MONOTONIC (with a fallback to use the sched_clock stuff on time challenged hardware) in order to ease the correlation between other trace thingies, but even then it makes sense to have this, having it here and reading the TSC within the seqcount loop ensures you've got consistent data and touch less cachelines for reading.

Quoted 10 lines · Peter Zijlstra

Quoted 3 lines

Also, I don't understand the purpose of cap_user_time. Wouldn't it be easier to just record the last CLOCK_MONOTONIC time and let the user call __vdso_clock_gettime if they need an updated time?

Because perf doesn't use CLOCK_MONOTONIC. Due to performance considerations we used the sched_clock stuff, which tries its best to make the best of the TSC without reverting to HPET and the like.

Not to mention that CLOCK_MONOTONIC was not available from NMI context until very recently.

I'm only talking about the userspace access to when an event was enabled and how long it's been running. I think that's what the cap_user_time stuff is for. I don't think those parameters are touched from NMI, right?

Point taken about sched_clock, though.

Quoted 10 lines

Also, things like c73deb6aecda ("perf/x86: Add ability to calculate TSC from perf sample timestamps") seem to suggest people actually use TSC for things as well.

Now we might change to using the new NMI safe CLOCK_MONOTONIC (with a fallback to use the sched_clock stuff on time challenged hardware) in order to ease the correlation between other trace thingies, but even then it makes sense to have this, having it here and reading the TSC within the seqcount loop ensures you've got consistent data and touch less cachelines for reading.

True.

OTOH, people (i.e. I) have optimized the crap out of __vdso_clock_gettime, and __vdso_perf_event_whatever could be similarly optimized.

Quoted 18 lines · Andy Lutomirski

Quoted 10 lines · Peter Zijlstra

Quoted 3 lines

Also, I don't understand the purpose of cap_user_time. Wouldn't it be easier to just record the last CLOCK_MONOTONIC time and let the user call __vdso_clock_gettime if they need an updated time?

Because perf doesn't use CLOCK_MONOTONIC. Due to performance considerations we used the sched_clock stuff, which tries its best to make the best of the TSC without reverting to HPET and the like.

Not to mention that CLOCK_MONOTONIC was not available from NMI context until very recently.

I'm only talking about the userspace access to when an event was enabled and how long it's been running. I think that's what the cap_user_time stuff is for. I don't think those parameters are touched from NMI, right?

Quoted 1 line

Point taken about sched_clock, though.

Well, mixing two time bases, one TSC based and one CLOCK_MONOTONIC is just asking for trouble IMO ;-)

Quoted 12 lines

Quoted 11 lines

Also, things like c73deb6aecda ("perf/x86: Add ability to calculate TSC from perf sample timestamps") seem to suggest people actually use TSC for things as well.

Now we might change to using the new NMI safe CLOCK_MONOTONIC (with a fallback to use the sched_clock stuff on time challenged hardware) in order to ease the correlation between other trace thingies, but even then it makes sense to have this, having it here and reading the TSC within the seqcount loop ensures you've got consistent data and touch less cachelines for reading.

Quoted 5 lines

True.

OTOH, people (i.e. I) have optimized the crap out of __vdso_clock_gettime, and __vdso_perf_event_whatever could be similarly optimized.

Maybe, but at that point we commit to yet another ABI... I'd rather just put a 'sane' implementation in a library or so.

Quoted 2 lines

Maybe, but at that point we commit to yet another ABI... I'd rather just put a 'sane' implementation in a library or so.

This cuts both ways, though. For vdso timekeeping, the underlying data structure has changed repeatedly, sometimes to add features, and sometimes for performance, and the vdso has done a good job insulating userspace from it. (In fact, until 3.16, even the same exact kernel version couldn't be relied on to have the same data structure with different configs, and even now, no one really wants to teach user libraries how to parse the pvclock data structures.)

I would certainly not suggest putting anything beyond the bare minimum into the vdso.

FWIW, something should probably specify exactly when it's safe to try a userspace rdpmc. I think that the answer is that, for a perf event watching a pid, only that pid can do it (in particular, other threads must not try). For a perf event monitoring a whole cpu, the answer is less clear to me.

Quoted 10 lines · Andy Lutomirski

Quoted 2 lines

Maybe, but at that point we commit to yet another ABI... I'd rather just put a 'sane' implementation in a library or so.

This cuts both ways, though. For vdso timekeeping, the underlying data structure has changed repeatedly, sometimes to add features, and sometimes for performance, and the vdso has done a good job insulating userspace from it. (In fact, until 3.16, even the same exact kernel version couldn't be relied on to have the same data structure with different configs, and even now, no one really wants to teach user libraries how to parse the pvclock data structures.)

Fair enough, but as it stands we've already committed to the data structure exposed to userspace.

Quoted 5 lines

FWIW, something should probably specify exactly when it's safe to try a userspace rdpmc. I think that the answer is that, for a perf event watching a pid, only that pid can do it (in particular, other threads must not try). For a perf event monitoring a whole cpu, the answer is less clear to me.

This all was really only meant to be used for self-monitoring, so where an event is attached to the very same task, anything else and I'm find disabling it.

Quoted 12 lines · Peter Zijlstra

Quoted 10 lines · Andy Lutomirski

Quoted 2 lines

Maybe, but at that point we commit to yet another ABI... I'd rather just put a 'sane' implementation in a library or so.

This cuts both ways, though. For vdso timekeeping, the underlying data structure has changed repeatedly, sometimes to add features, and sometimes for performance, and the vdso has done a good job insulating userspace from it. (In fact, until 3.16, even the same exact kernel version couldn't be relied on to have the same data structure with different configs, and even now, no one really wants to teach user libraries how to parse the pvclock data structures.)

Quoted 2 lines

Fair enough, but as it stands we've already committed to the data structure exposed to userspace.

True.

OTOH, if a vdso function gets added, a few releases go by, and all the userspace tools get updated, then the old data structure could be dropped if needed by clearing cap_user_rdpmc.

Anyway, this is so far out of scope for the current project that I'm going to ignore it.

Quoted 16 lines · Andy Lutomirski

Quoted 12 lines · Peter Zijlstra

Quoted 10 lines · Andy Lutomirski

Quoted 2 lines

Maybe, but at that point we commit to yet another ABI... I'd rather just put a 'sane' implementation in a library or so.

This cuts both ways, though. For vdso timekeeping, the underlying data structure has changed repeatedly, sometimes to add features, and sometimes for performance, and the vdso has done a good job insulating userspace from it. (In fact, until 3.16, even the same exact kernel version couldn't be relied on to have the same data structure with different configs, and even now, no one really wants to teach user libraries how to parse the pvclock data structures.)

Quoted 2 lines

Fair enough, but as it stands we've already committed to the data structure exposed to userspace.

Quoted 8 lines

True.

OTOH, if a vdso function gets added, a few releases go by, and all the userspace tools get updated, then the old data structure could be dropped if needed by clearing cap_user_rdpmc.

Anyway, this is so far out of scope for the current project that I'm going to ignore it.

OK, I lied.

I haven't tested it, but it looks like any existing users of cap_user_rdpmc may have serious issues. That flag is set to 1 for essentially all perf_events on x86, even events that aren't part of the x86_pmu. Since the default .event_idx callback doesn't return zero, lots of other events will appear to be rdpmcable. This includes the AMD uncore pmu, which looks like it actually supports rdpmc, but hwc->idx seems to be missing an offset.

If this is the case, then user code can't reliably use the userpage rdpmc mechanism, so maybe it should be deprecated (or at least get a new flag bit).

The current cap_user_rdpmc code seems rather confused to me. On x86, *all* events set cap_user_rdpmc if the global rdpmc control is set. But only x86_pmu events define .event_idx, so amd uncore events won't actually expose their rdpmc index to userspace.

Would it make more sense to add a flag PERF_X86_EVENT_RDPMC_PERMITTED that gets set on all events created while rdpmc == 1, to change x86_pmu_event_idx to do something like:

if (event->hw.flags & PERF_X86_EVENT_RDPMC_PERMITTED)
return event->hw.event_base_rdpmc + 1;
else
return 0;

and to change arch_perf_update_userpage cap_user_rdpmc to match PERF_X86_EVENT_RDPMC_PERMITTED?

Then we could ditch the static key and greatly simplify writes to the rdpmc flag by just counting PERF_X86_EVENT_RDPMC_PERMITTED events.

This would be a user-visible change on AMD, and I can't test it.

Quoted 17 lines · Andy Lutomirski

The current cap_user_rdpmc code seems rather confused to me. On x86, *all* events set cap_user_rdpmc if the global rdpmc control is set. But only x86_pmu events define .event_idx, so amd uncore events won't actually expose their rdpmc index to userspace.

Would it make more sense to add a flag PERF_X86_EVENT_RDPMC_PERMITTED that gets set on all events created while rdpmc == 1, to change x86_pmu_event_idx to do something like:

if (event->hw.flags & PERF_X86_EVENT_RDPMC_PERMITTED)
return event->hw.event_base_rdpmc + 1;
else
return 0;

and to change arch_perf_update_userpage cap_user_rdpmc to match PERF_X86_EVENT_RDPMC_PERMITTED?

Quoted 4 lines

Then we could ditch the static key and greatly simplify writes to the rdpmc flag by just counting PERF_X86_EVENT_RDPMC_PERMITTED events.

This would be a user-visible change on AMD, and I can't test it.

I have AMD hardware to test this. But yes something like that seems fine.

Before I totally screw this up: is .event_idx used for anything except userspace rdpmc?

There are a whole bunch of implementations of that callback:

- perf_event_idx_default seems fishy

- power_pmu_event_idx seems even fishier

- cpumsf_pmu_event_idx is the same as power_pmu_event_idx.

- perf_swevent_event_idx returns 0.

etc.

x86 is the only implementation of arch_perf_update_userpage, which makes me think that the .event_idx callback should just be removed and that arch_perf_update_userpage should be responsible for filling it in if needed.

Quoted 1 line

- cpumsf_pmu_event_idx is the same as power_pmu_event_idx.

Oh cute, lets ask the s390 people, do you guys have a userspace instruction to read the actual counter value?

Quoted 4 lines · Andy Lutomirski

There are a whole bunch of implementations of that callback:

- perf_event_idx_default seems fishy

I suppose I did that to encode the rule that 0 := disabled, figuring that if there is no actual instruction to access the data, all of this would be pointless anyhow.

Quoted 2 lines

- cpumsf_pmu_event_idx is the same as power_pmu_event_idx.

Oh cute, lets ask the s390 people, do you guys have a userspace instruction to read the actual counter value?

Quoted 2 lines

- perf_swevent_event_idx returns 0.

Right, guaranteed no way to access any of that, maybe we should make the default like that too.

I think it would makes sense to return 0 as default in the perf_event_idx_default() and let each PMU/arch that actually supports reading PMCs from user space return the proper index. And according to tools/perf/design.txt, index must be non-zero to trigger a user space read.

Quoted 5 lines · Hendrik Brueckner

I think it would makes sense to return 0 as default in the perf_event_idx_default() and let each PMU/arch that actually supports reading PMCs from user space return the proper index. And according to tools/perf/design.txt, index must be non-zero to trigger a user space read.

OK, I've got something like the below. Michael/Anton, would you please clarify the ppc book3s capabilities?

---
Subject: perf: Clean up pmu::event_idx
From: Peter Zijlstra <[email protected]>
Date: Tue Oct 21 11:10:21 CEST 2014

Andy reported that the current state of event_idx is rather confused. So remove all but the x86_pmu implementation and change the default to return 0 (the safe option).

Reported-by Andy Lutomirski <[email protected]>

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

---
 arch/powerpc/perf/hv-24x7.c     |    6 ------
 arch/powerpc/perf/hv-gpci.c     |    6 ------
 arch/s390/kernel/perf_cpum_sf.c |    6 ------
 kernel/events/core.c            |   15 +--------------
 kernel/events/hw_breakpoint.c   |    7 -------
 5 files changed, 1 insertion(+), 39 deletions(-)

arch/powerpc/perf/hv-24x7.c +0 -6

@@ -417,11 +417,6 @@ static int h_24x7_event_add(struct perf_
        return 0;
 }
 
-static int h_24x7_event_idx(struct perf_event *event)
-{
-       return 0;
-}
-
 static struct pmu h_24x7_pmu = {
        .task_ctx_nr = perf_invalid_context,
 

@@ -433,7 +428,6 @@ static struct pmu h_24x7_pmu = {
        .start       = h_24x7_event_start,
        .stop        = h_24x7_event_stop,
        .read        = h_24x7_event_update,
-       .event_idx   = h_24x7_event_idx,
 };
 
 static int hv_24x7_init(void)

arch/powerpc/perf/hv-gpci.c +0 -6

@@ -246,11 +246,6 @@ static int h_gpci_event_init(struct perf
        return 0;
 }
 
-static int h_gpci_event_idx(struct perf_event *event)
-{
-       return 0;
-}
-
 static struct pmu h_gpci_pmu = {
        .task_ctx_nr = perf_invalid_context,
 

@@ -262,7 +257,6 @@ static struct pmu h_gpci_pmu = {
        .start       = h_gpci_event_start,
        .stop        = h_gpci_event_stop,
        .read        = h_gpci_event_update,
-       .event_idx   = h_gpci_event_idx,
 };
 
 static int hv_gpci_init(void)

arch/s390/kernel/perf_cpum_sf.c +0 -6

@@ -1411,11 +1411,6 @@ static void cpumsf_pmu_del(struct perf_e
        perf_pmu_enable(event->pmu);
 }
 
-static int cpumsf_pmu_event_idx(struct perf_event *event)
-{
-       return event->hw.idx;
-}
-
 CPUMF_EVENT_ATTR(SF, SF_CYCLES_BASIC, PERF_EVENT_CPUM_SF);
 CPUMF_EVENT_ATTR(SF, SF_CYCLES_BASIC_DIAG, PERF_EVENT_CPUM_SF_DIAG);
 

@@ -1458,7 +1453,6 @@ static struct pmu cpumf_sampling = {
        .stop         = cpumsf_pmu_stop,
        .read         = cpumsf_pmu_read,
 
-       .event_idx    = cpumsf_pmu_event_idx,
        .attr_groups  = cpumsf_pmu_attr_groups,
 };
 

kernel/events/core.c +1 -14

@@ -6071,11 +6071,6 @@ static int perf_swevent_init(struct perf
        return 0;
 }
 
-static int perf_swevent_event_idx(struct perf_event *event)
-{
-       return 0;
-}
-
 static struct pmu perf_swevent = {
        .task_ctx_nr    = perf_sw_context,
 

@@ -6085,8 +6080,6 @@ static struct pmu perf_swevent = {
        .start          = perf_swevent_start,
        .stop           = perf_swevent_stop,
        .read           = perf_swevent_read,
-
-       .event_idx      = perf_swevent_event_idx,
 };
 
 #ifdef CONFIG_EVENT_TRACING

@@ -6204,8 +6197,6 @@ static struct pmu perf_tracepoint = {
        .start          = perf_swevent_start,
        .stop           = perf_swevent_stop,
        .read           = perf_swevent_read,
-
-       .event_idx      = perf_swevent_event_idx,
 };
 
 static inline void perf_tp_register(void)

@@ -6431,8 +6422,6 @@ static struct pmu perf_cpu_clock = {
        .start          = cpu_clock_event_start,
        .stop           = cpu_clock_event_stop,
        .read           = cpu_clock_event_read,
-
-       .event_idx      = perf_swevent_event_idx,
 };
 
 /*

@@ -6511,8 +6500,6 @@ static struct pmu perf_task_clock = {
        .start          = task_clock_event_start,
        .stop           = task_clock_event_stop,
        .read           = task_clock_event_read,
-
-       .event_idx      = perf_swevent_event_idx,
 };
 
 static void perf_pmu_nop_void(struct pmu *pmu)

@@ -6542,7 +6529,7 @@ static void perf_pmu_cancel_txn(struct p
 
 static int perf_event_idx_default(struct perf_event *event)
 {
-       return event->hw.idx + 1;
+       return 0;
 }
 
 /*

kernel/events/hw_breakpoint.c +0 -7

@@ -605,11 +605,6 @@ static void hw_breakpoint_stop(struct pe
        bp->hw.state = PERF_HES_STOPPED;
 }
 
-static int hw_breakpoint_event_idx(struct perf_event *bp)
-{
-       return 0;
-}
-
 static struct pmu perf_breakpoint = {
        .task_ctx_nr    = perf_sw_context, /* could eventually get its own */
 

@@ -619,8 +614,6 @@ static struct pmu perf_breakpoint = {
        .start          = hw_breakpoint_start,
        .stop           = hw_breakpoint_stop,
        .read           = hw_breakpoint_pmu_read,
-
-       .event_idx      = hw_breakpoint_event_idx,
 };
 
 int __init init_hw_breakpoint(void)

This little series tightens up rdpmc permissions. With it applied, rdpmc can only be used if a perf_event is actually mmapped. For now, this is only really useful for seccomp.

Quoted 3 lines · Andy Lutomirski

This little series tightens up rdpmc permissions. With it applied, rdpmc can only be used if a perf_event is actually mmapped. For now, this is only really useful for seccomp.

So just to be difficult...

I am aware of at least one group who is doing low-latency performance measures using rdpmc on Linux.

They start the counters manually by poking the MSRs directly (bypassing perf_event_open()).

They use rdpmc, grateful for the fact that currently CR4 is set up so they can do this w/o patching the kernel.

These patches of course would break this use case...

ISTM it would be a lot better to use the perf subsystem for this. You can probably pin an event to a pmu.

Quoted 2 lines · Andy Lutomirski

ISTM it would be a lot better to use the perf subsystem for this. You can probably pin an event to a pmu.

No, you cannot pin an event to a counter with perf_event. That's one of the big differences between perf_event and, say, perfmon2.

With perf_event the kernel controls which events go in which counters and the user has no say. That's part of why you need to check the mmap page every time you want to use rdpmc because there's no other way of knowing which counter to read to get the event you want.

perf_event is also fairly high overhead for setting up and starting events,

and mildly high overhead when doing a proper rdpmc call (due to the required looking at mmap, and the fact that you need to do two rdpmc calls before/after to get your value). This is why people really worried about low-latency measurements bypass as much of perf_event as possible.

Quoted 2 lines

perf_event is also fairly high overhead for setting up and starting events,

Which you only do once at the start, so is that really a problem?

I still don't get that argument, 2 rdpmc's is cheaper than doing wrmsr, not to mention doing wrmsr through a syscall. And looking at that mmap page is 1 cacheline. Is that cacheline read (assuming you miss) the real problem?

Quoted 4 lines · Peter Zijlstra

Quoted 2 lines

perf_event is also fairly high overhead for setting up and starting events,

Which you only do once at the start, so is that really a problem?

There are various reasons why you might want to start events at times other than the beginning of the program. Some people don't like kernel multiplexing so they start/stop manually if they want to switch eventsets.

But no, I suppose you could ask anyone wanting to use rdpmc to open some sort of dummy event at startup just to get cr4 enabled.

Quoted 4 lines

I still don't get that argument, 2 rdpmc's is cheaper than doing wrmsr, not to mention doing wrmsr through a syscall. And looking at that mmap page is 1 cacheline. Is that cacheline read (assuming you miss) the real problem?

Well at least by default the first read of the mmap page causes a pagefault which adds a few thousand cycles of latency. Though you can somewhat get around this by prefaulting it in at some point.

Anyway I'm just reporting numbers I get when measuring the overhead of the old perfctr interface vs perf_event on typical PAPI workloads. It's true you can re-arrange calls and such so that perf_event behaves better but that involves redoing a lot of existing code.

I do appreciate the trouble you've gone through keeping self-monitoring working considering the fact that I'm the only user admitting to using it.

Quoted 3 lines · Vince Weaver

There are various reasons why you might want to start events at times other than the beginning of the program. Some people don't like kernel multiplexing so they start/stop manually if they want to switch eventsets.

I suppose you could pre-create all events and use ioctl()s to start/stop them where/when desired, this should be faster I think. But yes, this is not a use-case I've though much about.

MAP_POPULATE is your friend there, but yes manually prefaulting is perfectly fine too, and the HPC people are quite familiar with the concept, they do it for a lot of things.

Quoted 9 lines · Peter Zijlstra

Quoted 3 lines · Vince Weaver

There are various reasons why you might want to start events at times other than the beginning of the program. Some people don't like kernel multiplexing so they start/stop manually if they want to switch eventsets.

I suppose you could pre-create all events and use ioctl()s to start/stop them where/when desired, this should be faster I think. But yes, this is not a use-case I've though much about.

The scheduling step is most of what makes the perf_event start call have high overhead. The other annoyance is the fact that due to the NMI watchdog your can successfully perf_event_open() an event group but still have it fail at start time, so a lot of code has to be done that does extraneous open/start/close calls to make sure the events really fit.

Quoted 3 lines

MAP_POPULATE is your friend there, but yes manually prefaulting is perfectly fine too, and the HPC people are quite familiar with the concept, they do it for a lot of things.

MAP_POPULATE actually has noticably more overhead than manually prefaulting. It's on my todo list to drop ftrace on there and find out why, but I've been stuck chasing kernel-crashing fuzzer bugs instead in my spare time.

perfctr and possibly perfmon2 would automatically pre-fault the mmap page for you in the kernel, so there was no need for the user to do it.

In any case I wasn't really trying to make trouble here, it's just I came across the people using rdpmc w/o perf_event just the other day (on USENET of all places). They were so happy it worked w/o patches now, that I felt bad breaking it to them that there were patches floating around that were going to make their usecase not work anymore.

I guess like all things though, you can't have anything fun and useful in the kernel without the security people taking it away.

Vince