On Tue, Jul 21, 2020 at 1:57 PM Ravi Bangoria
<ravi.bango...@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.bango...@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.bango...@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 <pra...@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
