Re: [PATCH v4 09/10] powerpc/watchpoint: Return available watchpoints dynamically

[Jordan Niethe]

On Tue, Jul 21, 2020 at 1:57 PM Ravi Bangoria
<ravi.bangoria@xxxxxxxxxxxxx> wrote:
>
>
>
> On 7/20/20 9:12 AM, Jordan Niethe wrote:
> > On Fri, Jul 17, 2020 at 2:11 PM Ravi Bangoria
> > <ravi.bangoria@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >> ---
> >>   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@xxxxxxxxxxxxxxxxxx>
> >>    */
> >> -
> > 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