perf/x86: Fix USER/KERNEL tagging of samples
Subsystems (2)
Commit-ID: ce5c1fe9a9e059b5c58f0a7e2a3e687d0efac815 Gitweb: http://git.kernel.org/tip/ce5c1fe9a9e059b5c58f0a7e2a3e687d0efac815 Author: Peter Zijlstra <[email protected]> AuthorDate: Wed, 20 Jun 2012 11:11:38 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 5 Jul 2012 20:59:07 +0200
perf/x86: Fix USER/KERNEL tagging of samples
Several perf interrupt handlers (PEBS,IBS,BTS) re-write regs->ip but do not update the segment registers. So use an regs->ip based test instead of an regs->cs/regs->flags based test.
--- arch/x86/kernel/cpu/perf_event.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/cpu/perf_event.c +1 -1
@@ -1863,7 +1863,7 @@ unsigned long perf_misc_flags(struct pt_regs *regs)else misc |=PERF_RECORD_MISC_GUEST_KERNEL ; }else { -if (user_mode (regs)) +if (!kernel_ip (regs->ip)) misc |=PERF_RECORD_MISC_USER ;else misc |=PERF_RECORD_MISC_KERNEL ;
Quoted 4 lines · tip-bot for Peter Zijlstra
Several perf interrupt handlers (PEBS,IBS,BTS) re-write
regs->ipbut do not update the segment registers. So use anregs->ipbased test instead of an regs->cs/regs->flags based test.
Christ, people, YOU CANNOT DO THIS!
It is never *ever* valid to test the IP to see if you're in kernel space or user space. People can do various odd segments etc, the IP is totally meaningless.
If the perf handlers fake the IP information, they had better fake the CS/eflags information too. Because it is *wrong* to look at IP. Don't do it.
Linus
Quoted 9 lines · Linus Torvalds
Quoted 4 lines · tip-bot for Peter Zijlstra
Several perf interrupt handlers (PEBS,IBS,BTS) re-write
regs->ipbut do not update the segment registers. So use anregs->ipbased test instead of an regs->cs/regs->flags based test.Christ, people, YOU CANNOT DO THIS!
Quoted 7 lines
It is never *ever* valid to test the IP to see if you're in kernel space or user space. People can do various odd segments etc, the IP is totally meaningless.
If the perf handlers fake the IP information, they had better fake the CS/eflags information too. Because it is *wrong* to look at IP. Don't do it.
PEBS, BTS, LBR don't have CS. PEBS does have eflags.
If we cannot do this I'm not sure what we can do :/
Quoted 4 lines · Peter Zijlstra
PEBS, BTS, LBR don't have CS. PEBS does have eflags.
If we cannot do this I'm not sure what we can do :/
Well, you're passed in a "pt_regs". Which *does* have CS, and has it right.
If some code then changes the values in the pt_regs, it is *that* code that needs to think twice about what it does. Where is that code?
Linus
Quoted 5 lines · Linus Torvalds
If some code then changes the values in the
pt_regs, it is *that* code that needs to think twice about what it does. Where is that code?From a quick grep it looks like it is
__intel_pmu_pebs_event()that does this.
THAT is where you would possibly have a huge honking big comment about how you have to fake the CS register contents because the PEBS information is incomplete. But make it clear that it is a total hack.
Also, somebody should check. Is the PEBS information *actually* the instruction pointer (address within the code segment), or is it the "linear address" (segment base + rip)? I hope it is the latter, because in the absense of CS, the segment-based address is very unclear indeed.
And if it *is* the linear address, then at that point you could do
regs->cs = kernel_ip(ip) ? __KERNEL_CS : __USER_CS; regs->eflags &= ~X86_EFLAGS_VM;
and document this as a "we fake the CS and vm86 mode, using the known zero-based code segments". At that point it would be technically correct.
But any code that does "kernel_ip(regs->ip)" is just terminally confused and can never be sane.
Linus
Quoted 7 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
If some code then changes the values in the
pt_regs, it is *that* code that needs to think twice about what it does. Where is that code?From a quick grep it looks like it is
__intel_pmu_pebs_event()that does this.
Correct, as well as perf_ibs_handle_irq() and I'll have to have a good look at the BTS stuff.
Quoted 3 lines
THAT is where you would possibly have a huge honking big comment about how you have to fake the CS register contents because the PEBS information is incomplete. But make it clear that it is a total hack.
OK.
Quoted 5 lines
Also, somebody should check. Is the PEBS information *actually* the instruction pointer (address within the code segment), or is it the "linear address" (segment base + rip)? I hope it is the latter, because in the absense of CS, the segment-based address is very unclear indeed.
I _think_ it is just the RIP since its part of a more general register dump (which does not include the segment registers).
I can't find anything in the SDM clarifying it one way or the other. Stephane do you know?
Quoted 8 lines
And if it *is* the linear address, then at that point you could do
regs->cs = kernel_ip(ip) ? __KERNEL_CS : __USER_CS; regs->eflags &= ~X86_EFLAGS_VM;and document this as a "we fake the CS and vm86 mode, using the known zero-based code segments". At that point it would be technically correct.
OK, thanks.. At least the AMB IBS case provides the Linear address of the instruction so the IBS code should do it this way. We'll need to check with Intel to see what PEBS does.
Supposing the worst case and Intel simply provides the RIP as is, what should we do then?
Quoted 9 lines · Peter Zijlstra
Quoted 5 lines · Linus Torvalds
Also, somebody should check. Is the PEBS information *actually* the instruction pointer (address within the code segment), or is it the "linear address" (segment base + rip)? I hope it is the latter, because in the absense of CS, the segment-based address is very unclear indeed.
I _think_ it is just the RIP since its part of a more general register dump (which does not include the segment registers).
Ugh. That would be really sad if true (and I would not be at all surprised if it *is* true). Sure, flat segments are standard, and once you get into 64-bit mode I don't think you can even do any CS base, but afaik the code there is supposed to work on x86-32 too, no?
Quoted 2 lines
Supposing the worst case and Intel simply provides the RIP as is, what should we do then?
Well, if the RIP is all you have, the RIP is all you can use, and you have to just assume flat segments. That will be true in practice, and if it means that you can't sanely profile vm86 mode etc on x86-32, that's sad but you have to blame the hardware for that.
And I don't have any huge problems with assuming flat segments and just "work in the simple cases".
What I reacted negatively to is literally that
kernel_ip(regs->ip)
test, because once you have "regs", that's absolutely the wrong thing to do. That kind of test just shows that somebody filled in "regs" incorrectly. So if you have a pt_regs pointer, that kind of test is simply crap, and indicates a bug somewhere.
Linus
Quoted 2 lines · Linus Torvalds
But any code that does "
kernel_ip(regs->ip)" is just terminally confused and can never be sane.
How about something like the below?
I've also modified perf_instruction_pointer() to account for the VM86 and IA32 non-zero segment base cases. At least, I tried to do so, I've never had the 'pleasure' of poking at this segment descriptor stuff before.
Ingo didn't really like doing that though, his suggestion was to kill all those IPs by mapping them to a special value (~0UL or so).
---
Subject: perf/x86: Fix USER/KERNEL tagging of samples properly
Some PMUs don't provide a full register set for their sample, specifically 'advanced' PMUs like AMD IBS and Intel PEBS which provide 'better' then regular interrupt accuracy.
In this case we use the interrupt regs as basis and over-write some fields (typically IP) with different information.
The perf core however users user_mode() to distinguish user/kernel samples, user_mode() relies on regs->cs. If the interrupt skid pushed us over a boundary the new IP might not be in the same domain as the interrupt.
Commit ce5c1fe9a9e ("perf/x86: Fix USER/KERNEL tagging of samples") tried to fix this by making the perf core use kernel_ip(). This however is wrong (TM), as pointed out by Linus, since it doesn't allow for VM86 and non-zero based segments in IA32 mode.
Therefore, provide a new helper to set the regs->ip field, set_linear_ip(), which massages the regs into a suitable state assuming the provided IP is in fact a linear address.
Also modify perf_instruction_pointer() to deal with these 'fun' cases.
--- arch/x86/kernel/cpu/perf_event.c | 58 ++++++++++++++++++++++++++++--- arch/x86/kernel/cpu/perf_event.h | 19 ++++++++++ arch/x86/kernel/cpu/perf_event_amd_ibs.c | 4 ++- arch/x86/kernel/cpu/perf_event_intel_ds.c | 6 ++-- 4 files changed, 79 insertions(+), 8 deletions(-)
arch/x86/kernel/cpu/perf_event.c +54 -4
@@ -32,6 +32,8 @@#include <asm/smp.h> #include <asm/alternative.h> #include <asm/timer.h> +#include <asm/desc.h> +#include <asm/ldt.h> #include "perf_event.h"
@@ -1816,14 +1818,62 @@ perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs) } } +static unsigned long get_segment_base (unsigned int segment) +{ +struct desc_struct *desc; +int idx = segment >>3 ; + +if ((segment &SEGMENT_TI_MASK ) ==SEGMENT_LDT ) { +if (idx >LDT_ENTRIES ) +return 0 ; + + desc = current->active_mm->context.ldt; + }else { +if (idx >GDT_ENTRIES ) +return 0 ; + + desc =__this_cpu_ptr (&gdt_page.gdt[0 ]); + } + +return get_desc_base (desc + idx); +} + +static unsigned long code_segment_base (struct pt_regs *regs) +{ +#ifdef CONFIG_32BIT +if (user_mode (regs) && regs->cs != __USER_CS) +return get_segment_base (regs->cs); +#else +if (test_thread_flag (TIF_IA32 )) { +if (user_mode (regs) && regs->cs != __USER32_CS) +return get_segment_base (regs->cs); + } +#endif +return 0 ; +} +unsigned long perf_instruction_pointer (struct pt_regs *regs) {unsigned long ip;if (perf_guest_cbs && perf_guest_cbs->is_in_guest ()) - ip = perf_guest_cbs->get_guest_ip (); -else - ip =instruction_pointer (regs); +return perf_guest_cbs->get_guest_ip (); + + ip = regs->ip; + +if (regs->flags &X86_VM_MASK ) { +/* +* If we are in VM86 mode, add the segment offset to convert to +* a linear address. +*/ + ip +=0x10 * regs->cs; + }else { +/* +* For IA32 we look at the GDT/LDT segment base to convert the +* effective IP to a linear address. +*/ + ip +=code_segment_base (regs); + }return ip; }
@@ -1838,7 +1888,7 @@ unsigned long perf_misc_flags(struct pt_regs *regs)else misc |=PERF_RECORD_MISC_GUEST_KERNEL ; }else { -if (!kernel_ip (regs->ip)) +if (user_mode (regs)) misc |=PERF_RECORD_MISC_USER ;else misc |=PERF_RECORD_MISC_KERNEL ;
arch/x86/kernel/cpu/perf_event.h +19 -0
@@ -516,6 +516,25 @@ static inline bool kernel_ip(unsigned long ip)#endif } +/* +* Not all PMUs provide the right context information to place the reported IP +* into full context. Specifically segment registers are typically not +* supplied. +* +* Assuming the address is a linear address (it is for IBS), we fake the CS and +* vm86 mode using the known zero-based code segment and 'fix up' the registers +* to reflect this. +* +* Intel PEBS/LBR appear to typically provide the effective address, nothing +* much we can do about that but pray and treat it like a linear address. +*/ +static inline void set_linear_ip (struct pt_regs *regs,unsigned long ip) +{ + regs->cs =kernel_ip (ip) ? __KERNEL_CS : __USER_CS; + regs->flags &= ~X86_VM_MASK ; + regs->ip = ip; +} +#ifdef CONFIG_CPU_SUP_AMD int amd_pmu_init (void );
arch/x86/kernel/cpu/perf_event_amd_ibs.c +3 -1
@@ -13,6 +13,8 @@#include <asm/apic.h> +#include "perf_event.h" +static u32 ibs_caps;#if defined(CONFIG_PERF_EVENTS ) && defined(CONFIG_CPU_SUP_AMD )
@@ -536,7 +538,7 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)if (check_rip && (ibs_data.regs[2 ] &IBS_RIP_INVALID )) { regs.flags &= ~PERF_EFLAGS_EXACT ; }else { -instruction_pointer_set (®s, ibs_data.regs[1 ]); +set_linear_ip (®s, ibs_data.regs[1 ]); regs.flags |=PERF_EFLAGS_EXACT ; }
arch/x86/kernel/cpu/perf_event_intel_ds.c +3 -3
@@ -499,7 +499,7 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)* We sampled a branch insn, rewind using the LBR stack */ if (ip == to) { - regs->ip = from; +set_linear_ip (regs, from);return 1 ; }
@@ -529,7 +529,7 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs) }while (to < ip);if (to == ip) { - regs->ip = old_to; +set_linear_ip (regs, old_to);return 1 ; }
@@ -569,7 +569,7 @@ static void __intel_pmu_pebs_event(struct perf_event *event,* A possible PERF_SAMPLE_REGS will have to transfer all regs. */ regs = *iregs; - regs.ip = pebs->ip; +set_linear_ip (®s, pebs->ip); regs.bp = pebs->bp; regs.sp = pebs->sp;
Quoted 7 lines · Peter Zijlstra
How about something like the below?
I've also modified
perf_instruction_pointer()to account for the VM86 and IA32 non-zero segment base cases. At least, I tried to do so, I've never had the 'pleasure' of poking at this segment descriptor stuff before.
The code base calculations look correct to me per se, although I'd put the VM86 check into code_segment_base(). But whatever.
Quoted 2 lines
Ingo didn't really like doing that though, his suggestion was to kill all those IPs by mapping them to a special value (~0UL or so).
Hmm. I don't care much one way or another, but I don't really see the advantage of doing that.
The expensive part is the checking - since in all normal cases we'll be using the standard CS and not be in vm86 mode. And you have to do the checking whether you then map it to ~0ul or do a proper segment base calculation. So why not just do it right? Your code seems sane enough to me.
Now "regs" always contains a proper register state, and when the perf code wants a linear address, perf_instruction_pointer() seems to do the right translation. Looks good to me, although I didn't check all the uses.
However, it is worth pointing out that sp/bp have exactly the same segment base issue. So if you do stack tracing into user mode, you should really do the same thing for those. And quite frankly, at that point vm86 mode and the stack segment matters in other ways than just the base pointer: a 16-bit stack segment acts fundamentally differently from a 32-bit one. So at that point it may well make much more sense to take the approach Ingo suggests, and simply not follow stack frames at all.
Anyway, ACK for that patch from me. It may not solve all the problems, but it looks ok.
Linus
Quoted 8 lines · Linus Torvalds
However, it is worth pointing out that sp/bp have exactly the same segment base issue. So if you do stack tracing into user mode, you should really do the same thing for those. And quite frankly, at that point vm86 mode and the stack segment matters in other ways than just the base pointer: a 16-bit stack segment acts fundamentally differently from a 32-bit one. So at that point it may well make much more sense to take the approach Ingo suggests, and simply not follow stack frames at all.
Right, so I amended the patch to ignore vm86 stacks and added {cs,ss}_base magic to ia32 stacks.
Ingo, do you want me to do a version where I simply bail on everything if regs->{cs,ss} != {__USER_CS, __USER32_CS} || regs->flags & VM ?
---
Subject: perf/x86: Fix USER/KERNEL tagging of samples properly
From: Peter Zijlstra <[email protected]>
Date: Tue Jul 10 09:42:15 CEST 2012
Some PMUs don't provide a full register set for their sample, specifically 'advanced' PMUs like AMD IBS and Intel PEBS which provide 'better' than regular interrupt accuracy.
In this case we use the interrupt regs as basis and over-write some fields (typically IP) with different information.
The perf core however uses user_mode() to distinguish user/kernel samples, user_mode() relies on regs->cs. If the interrupt skid pushed us over a boundary the new IP might not be in the same domain as the interrupt.
Commit ce5c1fe9a9e ("perf/x86: Fix USER/KERNEL tagging of samples") tried to fix this by making the perf core use kernel_ip(). This however is wrong (TM), as pointed out by Linus, since it doesn't allow for VM86 and non-zero based segments in IA32 mode.
Therefore, provide a new helper to set the regs->ip field, set_linear_ip(), which massages the regs into a suitable state assuming the provided IP is in fact a linear address.
Also modify perf_instruction_pointer() and perf_callchain_user() to deal with segments base offsets.
--- arch/x86/include/asm/perf_event.h | 11 ++- arch/x86/kernel/cpu/perf_event.c | 89 ++++++++++++++++++++++++++---- arch/x86/kernel/cpu/perf_event.h | 20 ++++++ arch/x86/kernel/cpu/perf_event_amd_ibs.c | 4 + arch/x86/kernel/cpu/perf_event_intel_ds.c | 7 +- 5 files changed, 114 insertions(+), 17 deletions(-)
arch/x86/include/asm/perf_event.h +8 -3
@@ -196,11 +196,16 @@ static inline u32 get_ibs_caps(void) { rextern void perf_events_lapic_init (void );/* -* Abuse bit 3 of the cpu eflags register to indicate proper PEBS IP fixups. -* This flag is otherwise unused and ABI specified to be 0, so nobody should -* care what we do with it. +* Abuse bits {3,5} of the cpu eflags register. These flags are otherwise +* unused and ABI specified to be 0, so nobody should care what we do with +* them. +* +* EXACT - the IP points to the exact instruction that triggered the +* event (HW bugs exempt). +* NOUNWIND - do no unwind the stack. */ #define PERF_EFLAGS_EXACT (1UL << 3) +#define PERF_EFLAGS_NOUNWIND (1UL << 5)struct pt_regs ;extern unsigned long perf_instruction_pointer (struct pt_regs *regs);
arch/x86/kernel/cpu/perf_event.c +79 -10
141 lines not on this page · raw message
arch/x86/kernel/cpu/perf_event.h +20 -0
@@ -516,6 +516,26 @@ static inline bool kernel_ip(unsigned lo#endif } +/* +* Not all PMUs provide the right context information to place the reported IP +* into full context. Specifically segment registers are typically not +* supplied. +* +* Assuming the address is a linear address (it is for IBS), we fake the CS and +* vm86 mode using the known zero-based code segment and 'fix up' the registers +* to reflect this. +* +* Intel PEBS/LBR appear to typically provide the effective address, nothing +* much we can do about that but pray and treat it like a linear address. +*/ +static inline void set_linear_ip (struct pt_regs *regs,unsigned long ip) +{ + regs->cs =kernel_ip (ip) ? __KERNEL_CS : __USER_CS; +if (regs->flags &X86_VM_MASK ) + regs->flags ^= (PERF_EFLAGS_NOUNWIND |X86_VM_MASK ); + regs->ip = ip; +} +#ifdef CONFIG_CPU_SUP_AMD int amd_pmu_init (void );
arch/x86/kernel/cpu/perf_event_amd_ibs.c +3 -1
@@ -13,6 +13,8 @@#include <asm/apic.h> +#include "perf_event.h" +static u32 ibs_caps;#if defined(CONFIG_PERF_EVENTS ) && defined(CONFIG_CPU_SUP_AMD )
@@ -536,7 +538,7 @@ static int perf_ibs_handle_irq(struct peif (check_rip && (ibs_data.regs[2 ] &IBS_RIP_INVALID )) { regs.flags &= ~PERF_EFLAGS_EXACT ; }else { -instruction_pointer_set (®s, ibs_data.regs[1 ]); +set_linear_ip (®s, ibs_data.regs[1 ]); regs.flags |=PERF_EFLAGS_EXACT ; }
arch/x86/kernel/cpu/perf_event_intel_ds.c +4 -3
@@ -499,7 +499,7 @@ static int intel_pmu_pebs_fixup_ip(struc* We sampled a branch insn, rewind using the LBR stack */ if (ip == to) { - regs->ip = from; +set_linear_ip (regs, from);return 1 ; }
@@ -529,7 +529,7 @@ static int intel_pmu_pebs_fixup_ip(struc }while (to < ip);if (to == ip) { - regs->ip = old_to; +set_linear_ip (regs, old_to);return 1 ; }
@@ -569,7 +569,8 @@ static void __intel_pmu_pebs_event(struc* A possible PERF_SAMPLE_REGS will have to transfer all regs. */ regs = *iregs; - regs.ip = pebs->ip; + regs.flags = pebs->flags; +set_linear_ip (®s, pebs->ip); regs.bp = pebs->bp; regs.sp = pebs->sp;
Quoted 10 lines · Peter Zijlstra
Quoted 8 lines · Linus Torvalds
However, it is worth pointing out that sp/bp have exactly the same segment base issue. So if you do stack tracing into user mode, you should really do the same thing for those. And quite frankly, at that point vm86 mode and the stack segment matters in other ways than just the base pointer: a 16-bit stack segment acts fundamentally differently from a 32-bit one. So at that point it may well make much more sense to take the approach Ingo suggests, and simply not follow stack frames at all.
Quoted 6 lines
Right, so I amended the patch to ignore vm86 stacks and added {cs,ss}_base magic to ia32 stacks.
Ingo, do you want me to do a version where I simply bail on everything if regs->{cs,ss} != {
__USER_CS,__USER32_CS} ||regs->flags& VM ?
Only if it's really simple to do - out of morbid curiosity, to compare the two diffstats and such.
Thanks,
Ingo
Peter Zijlstra
maintainerQuoted 2 lines · Peter Zijlstra
Ingo, do you want me to do a version where I simply bail on everything if regs->{cs,ss} != {
__USER_CS,__USER32_CS} ||regs->flags& VM ?
Here's a variant that does that..
---
Subject: perf/x86: Fix USER/KERNEL tagging of samples properly
From: Peter Zijlstra <[email protected]>
Date: Tue Jul 10 09:42:15 CEST 2012
Some PMUs don't provide a full register set for their sample, specifically 'advanced' PMUs like AMD IBS and Intel PEBS which provide 'better' than regular interrupt accuracy.
In this case we use the interrupt regs as basis and over-write some fields (typically IP) with different information.
The perf core however uses user_mode() to distinguish user/kernel samples, user_mode() relies on regs->cs. If the interrupt skid pushed us over a boundary the new IP might not be in the same domain as the interrupt.
Commit ce5c1fe9a9e ("perf/x86: Fix USER/KERNEL tagging of samples") tried to fix this by making the perf core use kernel_ip(). This however is wrong (TM), as pointed out by Linus, since it doesn't allow for VM86 and non-zero based segments in IA32 mode.
Therefore, provide a new helper to set the regs->ip field, set_linear_ip(), which massages the regs into a suitable state assuming the provided IP is in fact a linear address.
Also modify perf_instruction_pointer() and perf_callchain_user() to deal with segments base offsets.
--- arch/x86/include/asm/perf_event.h | 11 +++++-- arch/x86/kernel/cpu/perf_event.c | 46 +++++++++++++++++++++++++----- arch/x86/kernel/cpu/perf_event.h | 20 +++++++++++++ arch/x86/kernel/cpu/perf_event_amd_ibs.c | 4 +- arch/x86/kernel/cpu/perf_event_intel_ds.c | 7 ++-- 5 files changed, 74 insertions(+), 14 deletions(-)
arch/x86/include/asm/perf_event.h +8 -3
@@ -196,11 +196,16 @@ static inline u32 get_ibs_caps(void) { rextern void perf_events_lapic_init (void );/* -* Abuse bit 3 of the cpu eflags register to indicate proper PEBS IP fixups. -* This flag is otherwise unused and ABI specified to be 0, so nobody should -* care what we do with it. +* Abuse bits {3,5} of the cpu eflags register. These flags are otherwise +* unused and ABI specified to be 0, so nobody should care what we do with +* them. +* +* EXACT - the IP points to the exact instruction that triggered the +* event (HW bugs exempt). +* VM - original X86_VM_MASK; see set_linear_ip(). */ #define PERF_EFLAGS_EXACT (1UL << 3) +#define PERF_EFLAGS_VM (1UL << 5)struct pt_regs ;extern unsigned long perf_instruction_pointer (struct pt_regs *regs);
arch/x86/kernel/cpu/perf_event.c +39 -7
80 lines not on this page · raw message
arch/x86/kernel/cpu/perf_event.h +20 -0
@@ -516,6 +516,26 @@ static inline bool kernel_ip(unsigned lo#endif } +/* +* Not all PMUs provide the right context information to place the reported IP +* into full context. Specifically segment registers are typically not +* supplied. +* +* Assuming the address is a linear address (it is for IBS), we fake the CS and +* vm86 mode using the known zero-based code segment and 'fix up' the registers +* to reflect this. +* +* Intel PEBS/LBR appear to typically provide the effective address, nothing +* much we can do about that but pray and treat it like a linear address. +*/ +static inline void set_linear_ip (struct pt_regs *regs,unsigned long ip) +{ + regs->cs =kernel_ip (ip) ? __KERNEL_CS : __USER_CS; +if (regs->flags &X86_VM_MASK ) + regs->flags ^= (PERF_EFLAGS_VM |X86_VM_MASK ); + regs->ip = ip; +} +#ifdef CONFIG_CPU_SUP_AMD int amd_pmu_init (void );
arch/x86/kernel/cpu/perf_event_amd_ibs.c +3 -1
@@ -13,6 +13,8 @@#include <asm/apic.h> +#include "perf_event.h" +static u32 ibs_caps;#if defined(CONFIG_PERF_EVENTS ) && defined(CONFIG_CPU_SUP_AMD )
@@ -536,7 +538,7 @@ static int perf_ibs_handle_irq(struct peif (check_rip && (ibs_data.regs[2 ] &IBS_RIP_INVALID )) { regs.flags &= ~PERF_EFLAGS_EXACT ; }else { -instruction_pointer_set (®s, ibs_data.regs[1 ]); +set_linear_ip (®s, ibs_data.regs[1 ]); regs.flags |=PERF_EFLAGS_EXACT ; }
arch/x86/kernel/cpu/perf_event_intel_ds.c +4 -3
@@ -499,7 +499,7 @@ static int intel_pmu_pebs_fixup_ip(struc* We sampled a branch insn, rewind using the LBR stack */ if (ip == to) { - regs->ip = from; +set_linear_ip (regs, from);return 1 ; }
@@ -529,7 +529,7 @@ static int intel_pmu_pebs_fixup_ip(struc }while (to < ip);if (to == ip) { - regs->ip = old_to; +set_linear_ip (regs, old_to);return 1 ; }
@@ -569,7 +569,8 @@ static void __intel_pmu_pebs_event(struc* A possible PERF_SAMPLE_REGS will have to transfer all regs. */ regs = *iregs; - regs.ip = pebs->ip; + regs.flags = pebs->flags; +set_linear_ip (®s, pebs->ip); regs.bp = pebs->bp; regs.sp = pebs->sp;
Peter Zijlstra
maintainerarch/x86/kernel/cpu/perf_event.c perf_callchain_user32(struct pt_regs *re · line 1761
Quoted 5 lines · Peter Zijlstra
arch/x86/kernel/cpu/perf_event.c +5
-0+if (regs->cs != __USER32_CS && regs->cs != __USER_CS) +return 0 ; + +if (regs->ss != __USER32_DS && regs->ss != __USER_DS) +return 0 ;
Those should be return 1, they are a 32bit callchain, we just don't do anything with it.
Quoted 4 lines · Peter Zijlstra
Quoted 2 lines · Peter Zijlstra
Ingo, do you want me to do a version where I simply bail on everything if regs->{cs,ss} != {
__USER_CS,__USER32_CS} ||regs->flags& VM ?
Quoted 8 lines
Here's a variant that does that..
arch/x86/include/asm/perf_event.h | 11 +++++-- arch/x86/kernel/cpu/perf_event.c | 46 +++++++++++++++++++++++++----- arch/x86/kernel/cpu/perf_event.h | 20 +++++++++++++ arch/x86/kernel/cpu/perf_event_amd_ibs.c | 4 +- arch/x86/kernel/cpu/perf_event_intel_ds.c | 7 ++-- 5 files changed, 74 insertions(+), 14 deletions(-)
this is the full thing:
Quoted 6 lines
arch/x86/include/asm/perf_event.h | 11 ++- arch/x86/kernel/cpu/perf_event.c | 89 ++++++++++++++++++++++++++---- arch/x86/kernel/cpu/perf_event.h | 20 ++++++ arch/x86/kernel/cpu/perf_event_amd_ibs.c | 4 + arch/x86/kernel/cpu/perf_event_intel_ds.c | 7 +- 5 files changed, 114 insertions(+), 17 deletions(-)
so that's 40 LOC difference.
Hm, I expected there to be more of a difference, so let me change my mind again in view of the evidence: now I tend to lean Linus's way, we might as well apply those extra 40 lines now that you've written them :-)
Even if it is not enough to do proper segmented profiling, should anyone be interested in such a profiling mode they'll have a much easier job making it work, the rest looks mostly a user space side job. Your larger patch looks safe enough at the boundaries.
Thanks,
Ingo
[tip:perf/urgent] perf/x86: Fix USER/ KERNEL tagging of samples properly
In reply to Peter ZijlstraCommit-ID: d07bdfd322d307789f15b427dbcc39257665356f Gitweb: http://git.kernel.org/tip/d07bdfd322d307789f15b427dbcc39257665356f Author: Peter Zijlstra <[email protected]> AuthorDate: Tue, 10 Jul 2012 09:42:15 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 31 Jul 2012 17:02:04 +0200
perf/x86: Fix USER/KERNEL tagging of samples properly
Some PMUs don't provide a full register set for their sample, specifically 'advanced' PMUs like AMD IBS and Intel PEBS which provide 'better' than regular interrupt accuracy.
In this case we use the interrupt regs as basis and over-write some fields (typically IP) with different information.
The perf core however uses user_mode() to distinguish user/kernel samples, user_mode() relies on regs->cs. If the interrupt skid pushed us over a boundary the new IP might not be in the same domain as the interrupt.
Commit ce5c1fe9a9e ("perf/x86: Fix USER/KERNEL tagging of samples") tried to fix this by making the perf core use kernel_ip(). This however is wrong (TM), as pointed out by Linus, since it doesn't allow for VM86 and non-zero based segments in IA32 mode.
Therefore, provide a new helper to set the regs->ip field, set_linear_ip(), which massages the regs into a suitable state assuming the provided IP is in fact a linear address.
Also modify perf_instruction_pointer() and perf_callchain_user() to deal with segments base offsets.
--- arch/x86/include/asm/perf_event.h | 11 +++- arch/x86/kernel/cpu/perf_event.c | 89 +++++++++++++++++++++++++--- arch/x86/kernel/cpu/perf_event.h | 20 +++++++ arch/x86/kernel/cpu/perf_event_amd_ibs.c | 4 +- arch/x86/kernel/cpu/perf_event_intel_ds.c | 7 +- 5 files changed, 114 insertions(+), 17 deletions(-)
arch/x86/include/asm/perf_event.h +8 -3
@@ -196,11 +196,16 @@ static inline u32 get_ibs_caps(void) { return 0; }extern void perf_events_lapic_init (void );/* -* Abuse bit 3 of the cpu eflags register to indicate proper PEBS IP fixups. -* This flag is otherwise unused and ABI specified to be 0, so nobody should -* care what we do with it. +* Abuse bits {3,5} of the cpu eflags register. These flags are otherwise +* unused and ABI specified to be 0, so nobody should care what we do with +* them. +* +* EXACT - the IP points to the exact instruction that triggered the +* event (HW bugs exempt). +* VM - original X86_VM_MASK; see set_linear_ip(). */ #define PERF_EFLAGS_EXACT (1UL << 3) +#define PERF_EFLAGS_VM (1UL << 5)struct pt_regs ;extern unsigned long perf_instruction_pointer (struct pt_regs *regs);
arch/x86/kernel/cpu/perf_event.c +79 -10
141 lines not on this page · raw message
arch/x86/kernel/cpu/perf_event.h +20 -0
@@ -516,6 +516,26 @@ static inline bool kernel_ip(unsigned long ip)#endif } +/* +* Not all PMUs provide the right context information to place the reported IP +* into full context. Specifically segment registers are typically not +* supplied. +* +* Assuming the address is a linear address (it is for IBS), we fake the CS and +* vm86 mode using the known zero-based code segment and 'fix up' the registers +* to reflect this. +* +* Intel PEBS/LBR appear to typically provide the effective address, nothing +* much we can do about that but pray and treat it like a linear address. +*/ +static inline void set_linear_ip (struct pt_regs *regs,unsigned long ip) +{ + regs->cs =kernel_ip (ip) ? __KERNEL_CS : __USER_CS; +if (regs->flags &X86_VM_MASK ) + regs->flags ^= (PERF_EFLAGS_VM |X86_VM_MASK ); + regs->ip = ip; +} +#ifdef CONFIG_CPU_SUP_AMD int amd_pmu_init (void );
arch/x86/kernel/cpu/perf_event_amd_ibs.c +3 -1
@@ -13,6 +13,8 @@#include <asm/apic.h> +#include "perf_event.h" +static u32 ibs_caps;#if defined(CONFIG_PERF_EVENTS ) && defined(CONFIG_CPU_SUP_AMD )
@@ -536,7 +538,7 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)if (check_rip && (ibs_data.regs[2 ] &IBS_RIP_INVALID )) { regs.flags &= ~PERF_EFLAGS_EXACT ; }else { -instruction_pointer_set (®s, ibs_data.regs[1 ]); +set_linear_ip (®s, ibs_data.regs[1 ]); regs.flags |=PERF_EFLAGS_EXACT ; }
arch/x86/kernel/cpu/perf_event_intel_ds.c +4 -3
@@ -499,7 +499,7 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)* We sampled a branch insn, rewind using the LBR stack */ if (ip == to) { - regs->ip = from; +set_linear_ip (regs, from);return 1 ; }
@@ -529,7 +529,7 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs) }while (to < ip);if (to == ip) { - regs->ip = old_to; +set_linear_ip (regs, old_to);return 1 ; }
@@ -569,7 +569,8 @@ static void __intel_pmu_pebs_event(struct perf_event *event,* A possible PERF_SAMPLE_REGS will have to transfer all regs. */ regs = *iregs; - regs.ip = pebs->ip; + regs.flags = pebs->flags; +set_linear_ip (®s, pebs->ip); regs.bp = pebs->bp; regs.sp = pebs->sp;
Quoted 7 lines · Peter Zijlstra
Quoted 2 lines · Linus Torvalds
But any code that does "
kernel_ip(regs->ip)" is just terminally confused and can never be sane.How about something like the below?
Quoted 8 lines
I've also modified
perf_instruction_pointer()to account for the VM86 and IA32 non-zero segment base cases. At least, I tried to do so, I've never had the 'pleasure' of poking at this segment descriptor stuff before.Ingo didn't really like doing that though, his suggestion was to kill all those IPs by mapping them to a special value (~0UL or so).
So, my main worry is that the complexity/actual_use ratio feels rather high. Very few (if any) people will explicitly test the profiling of segmented x86 code - and even if they sample, chances are that it's a Windows COFF/who-knows binary that we don't symbol-decode in user-space at the moment.
Open coded calculations like this are easy to get wrong:
Quoted 8 lines
arch/x86/kernel/cpu/perf_event.c +8
-0+static unsigned long get_segment_base (unsigned int segment) +{ +struct desc_struct *desc; +int idx = segment >>3 ; + +if ((segment &SEGMENT_TI_MASK ) ==SEGMENT_LDT ) { +if (idx >LDT_ENTRIES ) +return 0 ;
arch/x86/kernel/cpu/perf_event.c perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs) · line 1838
Quoted 10 lines
arch/x86/kernel/cpu/perf_event.c +10
-0+ + desc = current->active_mm->context.ldt; + }else { +if (idx >GDT_ENTRIES ) +return 0 ; + + desc =__this_cpu_ptr (&gdt_page.gdt[0 ]); + } + +return get_desc_base (desc + idx);
Shouldn't idx be checked against active_mm->context.ldt.size, not LDT_ENTRIES (which is really just an upper limit)?
arch/x86/kernel/cpu/perf_event.c perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs) · line 1853
Quoted 13 lines
arch/x86/kernel/cpu/perf_event.c +13
-0+static unsigned long code_segment_base (struct pt_regs *regs) +{ +#ifdef CONFIG_32BIT +if (user_mode (regs) && regs->cs != __USER_CS) +return get_segment_base (regs->cs); +#else +if (test_thread_flag (TIF_IA32 )) { +if (user_mode (regs) && regs->cs != __USER32_CS) +return get_segment_base (regs->cs); + } +#endif +return 0 ; +}
Will this do the right thing for x32 mode?
Quoted 8 lines
unsigned long
perf_instruction_pointer(struct pt_regs *regs){arch/x86/kernel/cpu/perf_event.c +0
-3unsigned long ip;if (perf_guest_cbs && perf_guest_cbs->is_in_guest ()) - ip = perf_guest_cbs->get_guest_ip (); -else - ip =instruction_pointer (regs);
arch/x86/kernel/cpu/perf_event.c perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs) · line 1869
Quoted 10 lines
arch/x86/kernel/cpu/perf_event.c +10
-0+return perf_guest_cbs->get_guest_ip (); + + ip = regs->ip; + +if (regs->flags &X86_VM_MASK ) { +/* +* If we are in VM86 mode, add the segment offset to convert to +* a linear address. +*/ + ip +=0x10 * regs->cs;
Sweet nostalgic memories ;-)
arch/x86/kernel/cpu/perf_event.c perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs) · line 1876
Quoted 7 lines
arch/x86/kernel/cpu/perf_event.c +7
-0+ }else { +/* +* For IA32 we look at the GDT/LDT segment base to convert the +* effective IP to a linear address. +*/ + ip +=code_segment_base (regs); + }
I'm also not entirely sure about skid across context switches and all that, the idx might not relate to the current LDT anymore - but I suspect we can ignore that problem.
( Another race is skid across descriptor updates - fortunately
sys_modify_ldt() is thick enough to be a practical barrier against that and we were never crazy enough to mmap() portions of the LDT to user-space or so. )
But no big fundamental objections from me, it would just be awfully nice to double check all the boundary conditions in this new code.
Thanks,
Ingo
Quoted 13 lines · Ingo Molnar
Quoted 13 lines
arch/x86/kernel/cpu/perf_event.c +13
-0+static unsigned long get_segment_base (unsigned int segment) +{ +struct desc_struct *desc; +int idx = segment >>3 ; + +if ((segment &SEGMENT_TI_MASK ) ==SEGMENT_LDT ) { +if (idx >LDT_ENTRIES ) +return 0 ; + + desc = current->active_mm->context.ldt; + }else { +if (idx >GDT_ENTRIES ) +return 0 ;
Quoted 8 lines
arch/x86/kernel/cpu/perf_event.c perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs) · line 1838
Quoted 5 lines
arch/x86/kernel/cpu/perf_event.c +5
-0+ + desc =__this_cpu_ptr (&gdt_page.gdt[0 ]); + } + +return get_desc_base (desc + idx);Shouldn't idx be checked against
active_mm->context.ldt.size, notLDT_ENTRIES(which is really just an upper limit)?
Ah indeed, fixed that.
Quoted 7 lines
Quoted 7 lines
arch/x86/kernel/cpu/perf_event.c +7
-0+static unsigned long code_segment_base (struct pt_regs *regs) +{ +#ifdef CONFIG_32BIT +if (user_mode (regs) && regs->cs != __USER_CS) +return get_segment_base (regs->cs); +#else +if (test_thread_flag (TIF_IA32 )) {
Quoted 8 lines
arch/x86/kernel/cpu/perf_event.c perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs) · line 1853
Quoted 6 lines
arch/x86/kernel/cpu/perf_event.c +6
-0+if (user_mode (regs) && regs->cs != __USER32_CS) + return get_segment_base(regs->cs); + } +#endif +return 0 ; +}Will this do the right thing for x32 mode?
hpa? It looks like x32 has TIF_X32, but from the kernel's POV its really just another 64bit process, so as long as we don't trigger the TIF_IA32 case we'll just return 0.
set_personality_ia32() looks like TIF_IA32 and TIF_X32 are mutually exclusive, so I think we do the right thing.
Quoted 16 lines · Peter Zijlstra
Quoted 7 lines
Quoted 7 lines
arch/x86/kernel/cpu/perf_event.c +7
-0+static unsigned long code_segment_base (struct pt_regs *regs) +{ +#ifdef CONFIG_32BIT +if (user_mode (regs) && regs->cs != __USER_CS) +return get_segment_base (regs->cs); +#else +if (test_thread_flag (TIF_IA32 )) {Quoted 8 lines
arch/x86/kernel/cpu/perf_event.c perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs) · line 1853
Quoted 6 lines
arch/x86/kernel/cpu/perf_event.c +6
-0+if (user_mode (regs) && regs->cs != __USER32_CS) + return get_segment_base(regs->cs); + } +#endif +return 0 ; +}Will this do the right thing for x32 mode?
Quoted 3 lines
hpa? It looks like x32 has
TIF_X32, but from the kernel's POV its really just another 64bit process, so as long as we don't trigger theTIF_IA32case we'll just return 0.
Maybe add that as a comment or so, to stop future wondering. Maybe list all the states an x86 process can be in, and describe what we intend to do for each. That will make it easier to match up intent with actual code and will make it easier to keep it correct in the future as well.
Quoted 2 lines
set_personality_ia32()looks likeTIF_IA32andTIF_X32are mutually exclusive, so I think we do the right thing.
Ok.
Thanks,
Ingo
Quoted 23 lines · Peter Zijlstra
Quoted 13 lines · Ingo Molnar
Quoted 13 lines
arch/x86/kernel/cpu/perf_event.c +13
-0+static unsigned long get_segment_base (unsigned int segment) +{ +struct desc_struct *desc; +int idx = segment >>3 ; + +if ((segment &SEGMENT_TI_MASK ) ==SEGMENT_LDT ) { +if (idx >LDT_ENTRIES ) +return 0 ; + + desc = current->active_mm->context.ldt; + }else { +if (idx >GDT_ENTRIES ) +return 0 ;Quoted 8 lines
arch/x86/kernel/cpu/perf_event.c perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs) · line 1838
Quoted 5 lines
arch/x86/kernel/cpu/perf_event.c +5
-0+ + desc =__this_cpu_ptr (&gdt_page.gdt[0 ]); + } + +return get_desc_base (desc + idx);Shouldn't idx be checked against
active_mm->context.ldt.size, notLDT_ENTRIES(which is really just an upper limit)?
Quoted 1 line
Ah indeed, fixed that.
Another boundary condition would be when we intentionally twiddle the GDT: such as during suspend or during BIOS upcalls. Can we then get a PMU interrupt? If yes then this will probably result in garbage:
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ desc =
__this_cpu_ptr(&gdt_page.gdt[0]);
it won't outright crash, we don't ever deallocate our GDT - but it will return a garbage RIP.
Then there's also all the Xen craziness with segments ...
Both ought to be rare an uninteresting - but then again, segmented execution is already rare and uninteresting to begin with.
So, instead of trying to discover all these weird x86 cases - with little to no testing done after that - I thought that it might be more future proof to just handle the cases we are explicitly interested in: flat code, and pounce in some well defined way in all the other situations by returning the RIP to an empty __X86_LEGACY_SEGMENTED_CODE() symbol.
That way we will at least give *some* useful information to the poor segmented code user, if the profile says:
21.32% [kernel] [k] __X86_LEGACY_SEGMENTED_CODE
11.01% [kernel] [k] kallsyms_expand_symbol
8.29% [kernel] [k] vsnprintf
7.37% libc-2.15.so [.] __strcmp_sse42
6.93% perf [.] symbol_filter
4.20% perf [.] kallsyms__parse
3.92% [kernel] [k] format_decode
3.62% [kernel] [k] string.isra.4
3.59% [kernel] [k] memcpy
3.11% [kernel] [k] strnlen then the user at least knows that there's 21% of overhead in some sort of segmented x86 code. Or if they *really* want to resolve that, they can take your patch and add symbol decoding to user-space and test it all.
KISS and such.
Linus?
Thanks,
Ingo
Quoted 9 lines · Ingo Molnar
Another boundary condition would be when we intentionally twiddle the GDT: such as during suspend or during BIOS upcalls. Can we then get a PMU interrupt? If yes then this will probably result in garbage:
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ desc =
__this_cpu_ptr(&gdt_page.gdt[0]);it won't outright crash, we don't ever deallocate our GDT - but it will return a garbage RIP.
Nothing we can do about that though..
Quoted 1 line
Then there's also all the Xen craziness with segments ...
I don't think Xen Dom0 has PMU access, that would be their Hyper-visor thingy's job, no?
Anyway, that seems a problem for Jeremy and Konrad..
Quoted 6 lines · Peter Zijlstra
Quoted 5 lines · Ingo Molnar
Another boundary condition would be when we intentionally twiddle the GDT: such as during suspend or during BIOS upcalls. Can we then get a PMU interrupt? If yes then this will probably result in garbage:
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ desc =
__this_cpu_ptr(&gdt_page.gdt[0]);it won't outright crash, we don't ever deallocate our GDT - but it will return a garbage RIP.
Nothing we can do about that though..
We could read out the current GDT [the SGDT instruction] instead of looking at gdt_page.
Then we'd have to decode that descriptor, the limit. Decide whether the selector points to the GDT or LDT. All the fun x86 legacies that we mostly forgot already after two decades of running the kernel in flat linear mode...
Quoted 4 lines
Quoted 1 line
Then there's also all the Xen craziness with segments ...
I don't think Xen Dom0 has PMU access, that would be their Hyper-visor thingy's job, no?
Unless Xen is destined to become dead code it will eventually be interested in PMU based measurements, right?
Quoted 1 line
Anyway, that seems a problem for Jeremy and Konrad..
Yeah, quite probably so.
Thanks,
Ingo
H. Peter Anvin
reviewerQuoted 9 lines · Ingo Molnar
We could read out the current GDT [the SGDT instruction] instead of looking at
gdt_page.Then we'd have to decode that descriptor, the limit. Decide whether the selector points to the GDT or LDT. All the fun x86 legacies that we mostly forgot already after two decades of running the kernel in flat linear mode...
In 32-bit mode there are actually instructions that do that work for you.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.