| Message ID | 20200717040958.70561-10-ravi.bangoria@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/watchpoint: Enable 2nd DAWR on baremetal and powervm | expand |
On Fri, Jul 17, 2020 at 2:11 PM Ravi Bangoria <ravi.bangoria@linux.ibm.com> wrote: > > So far Book3S Powerpc supported only one watchpoint. Power10 is > introducing 2nd DAWR. Enable 2nd DAWR support for Power10. > Availability of 2nd DAWR will depend on CPU_FTR_DAWR1. > > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> > --- > arch/powerpc/include/asm/cputable.h | 4 +++- > arch/powerpc/include/asm/hw_breakpoint.h | 5 +++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h > index 3445c86e1f6f..36a0851a7a9b 100644 > --- a/arch/powerpc/include/asm/cputable.h > +++ b/arch/powerpc/include/asm/cputable.h > @@ -633,7 +633,9 @@ enum { > * Maximum number of hw breakpoint supported on powerpc. Number of > * breakpoints supported by actual hw might be less than this. > */ > -#define HBP_NUM_MAX 1 > +#define HBP_NUM_MAX 2 > +#define HBP_NUM_ONE 1 > +#define HBP_NUM_TWO 2 I wonder if these defines are necessary - has it any advantage over just using the literal? > > #endif /* !__ASSEMBLY__ */ > > diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h > index cb424799da0d..d4eab1694bcd 100644 > --- a/arch/powerpc/include/asm/hw_breakpoint.h > +++ b/arch/powerpc/include/asm/hw_breakpoint.h > @@ -5,10 +5,11 @@ > * Copyright 2010, IBM Corporation. > * Author: K.Prasad <prasad@linux.vnet.ibm.com> > */ > - Was removing this line deliberate? > #ifndef _PPC_BOOK3S_64_HW_BREAKPOINT_H > #define _PPC_BOOK3S_64_HW_BREAKPOINT_H > > +#include <asm/cpu_has_feature.h> > + > #ifdef __KERNEL__ > struct arch_hw_breakpoint { > unsigned long address; > @@ -46,7 +47,7 @@ struct arch_hw_breakpoint { > > static inline int nr_wp_slots(void) > { > - return HBP_NUM_MAX; > + return cpu_has_feature(CPU_FTR_DAWR1) ? HBP_NUM_TWO : HBP_NUM_ONE; So it'd be something like: + return cpu_has_feature(CPU_FTR_DAWR1) ? HBP_NUM_MAX : 1; But thinking that there might be more slots added in the future, it may be better to make the number of slots a variable that is set during the init and then have this function return that. > } > > #ifdef CONFIG_HAVE_HW_BREAKPOINT > -- > 2.26.2 >
On 7/20/20 9:12 AM, Jordan Niethe wrote: > On Fri, Jul 17, 2020 at 2:11 PM Ravi Bangoria > <ravi.bangoria@linux.ibm.com> wrote: >> >> So far Book3S Powerpc supported only one watchpoint. Power10 is >> introducing 2nd DAWR. Enable 2nd DAWR support for Power10. >> Availability of 2nd DAWR will depend on CPU_FTR_DAWR1. >> >> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> >> --- >> arch/powerpc/include/asm/cputable.h | 4 +++- >> arch/powerpc/include/asm/hw_breakpoint.h | 5 +++-- >> 2 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h >> index 3445c86e1f6f..36a0851a7a9b 100644 >> --- a/arch/powerpc/include/asm/cputable.h >> +++ b/arch/powerpc/include/asm/cputable.h >> @@ -633,7 +633,9 @@ enum { >> * Maximum number of hw breakpoint supported on powerpc. Number of >> * breakpoints supported by actual hw might be less than this. >> */ >> -#define HBP_NUM_MAX 1 >> +#define HBP_NUM_MAX 2 >> +#define HBP_NUM_ONE 1 >> +#define HBP_NUM_TWO 2 > I wonder if these defines are necessary - has it any advantage over > just using the literal? No, not really. Initially I had something like: #define HBP_NUM_MAX 2 #define HBP_NUM_P8_P9 1 #define HBP_NUM_P10 2 But then I thought it's also not right. So I made it _ONE and _TWO. Now the function that decides nr watchpoints dynamically (nr_wp_slots) is in different file, I thought to keep it like this so it would be easier to figure out why _MAX is 2. >> >> #endif /* !__ASSEMBLY__ */ >> >> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h >> index cb424799da0d..d4eab1694bcd 100644 >> --- a/arch/powerpc/include/asm/hw_breakpoint.h >> +++ b/arch/powerpc/include/asm/hw_breakpoint.h >> @@ -5,10 +5,11 @@ >> * Copyright 2010, IBM Corporation. >> * Author: K.Prasad <prasad@linux.vnet.ibm.com> >> */ >> - > Was removing this line deliberate? Nah. Will remove that hunk. >> #ifndef _PPC_BOOK3S_64_HW_BREAKPOINT_H >> #define _PPC_BOOK3S_64_HW_BREAKPOINT_H >> >> +#include <asm/cpu_has_feature.h> >> + >> #ifdef __KERNEL__ >> struct arch_hw_breakpoint { >> unsigned long address; >> @@ -46,7 +47,7 @@ struct arch_hw_breakpoint { >> >> static inline int nr_wp_slots(void) >> { >> - return HBP_NUM_MAX; >> + return cpu_has_feature(CPU_FTR_DAWR1) ? HBP_NUM_TWO : HBP_NUM_ONE; > So it'd be something like: > + return cpu_has_feature(CPU_FTR_DAWR1) ? HBP_NUM_MAX : 1; > But thinking that there might be more slots added in the future, it > may be better to make the number of slots a variable that is set > during the init and then have this function return that. Not sure I follow. What do you mean by setting number of slots a variable that is set during the init? Thanks, Ravi
On Tue, Jul 21, 2020 at 1:57 PM Ravi Bangoria <ravi.bangoria@linux.ibm.com> wrote: > > > > On 7/20/20 9:12 AM, Jordan Niethe wrote: > > On Fri, Jul 17, 2020 at 2:11 PM Ravi Bangoria > > <ravi.bangoria@linux.ibm.com> wrote: > >> > >> So far Book3S Powerpc supported only one watchpoint. Power10 is > >> introducing 2nd DAWR. Enable 2nd DAWR support for Power10. > >> Availability of 2nd DAWR will depend on CPU_FTR_DAWR1. > >> > >> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> > >> --- > >> arch/powerpc/include/asm/cputable.h | 4 +++- > >> arch/powerpc/include/asm/hw_breakpoint.h | 5 +++-- > >> 2 files changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h > >> index 3445c86e1f6f..36a0851a7a9b 100644 > >> --- a/arch/powerpc/include/asm/cputable.h > >> +++ b/arch/powerpc/include/asm/cputable.h > >> @@ -633,7 +633,9 @@ enum { > >> * Maximum number of hw breakpoint supported on powerpc. Number of > >> * breakpoints supported by actual hw might be less than this. > >> */ > >> -#define HBP_NUM_MAX 1 > >> +#define HBP_NUM_MAX 2 > >> +#define HBP_NUM_ONE 1 > >> +#define HBP_NUM_TWO 2 > > I wonder if these defines are necessary - has it any advantage over > > just using the literal? > > No, not really. Initially I had something like: > > #define HBP_NUM_MAX 2 > #define HBP_NUM_P8_P9 1 > #define HBP_NUM_P10 2 > > But then I thought it's also not right. So I made it _ONE and _TWO. > Now the function that decides nr watchpoints dynamically (nr_wp_slots) > is in different file, I thought to keep it like this so it would be > easier to figure out why _MAX is 2. > > >> > >> #endif /* !__ASSEMBLY__ */ > >> > >> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h > >> index cb424799da0d..d4eab1694bcd 100644 > >> --- a/arch/powerpc/include/asm/hw_breakpoint.h > >> +++ b/arch/powerpc/include/asm/hw_breakpoint.h > >> @@ -5,10 +5,11 @@ > >> * Copyright 2010, IBM Corporation. > >> * Author: K.Prasad <prasad@linux.vnet.ibm.com> > >> */ > >> - > > Was removing this line deliberate? > > Nah. Will remove that hunk. > > >> #ifndef _PPC_BOOK3S_64_HW_BREAKPOINT_H > >> #define _PPC_BOOK3S_64_HW_BREAKPOINT_H > >> > >> +#include <asm/cpu_has_feature.h> > >> + > >> #ifdef __KERNEL__ > >> struct arch_hw_breakpoint { > >> unsigned long address; > >> @@ -46,7 +47,7 @@ struct arch_hw_breakpoint { > >> > >> static inline int nr_wp_slots(void) > >> { > >> - return HBP_NUM_MAX; > >> + return cpu_has_feature(CPU_FTR_DAWR1) ? HBP_NUM_TWO : HBP_NUM_ONE; > > So it'd be something like: > > + return cpu_has_feature(CPU_FTR_DAWR1) ? HBP_NUM_MAX : 1; > > But thinking that there might be more slots added in the future, it > > may be better to make the number of slots a variable that is set > > during the init and then have this function return that. > > Not sure I follow. What do you mean by setting number of slots a > variable that is set during the init? Sorry I was unclear there. I was just looking and saw arm also has a variable number of hw breakpoints. If we did something like how they handle it, it might look something like: static int num_wp_slots __ro_after_init; int nr_wp_slots(void) { return num_wp_slots; } static int __init arch_hw_breakpoint_init(void) { num_wp_slots = work out how many wp_slots } arch_initcall(arch_hw_breakpoint_init); Then we wouldn't have to calculate everytime nr_wp_slots() is called. In the future if more wp's are added nr_wp_slots() will get more complicated. But just an idea, feel free to ignore. > > Thanks, > Ravi
>>>> @@ -46,7 +47,7 @@ struct arch_hw_breakpoint { >>>> >>>> static inline int nr_wp_slots(void) >>>> { >>>> - return HBP_NUM_MAX; >>>> + return cpu_has_feature(CPU_FTR_DAWR1) ? HBP_NUM_TWO : HBP_NUM_ONE; >>> So it'd be something like: >>> + return cpu_has_feature(CPU_FTR_DAWR1) ? HBP_NUM_MAX : 1; >>> But thinking that there might be more slots added in the future, it >>> may be better to make the number of slots a variable that is set >>> during the init and then have this function return that. >> >> Not sure I follow. What do you mean by setting number of slots a >> variable that is set during the init? > Sorry I was unclear there. > I was just looking and saw arm also has a variable number of hw breakpoints. > If we did something like how they handle it, it might look something like: > > static int num_wp_slots __ro_after_init; > > int nr_wp_slots(void) { > return num_wp_slots; > } > > static int __init arch_hw_breakpoint_init(void) { > num_wp_slots = work out how many wp_slots > } > arch_initcall(arch_hw_breakpoint_init); > > Then we wouldn't have to calculate everytime nr_wp_slots() is called. > In the future if more wp's are added nr_wp_slots() will get more complicated. > But just an idea, feel free to ignore. Ok I got the idea. But ARM arch_hw_breakpoint_init() is much more complex compared to our nr_wp_slots(). I don't see any benefit by making our code like ARM. Thanks for the idea though :) Ravi
Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes: > On 7/20/20 9:12 AM, Jordan Niethe wrote: >> On Fri, Jul 17, 2020 at 2:11 PM Ravi Bangoria >> <ravi.bangoria@linux.ibm.com> wrote: >>> >>> So far Book3S Powerpc supported only one watchpoint. Power10 is >>> introducing 2nd DAWR. Enable 2nd DAWR support for Power10. >>> Availability of 2nd DAWR will depend on CPU_FTR_DAWR1. >>> >>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> >>> --- >>> arch/powerpc/include/asm/cputable.h | 4 +++- >>> arch/powerpc/include/asm/hw_breakpoint.h | 5 +++-- >>> 2 files changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h >>> index 3445c86e1f6f..36a0851a7a9b 100644 >>> --- a/arch/powerpc/include/asm/cputable.h >>> +++ b/arch/powerpc/include/asm/cputable.h >>> @@ -633,7 +633,9 @@ enum { >>> * Maximum number of hw breakpoint supported on powerpc. Number of >>> * breakpoints supported by actual hw might be less than this. >>> */ >>> -#define HBP_NUM_MAX 1 >>> +#define HBP_NUM_MAX 2 >>> +#define HBP_NUM_ONE 1 >>> +#define HBP_NUM_TWO 2 >> I wonder if these defines are necessary - has it any advantage over >> just using the literal? > > No, not really. Initially I had something like: > > #define HBP_NUM_MAX 2 > #define HBP_NUM_P8_P9 1 > #define HBP_NUM_P10 2 > > But then I thought it's also not right. So I made it _ONE and _TWO. > Now the function that decides nr watchpoints dynamically (nr_wp_slots) > is in different file, I thought to keep it like this so it would be > easier to figure out why _MAX is 2. I don't think it makes anything clearer. I had to stare at it thinking there was some sort of mapping or indirection going on, before I realised it's just literally the number of breakpoints. So please just do: static inline int nr_wp_slots(void) { return cpu_has_feature(CPU_FTR_DAWR1) ? 2 : 1; } If you think HBP_NUM_MAX needs explanation then do that with a comment, it can refer to nr_wp_slots() if that's helpful. cheers
On 7/21/20 5:06 PM, Michael Ellerman wrote: > Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes: >> On 7/20/20 9:12 AM, Jordan Niethe wrote: >>> On Fri, Jul 17, 2020 at 2:11 PM Ravi Bangoria >>> <ravi.bangoria@linux.ibm.com> wrote: >>>> >>>> So far Book3S Powerpc supported only one watchpoint. Power10 is >>>> introducing 2nd DAWR. Enable 2nd DAWR support for Power10. >>>> Availability of 2nd DAWR will depend on CPU_FTR_DAWR1. >>>> >>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> >>>> --- >>>> arch/powerpc/include/asm/cputable.h | 4 +++- >>>> arch/powerpc/include/asm/hw_breakpoint.h | 5 +++-- >>>> 2 files changed, 6 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h >>>> index 3445c86e1f6f..36a0851a7a9b 100644 >>>> --- a/arch/powerpc/include/asm/cputable.h >>>> +++ b/arch/powerpc/include/asm/cputable.h >>>> @@ -633,7 +633,9 @@ enum { >>>> * Maximum number of hw breakpoint supported on powerpc. Number of >>>> * breakpoints supported by actual hw might be less than this. >>>> */ >>>> -#define HBP_NUM_MAX 1 >>>> +#define HBP_NUM_MAX 2 >>>> +#define HBP_NUM_ONE 1 >>>> +#define HBP_NUM_TWO 2 > >>> I wonder if these defines are necessary - has it any advantage over >>> just using the literal? >> >> No, not really. Initially I had something like: >> >> #define HBP_NUM_MAX 2 >> #define HBP_NUM_P8_P9 1 >> #define HBP_NUM_P10 2 >> >> But then I thought it's also not right. So I made it _ONE and _TWO. >> Now the function that decides nr watchpoints dynamically (nr_wp_slots) >> is in different file, I thought to keep it like this so it would be >> easier to figure out why _MAX is 2. > > I don't think it makes anything clearer. > > I had to stare at it thinking there was some sort of mapping or > indirection going on, before I realised it's just literally the number > of breakpoints. > > So please just do: > > static inline int nr_wp_slots(void) > { > return cpu_has_feature(CPU_FTR_DAWR1) ? 2 : 1; > } > > If you think HBP_NUM_MAX needs explanation then do that with a comment, > it can refer to nr_wp_slots() if that's helpful. Agreed. By adding a comment, we can remove those macros. Will change it. Thanks, Ravi
diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h index 3445c86e1f6f..36a0851a7a9b 100644 --- a/arch/powerpc/include/asm/cputable.h +++ b/arch/powerpc/include/asm/cputable.h @@ -633,7 +633,9 @@ enum { * Maximum number of hw breakpoint supported on powerpc. Number of * breakpoints supported by actual hw might be less than this. */ -#define HBP_NUM_MAX 1 +#define HBP_NUM_MAX 2 +#define HBP_NUM_ONE 1 +#define HBP_NUM_TWO 2 #endif /* !__ASSEMBLY__ */ diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h index cb424799da0d..d4eab1694bcd 100644 --- a/arch/powerpc/include/asm/hw_breakpoint.h +++ b/arch/powerpc/include/asm/hw_breakpoint.h @@ -5,10 +5,11 @@ * Copyright 2010, IBM Corporation. * Author: K.Prasad <prasad@linux.vnet.ibm.com> */ - #ifndef _PPC_BOOK3S_64_HW_BREAKPOINT_H #define _PPC_BOOK3S_64_HW_BREAKPOINT_H +#include <asm/cpu_has_feature.h> + #ifdef __KERNEL__ struct arch_hw_breakpoint { unsigned long address; @@ -46,7 +47,7 @@ struct arch_hw_breakpoint { static inline int nr_wp_slots(void) { - return HBP_NUM_MAX; + return cpu_has_feature(CPU_FTR_DAWR1) ? HBP_NUM_TWO : HBP_NUM_ONE; } #ifdef CONFIG_HAVE_HW_BREAKPOINT
So far Book3S Powerpc supported only one watchpoint. Power10 is introducing 2nd DAWR. Enable 2nd DAWR support for Power10. Availability of 2nd DAWR will depend on CPU_FTR_DAWR1. Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> --- arch/powerpc/include/asm/cputable.h | 4 +++- arch/powerpc/include/asm/hw_breakpoint.h | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-)