Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
On Wed, 04 Nov 2015 04:21:51 PST (-0800), pet...@infradead.org wrote: > On Wed, Nov 04, 2015 at 12:41:06PM +0100, Peter Zijlstra wrote: >> On Tue, Nov 03, 2015 at 11:46:30AM -0800, Palmer Dabbelt wrote: >> > This has a "#ifdef CONFIG_*" that used to be exposed to userspace. >> > >> > The names in here are so generic that I don't think it's a good idea >> > to expose them to userspace (or even the rest of the kernel). Since >> > there's only one kernel user, it's been moved to that file. >> > >> > Signed-off-by: Palmer Dabbelt >> > Reviewed-by: Andrew Waterman >> > Reviewed-by: Albert Ou >> >> Assuming you want to keep all these patches together in a tree or so. >> Let me know if you want me to take this patch. Well, the plan was to try to get the whole patch set all upstream together. I'd prefer that, because it'll be easier to make sure everything gets in before the last checker patch. Since it touches so many different places I'd be OK with doing it peicemeal. I'm kind of new to this whole process: do you think someone is likely to take this patch set all together? >> Acked-by: Peter Zijlstra (Intel) > > Ah, build-bot finds your change is broken and you didn't grep right. It > is used in more places. Sorry about that, I must have mis-grep'd. I guess that's what the build-bot is for :). > How about moving it to include/linux/hw_breakpoint.h, instead of having > it in the uapi crud? Yep, that makes sense. I'm going to re-submit a v5 of this patch set, since there was also a missing patch for blackfin that the buildbot found. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
On Wed, Nov 04, 2015 at 12:41:06PM +0100, Peter Zijlstra wrote: > On Tue, Nov 03, 2015 at 11:46:30AM -0800, Palmer Dabbelt wrote: > > This has a "#ifdef CONFIG_*" that used to be exposed to userspace. > > > > The names in here are so generic that I don't think it's a good idea > > to expose them to userspace (or even the rest of the kernel). Since > > there's only one kernel user, it's been moved to that file. > > > > Signed-off-by: Palmer Dabbelt > > Reviewed-by: Andrew Waterman > > Reviewed-by: Albert Ou > > Assuming you want to keep all these patches together in a tree or so. > Let me know if you want me to take this patch. > > Acked-by: Peter Zijlstra (Intel) Ah, build-bot finds your change is broken and you didn't grep right. It is used in more places. How about moving it to include/linux/hw_breakpoint.h, instead of having it in the uapi crud? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
On Tue, Nov 03, 2015 at 11:46:30AM -0800, Palmer Dabbelt wrote: > This has a "#ifdef CONFIG_*" that used to be exposed to userspace. > > The names in here are so generic that I don't think it's a good idea > to expose them to userspace (or even the rest of the kernel). Since > there's only one kernel user, it's been moved to that file. > > Signed-off-by: Palmer Dabbelt > Reviewed-by: Andrew Waterman > Reviewed-by: Albert Ou Assuming you want to keep all these patches together in a tree or so. Let me know if you want me to take this patch. Acked-by: Peter Zijlstra (Intel) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
Hi Palmer,
[auto build test ERROR on v4.3-rc7]
[also ERROR on: next-20151103]
url:
https://github.com/0day-ci/linux/commits/Palmer-Dabbelt/Remove-ifdef-CONFIG_64BIT-from-all-asm-generic-fcntl-h/20151104-035501
base: https://github.com/0day-ci/linux
Palmer-Dabbelt/Remove-ifdef-CONFIG_64BIT-from-all-asm-generic-fcntl-h/20151104-035501
config: arm-socfpga_defconfig (attached as .config)
reproduce:
wget
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
arch/arm/kernel/ptrace.c: In function 'ptrace_get_hbp_resource_info':
>> arch/arm/kernel/ptrace.c:440:33: error: 'TYPE_INST' undeclared (first use in
>> this function)
num_brps = hw_breakpoint_slots(TYPE_INST);
^
arch/arm/kernel/ptrace.c:440:33: note: each undeclared identifier is
reported only once for each function it appears in
>> arch/arm/kernel/ptrace.c:441:33: error: 'TYPE_DATA' undeclared (first use in
>> this function)
num_wrps = hw_breakpoint_slots(TYPE_DATA);
^
--
arch/arm/kernel/hw_breakpoint.c: In function 'hw_breakpoint_slots':
>> arch/arm/kernel/hw_breakpoint.c:290:7: error: 'TYPE_INST' undeclared (first
>> use in this function)
case TYPE_INST:
^
arch/arm/kernel/hw_breakpoint.c:290:7: note: each undeclared identifier is
reported only once for each function it appears in
>> arch/arm/kernel/hw_breakpoint.c:292:7: error: 'TYPE_DATA' undeclared (first
>> use in this function)
case TYPE_DATA:
^
vim +/TYPE_INST +440 arch/arm/kernel/ptrace.c
864232fa Will Deacon 2010-09-03 434
864232fa Will Deacon 2010-09-03 435 static u32
ptrace_get_hbp_resource_info(void)
864232fa Will Deacon 2010-09-03 436 {
864232fa Will Deacon 2010-09-03 437u8 num_brps, num_wrps, debug_arch,
wp_len;
864232fa Will Deacon 2010-09-03 438u32 reg = 0;
864232fa Will Deacon 2010-09-03 439
864232fa Will Deacon 2010-09-03 @440num_brps=
hw_breakpoint_slots(TYPE_INST);
864232fa Will Deacon 2010-09-03 @441num_wrps=
hw_breakpoint_slots(TYPE_DATA);
864232fa Will Deacon 2010-09-03 442debug_arch = arch_get_debug_arch();
864232fa Will Deacon 2010-09-03 443wp_len = arch_get_max_wp_len();
864232fa Will Deacon 2010-09-03 444
:: The code at line 440 was first introduced by commit
:: 864232fa1a2f8dfe003438ef0851a56722740f3e ARM: 6357/1: hw-breakpoint: add
new ptrace requests for hw-breakpoint interaction
:: TO: Will Deacon
:: CC: Russell King
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: Binary data
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
Hi Palmer,
[auto build test ERROR on v4.3-rc7]
[also ERROR on: next-20151103]
url:
https://github.com/0day-ci/linux/commits/Palmer-Dabbelt/Remove-ifdef-CONFIG_64BIT-from-all-asm-generic-fcntl-h/20151104-035501
base: https://github.com/0day-ci/linux
Palmer-Dabbelt/Remove-ifdef-CONFIG_64BIT-from-all-asm-generic-fcntl-h/20151104-035501
config: powerpc-defconfig (attached as .config)
reproduce:
wget
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
arch/powerpc/kernel/hw_breakpoint.c: In function 'hw_breakpoint_slots':
>> arch/powerpc/kernel/hw_breakpoint.c:49:14: error: 'TYPE_DATA' undeclared
>> (first use in this function)
if (type == TYPE_DATA)
^
arch/powerpc/kernel/hw_breakpoint.c:49:14: note: each undeclared identifier
is reported only once for each function it appears in
vim +/TYPE_DATA +49 arch/powerpc/kernel/hw_breakpoint.c
5aae8a53 K.Prasad 2010-06-15 43
5aae8a53 K.Prasad 2010-06-15 44 /*
d09ec738 Paul Mackerras 2010-06-29 45 * Returns total number of data or
instruction breakpoints available.
d09ec738 Paul Mackerras 2010-06-29 46 */
d09ec738 Paul Mackerras 2010-06-29 47 int hw_breakpoint_slots(int type)
d09ec738 Paul Mackerras 2010-06-29 48 {
d09ec738 Paul Mackerras 2010-06-29 @49 if (type == TYPE_DATA)
d09ec738 Paul Mackerras 2010-06-29 50 return HBP_NUM;
d09ec738 Paul Mackerras 2010-06-29 51 return 0; /* no
instruction breakpoints available */
d09ec738 Paul Mackerras 2010-06-29 52 }
:: The code at line 49 was first introduced by commit
:: d09ec7387184eba9e3030496f0451204090ff610 powerpc, hw_breakpoint: Tell
generic code we have no instruction breakpoints
:: TO: Paul Mackerras
:: CC: Paul Mackerras
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: Binary data
[PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
This has a "#ifdef CONFIG_*" that used to be exposed to userspace.
The names in here are so generic that I don't think it's a good idea
to expose them to userspace (or even the rest of the kernel). Since
there's only one kernel user, it's been moved to that file.
Signed-off-by: Palmer Dabbelt
Reviewed-by: Andrew Waterman
Reviewed-by: Albert Ou
---
include/uapi/linux/hw_breakpoint.h | 10 --
kernel/events/hw_breakpoint.c | 10 ++
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/include/uapi/linux/hw_breakpoint.h
b/include/uapi/linux/hw_breakpoint.h
index b04000a..7a6a5a7 100644
--- a/include/uapi/linux/hw_breakpoint.h
+++ b/include/uapi/linux/hw_breakpoint.h
@@ -17,14 +17,4 @@ enum {
HW_BREAKPOINT_INVALID = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
};
-enum bp_type_idx {
- TYPE_INST = 0,
-#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
- TYPE_DATA = 0,
-#else
- TYPE_DATA = 1,
-#endif
- TYPE_MAX
-};
-
#endif /* _UAPI_LINUX_HW_BREAKPOINT_H */
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 92ce5f4..5ad117e 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -58,6 +58,16 @@ struct bp_cpuinfo {
unsigned intflexible; /* XXX: placeholder, see fetch_this_slot() */
};
+enum bp_type_idx {
+ TYPE_INST = 0,
+#if defined(CONFIG_HAVE_MIXED_BREAKPOINTS_REGS)
+ TYPE_DATA = 0,
+#else
+ TYPE_DATA = 1,
+#endif
+ TYPE_MAX
+};
+
static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);
static int nr_slots[TYPE_MAX];
--
2.4.10
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
On Tue, Sep 15, 2015 at 11:15:29PM +0200, Arnd Bergmann wrote:
> On Tuesday 15 September 2015 10:06:07 Peter Zijlstra wrote:
> > > diff --git a/include/uapi/linux/hw_breakpoint.h
> > > b/include/uapi/linux/hw_breakpoint.h
> > > index b04000a2296a..7a6a5a7f9511 100644
> > > --- a/include/uapi/linux/hw_breakpoint.h
> > > +++ b/include/uapi/linux/hw_breakpoint.h
> > > @@ -17,14 +17,4 @@ enum {
> > > HW_BREAKPOINT_INVALID = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
> > > };
> > >
> > > -enum bp_type_idx {
> > > - TYPE_INST = 0,
> > > -#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
> > > - TYPE_DATA = 0,
> > > -#else
> > > - TYPE_DATA = 1,
> > > -#endif
> > > - TYPE_MAX
> > > -};
> >
> > This is rather unfortunate; you are correct that the naming is too
> > generic (and I tend to agree), but I think these values are required by
> > userspace to fill out:
> >
> > perf_event_attr::bp_type
> >
> > So removing them will break things.
> >
> > Frederic?
>
> If user space actually relies on the definition from this header file,
> then it will use the wrong one on x86 and get 'TYPE_DATA = 1', while the
> kernel uses 'TYPE_DATA = 0'.
>
> That seems unlikely to work, so I suspect it gets a different definition.
> If it uses this definition and it does work, we can probably use
>
> #if defined(__KERNEL__) && defined(CONFIG_HAVE_MIXED_BREAKPOINTS_REGS)
>
> but that requires a comment explaining exactly why that works.
I think this TYPE_DATA/TYPE_INST can be safely removed from uapi. This is only
about internal kernel code. Userspace only relies on HW_BREAKPOINT_[R/W/X]
to tell about the nature of the breakpoint.
If userspace ever relies on it, which I have no idea why, it even needs to
define
CONFIG_HAVE_MIXED_BREAKPOINTS_REGS. No really there shouldn't be any user of
that outside
the kernel.
Thanks.
>
> Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
Arnd Bergmann wrote: > That seems unlikely to work, so I suspect it gets a different definition. > If it uses this definition and it does work, we can probably use > > #if defined(__KERNEL__) && defined(CONFIG_HAVE_MIXED_BREAKPOINTS_REGS) > > but that requires a comment explaining exactly why that works. Are you suggesting wrapping the entire construct in this within the UAPI header? David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
On Tue, 15 Sep 2015 12:39:10 PDT (-0700), pet...@infradead.org wrote:
> On Tue, Sep 15, 2015 at 11:40:11AM -0700, Palmer Dabbelt wrote:
>> On Tue, 15 Sep 2015 01:06:07 PDT (-0700), pet...@infradead.org wrote:
>> > On Mon, Sep 14, 2015 at 03:50:43PM -0700, Palmer Dabbelt wrote:
>> >> This has a "#ifdef CONFIG_*" that used to be exposed to userspace.
>> >>
>> >> The names in here are so generic that I don't think it's a good idea
>> >> to expose them to userspace (or even the rest of the kernel). Since
>> >> there's only one kernel user, it's been moved to that file.
>> >>
>> >> Signed-off-by: Palmer Dabbelt
>> >> Reviewed-by: Andrew Waterman
>> >> Reviewed-by: Albert Ou
>> >> ---
>> >> include/uapi/linux/hw_breakpoint.h | 10 --
>> >> kernel/events/hw_breakpoint.c | 10 ++
>> >> 2 files changed, 10 insertions(+), 10 deletions(-)
>> >>
>> >> diff --git a/include/uapi/linux/hw_breakpoint.h
>> >> b/include/uapi/linux/hw_breakpoint.h
>> >> index b04000a2296a..7a6a5a7f9511 100644
>> >> --- a/include/uapi/linux/hw_breakpoint.h
>> >> +++ b/include/uapi/linux/hw_breakpoint.h
>> >> @@ -17,14 +17,4 @@ enum {
>> >> HW_BREAKPOINT_INVALID = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
>> >> };
>> >>
>> >> -enum bp_type_idx {
>> >> - TYPE_INST = 0,
>> >> -#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
>> >> - TYPE_DATA = 0,
>> >> -#else
>> >> - TYPE_DATA = 1,
>> >> -#endif
>> >> - TYPE_MAX
>> >> -};
>> >
>> > This is rather unfortunate; you are correct that the naming is too
>> > generic (and I tend to agree), but I think these values are required by
>> > userspace to fill out:
>> >
>> > perf_event_attr::bp_type
>> >
>> > So removing them will break things.
>> >
>> > Frederic?
>>
>> perf_event_open(2) says
>>
>>bp_type (since Linux 2.6.33)
>> This chooses the breakpoint type. It is one of:
>>
>> HW_BREAKPOINT_EMPTY
>> No breakpoint.
>>
>> HW_BREAKPOINT_R
>> Count when we read the memory location.
>>
>> HW_BREAKPOINT_W
>> Count when we write the memory location.
>>
>> HW_BREAKPOINT_RW
>> Count when we read or write the memory location.
>>
>> HW_BREAKPOINT_X
>> Count when we execute code at the memory location.
>>
>> The values can be combined via a bitwise or, but the
>> combination
>> of HW_BREAKPOINT_R or HW_BREAKPOINT_W with HW_BREAKPOINT_X
>> is
>> not allowed.
>>
>> so I think removing this enum from userspace is OK. Did I miss
>> something?
>
> Nah, could've just been me not being awake. Unless Frederic says
> otherwise I'll chalk it up to not having drank enough morning juice.
Well, I'm going to leave this alone, then -- as Arnd pointed out in a
later email that got mis-threaded, this would be super tricky to fix
if userspace actually relied on it (which is why I was scared I was
wrong).
v4 (which is hopefully the final version of the patch set) will leave
this as it is.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
On Tuesday 15 September 2015 10:06:07 Peter Zijlstra wrote:
> > diff --git a/include/uapi/linux/hw_breakpoint.h
> > b/include/uapi/linux/hw_breakpoint.h
> > index b04000a2296a..7a6a5a7f9511 100644
> > --- a/include/uapi/linux/hw_breakpoint.h
> > +++ b/include/uapi/linux/hw_breakpoint.h
> > @@ -17,14 +17,4 @@ enum {
> > HW_BREAKPOINT_INVALID = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
> > };
> >
> > -enum bp_type_idx {
> > - TYPE_INST = 0,
> > -#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
> > - TYPE_DATA = 0,
> > -#else
> > - TYPE_DATA = 1,
> > -#endif
> > - TYPE_MAX
> > -};
>
> This is rather unfortunate; you are correct that the naming is too
> generic (and I tend to agree), but I think these values are required by
> userspace to fill out:
>
> perf_event_attr::bp_type
>
> So removing them will break things.
>
> Frederic?
If user space actually relies on the definition from this header file,
then it will use the wrong one on x86 and get 'TYPE_DATA = 1', while the
kernel uses 'TYPE_DATA = 0'.
That seems unlikely to work, so I suspect it gets a different definition.
If it uses this definition and it does work, we can probably use
#if defined(__KERNEL__) && defined(CONFIG_HAVE_MIXED_BREAKPOINTS_REGS)
but that requires a comment explaining exactly why that works.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
On Tue, Sep 15, 2015 at 11:40:11AM -0700, Palmer Dabbelt wrote:
> On Tue, 15 Sep 2015 01:06:07 PDT (-0700), pet...@infradead.org wrote:
> > On Mon, Sep 14, 2015 at 03:50:43PM -0700, Palmer Dabbelt wrote:
> >> This has a "#ifdef CONFIG_*" that used to be exposed to userspace.
> >>
> >> The names in here are so generic that I don't think it's a good idea
> >> to expose them to userspace (or even the rest of the kernel). Since
> >> there's only one kernel user, it's been moved to that file.
> >>
> >> Signed-off-by: Palmer Dabbelt
> >> Reviewed-by: Andrew Waterman
> >> Reviewed-by: Albert Ou
> >> ---
> >> include/uapi/linux/hw_breakpoint.h | 10 --
> >> kernel/events/hw_breakpoint.c | 10 ++
> >> 2 files changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/hw_breakpoint.h
> >> b/include/uapi/linux/hw_breakpoint.h
> >> index b04000a2296a..7a6a5a7f9511 100644
> >> --- a/include/uapi/linux/hw_breakpoint.h
> >> +++ b/include/uapi/linux/hw_breakpoint.h
> >> @@ -17,14 +17,4 @@ enum {
> >>HW_BREAKPOINT_INVALID = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
> >> };
> >>
> >> -enum bp_type_idx {
> >> - TYPE_INST = 0,
> >> -#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
> >> - TYPE_DATA = 0,
> >> -#else
> >> - TYPE_DATA = 1,
> >> -#endif
> >> - TYPE_MAX
> >> -};
> >
> > This is rather unfortunate; you are correct that the naming is too
> > generic (and I tend to agree), but I think these values are required by
> > userspace to fill out:
> >
> > perf_event_attr::bp_type
> >
> > So removing them will break things.
> >
> > Frederic?
>
> perf_event_open(2) says
>
>bp_type (since Linux 2.6.33)
> This chooses the breakpoint type. It is one of:
>
> HW_BREAKPOINT_EMPTY
> No breakpoint.
>
> HW_BREAKPOINT_R
> Count when we read the memory location.
>
> HW_BREAKPOINT_W
> Count when we write the memory location.
>
> HW_BREAKPOINT_RW
> Count when we read or write the memory location.
>
> HW_BREAKPOINT_X
> Count when we execute code at the memory location.
>
> The values can be combined via a bitwise or, but the combination
> of HW_BREAKPOINT_R or HW_BREAKPOINT_W with HW_BREAKPOINT_X is
> not allowed.
>
> so I think removing this enum from userspace is OK. Did I miss
> something?
Nah, could've just been me not being awake. Unless Frederic says
otherwise I'll chalk it up to not having drank enough morning juice.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
On Tue, 15 Sep 2015 01:06:07 PDT (-0700), pet...@infradead.org wrote:
> On Mon, Sep 14, 2015 at 03:50:43PM -0700, Palmer Dabbelt wrote:
>> This has a "#ifdef CONFIG_*" that used to be exposed to userspace.
>>
>> The names in here are so generic that I don't think it's a good idea
>> to expose them to userspace (or even the rest of the kernel). Since
>> there's only one kernel user, it's been moved to that file.
>>
>> Signed-off-by: Palmer Dabbelt
>> Reviewed-by: Andrew Waterman
>> Reviewed-by: Albert Ou
>> ---
>> include/uapi/linux/hw_breakpoint.h | 10 --
>> kernel/events/hw_breakpoint.c | 10 ++
>> 2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/uapi/linux/hw_breakpoint.h
>> b/include/uapi/linux/hw_breakpoint.h
>> index b04000a2296a..7a6a5a7f9511 100644
>> --- a/include/uapi/linux/hw_breakpoint.h
>> +++ b/include/uapi/linux/hw_breakpoint.h
>> @@ -17,14 +17,4 @@ enum {
>> HW_BREAKPOINT_INVALID = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
>> };
>>
>> -enum bp_type_idx {
>> -TYPE_INST = 0,
>> -#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
>> -TYPE_DATA = 0,
>> -#else
>> -TYPE_DATA = 1,
>> -#endif
>> -TYPE_MAX
>> -};
>
> This is rather unfortunate; you are correct that the naming is too
> generic (and I tend to agree), but I think these values are required by
> userspace to fill out:
>
> perf_event_attr::bp_type
>
> So removing them will break things.
>
> Frederic?
perf_event_open(2) says
bp_type (since Linux 2.6.33)
This chooses the breakpoint type. It is one of:
HW_BREAKPOINT_EMPTY
No breakpoint.
HW_BREAKPOINT_R
Count when we read the memory location.
HW_BREAKPOINT_W
Count when we write the memory location.
HW_BREAKPOINT_RW
Count when we read or write the memory location.
HW_BREAKPOINT_X
Count when we execute code at the memory location.
The values can be combined via a bitwise or, but the combination
of HW_BREAKPOINT_R or HW_BREAKPOINT_W with HW_BREAKPOINT_X is
not allowed.
so I think removing this enum from userspace is OK. Did I miss
something?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
On Mon, Sep 14, 2015 at 03:50:43PM -0700, Palmer Dabbelt wrote:
> This has a "#ifdef CONFIG_*" that used to be exposed to userspace.
>
> The names in here are so generic that I don't think it's a good idea
> to expose them to userspace (or even the rest of the kernel). Since
> there's only one kernel user, it's been moved to that file.
>
> Signed-off-by: Palmer Dabbelt
> Reviewed-by: Andrew Waterman
> Reviewed-by: Albert Ou
> ---
> include/uapi/linux/hw_breakpoint.h | 10 --
> kernel/events/hw_breakpoint.c | 10 ++
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/include/uapi/linux/hw_breakpoint.h
> b/include/uapi/linux/hw_breakpoint.h
> index b04000a2296a..7a6a5a7f9511 100644
> --- a/include/uapi/linux/hw_breakpoint.h
> +++ b/include/uapi/linux/hw_breakpoint.h
> @@ -17,14 +17,4 @@ enum {
> HW_BREAKPOINT_INVALID = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
> };
>
> -enum bp_type_idx {
> - TYPE_INST = 0,
> -#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
> - TYPE_DATA = 0,
> -#else
> - TYPE_DATA = 1,
> -#endif
> - TYPE_MAX
> -};
This is rather unfortunate; you are correct that the naming is too
generic (and I tend to agree), but I think these values are required by
userspace to fill out:
perf_event_attr::bp_type
So removing them will break things.
Frederic?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c
This has a "#ifdef CONFIG_*" that used to be exposed to userspace.
The names in here are so generic that I don't think it's a good idea
to expose them to userspace (or even the rest of the kernel). Since
there's only one kernel user, it's been moved to that file.
Signed-off-by: Palmer Dabbelt
Reviewed-by: Andrew Waterman
Reviewed-by: Albert Ou
---
include/uapi/linux/hw_breakpoint.h | 10 --
kernel/events/hw_breakpoint.c | 10 ++
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/include/uapi/linux/hw_breakpoint.h
b/include/uapi/linux/hw_breakpoint.h
index b04000a2296a..7a6a5a7f9511 100644
--- a/include/uapi/linux/hw_breakpoint.h
+++ b/include/uapi/linux/hw_breakpoint.h
@@ -17,14 +17,4 @@ enum {
HW_BREAKPOINT_INVALID = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
};
-enum bp_type_idx {
- TYPE_INST = 0,
-#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
- TYPE_DATA = 0,
-#else
- TYPE_DATA = 1,
-#endif
- TYPE_MAX
-};
-
#endif /* _UAPI_LINUX_HW_BREAKPOINT_H */
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 92ce5f4ccc26..5ad117e0f55b 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -58,6 +58,16 @@ struct bp_cpuinfo {
unsigned intflexible; /* XXX: placeholder, see fetch_this_slot() */
};
+enum bp_type_idx {
+ TYPE_INST = 0,
+#if defined(CONFIG_HAVE_MIXED_BREAKPOINTS_REGS)
+ TYPE_DATA = 0,
+#else
+ TYPE_DATA = 1,
+#endif
+ TYPE_MAX
+};
+
static DEFINE_PER_CPU(struct bp_cpuinfo, bp_cpuinfo[TYPE_MAX]);
static int nr_slots[TYPE_MAX];
--
2.4.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
