RE: [patch][v2] x86, ptrace: support for branch trace store(BTS)
>Your patch seems to be still word wrapped. I hope this is better with the next version I'm going to send out in a few minutes. Sorry about that. >The noflags variant should be probably data driven too. I rewrote the entire code to use an offset/size configuration instead of declaring structs for the various architecture variants. > > >> >> +case PTRACE_BTS_ALLOCATE_BUFFER: >> +ret = ptrace_bts_allocate_bts(child, data); >> +break; >> + >> +case PTRACE_BTS_GET_BUFFER_SIZE: >> +ret = ptrace_bts_get_buffer_size(child); >> +break; >> + >> +case PTRACE_BTS_GET_INDEX: >> +ret = ptrace_bts_get_index(child); >> +break; >> + >> +case PTRACE_BTS_READ_RECORD: >> +ret = ptrace_bts_read_record >> +(child, data, >> + (struct ptrace_bts_record __user *) addr); >> +break; >> + >> +case PTRACE_BTS_CONFIG: >> +ptrace_bts_config(child, data); >> +ret = 0; >> +break; >> + >> +case PTRACE_BTS_STATUS: >> +ret = ptrace_bts_status(child); >> +break; >> + > >Regarding your interface (can you please write those manpages to get a >full rationable)? They will be part of the next version. I keep the two patches separate, I hope that does not confuse any scripts that try to extract patches from the email. >I'm not sure it's a good idea to have a READ_RECORD -- better would >be likely a batched interface. Probably it would >be cleaner to combine get_index and read_record into a higher >level interface that works like normal read() -- gives you data >not read before for multiple records. I tried to keep the interface as simple and flexible as possible. A user would typically want to read the trace from back to front until he read enough trace. But I could also think of a more random accesses pattern. If you know you're going to read the entire buffer, reading it from front to back might be preferrable. The memory interface uses peek and poke to read and write a single word, respectively. I think that the READ_RECORD command matches the PEEK command pretty well. I would expect a user to provide a stream view to higher levels if it is beneficial for him. Such a view can be easily implemented with the current interface. >Also not sure why you have separate config and allocate_buffer steps. >They could be easily combined, couldn't they? The buffer is typically allocated once per session, whereas a user may want to turn on and off tracing several times during a session. >> +/* >> + * Maximal BTS size in number of records >> + */ >> +#define PTRACE_BTS_MAX_BTS_SIZE 4000 > >This should be likely some sort of sysctl. Or perhaps just use >user supplied >buffers limited by the mlock ulimit (that would allow zero copy). Ok >it means the high level read interface proposed above wouldn't work. >Perhaps the high level interface is better, although zero copy would >be more efficient. Not 100% sure what is better. I would not expect many users to want to change that value. I would make this go into the /usr/include/sys/ptrace.h header file, so the ptrace user is aware of the limit. If it turns out that there is a need to make this more flexible, we can always do it with a later patch. >> +rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl_msr); >> +debugctl_msr |= ptrace_bts_cfg.debugctrl_mask; >> +wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl_msr); > >I still think you should cache the DEBUGCTLMSR. If you worry >about other people changing it provide a general accessor. I cached the values. The MSR is read during initialization and the enabled and disabled variants are stored in the processor configuration. grep did not find another use of MSR_IA32_DEBUGCTLMSR anywhere in the kernel. thanks and regards, markus. - Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen Germany Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer Registergericht: Muenchen HRB 47456 Ust.-IdNr. VAT Registration No.: DE129385895 Citibank Frankfurt (BLZ 502 109 00) 600119052 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. - 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/
RE: [patch][v2] x86, ptrace: support for branch trace store(BTS)
Your patch seems to be still word wrapped. I hope this is better with the next version I'm going to send out in a few minutes. Sorry about that. The noflags variant should be probably data driven too. I rewrote the entire code to use an offset/size configuration instead of declaring structs for the various architecture variants. +case PTRACE_BTS_ALLOCATE_BUFFER: +ret = ptrace_bts_allocate_bts(child, data); +break; + +case PTRACE_BTS_GET_BUFFER_SIZE: +ret = ptrace_bts_get_buffer_size(child); +break; + +case PTRACE_BTS_GET_INDEX: +ret = ptrace_bts_get_index(child); +break; + +case PTRACE_BTS_READ_RECORD: +ret = ptrace_bts_read_record +(child, data, + (struct ptrace_bts_record __user *) addr); +break; + +case PTRACE_BTS_CONFIG: +ptrace_bts_config(child, data); +ret = 0; +break; + +case PTRACE_BTS_STATUS: +ret = ptrace_bts_status(child); +break; + Regarding your interface (can you please write those manpages to get a full rationable)? They will be part of the next version. I keep the two patches separate, I hope that does not confuse any scripts that try to extract patches from the email. I'm not sure it's a good idea to have a READ_RECORD -- better would be likely a batched interface. Probably it would be cleaner to combine get_index and read_record into a higher level interface that works like normal read() -- gives you data not read before for multiple records. I tried to keep the interface as simple and flexible as possible. A user would typically want to read the trace from back to front until he read enough trace. But I could also think of a more random accesses pattern. If you know you're going to read the entire buffer, reading it from front to back might be preferrable. The memory interface uses peek and poke to read and write a single word, respectively. I think that the READ_RECORD command matches the PEEK command pretty well. I would expect a user to provide a stream view to higher levels if it is beneficial for him. Such a view can be easily implemented with the current interface. Also not sure why you have separate config and allocate_buffer steps. They could be easily combined, couldn't they? The buffer is typically allocated once per session, whereas a user may want to turn on and off tracing several times during a session. +/* + * Maximal BTS size in number of records + */ +#define PTRACE_BTS_MAX_BTS_SIZE 4000 This should be likely some sort of sysctl. Or perhaps just use user supplied buffers limited by the mlock ulimit (that would allow zero copy). Ok it means the high level read interface proposed above wouldn't work. Perhaps the high level interface is better, although zero copy would be more efficient. Not 100% sure what is better. I would not expect many users to want to change that value. I would make this go into the /usr/include/sys/ptrace.h header file, so the ptrace user is aware of the limit. If it turns out that there is a need to make this more flexible, we can always do it with a later patch. +rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl_msr); +debugctl_msr |= ptrace_bts_cfg.debugctrl_mask; +wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl_msr); I still think you should cache the DEBUGCTLMSR. If you worry about other people changing it provide a general accessor. I cached the values. The MSR is read during initialization and the enabled and disabled variants are stored in the processor configuration. grep did not find another use of MSR_IA32_DEBUGCTLMSR anywhere in the kernel. thanks and regards, markus. - Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen Germany Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer Registergericht: Muenchen HRB 47456 Ust.-IdNr. VAT Registration No.: DE129385895 Citibank Frankfurt (BLZ 502 109 00) 600119052 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. - 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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
On Wednesday 21 November 2007 12:02:38 Metzger, Markus T wrote:
Your patch seems to be still word wrapped.
>
> It seems we're avoiding to declare a structured data type and instead
> prefer to describe the type indirectly.
> We gain the flexibility to work with different data layouts.
> We loose language support for working with structured types.
There is nothing that guarantees you that your structured types
match what the hardware generates anyways. So it doesn't make much difference --
your type system is already holey.
> What we would really want here are templates.
No, that would lead to bloated code again.
> I think that the absence of an explicit type declaration makes the code
> harder to understand and makes it easier to get it wrong when adding a
> new processor.
There are not that many new processors so that shouldn't be a huge issue.
> The code is not robust wrt small mistakes in the data layout spec. It
> might check that the field we're writing to is at least as big as the
> value we're writing; the same for reads. We would end up replacing
> static
> type checking with dynamic type checking.
Neither is the current code -- you could as well get the types wrong
(unless you auto generate them from RTL or something)
> I would prefer to work with explicit type declarations, even if this
> means a small increase in code size. It looks like the hardware settled
> on 64bit pointers everywhere, so I would not expect much more variation
> in DS. BTS has an unused field, which might go away some day.
>
> What do you think?
The i386 ifdefs should really go away, except around the 32bit record
structure definitions.
The noflags variant should be probably data driven too.
>
> + case PTRACE_BTS_ALLOCATE_BUFFER:
> + ret = ptrace_bts_allocate_bts(child, data);
> + break;
> +
> + case PTRACE_BTS_GET_BUFFER_SIZE:
> + ret = ptrace_bts_get_buffer_size(child);
> + break;
> +
> + case PTRACE_BTS_GET_INDEX:
> + ret = ptrace_bts_get_index(child);
> + break;
> +
> + case PTRACE_BTS_READ_RECORD:
> + ret = ptrace_bts_read_record
> + (child, data,
> + (struct ptrace_bts_record __user *) addr);
> + break;
> +
> + case PTRACE_BTS_CONFIG:
> + ptrace_bts_config(child, data);
> + ret = 0;
> + break;
> +
> + case PTRACE_BTS_STATUS:
> + ret = ptrace_bts_status(child);
> + break;
> +
Regarding your interface (can you please write those manpages to get a
full rationable)?
I'm not sure it's a good idea to have a READ_RECORD -- better would
be likely a batched interface. Probably it would
be cleaner to combine get_index and read_record into a higher
level interface that works like normal read() -- gives you data
not read before for multiple records.
Also not sure why you have separate config and allocate_buffer steps.
They could be easily combined, couldn't they?
> +/*
> + * Maximal BTS size in number of records
> + */
> +#define PTRACE_BTS_MAX_BTS_SIZE 4000
This should be likely some sort of sysctl. Or perhaps just use user supplied
buffers limited by the mlock ulimit (that would allow zero copy). Ok
it means the high level read interface proposed above wouldn't work.
Perhaps the high level interface is better, although zero copy would
be more efficient. Not 100% sure what is better.
> +
> +
> +struct ptrace_bts_configuration ptrace_bts_cfg;
> +
> +/*
> + * Configure trace hardware to enable tracing
> + * Return 0, on success; -Eerrno, otherwise.
> + */
> +static int ptrace_bts_enable(void)
> +{
> + unsigned long long debugctl_msr = 0;
> +
> + if (!ptrace_bts_cfg.debugctrl_mask)
> + return -EOPNOTSUPP;
> +
> + rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl_msr);
> + debugctl_msr |= ptrace_bts_cfg.debugctrl_mask;
> + wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl_msr);
I still think you should cache the DEBUGCTLMSR. If you worry
about other people changing it provide a general accessor.
-Andi
-
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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
Hi, > >and it seems like this patch and perfmon2 are going to have to > >live with > >each other... since they both require the use of the DS save area... > > Hmmm, this might require some synchronization between those two. > > Do you know how (accesses to) MSR's are managed by the kernel? There is a simple MSR allocator in the nmi watchdog code. It is very simple though and was only intended for performance counters originally so you might need to enhance it first for complicated things. I agree it needs to be extended to manage other not necessarily contiguous MSR registers. As for BTS, I am happy to see this resource exposed for debugging purposes. Note that it could also be used for performance monitoring purposes and it could be exploited by the perfmon2 subsystem via a new sampling format. This way one could for instance figure out the path that led to a cache miss. Of course, this requires that some filtering be applied to BTS which today does not differentiate loop vs. function branches, AFAIR. The current cost can be mitigated by using a long sampling period and by monitoring longer. -- -Stephane - 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/
RE: [patch][v2] x86, ptrace: support for branch trace store(BTS)
>> - the internal buffer interpretation as well as the corresponding
>> operations are selected at run-time by hardware detection
>> - different processors use different branch record formats
>
>I still think it would be far better if you would switch this
>over to be table
>driven. e.g. define a record that contains offsetof()/sizeof() of the
>different formats and use generic functions. That would decrease
>code size considerably.
Here's how this may look like.
I rewrote it for the DS access. Since DS is homogenous and only varies
in the address size, I made the address size a field in the
configuration
struct and do the address computation in macros.
I could alternatively encode the offset and size for every field in the
configuration struct instead of computing it. I think that is what you
originally suggested.
The BTS record format is inhomogenous and thus would require storing the
offset and size of every field in the configuration struct. I did not
do those changes, yet, since I would first like to get feedback.
Regarding code size, the original version requires ~500 bytes for a new
BTS architecture (for x86_64, measured by counting sizes obtained by
readelf); this could be reduced to ~100 bytes.
It seems we're avoiding to declare a structured data type and instead
prefer to describe the type indirectly.
We gain the flexibility to work with different data layouts.
We loose language support for working with structured types.
What we would really want here are templates.
I think that the absence of an explicit type declaration makes the code
harder to understand and makes it easier to get it wrong when adding a
new processor.
The code is not robust wrt small mistakes in the data layout spec. It
might check that the field we're writing to is at least as big as the
value we're writing; the same for reads. We would end up replacing
static
type checking with dynamic type checking.
This might not be strictly necessary, but I would argue that it's easier
to get a type declaration correct than to get the offsetof/sizeof pairs
correct for each field of the type.
I would prefer to work with explicit type declarations, even if this
means a small increase in code size. It looks like the hardware settled
on 64bit pointers everywhere, so I would not expect much more variation
in DS. BTS has an unused field, which might go away some day.
What do you think?
Index: linux-2.6/arch/x86/kernel/process_32.c
===
--- linux-2.6.orig/arch/x86/kernel/process_32.c 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/arch/x86/kernel/process_32.c 2007-11-19 15:49:53.%N
+0100
@@ -623,6 +623,19 @@
}
#endif
+ /*
+* Last branch recording recofiguration of trace hardware and
+* disentangling of trace data per task.
+*/
+ if (test_tsk_thread_flag(prev_p, TIF_BTS_TRACE) ||
+ test_tsk_thread_flag(prev_p, TIF_BTS_TRACE_TS))
+ ptrace_bts_task_departs(prev_p);
+
+ if (test_tsk_thread_flag(next_p, TIF_BTS_TRACE) ||
+ test_tsk_thread_flag(next_p, TIF_BTS_TRACE_TS))
+ ptrace_bts_task_arrives(next_p);
+
+
if (!test_tsk_thread_flag(next_p, TIF_IO_BITMAP)) {
/*
* Disable the bitmap via an invalid offset. We still
cache
Index: linux-2.6/arch/x86/kernel/ptrace_32.c
===
--- linux-2.6.orig/arch/x86/kernel/ptrace_32.c 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/arch/x86/kernel/ptrace_32.c 2007-11-19 13:11:46.%N
+0100
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
/*
* does not yet catch signals sent when the child dies.
@@ -274,6 +275,7 @@
{
clear_singlestep(child);
clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
+ ptrace_bts_task_detached(child);
}
/*
@@ -610,6 +612,33 @@
(struct user_desc __user *)
data);
break;
+ case PTRACE_BTS_ALLOCATE_BUFFER:
+ ret = ptrace_bts_allocate_bts(child, data);
+ break;
+
+ case PTRACE_BTS_GET_BUFFER_SIZE:
+ ret = ptrace_bts_get_buffer_size(child);
+ break;
+
+ case PTRACE_BTS_GET_INDEX:
+ ret = ptrace_bts_get_index(child);
+ break;
+
+ case PTRACE_BTS_READ_RECORD:
+ ret = ptrace_bts_read_record
+ (child, data,
+(struct ptrace_bts_record __user *) addr);
+ break;
+
+ case PTRACE_BTS_CONFIG:
+ ptrace_bts_config(child, data);
+ ret = 0;
+ break;
+
+ case PTRACE_BTS_STATUS:
+ ret = ptrace_bts_status(child);
+ break;
+
default:
ret = ptrace_request(child, request, addr, data);
break;
Index:
RE: [patch][v2] x86, ptrace: support for branch trace store(BTS)
- the internal buffer interpretation as well as the corresponding
operations are selected at run-time by hardware detection
- different processors use different branch record formats
I still think it would be far better if you would switch this
over to be table
driven. e.g. define a record that contains offsetof()/sizeof() of the
different formats and use generic functions. That would decrease
code size considerably.
Here's how this may look like.
I rewrote it for the DS access. Since DS is homogenous and only varies
in the address size, I made the address size a field in the
configuration
struct and do the address computation in macros.
I could alternatively encode the offset and size for every field in the
configuration struct instead of computing it. I think that is what you
originally suggested.
The BTS record format is inhomogenous and thus would require storing the
offset and size of every field in the configuration struct. I did not
do those changes, yet, since I would first like to get feedback.
Regarding code size, the original version requires ~500 bytes for a new
BTS architecture (for x86_64, measured by counting sizes obtained by
readelf); this could be reduced to ~100 bytes.
It seems we're avoiding to declare a structured data type and instead
prefer to describe the type indirectly.
We gain the flexibility to work with different data layouts.
We loose language support for working with structured types.
What we would really want here are templates.
I think that the absence of an explicit type declaration makes the code
harder to understand and makes it easier to get it wrong when adding a
new processor.
The code is not robust wrt small mistakes in the data layout spec. It
might check that the field we're writing to is at least as big as the
value we're writing; the same for reads. We would end up replacing
static
type checking with dynamic type checking.
This might not be strictly necessary, but I would argue that it's easier
to get a type declaration correct than to get the offsetof/sizeof pairs
correct for each field of the type.
I would prefer to work with explicit type declarations, even if this
means a small increase in code size. It looks like the hardware settled
on 64bit pointers everywhere, so I would not expect much more variation
in DS. BTS has an unused field, which might go away some day.
What do you think?
Index: linux-2.6/arch/x86/kernel/process_32.c
===
--- linux-2.6.orig/arch/x86/kernel/process_32.c 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/arch/x86/kernel/process_32.c 2007-11-19 15:49:53.%N
+0100
@@ -623,6 +623,19 @@
}
#endif
+ /*
+* Last branch recording recofiguration of trace hardware and
+* disentangling of trace data per task.
+*/
+ if (test_tsk_thread_flag(prev_p, TIF_BTS_TRACE) ||
+ test_tsk_thread_flag(prev_p, TIF_BTS_TRACE_TS))
+ ptrace_bts_task_departs(prev_p);
+
+ if (test_tsk_thread_flag(next_p, TIF_BTS_TRACE) ||
+ test_tsk_thread_flag(next_p, TIF_BTS_TRACE_TS))
+ ptrace_bts_task_arrives(next_p);
+
+
if (!test_tsk_thread_flag(next_p, TIF_IO_BITMAP)) {
/*
* Disable the bitmap via an invalid offset. We still
cache
Index: linux-2.6/arch/x86/kernel/ptrace_32.c
===
--- linux-2.6.orig/arch/x86/kernel/ptrace_32.c 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/arch/x86/kernel/ptrace_32.c 2007-11-19 13:11:46.%N
+0100
@@ -24,6 +24,7 @@
#include asm/debugreg.h
#include asm/ldt.h
#include asm/desc.h
+#include asm/ptrace_bts.h
/*
* does not yet catch signals sent when the child dies.
@@ -274,6 +275,7 @@
{
clear_singlestep(child);
clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
+ ptrace_bts_task_detached(child);
}
/*
@@ -610,6 +612,33 @@
(struct user_desc __user *)
data);
break;
+ case PTRACE_BTS_ALLOCATE_BUFFER:
+ ret = ptrace_bts_allocate_bts(child, data);
+ break;
+
+ case PTRACE_BTS_GET_BUFFER_SIZE:
+ ret = ptrace_bts_get_buffer_size(child);
+ break;
+
+ case PTRACE_BTS_GET_INDEX:
+ ret = ptrace_bts_get_index(child);
+ break;
+
+ case PTRACE_BTS_READ_RECORD:
+ ret = ptrace_bts_read_record
+ (child, data,
+(struct ptrace_bts_record __user *) addr);
+ break;
+
+ case PTRACE_BTS_CONFIG:
+ ptrace_bts_config(child, data);
+ ret = 0;
+ break;
+
+ case PTRACE_BTS_STATUS:
+ ret = ptrace_bts_status(child);
+ break;
+
default:
ret = ptrace_request(child, request, addr, data);
break;
Index:
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
Hi, and it seems like this patch and perfmon2 are going to have to live with each other... since they both require the use of the DS save area... Hmmm, this might require some synchronization between those two. Do you know how (accesses to) MSR's are managed by the kernel? There is a simple MSR allocator in the nmi watchdog code. It is very simple though and was only intended for performance counters originally so you might need to enhance it first for complicated things. I agree it needs to be extended to manage other not necessarily contiguous MSR registers. As for BTS, I am happy to see this resource exposed for debugging purposes. Note that it could also be used for performance monitoring purposes and it could be exploited by the perfmon2 subsystem via a new sampling format. This way one could for instance figure out the path that led to a cache miss. Of course, this requires that some filtering be applied to BTS which today does not differentiate loop vs. function branches, AFAIR. The current cost can be mitigated by using a long sampling period and by monitoring longer. -- -Stephane - 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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
On Wednesday 21 November 2007 12:02:38 Metzger, Markus T wrote:
Your patch seems to be still word wrapped.
It seems we're avoiding to declare a structured data type and instead
prefer to describe the type indirectly.
We gain the flexibility to work with different data layouts.
We loose language support for working with structured types.
There is nothing that guarantees you that your structured types
match what the hardware generates anyways. So it doesn't make much difference --
your type system is already holey.
What we would really want here are templates.
No, that would lead to bloated code again.
I think that the absence of an explicit type declaration makes the code
harder to understand and makes it easier to get it wrong when adding a
new processor.
There are not that many new processors so that shouldn't be a huge issue.
The code is not robust wrt small mistakes in the data layout spec. It
might check that the field we're writing to is at least as big as the
value we're writing; the same for reads. We would end up replacing
static
type checking with dynamic type checking.
Neither is the current code -- you could as well get the types wrong
(unless you auto generate them from RTL or something)
I would prefer to work with explicit type declarations, even if this
means a small increase in code size. It looks like the hardware settled
on 64bit pointers everywhere, so I would not expect much more variation
in DS. BTS has an unused field, which might go away some day.
What do you think?
The i386 ifdefs should really go away, except around the 32bit record
structure definitions.
The noflags variant should be probably data driven too.
+ case PTRACE_BTS_ALLOCATE_BUFFER:
+ ret = ptrace_bts_allocate_bts(child, data);
+ break;
+
+ case PTRACE_BTS_GET_BUFFER_SIZE:
+ ret = ptrace_bts_get_buffer_size(child);
+ break;
+
+ case PTRACE_BTS_GET_INDEX:
+ ret = ptrace_bts_get_index(child);
+ break;
+
+ case PTRACE_BTS_READ_RECORD:
+ ret = ptrace_bts_read_record
+ (child, data,
+ (struct ptrace_bts_record __user *) addr);
+ break;
+
+ case PTRACE_BTS_CONFIG:
+ ptrace_bts_config(child, data);
+ ret = 0;
+ break;
+
+ case PTRACE_BTS_STATUS:
+ ret = ptrace_bts_status(child);
+ break;
+
Regarding your interface (can you please write those manpages to get a
full rationable)?
I'm not sure it's a good idea to have a READ_RECORD -- better would
be likely a batched interface. Probably it would
be cleaner to combine get_index and read_record into a higher
level interface that works like normal read() -- gives you data
not read before for multiple records.
Also not sure why you have separate config and allocate_buffer steps.
They could be easily combined, couldn't they?
+/*
+ * Maximal BTS size in number of records
+ */
+#define PTRACE_BTS_MAX_BTS_SIZE 4000
This should be likely some sort of sysctl. Or perhaps just use user supplied
buffers limited by the mlock ulimit (that would allow zero copy). Ok
it means the high level read interface proposed above wouldn't work.
Perhaps the high level interface is better, although zero copy would
be more efficient. Not 100% sure what is better.
+
+
+struct ptrace_bts_configuration ptrace_bts_cfg;
+
+/*
+ * Configure trace hardware to enable tracing
+ * Return 0, on success; -Eerrno, otherwise.
+ */
+static int ptrace_bts_enable(void)
+{
+ unsigned long long debugctl_msr = 0;
+
+ if (!ptrace_bts_cfg.debugctrl_mask)
+ return -EOPNOTSUPP;
+
+ rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl_msr);
+ debugctl_msr |= ptrace_bts_cfg.debugctrl_mask;
+ wrmsrl(MSR_IA32_DEBUGCTLMSR, debugctl_msr);
I still think you should cache the DEBUGCTLMSR. If you worry
about other people changing it provide a general accessor.
-Andi
-
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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
> >and it seems like this patch and perfmon2 are going to have to > >live with > >each other... since they both require the use of the DS save area... > > Hmmm, this might require some synchronization between those two. > > Do you know how (accesses to) MSR's are managed by the kernel? There is a simple MSR allocator in the nmi watchdog code. It is very simple though and was only intended for performance counters originally so you might need to enhance it first for complicated things. -Andi - 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/
RE: [patch][v2] x86, ptrace: support for branch trace store(BTS)
>-Original Message-
>From: dean gaudet [mailto:[EMAIL PROTECTED]
>Sent: Dienstag, 20. November 2007 16:37
>To: Metzger, Markus T
>Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED];
>[EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Siddha, Suresh
>B; [EMAIL PROTECTED]
>Subject: Re: [patch][v2] x86, ptrace: support for branch trace
>store(BTS)
>
>On Tue, 20 Nov 2007, dean gaudet wrote:
>
>> On Tue, 20 Nov 2007, Metzger, Markus T wrote:
>>
>> > +__cpuinit void ptrace_bts_init_intel(struct cpuinfo_x86 *c)
>> > +{
>> > + switch (c->x86) {
>> > + case 0x6:
>> > + switch (c->x86_model) {
>> > +#ifdef __i386__
>> > + case 0xD:
>> > + case 0xE: /* Pentium M */
>> > + ptrace_bts_ops = ptrace_bts_ops_pentium_m;
>> > + break;
>> > +#endif /* _i386_ */
>> > + case 0xF: /* Core2 */
>> > + ptrace_bts_ops = ptrace_bts_ops_core2;
>> > + break;
>> > + default:
>> > + /* sorry, don't know about them */
>> > + break;
>> > + }
>> > + break;
>> > + case 0xF:
>> > + switch (c->x86_model) {
>> > +#ifdef __i386__
>> > + case 0x0:
>> > + case 0x1:
>> > + case 0x2:
>> > + case 0x3: /* Netburst */
>> > + ptrace_bts_ops = ptrace_bts_ops_netburst;
>> > + break;
>> > +#endif /* _i386_ */
>> > + default:
>> > + /* sorry, don't know about them */
>> > + break;
>> > + }
>> > + break;
>>
>> is this right? i thought intel family 15 models 3 and 4
>supported amd64
>> mode...
>
>actually... why aren't you using cpuid level 1 edx bit 21 to
>enable/disable this feature? isn't that the bit defined to indicate
>whether this feature is supported or not?
This function is not checking for the presence of the feature
but for the BTS record format.
The feature check is done in init_intel().
>and it seems like this patch and perfmon2 are going to have to
>live with
>each other... since they both require the use of the DS save area...
Hmmm, this might require some synchronization between those two.
Do you know how (accesses to) MSR's are managed by the kernel?
regards,
markus.
>
>-dean
>
-
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-
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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
> We might want to add support for Netburst in 64bit mode some day. > For today, I simply exclude Netburst for x86_64. If you switched to table driven then adding another format like this would be likely very easy. It's just that with the "own code for everything" method it becomes difficult. -Andi - 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/
RE: [patch][v2] x86, ptrace: support for branch trace store(BTS)
>-Original Message-
>From: dean gaudet [mailto:[EMAIL PROTECTED]
>Sent: Dienstag, 20. November 2007 16:27
>To: Metzger, Markus T
>Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED];
>[EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Siddha, Suresh
>B; [EMAIL PROTECTED]
>Subject: Re: [patch][v2] x86, ptrace: support for branch trace
>store(BTS)
>
>On Tue, 20 Nov 2007, Metzger, Markus T wrote:
>
>> +__cpuinit void ptrace_bts_init_intel(struct cpuinfo_x86 *c)
>> +{
>> +switch (c->x86) {
>> +case 0x6:
>> +switch (c->x86_model) {
>> +#ifdef __i386__
>> +case 0xD:
>> +case 0xE: /* Pentium M */
>> +ptrace_bts_ops = ptrace_bts_ops_pentium_m;
>> +break;
>> +#endif /* _i386_ */
>> +case 0xF: /* Core2 */
>> +ptrace_bts_ops = ptrace_bts_ops_core2;
>> +break;
>> +default:
>> +/* sorry, don't know about them */
>> +break;
>> +}
>> +break;
>> +case 0xF:
>> +switch (c->x86_model) {
>> +#ifdef __i386__
>> +case 0x0:
>> +case 0x1:
>> +case 0x2:
>> +case 0x3: /* Netburst */
>> +ptrace_bts_ops = ptrace_bts_ops_netburst;
>> +break;
>> +#endif /* _i386_ */
>> +default:
>> +/* sorry, don't know about them */
>> +break;
>> +}
>> +break;
>
>is this right? i thought intel family 15 models 3 and 4
>supported amd64
>mode...
That's correct.
In 32bit mode, they would use the BTS format that I implemented for
Netburst.
In 64bit mode, they would use a slightly different format, which is not
implemented.
We might want to add support for Netburst in 64bit mode some day.
For today, I simply exclude Netburst for x86_64.
regards,
markus.
>
>-dean
>
-
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-
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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
On Tue, 20 Nov 2007, dean gaudet wrote:
> On Tue, 20 Nov 2007, Metzger, Markus T wrote:
>
> > +__cpuinit void ptrace_bts_init_intel(struct cpuinfo_x86 *c)
> > +{
> > + switch (c->x86) {
> > + case 0x6:
> > + switch (c->x86_model) {
> > +#ifdef __i386__
> > + case 0xD:
> > + case 0xE: /* Pentium M */
> > + ptrace_bts_ops = ptrace_bts_ops_pentium_m;
> > + break;
> > +#endif /* _i386_ */
> > + case 0xF: /* Core2 */
> > + ptrace_bts_ops = ptrace_bts_ops_core2;
> > + break;
> > + default:
> > + /* sorry, don't know about them */
> > + break;
> > + }
> > + break;
> > + case 0xF:
> > + switch (c->x86_model) {
> > +#ifdef __i386__
> > + case 0x0:
> > + case 0x1:
> > + case 0x2:
> > + case 0x3: /* Netburst */
> > + ptrace_bts_ops = ptrace_bts_ops_netburst;
> > + break;
> > +#endif /* _i386_ */
> > + default:
> > + /* sorry, don't know about them */
> > + break;
> > + }
> > + break;
>
> is this right? i thought intel family 15 models 3 and 4 supported amd64
> mode...
actually... why aren't you using cpuid level 1 edx bit 21 to
enable/disable this feature? isn't that the bit defined to indicate
whether this feature is supported or not?
and it seems like this patch and perfmon2 are going to have to live with
each other... since they both require the use of the DS save area...
-dean
-
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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
On Tue, 20 Nov 2007, Metzger, Markus T wrote:
> +__cpuinit void ptrace_bts_init_intel(struct cpuinfo_x86 *c)
> +{
> + switch (c->x86) {
> + case 0x6:
> + switch (c->x86_model) {
> +#ifdef __i386__
> + case 0xD:
> + case 0xE: /* Pentium M */
> + ptrace_bts_ops = ptrace_bts_ops_pentium_m;
> + break;
> +#endif /* _i386_ */
> + case 0xF: /* Core2 */
> + ptrace_bts_ops = ptrace_bts_ops_core2;
> + break;
> + default:
> + /* sorry, don't know about them */
> + break;
> + }
> + break;
> + case 0xF:
> + switch (c->x86_model) {
> +#ifdef __i386__
> + case 0x0:
> + case 0x1:
> + case 0x2:
> + case 0x3: /* Netburst */
> + ptrace_bts_ops = ptrace_bts_ops_netburst;
> + break;
> +#endif /* _i386_ */
> + default:
> + /* sorry, don't know about them */
> + break;
> + }
> + break;
is this right? i thought intel family 15 models 3 and 4 supported amd64
mode...
-dean
-
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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
> - the internal buffer interpretation as well as the corresponding > operations are selected at run-time by hardware detection > - different processors use different branch record formats I still think it would be far better if you would switch this over to be table driven. e.g. define a record that contains offsetof()/sizeof() of the different formats and use generic functions. That would decrease code size considerably. Also those manpages are really needed. And your patch seems to be word wrapped. -Andi - 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/
[patch][v2] x86, ptrace: support for branch trace store(BTS)
Changes to previous version:
- moved task arrives/departs notifications to __switch_to_xtra()
- added _TIF_BTS_TRACE and _TIF_BTS_TRACE_TS to _TIF_WORK_CTXSW_*
- split _TIF_WORK_CTXSW into ~_PREV and ~_NEXT for x86_64
- ptrace_bts_init_intel() function called from init_intel()
- removed PTRACE_BTS_INIT ptrace command
Support for Intel's last branch recording to ptrace. This gives
debuggers
access to this hardware feature and allows them to show an execution
trace
of the debugged application.
Last branch recording (see section 18.5 in the Intel 64 and IA-32
Architectures Software Developer's Manual) allows taking an execution
trace of the running application without instrumentation. When a branch
is executed, the hardware logs the source and destination address in a
cyclic buffer given to it by the OS.
This can be a great debugging aid. It shows you how exactly you got
where you currently are without requiring you to do lots of single
stepping and rerunning.
This patch manages the various buffers, configures the trace
hardware, disentangles the trace, and provides a user interface via
ptrace. On the high-level design:
- there is one optional trace buffer per thread_struct
- upon a context switch, the trace hardware is reconfigured to either
disable tracing or to use the appropriate buffer for the new task.
- tracing induces ~20% overhead as branch records are sent out on
the bus.
- the hardware collects trace per processor. To disentangle the
traces for different tasks, we use separate buffers and reconfigure
the trace hardware.
- the internal buffer interpretation as well as the corresponding
operations are selected at run-time by hardware detection
- different processors use different branch record formats
Opens:
- warnings due to code sharing between i386 and x86_64
- support for more processors (to come)
- ptrace command numbers (just picked some numbers randomly)
Signed-off-by: Markus Metzger <[EMAIL PROTECTED]>
Signed-off-by: Suresh Siddha <[EMAIL PROTECTED]>
---
Index: linux-2.6/arch/x86/kernel/process_32.c
===
--- linux-2.6.orig/arch/x86/kernel/process_32.c 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/arch/x86/kernel/process_32.c 2007-11-19 15:49:53.%N
+0100
@@ -623,6 +623,19 @@
}
#endif
+ /*
+* Last branch recording recofiguration of trace hardware and
+* disentangling of trace data per task.
+*/
+ if (test_tsk_thread_flag(prev_p, TIF_BTS_TRACE) ||
+ test_tsk_thread_flag(prev_p, TIF_BTS_TRACE_TS))
+ ptrace_bts_task_departs(prev_p);
+
+ if (test_tsk_thread_flag(next_p, TIF_BTS_TRACE) ||
+ test_tsk_thread_flag(next_p, TIF_BTS_TRACE_TS))
+ ptrace_bts_task_arrives(next_p);
+
+
if (!test_tsk_thread_flag(next_p, TIF_IO_BITMAP)) {
/*
* Disable the bitmap via an invalid offset. We still
cache
Index: linux-2.6/arch/x86/kernel/ptrace_32.c
===
--- linux-2.6.orig/arch/x86/kernel/ptrace_32.c 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/arch/x86/kernel/ptrace_32.c 2007-11-19 13:11:46.%N
+0100
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
/*
* does not yet catch signals sent when the child dies.
@@ -274,6 +275,7 @@
{
clear_singlestep(child);
clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
+ ptrace_bts_task_detached(child);
}
/*
@@ -610,6 +612,33 @@
(struct user_desc __user *)
data);
break;
+ case PTRACE_BTS_ALLOCATE_BUFFER:
+ ret = ptrace_bts_allocate_bts(child, data);
+ break;
+
+ case PTRACE_BTS_GET_BUFFER_SIZE:
+ ret = ptrace_bts_get_buffer_size(child);
+ break;
+
+ case PTRACE_BTS_GET_INDEX:
+ ret = ptrace_bts_get_index(child);
+ break;
+
+ case PTRACE_BTS_READ_RECORD:
+ ret = ptrace_bts_read_record
+ (child, data,
+(struct ptrace_bts_record __user *) addr);
+ break;
+
+ case PTRACE_BTS_CONFIG:
+ ptrace_bts_config(child, data);
+ ret = 0;
+ break;
+
+ case PTRACE_BTS_STATUS:
+ ret = ptrace_bts_status(child);
+ break;
+
default:
ret = ptrace_request(child, request, addr, data);
break;
Index: linux-2.6/include/asm-x86/ptrace-abi.h
===
--- linux-2.6.orig/include/asm-x86/ptrace-abi.h 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/include/asm-x86/ptrace-abi.h 2007-11-19 13:11:46.%N
+0100
@@ -78,4 +78,43 @@
# define PTRACE_SYSEMU_SINGLESTEP 32
#endif
+
+/* Allocate new bts buffer (free old one, if exists) of size DATA bts
records;
+
[patch][v2] x86, ptrace: support for branch trace store(BTS)
Changes to previous version:
- moved task arrives/departs notifications to __switch_to_xtra()
- added _TIF_BTS_TRACE and _TIF_BTS_TRACE_TS to _TIF_WORK_CTXSW_*
- split _TIF_WORK_CTXSW into ~_PREV and ~_NEXT for x86_64
- ptrace_bts_init_intel() function called from init_intel()
- removed PTRACE_BTS_INIT ptrace command
Support for Intel's last branch recording to ptrace. This gives
debuggers
access to this hardware feature and allows them to show an execution
trace
of the debugged application.
Last branch recording (see section 18.5 in the Intel 64 and IA-32
Architectures Software Developer's Manual) allows taking an execution
trace of the running application without instrumentation. When a branch
is executed, the hardware logs the source and destination address in a
cyclic buffer given to it by the OS.
This can be a great debugging aid. It shows you how exactly you got
where you currently are without requiring you to do lots of single
stepping and rerunning.
This patch manages the various buffers, configures the trace
hardware, disentangles the trace, and provides a user interface via
ptrace. On the high-level design:
- there is one optional trace buffer per thread_struct
- upon a context switch, the trace hardware is reconfigured to either
disable tracing or to use the appropriate buffer for the new task.
- tracing induces ~20% overhead as branch records are sent out on
the bus.
- the hardware collects trace per processor. To disentangle the
traces for different tasks, we use separate buffers and reconfigure
the trace hardware.
- the internal buffer interpretation as well as the corresponding
operations are selected at run-time by hardware detection
- different processors use different branch record formats
Opens:
- warnings due to code sharing between i386 and x86_64
- support for more processors (to come)
- ptrace command numbers (just picked some numbers randomly)
Signed-off-by: Markus Metzger [EMAIL PROTECTED]
Signed-off-by: Suresh Siddha [EMAIL PROTECTED]
---
Index: linux-2.6/arch/x86/kernel/process_32.c
===
--- linux-2.6.orig/arch/x86/kernel/process_32.c 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/arch/x86/kernel/process_32.c 2007-11-19 15:49:53.%N
+0100
@@ -623,6 +623,19 @@
}
#endif
+ /*
+* Last branch recording recofiguration of trace hardware and
+* disentangling of trace data per task.
+*/
+ if (test_tsk_thread_flag(prev_p, TIF_BTS_TRACE) ||
+ test_tsk_thread_flag(prev_p, TIF_BTS_TRACE_TS))
+ ptrace_bts_task_departs(prev_p);
+
+ if (test_tsk_thread_flag(next_p, TIF_BTS_TRACE) ||
+ test_tsk_thread_flag(next_p, TIF_BTS_TRACE_TS))
+ ptrace_bts_task_arrives(next_p);
+
+
if (!test_tsk_thread_flag(next_p, TIF_IO_BITMAP)) {
/*
* Disable the bitmap via an invalid offset. We still
cache
Index: linux-2.6/arch/x86/kernel/ptrace_32.c
===
--- linux-2.6.orig/arch/x86/kernel/ptrace_32.c 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/arch/x86/kernel/ptrace_32.c 2007-11-19 13:11:46.%N
+0100
@@ -24,6 +24,7 @@
#include asm/debugreg.h
#include asm/ldt.h
#include asm/desc.h
+#include asm/ptrace_bts.h
/*
* does not yet catch signals sent when the child dies.
@@ -274,6 +275,7 @@
{
clear_singlestep(child);
clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
+ ptrace_bts_task_detached(child);
}
/*
@@ -610,6 +612,33 @@
(struct user_desc __user *)
data);
break;
+ case PTRACE_BTS_ALLOCATE_BUFFER:
+ ret = ptrace_bts_allocate_bts(child, data);
+ break;
+
+ case PTRACE_BTS_GET_BUFFER_SIZE:
+ ret = ptrace_bts_get_buffer_size(child);
+ break;
+
+ case PTRACE_BTS_GET_INDEX:
+ ret = ptrace_bts_get_index(child);
+ break;
+
+ case PTRACE_BTS_READ_RECORD:
+ ret = ptrace_bts_read_record
+ (child, data,
+(struct ptrace_bts_record __user *) addr);
+ break;
+
+ case PTRACE_BTS_CONFIG:
+ ptrace_bts_config(child, data);
+ ret = 0;
+ break;
+
+ case PTRACE_BTS_STATUS:
+ ret = ptrace_bts_status(child);
+ break;
+
default:
ret = ptrace_request(child, request, addr, data);
break;
Index: linux-2.6/include/asm-x86/ptrace-abi.h
===
--- linux-2.6.orig/include/asm-x86/ptrace-abi.h 2007-11-19 11:50:05.%N
+0100
+++ linux-2.6/include/asm-x86/ptrace-abi.h 2007-11-19 13:11:46.%N
+0100
@@ -78,4 +78,43 @@
# define PTRACE_SYSEMU_SINGLESTEP 32
#endif
+
+/* Allocate new bts buffer (free old one, if
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
- the internal buffer interpretation as well as the corresponding operations are selected at run-time by hardware detection - different processors use different branch record formats I still think it would be far better if you would switch this over to be table driven. e.g. define a record that contains offsetof()/sizeof() of the different formats and use generic functions. That would decrease code size considerably. Also those manpages are really needed. And your patch seems to be word wrapped. -Andi - 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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
We might want to add support for Netburst in 64bit mode some day. For today, I simply exclude Netburst for x86_64. If you switched to table driven then adding another format like this would be likely very easy. It's just that with the own code for everything method it becomes difficult. -Andi - 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/
RE: [patch][v2] x86, ptrace: support for branch trace store(BTS)
-Original Message-
From: dean gaudet [mailto:[EMAIL PROTECTED]
Sent: Dienstag, 20. November 2007 16:37
To: Metzger, Markus T
Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED];
[EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Siddha, Suresh
B; [EMAIL PROTECTED]
Subject: Re: [patch][v2] x86, ptrace: support for branch trace
store(BTS)
On Tue, 20 Nov 2007, dean gaudet wrote:
On Tue, 20 Nov 2007, Metzger, Markus T wrote:
+__cpuinit void ptrace_bts_init_intel(struct cpuinfo_x86 *c)
+{
+ switch (c-x86) {
+ case 0x6:
+ switch (c-x86_model) {
+#ifdef __i386__
+ case 0xD:
+ case 0xE: /* Pentium M */
+ ptrace_bts_ops = ptrace_bts_ops_pentium_m;
+ break;
+#endif /* _i386_ */
+ case 0xF: /* Core2 */
+ ptrace_bts_ops = ptrace_bts_ops_core2;
+ break;
+ default:
+ /* sorry, don't know about them */
+ break;
+ }
+ break;
+ case 0xF:
+ switch (c-x86_model) {
+#ifdef __i386__
+ case 0x0:
+ case 0x1:
+ case 0x2:
+ case 0x3: /* Netburst */
+ ptrace_bts_ops = ptrace_bts_ops_netburst;
+ break;
+#endif /* _i386_ */
+ default:
+ /* sorry, don't know about them */
+ break;
+ }
+ break;
is this right? i thought intel family 15 models 3 and 4
supported amd64
mode...
actually... why aren't you using cpuid level 1 edx bit 21 to
enable/disable this feature? isn't that the bit defined to indicate
whether this feature is supported or not?
This function is not checking for the presence of the feature
but for the BTS record format.
The feature check is done in init_intel().
and it seems like this patch and perfmon2 are going to have to
live with
each other... since they both require the use of the DS save area...
Hmmm, this might require some synchronization between those two.
Do you know how (accesses to) MSR's are managed by the kernel?
regards,
markus.
-dean
-
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-
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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
and it seems like this patch and perfmon2 are going to have to live with each other... since they both require the use of the DS save area... Hmmm, this might require some synchronization between those two. Do you know how (accesses to) MSR's are managed by the kernel? There is a simple MSR allocator in the nmi watchdog code. It is very simple though and was only intended for performance counters originally so you might need to enhance it first for complicated things. -Andi - 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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
On Tue, 20 Nov 2007, dean gaudet wrote:
On Tue, 20 Nov 2007, Metzger, Markus T wrote:
+__cpuinit void ptrace_bts_init_intel(struct cpuinfo_x86 *c)
+{
+ switch (c-x86) {
+ case 0x6:
+ switch (c-x86_model) {
+#ifdef __i386__
+ case 0xD:
+ case 0xE: /* Pentium M */
+ ptrace_bts_ops = ptrace_bts_ops_pentium_m;
+ break;
+#endif /* _i386_ */
+ case 0xF: /* Core2 */
+ ptrace_bts_ops = ptrace_bts_ops_core2;
+ break;
+ default:
+ /* sorry, don't know about them */
+ break;
+ }
+ break;
+ case 0xF:
+ switch (c-x86_model) {
+#ifdef __i386__
+ case 0x0:
+ case 0x1:
+ case 0x2:
+ case 0x3: /* Netburst */
+ ptrace_bts_ops = ptrace_bts_ops_netburst;
+ break;
+#endif /* _i386_ */
+ default:
+ /* sorry, don't know about them */
+ break;
+ }
+ break;
is this right? i thought intel family 15 models 3 and 4 supported amd64
mode...
actually... why aren't you using cpuid level 1 edx bit 21 to
enable/disable this feature? isn't that the bit defined to indicate
whether this feature is supported or not?
and it seems like this patch and perfmon2 are going to have to live with
each other... since they both require the use of the DS save area...
-dean
-
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/
Re: [patch][v2] x86, ptrace: support for branch trace store(BTS)
On Tue, 20 Nov 2007, Metzger, Markus T wrote:
+__cpuinit void ptrace_bts_init_intel(struct cpuinfo_x86 *c)
+{
+ switch (c-x86) {
+ case 0x6:
+ switch (c-x86_model) {
+#ifdef __i386__
+ case 0xD:
+ case 0xE: /* Pentium M */
+ ptrace_bts_ops = ptrace_bts_ops_pentium_m;
+ break;
+#endif /* _i386_ */
+ case 0xF: /* Core2 */
+ ptrace_bts_ops = ptrace_bts_ops_core2;
+ break;
+ default:
+ /* sorry, don't know about them */
+ break;
+ }
+ break;
+ case 0xF:
+ switch (c-x86_model) {
+#ifdef __i386__
+ case 0x0:
+ case 0x1:
+ case 0x2:
+ case 0x3: /* Netburst */
+ ptrace_bts_ops = ptrace_bts_ops_netburst;
+ break;
+#endif /* _i386_ */
+ default:
+ /* sorry, don't know about them */
+ break;
+ }
+ break;
is this right? i thought intel family 15 models 3 and 4 supported amd64
mode...
-dean
-
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/
RE: [patch][v2] x86, ptrace: support for branch trace store(BTS)
-Original Message-
From: dean gaudet [mailto:[EMAIL PROTECTED]
Sent: Dienstag, 20. November 2007 16:27
To: Metzger, Markus T
Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED];
[EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Siddha, Suresh
B; [EMAIL PROTECTED]
Subject: Re: [patch][v2] x86, ptrace: support for branch trace
store(BTS)
On Tue, 20 Nov 2007, Metzger, Markus T wrote:
+__cpuinit void ptrace_bts_init_intel(struct cpuinfo_x86 *c)
+{
+switch (c-x86) {
+case 0x6:
+switch (c-x86_model) {
+#ifdef __i386__
+case 0xD:
+case 0xE: /* Pentium M */
+ptrace_bts_ops = ptrace_bts_ops_pentium_m;
+break;
+#endif /* _i386_ */
+case 0xF: /* Core2 */
+ptrace_bts_ops = ptrace_bts_ops_core2;
+break;
+default:
+/* sorry, don't know about them */
+break;
+}
+break;
+case 0xF:
+switch (c-x86_model) {
+#ifdef __i386__
+case 0x0:
+case 0x1:
+case 0x2:
+case 0x3: /* Netburst */
+ptrace_bts_ops = ptrace_bts_ops_netburst;
+break;
+#endif /* _i386_ */
+default:
+/* sorry, don't know about them */
+break;
+}
+break;
is this right? i thought intel family 15 models 3 and 4
supported amd64
mode...
That's correct.
In 32bit mode, they would use the BTS format that I implemented for
Netburst.
In 64bit mode, they would use a slightly different format, which is not
implemented.
We might want to add support for Netburst in 64bit mode some day.
For today, I simply exclude Netburst for x86_64.
regards,
markus.
-dean
-
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-
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/
