[PATCH 00/15] powerpc/watchpoint: Preparation for more than one watchpoint

[PATCH 00/15] powerpc/watchpoint: Preparation for more than one watchpoint

[Ravi Bangoria]

So far, powerpc Book3S code has been written with an assumption of only
one watchpoint. But future power architecture is introducing second
watchpoint register (DAWR). Even though this patchset does not enable
2nd DAWR, it make the infrastructure ready so that enabling 2nd DAWR
should just be a matter of changing count.

Existing functionality works fine with the patchset. I've tested it with
perf, ptrace(gdb), xmon. All hw-breakpoint selftests are passing as well.
And I've build tested for 8xx.

Note: kvm or PowerVM geust is not enabled yet.

The series applies fine to powerpc/next plus one more dependency patch:
https://git.kernel.org/powerpc/c/e08658a657f974590809290c62e889f0fd420200

Ravi Bangoria (15):
  powerpc/watchpoint: Rename current DAWR macros
  powerpc/watchpoint: Add SPRN macros for second DAWR
  powerpc/watchpoint: Introduce function to get nr watchpoints
    dynamically
  powerpc/watchpoint/ptrace: Return actual num of available watchpoints
  powerpc/watchpoint: Provide DAWR number to set_dawr
  powerpc/watchpoint: Provide DAWR number to __set_breakpoint
  powerpc/watchpoint: Get watchpoint count dynamically while disabling
    them
  powerpc/watchpoint: Disable all available watchpoints when
    !dawr_force_enable
  powerpc/watchpoint: Convert thread_struct->hw_brk to an array
  powerpc/watchpoint: Use loop for thread_struct->ptrace_bps
  powerpc/watchpoint: Introduce is_ptrace_bp() function
  powerpc/watchpoint: Prepare handler to handle more than one
    watcnhpoint
  powerpc/watchpoint: Don't allow concurrent perf and ptrace events
  powerpc/watchpoint/xmon: Don't allow breakpoint overwriting
  powerpc/watchpoint/xmon: Support 2nd dawr

 arch/powerpc/include/asm/cputable.h      |   6 +-
 arch/powerpc/include/asm/debug.h         |   2 +-
 arch/powerpc/include/asm/hw_breakpoint.h |  23 +-
 arch/powerpc/include/asm/processor.h     |   6 +-
 arch/powerpc/include/asm/reg.h           |   6 +-
 arch/powerpc/include/asm/sstep.h         |   2 +
 arch/powerpc/kernel/dawr.c               |  23 +-
 arch/powerpc/kernel/hw_breakpoint.c      | 628 +++++++++++++++++++----
 arch/powerpc/kernel/process.c            |  66 ++-
 arch/powerpc/kernel/ptrace.c             |  72 ++-
 arch/powerpc/kernel/ptrace32.c           |   4 +-
 arch/powerpc/kernel/signal.c             |   9 +-
 arch/powerpc/kvm/book3s_hv.c             |  12 +-
 arch/powerpc/kvm/book3s_hv_rmhandlers.S  |  18 +-
 arch/powerpc/xmon/xmon.c                 |  99 ++--
 kernel/events/hw_breakpoint.c            |  16 +
 16 files changed, 793 insertions(+), 199 deletions(-)

-- 
2.21.1

[PATCH 01/15] powerpc/watchpoint: Rename current DAWR macros

[Ravi Bangoria]

Future Power architecture is introducing second DAWR. Rename current
DAWR macros as:
 s/SPRN_DAWR/SPRN_DAWR0/
 s/SPRN_DAWRX/SPRN_DAWRX0/

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/reg.h          |  4 ++--
 arch/powerpc/kernel/dawr.c              |  4 ++--
 arch/powerpc/kvm/book3s_hv.c            | 12 ++++++------
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 18 +++++++++---------
 arch/powerpc/xmon/xmon.c                |  2 +-
 5 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index da5cab038e25..156ee89fa9be 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -283,14 +283,14 @@
 #define   CTRL_CT1	0x40000000	/* thread 1 */
 #define   CTRL_TE	0x00c00000	/* thread enable */
 #define   CTRL_RUNLATCH	0x1
-#define SPRN_DAWR	0xB4
+#define SPRN_DAWR0	0xB4
 #define SPRN_RPR	0xBA	/* Relative Priority Register */
 #define SPRN_CIABR	0xBB
 #define   CIABR_PRIV		0x3
 #define   CIABR_PRIV_USER	1
 #define   CIABR_PRIV_SUPER	2
 #define   CIABR_PRIV_HYPER	3
-#define SPRN_DAWRX	0xBC
+#define SPRN_DAWRX0	0xBC
 #define   DAWRX_USER	__MASK(0)
 #define   DAWRX_KERNEL	__MASK(1)
 #define   DAWRX_HYP	__MASK(2)
diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
index cc14aa6c4a1b..e91b613bf137 100644
--- a/arch/powerpc/kernel/dawr.c
+++ b/arch/powerpc/kernel/dawr.c
@@ -39,8 +39,8 @@ int set_dawr(struct arch_hw_breakpoint *brk)
 	if (ppc_md.set_dawr)
 		return ppc_md.set_dawr(dawr, dawrx);
 
-	mtspr(SPRN_DAWR, dawr);
-	mtspr(SPRN_DAWRX, dawrx);
+	mtspr(SPRN_DAWR0, dawr);
+	mtspr(SPRN_DAWRX0, dawrx);
 
 	return 0;
 }
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 33be4d93248a..498c57e1f5c9 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -3383,8 +3383,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
 	int trap;
 	unsigned long host_hfscr = mfspr(SPRN_HFSCR);
 	unsigned long host_ciabr = mfspr(SPRN_CIABR);
-	unsigned long host_dawr = mfspr(SPRN_DAWR);
-	unsigned long host_dawrx = mfspr(SPRN_DAWRX);
+	unsigned long host_dawr = mfspr(SPRN_DAWR0);
+	unsigned long host_dawrx = mfspr(SPRN_DAWRX0);
 	unsigned long host_psscr = mfspr(SPRN_PSSCR);
 	unsigned long host_pidr = mfspr(SPRN_PID);
 
@@ -3413,8 +3413,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
 	mtspr(SPRN_SPURR, vcpu->arch.spurr);
 
 	if (dawr_enabled()) {
-		mtspr(SPRN_DAWR, vcpu->arch.dawr);
-		mtspr(SPRN_DAWRX, vcpu->arch.dawrx);
+		mtspr(SPRN_DAWR0, vcpu->arch.dawr);
+		mtspr(SPRN_DAWRX0, vcpu->arch.dawrx);
 	}
 	mtspr(SPRN_CIABR, vcpu->arch.ciabr);
 	mtspr(SPRN_IC, vcpu->arch.ic);
@@ -3466,8 +3466,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
 	      (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
 	mtspr(SPRN_HFSCR, host_hfscr);
 	mtspr(SPRN_CIABR, host_ciabr);
-	mtspr(SPRN_DAWR, host_dawr);
-	mtspr(SPRN_DAWRX, host_dawrx);
+	mtspr(SPRN_DAWR0, host_dawr);
+	mtspr(SPRN_DAWRX0, host_dawrx);
 	mtspr(SPRN_PID, host_pidr);
 
 	/*
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index dbc2fecc37f0..f4b412b7cad8 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -707,8 +707,8 @@ BEGIN_FTR_SECTION
 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
 BEGIN_FTR_SECTION
 	mfspr	r5, SPRN_CIABR
-	mfspr	r6, SPRN_DAWR
-	mfspr	r7, SPRN_DAWRX
+	mfspr	r6, SPRN_DAWR0
+	mfspr	r7, SPRN_DAWRX0
 	mfspr	r8, SPRN_IAMR
 	std	r5, STACK_SLOT_CIABR(r1)
 	std	r6, STACK_SLOT_DAWR(r1)
@@ -803,8 +803,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
 	beq	1f
 	ld	r5, VCPU_DAWR(r4)
 	ld	r6, VCPU_DAWRX(r4)
-	mtspr	SPRN_DAWR, r5
-	mtspr	SPRN_DAWRX, r6
+	mtspr	SPRN_DAWR0, r5
+	mtspr	SPRN_DAWRX0, r6
 1:
 	ld	r7, VCPU_CIABR(r4)
 	ld	r8, VCPU_TAR(r4)
@@ -1772,8 +1772,8 @@ BEGIN_FTR_SECTION
 	 * If the DAWR doesn't work, it's ok to write these here as
 	 * this value should always be zero
 	*/
-	mtspr	SPRN_DAWR, r6
-	mtspr	SPRN_DAWRX, r7
+	mtspr	SPRN_DAWR0, r6
+	mtspr	SPRN_DAWRX0, r7
 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 BEGIN_FTR_SECTION
 	ld	r5, STACK_SLOT_TID(r1)
@@ -2583,8 +2583,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 	mfmsr	r6
 	andi.	r6, r6, MSR_DR		/* in real mode? */
 	bne	4f
-	mtspr	SPRN_DAWR, r4
-	mtspr	SPRN_DAWRX, r5
+	mtspr	SPRN_DAWR0, r4
+	mtspr	SPRN_DAWRX0, r5
 4:	li	r3, 0
 	blr
 
@@ -3340,7 +3340,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
 	mtspr	SPRN_AMR, r0
 	mtspr	SPRN_IAMR, r0
 	mtspr	SPRN_CIABR, r0
-	mtspr	SPRN_DAWRX, r0
+	mtspr	SPRN_DAWRX0, r0
 
 BEGIN_MMU_FTR_SECTION
 	b	4f
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index e8c84d265602..6c4a8f8c0bd8 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1938,7 +1938,7 @@ static void dump_207_sprs(void)
 	printf("hfscr  = %.16lx  dhdes = %.16lx rpr    = %.16lx\n",
 		mfspr(SPRN_HFSCR), mfspr(SPRN_DHDES), mfspr(SPRN_RPR));
 	printf("dawr   = %.16lx  dawrx = %.16lx ciabr  = %.16lx\n",
-		mfspr(SPRN_DAWR), mfspr(SPRN_DAWRX), mfspr(SPRN_CIABR));
+		mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0), mfspr(SPRN_CIABR));
 #endif
 }
 
-- 
2.21.1

Re: [PATCH 01/15] powerpc/watchpoint: Rename current DAWR macros

[Christophe Leroy]

Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
> Future Power architecture is introducing second DAWR. Rename current
> DAWR macros as:
>   s/SPRN_DAWR/SPRN_DAWR0/
>   s/SPRN_DAWRX/SPRN_DAWRX0/

I think you should tell that DAWR0 and DAWRX0 is the real name of the 
register as documented in (at least) power8 and power9 user manual.

Otherwise, we can't understand why you change the name of the register.

Christophe

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/reg.h          |  4 ++--
>   arch/powerpc/kernel/dawr.c              |  4 ++--
>   arch/powerpc/kvm/book3s_hv.c            | 12 ++++++------
>   arch/powerpc/kvm/book3s_hv_rmhandlers.S | 18 +++++++++---------
>   arch/powerpc/xmon/xmon.c                |  2 +-
>   5 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index da5cab038e25..156ee89fa9be 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -283,14 +283,14 @@
>   #define   CTRL_CT1	0x40000000	/* thread 1 */
>   #define   CTRL_TE	0x00c00000	/* thread enable */
>   #define   CTRL_RUNLATCH	0x1
> -#define SPRN_DAWR	0xB4
> +#define SPRN_DAWR0	0xB4
>   #define SPRN_RPR	0xBA	/* Relative Priority Register */
>   #define SPRN_CIABR	0xBB
>   #define   CIABR_PRIV		0x3
>   #define   CIABR_PRIV_USER	1
>   #define   CIABR_PRIV_SUPER	2
>   #define   CIABR_PRIV_HYPER	3
> -#define SPRN_DAWRX	0xBC
> +#define SPRN_DAWRX0	0xBC
>   #define   DAWRX_USER	__MASK(0)
>   #define   DAWRX_KERNEL	__MASK(1)
>   #define   DAWRX_HYP	__MASK(2)
> diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
> index cc14aa6c4a1b..e91b613bf137 100644
> --- a/arch/powerpc/kernel/dawr.c
> +++ b/arch/powerpc/kernel/dawr.c
> @@ -39,8 +39,8 @@ int set_dawr(struct arch_hw_breakpoint *brk)
>   	if (ppc_md.set_dawr)
>   		return ppc_md.set_dawr(dawr, dawrx);
>   
> -	mtspr(SPRN_DAWR, dawr);
> -	mtspr(SPRN_DAWRX, dawrx);
> +	mtspr(SPRN_DAWR0, dawr);
> +	mtspr(SPRN_DAWRX0, dawrx);
>   
>   	return 0;
>   }
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 33be4d93248a..498c57e1f5c9 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3383,8 +3383,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>   	int trap;
>   	unsigned long host_hfscr = mfspr(SPRN_HFSCR);
>   	unsigned long host_ciabr = mfspr(SPRN_CIABR);
> -	unsigned long host_dawr = mfspr(SPRN_DAWR);
> -	unsigned long host_dawrx = mfspr(SPRN_DAWRX);
> +	unsigned long host_dawr = mfspr(SPRN_DAWR0);
> +	unsigned long host_dawrx = mfspr(SPRN_DAWRX0);
>   	unsigned long host_psscr = mfspr(SPRN_PSSCR);
>   	unsigned long host_pidr = mfspr(SPRN_PID);
>   
> @@ -3413,8 +3413,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>   	mtspr(SPRN_SPURR, vcpu->arch.spurr);
>   
>   	if (dawr_enabled()) {
> -		mtspr(SPRN_DAWR, vcpu->arch.dawr);
> -		mtspr(SPRN_DAWRX, vcpu->arch.dawrx);
> +		mtspr(SPRN_DAWR0, vcpu->arch.dawr);
> +		mtspr(SPRN_DAWRX0, vcpu->arch.dawrx);
>   	}
>   	mtspr(SPRN_CIABR, vcpu->arch.ciabr);
>   	mtspr(SPRN_IC, vcpu->arch.ic);
> @@ -3466,8 +3466,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>   	      (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
>   	mtspr(SPRN_HFSCR, host_hfscr);
>   	mtspr(SPRN_CIABR, host_ciabr);
> -	mtspr(SPRN_DAWR, host_dawr);
> -	mtspr(SPRN_DAWRX, host_dawrx);
> +	mtspr(SPRN_DAWR0, host_dawr);
> +	mtspr(SPRN_DAWRX0, host_dawrx);
>   	mtspr(SPRN_PID, host_pidr);
>   
>   	/*
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index dbc2fecc37f0..f4b412b7cad8 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -707,8 +707,8 @@ BEGIN_FTR_SECTION
>   END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
>   BEGIN_FTR_SECTION
>   	mfspr	r5, SPRN_CIABR
> -	mfspr	r6, SPRN_DAWR
> -	mfspr	r7, SPRN_DAWRX
> +	mfspr	r6, SPRN_DAWR0
> +	mfspr	r7, SPRN_DAWRX0
>   	mfspr	r8, SPRN_IAMR
>   	std	r5, STACK_SLOT_CIABR(r1)
>   	std	r6, STACK_SLOT_DAWR(r1)
> @@ -803,8 +803,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
>   	beq	1f
>   	ld	r5, VCPU_DAWR(r4)
>   	ld	r6, VCPU_DAWRX(r4)
> -	mtspr	SPRN_DAWR, r5
> -	mtspr	SPRN_DAWRX, r6
> +	mtspr	SPRN_DAWR0, r5
> +	mtspr	SPRN_DAWRX0, r6
>   1:
>   	ld	r7, VCPU_CIABR(r4)
>   	ld	r8, VCPU_TAR(r4)
> @@ -1772,8 +1772,8 @@ BEGIN_FTR_SECTION
>   	 * If the DAWR doesn't work, it's ok to write these here as
>   	 * this value should always be zero
>   	*/
> -	mtspr	SPRN_DAWR, r6
> -	mtspr	SPRN_DAWRX, r7
> +	mtspr	SPRN_DAWR0, r6
> +	mtspr	SPRN_DAWRX0, r7
>   END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>   BEGIN_FTR_SECTION
>   	ld	r5, STACK_SLOT_TID(r1)
> @@ -2583,8 +2583,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>   	mfmsr	r6
>   	andi.	r6, r6, MSR_DR		/* in real mode? */
>   	bne	4f
> -	mtspr	SPRN_DAWR, r4
> -	mtspr	SPRN_DAWRX, r5
> +	mtspr	SPRN_DAWR0, r4
> +	mtspr	SPRN_DAWRX0, r5
>   4:	li	r3, 0
>   	blr
>   
> @@ -3340,7 +3340,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
>   	mtspr	SPRN_AMR, r0
>   	mtspr	SPRN_IAMR, r0
>   	mtspr	SPRN_CIABR, r0
> -	mtspr	SPRN_DAWRX, r0
> +	mtspr	SPRN_DAWRX0, r0
>   
>   BEGIN_MMU_FTR_SECTION
>   	b	4f
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index e8c84d265602..6c4a8f8c0bd8 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -1938,7 +1938,7 @@ static void dump_207_sprs(void)
>   	printf("hfscr  = %.16lx  dhdes = %.16lx rpr    = %.16lx\n",
>   		mfspr(SPRN_HFSCR), mfspr(SPRN_DHDES), mfspr(SPRN_RPR));
>   	printf("dawr   = %.16lx  dawrx = %.16lx ciabr  = %.16lx\n",
> -		mfspr(SPRN_DAWR), mfspr(SPRN_DAWRX), mfspr(SPRN_CIABR));
> +		mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0), mfspr(SPRN_CIABR));
>   #endif
>   }
>   
> 

Re: [PATCH 01/15] powerpc/watchpoint: Rename current DAWR macros

[Ravi Bangoria]

On 3/17/20 3:44 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>> Future Power architecture is introducing second DAWR. Rename current
>> DAWR macros as:
>>   s/SPRN_DAWR/SPRN_DAWR0/
>>   s/SPRN_DAWRX/SPRN_DAWRX0/
> 
> I think you should tell that DAWR0 and DAWRX0 is the real name of the register as documented in (at least) power8 and power9 user manual.
> 
> Otherwise, we can't understand why you change the name of the register.

Ok.

Thanks,
Ravi

[PATCH 02/15] powerpc/watchpoint: Add SPRN macros for second DAWR

[Ravi Bangoria]

Future Power architecture is introducing second DAWR. Add SPRN_ macros
for the same.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/reg.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 156ee89fa9be..062e74cf41fd 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -284,6 +284,7 @@
 #define   CTRL_TE	0x00c00000	/* thread enable */
 #define   CTRL_RUNLATCH	0x1
 #define SPRN_DAWR0	0xB4
+#define SPRN_DAWR1	0xB5
 #define SPRN_RPR	0xBA	/* Relative Priority Register */
 #define SPRN_CIABR	0xBB
 #define   CIABR_PRIV		0x3
@@ -291,6 +292,7 @@
 #define   CIABR_PRIV_SUPER	2
 #define   CIABR_PRIV_HYPER	3
 #define SPRN_DAWRX0	0xBC
+#define SPRN_DAWRX1	0xBD
 #define   DAWRX_USER	__MASK(0)
 #define   DAWRX_KERNEL	__MASK(1)
 #define   DAWRX_HYP	__MASK(2)
-- 
2.21.1

Re: [PATCH 02/15] powerpc/watchpoint: Add SPRN macros for second DAWR

[Christophe Leroy]

Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
> Future Power architecture is introducing second DAWR. Add SPRN_ macros
> for the same.

I'm not sure this is called 'macros'. For me a macro is something more 
complex.

For me those are 'constants'.

Christophe

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/reg.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 156ee89fa9be..062e74cf41fd 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -284,6 +284,7 @@
>   #define   CTRL_TE	0x00c00000	/* thread enable */
>   #define   CTRL_RUNLATCH	0x1
>   #define SPRN_DAWR0	0xB4
> +#define SPRN_DAWR1	0xB5
>   #define SPRN_RPR	0xBA	/* Relative Priority Register */
>   #define SPRN_CIABR	0xBB
>   #define   CIABR_PRIV		0x3
> @@ -291,6 +292,7 @@
>   #define   CIABR_PRIV_SUPER	2
>   #define   CIABR_PRIV_HYPER	3
>   #define SPRN_DAWRX0	0xBC
> +#define SPRN_DAWRX1	0xBD
>   #define   DAWRX_USER	__MASK(0)
>   #define   DAWRX_KERNEL	__MASK(1)
>   #define   DAWRX_HYP	__MASK(2)
> 

Re: [PATCH 02/15] powerpc/watchpoint: Add SPRN macros for second DAWR

[Segher Boessenkool]

On Tue, Mar 17, 2020 at 11:16:34AM +0100, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
> >Future Power architecture is introducing second DAWR. Add SPRN_ macros
> >for the same.
> 
> I'm not sure this is called 'macros'. For me a macro is something more 
> complex.

It is called "macros" in the C standard, and in common usage as well.
"Object-like macros", as opposed to "function-like macros": there are
no arguments.

> For me those are 'constants'.

That would be more like "static const" in C since 1990 ;-)


Segher

[PATCH 03/15] powerpc/watchpoint: Introduce function to get nr watchpoints dynamically

[Ravi Bangoria]

So far we had only one watchpoint, so we have hardcoded HBP_NUM to 1.
But future Power architecture is introducing 2nd DAWR and thus kernel
should be able to dynamically find actual number of watchpoints
supported by hw it's running on. Introduce function for the same.
Also convert HBP_NUM macro to HBP_NUM_MAX, which will now represent
maximum number of watchpoints supported by Powerpc.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/cputable.h      | 6 +++++-
 arch/powerpc/include/asm/hw_breakpoint.h | 2 ++
 arch/powerpc/include/asm/processor.h     | 2 +-
 arch/powerpc/kernel/hw_breakpoint.c      | 2 +-
 arch/powerpc/kernel/process.c            | 6 ++++++
 5 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
index 40a4d3c6fd99..c67b94f3334c 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -614,7 +614,11 @@ enum {
 };
 #endif /* __powerpc64__ */
 
-#define HBP_NUM 1
+/*
+ * 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
 
 #endif /* !__ASSEMBLY__ */
 
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index f2f8d8aa8e3b..741c4f7573c4 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -43,6 +43,8 @@ struct arch_hw_breakpoint {
 #define DABR_MAX_LEN	8
 #define DAWR_MAX_LEN	512
 
+extern int nr_wp_slots(void);
+
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 #include <linux/kdebug.h>
 #include <asm/reg.h>
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 8387698bd5b6..666b2825278c 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -176,7 +176,7 @@ struct thread_struct {
 	int		fpexc_mode;	/* floating-point exception mode */
 	unsigned int	align_ctl;	/* alignment handling control */
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
-	struct perf_event *ptrace_bps[HBP_NUM];
+	struct perf_event *ptrace_bps[HBP_NUM_MAX];
 	/*
 	 * Helps identify source of single-step exception and subsequent
 	 * hw-breakpoint enablement
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index d0854320bb50..e68798cee3fa 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -38,7 +38,7 @@ static DEFINE_PER_CPU(struct perf_event *, bp_per_reg);
 int hw_breakpoint_slots(int type)
 {
 	if (type == TYPE_DATA)
-		return HBP_NUM;
+		return nr_wp_slots();
 	return 0;		/* no instruction breakpoints available */
 }
 
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 110db94cdf3c..6d4b029532e2 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -835,6 +835,12 @@ static inline bool hw_brk_match(struct arch_hw_breakpoint *a,
 	return true;
 }
 
+/* Returns total number of data breakpoints available. */
+int nr_wp_slots(void)
+{
+	return HBP_NUM_MAX;
+}
+
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 
 static inline bool tm_enabled(struct task_struct *tsk)
-- 
2.21.1

Re: [PATCH 03/15] powerpc/watchpoint: Introduce function to get nr watchpoints dynamically

[Christophe Leroy]

Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
> So far we had only one watchpoint, so we have hardcoded HBP_NUM to 1.
> But future Power architecture is introducing 2nd DAWR and thus kernel
> should be able to dynamically find actual number of watchpoints
> supported by hw it's running on. Introduce function for the same.
> Also convert HBP_NUM macro to HBP_NUM_MAX, which will now represent
> maximum number of watchpoints supported by Powerpc.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/cputable.h      | 6 +++++-
>   arch/powerpc/include/asm/hw_breakpoint.h | 2 ++
>   arch/powerpc/include/asm/processor.h     | 2 +-
>   arch/powerpc/kernel/hw_breakpoint.c      | 2 +-
>   arch/powerpc/kernel/process.c            | 6 ++++++
>   5 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> index 40a4d3c6fd99..c67b94f3334c 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -614,7 +614,11 @@ enum {
>   };
>   #endif /* __powerpc64__ */
>   
> -#define HBP_NUM 1
> +/*
> + * 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
>   
>   #endif /* !__ASSEMBLY__ */
>   
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index f2f8d8aa8e3b..741c4f7573c4 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -43,6 +43,8 @@ struct arch_hw_breakpoint {
>   #define DABR_MAX_LEN	8
>   #define DAWR_MAX_LEN	512
>   
> +extern int nr_wp_slots(void);

'extern' keyword is unneeded and irrelevant here. Please remove it. Even 
checkpatch is unhappy 
(https://openpower.xyz/job/snowpatch/job/snowpatch-linux-checkpatch/12172//artifact/linux/checkpatch.log)


> +
>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
>   #include <linux/kdebug.h>
>   #include <asm/reg.h>
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index 8387698bd5b6..666b2825278c 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -176,7 +176,7 @@ struct thread_struct {
>   	int		fpexc_mode;	/* floating-point exception mode */
>   	unsigned int	align_ctl;	/* alignment handling control */
>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
> -	struct perf_event *ptrace_bps[HBP_NUM];
> +	struct perf_event *ptrace_bps[HBP_NUM_MAX];
>   	/*
>   	 * Helps identify source of single-step exception and subsequent
>   	 * hw-breakpoint enablement
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index d0854320bb50..e68798cee3fa 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -38,7 +38,7 @@ static DEFINE_PER_CPU(struct perf_event *, bp_per_reg);
>   int hw_breakpoint_slots(int type)
>   {
>   	if (type == TYPE_DATA)
> -		return HBP_NUM;
> +		return nr_wp_slots();
>   	return 0;		/* no instruction breakpoints available */
>   }
>   
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 110db94cdf3c..6d4b029532e2 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -835,6 +835,12 @@ static inline bool hw_brk_match(struct arch_hw_breakpoint *a,
>   	return true;
>   }
>   
> +/* Returns total number of data breakpoints available. */
> +int nr_wp_slots(void)
> +{
> +	return HBP_NUM_MAX;
> +}
> +

This is not worth a global function. At least it should be a static 
function located in hw_breakpoint.c. But it would be even better to have 
it as a static inline in asm/hw_breakpoint.h

Christophe

Re: [PATCH 03/15] powerpc/watchpoint: Introduce function to get nr watchpoints dynamically

[Ravi Bangoria]

>> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
>> index f2f8d8aa8e3b..741c4f7573c4 100644
>> --- a/arch/powerpc/include/asm/hw_breakpoint.h
>> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
>> @@ -43,6 +43,8 @@ struct arch_hw_breakpoint {
>>   #define DABR_MAX_LEN    8
>>   #define DAWR_MAX_LEN    512
>> +extern int nr_wp_slots(void);
> 
> 'extern' keyword is unneeded and irrelevant here. Please remove it. Even checkpatch is unhappy (https://openpower.xyz/job/snowpatch/job/snowpatch-linux-checkpatch/12172//artifact/linux/checkpatch.log)

Sure.

...
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index 110db94cdf3c..6d4b029532e2 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -835,6 +835,12 @@ static inline bool hw_brk_match(struct arch_hw_breakpoint *a,
>>       return true;
>>   }
>> +/* Returns total number of data breakpoints available. */
>> +int nr_wp_slots(void)
>> +{
>> +    return HBP_NUM_MAX;
>> +}
>> +
> 
> This is not worth a global function. At least it should be a static function located in hw_breakpoint.c. But it would be even better to have it as a static inline in asm/hw_breakpoint.h

Makes sense. Will change it.

Thanks.

[PATCH 04/15] powerpc/watchpoint/ptrace: Return actual num of available watchpoints

[Ravi Bangoria]

User can ask for num of available watchpoints(dbginfo.num_data_bps)
using ptrace(PPC_PTRACE_GETHWDBGINFO). Return actual number of
available watchpoints on the machine rather than hardcoded 1.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/kernel/ptrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 25c0424e8868..dd46e174dbe7 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3074,7 +3074,7 @@ long arch_ptrace(struct task_struct *child, long request,
 #else /* !CONFIG_PPC_ADV_DEBUG_REGS */
 		dbginfo.num_instruction_bps = 0;
 		if (ppc_breakpoint_available())
-			dbginfo.num_data_bps = 1;
+			dbginfo.num_data_bps = nr_wp_slots();
 		else
 			dbginfo.num_data_bps = 0;
 		dbginfo.num_condition_regs = 0;
-- 
2.21.1

[PATCH 05/15] powerpc/watchpoint: Provide DAWR number to set_dawr

[Ravi Bangoria]

Introduce new parameter 'nr' to set_dawr() which indicates which DAWR
should be programed.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/hw_breakpoint.h |  4 ++--
 arch/powerpc/kernel/dawr.c               | 15 ++++++++++-----
 arch/powerpc/kernel/process.c            |  2 +-
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 741c4f7573c4..c4e797753895 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -101,10 +101,10 @@ static inline bool dawr_enabled(void)
 {
 	return dawr_force_enable;
 }
-int set_dawr(struct arch_hw_breakpoint *brk);
+int set_dawr(struct arch_hw_breakpoint *brk, int nr);
 #else
 static inline bool dawr_enabled(void) { return false; }
-static inline int set_dawr(struct arch_hw_breakpoint *brk) { return -1; }
+static inline int set_dawr(struct arch_hw_breakpoint *brk, int nr) { return -1; }
 #endif
 
 #endif	/* __KERNEL__ */
diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
index e91b613bf137..311e51ee09f4 100644
--- a/arch/powerpc/kernel/dawr.c
+++ b/arch/powerpc/kernel/dawr.c
@@ -16,7 +16,7 @@
 bool dawr_force_enable;
 EXPORT_SYMBOL_GPL(dawr_force_enable);
 
-int set_dawr(struct arch_hw_breakpoint *brk)
+int set_dawr(struct arch_hw_breakpoint *brk, int nr)
 {
 	unsigned long dawr, dawrx, mrd;
 
@@ -39,15 +39,20 @@ int set_dawr(struct arch_hw_breakpoint *brk)
 	if (ppc_md.set_dawr)
 		return ppc_md.set_dawr(dawr, dawrx);
 
-	mtspr(SPRN_DAWR0, dawr);
-	mtspr(SPRN_DAWRX0, dawrx);
+	if (nr == 0) {
+		mtspr(SPRN_DAWR0, dawr);
+		mtspr(SPRN_DAWRX0, dawrx);
+	} else {
+		mtspr(SPRN_DAWR1, dawr);
+		mtspr(SPRN_DAWRX1, dawrx);
+	}
 
 	return 0;
 }
 
 static void set_dawr_cb(void *info)
 {
-	set_dawr(info);
+	set_dawr(info, 0);
 }
 
 static ssize_t dawr_write_file_bool(struct file *file,
@@ -60,7 +65,7 @@ static ssize_t dawr_write_file_bool(struct file *file,
 	/* Send error to user if they hypervisor won't allow us to write DAWR */
 	if (!dawr_force_enable &&
 	    firmware_has_feature(FW_FEATURE_LPAR) &&
-	    set_dawr(&null_brk) != H_SUCCESS)
+	    set_dawr(&null_brk, 0) != H_SUCCESS)
 		return -ENODEV;
 
 	rc = debugfs_write_file_bool(file, user_buf, count, ppos);
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 6d4b029532e2..0657b3a3792a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -799,7 +799,7 @@ void __set_breakpoint(struct arch_hw_breakpoint *brk)
 
 	if (dawr_enabled())
 		// Power8 or later
-		set_dawr(brk);
+		set_dawr(brk, 0);
 	else if (IS_ENABLED(CONFIG_PPC_8xx))
 		set_breakpoint_8xx(brk);
 	else if (!cpu_has_feature(CPU_FTR_ARCH_207S))
-- 
2.21.1

Re: [PATCH 05/15] powerpc/watchpoint: Provide DAWR number to set_dawr

[Christophe Leroy]

Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
> Introduce new parameter 'nr' to set_dawr() which indicates which DAWR
> should be programed.

While we are at it (In another patch I think), we should do the same to 
set_dabr() so that we can use both DABR and DABR2

Christophe

Re: [PATCH 05/15] powerpc/watchpoint: Provide DAWR number to set_dawr

[Ravi Bangoria]

On 3/17/20 3:58 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>> Introduce new parameter 'nr' to set_dawr() which indicates which DAWR
>> should be programed.
> 
> While we are at it (In another patch I think), we should do the same to set_dabr() so that we can use both DABR and DABR2

This series is for DAWR only and does not support DABR2. I'll look
at how other book3s family processors provides DABR2 supports.

Thanks,
Ravi

[PATCH 06/15] powerpc/watchpoint: Provide DAWR number to __set_breakpoint

[Ravi Bangoria]

Introduce new parameter 'nr' to __set_breakpoint() which indicates
which DAWR should be programed. Also convert current_brk variable
to an array.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/debug.h         |  2 +-
 arch/powerpc/include/asm/hw_breakpoint.h |  2 +-
 arch/powerpc/kernel/hw_breakpoint.c      |  8 ++++----
 arch/powerpc/kernel/process.c            | 14 +++++++-------
 arch/powerpc/kernel/signal.c             |  2 +-
 arch/powerpc/xmon/xmon.c                 |  2 +-
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/debug.h b/arch/powerpc/include/asm/debug.h
index 7756026b95ca..6228935a8b64 100644
--- a/arch/powerpc/include/asm/debug.h
+++ b/arch/powerpc/include/asm/debug.h
@@ -45,7 +45,7 @@ static inline int debugger_break_match(struct pt_regs *regs) { return 0; }
 static inline int debugger_fault_handler(struct pt_regs *regs) { return 0; }
 #endif
 
-void __set_breakpoint(struct arch_hw_breakpoint *brk);
+void __set_breakpoint(struct arch_hw_breakpoint *brk, int nr);
 bool ppc_breakpoint_available(void);
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 extern void do_send_trap(struct pt_regs *regs, unsigned long address,
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index c4e797753895..980ac7d9f267 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -82,7 +82,7 @@ static inline void hw_breakpoint_disable(void)
 	brk.len = 0;
 	brk.hw_len = 0;
 	if (ppc_breakpoint_available())
-		__set_breakpoint(&brk);
+		__set_breakpoint(&brk, 0);
 }
 extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs);
 int hw_breakpoint_handler(struct die_args *args);
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index e68798cee3fa..f4d48f87dcb8 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -63,7 +63,7 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 	 * If so, DABR will be populated in single_step_dabr_instruction().
 	 */
 	if (current->thread.last_hit_ubp != bp)
-		__set_breakpoint(info);
+		__set_breakpoint(info, 0);
 
 	return 0;
 }
@@ -221,7 +221,7 @@ void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
 
 	info = counter_arch_bp(tsk->thread.last_hit_ubp);
 	regs->msr &= ~MSR_SE;
-	__set_breakpoint(info);
+	__set_breakpoint(info, 0);
 	tsk->thread.last_hit_ubp = NULL;
 }
 
@@ -346,7 +346,7 @@ int hw_breakpoint_handler(struct die_args *args)
 	if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
 		perf_bp_event(bp, regs);
 
-	__set_breakpoint(info);
+	__set_breakpoint(info, 0);
 out:
 	rcu_read_unlock();
 	return rc;
@@ -379,7 +379,7 @@ static int single_step_dabr_instruction(struct die_args *args)
 	if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
 		perf_bp_event(bp, regs);
 
-	__set_breakpoint(info);
+	__set_breakpoint(info, 0);
 	current->thread.last_hit_ubp = NULL;
 
 	/*
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 0657b3a3792a..f6bb2586fa5d 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -630,7 +630,7 @@ void do_break (struct pt_regs *regs, unsigned long address,
 }
 #endif	/* CONFIG_PPC_ADV_DEBUG_REGS */
 
-static DEFINE_PER_CPU(struct arch_hw_breakpoint, current_brk);
+static DEFINE_PER_CPU(struct arch_hw_breakpoint, current_brk[HBP_NUM_MAX]);
 
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 /*
@@ -707,7 +707,7 @@ EXPORT_SYMBOL_GPL(switch_booke_debug_regs);
 static void set_breakpoint(struct arch_hw_breakpoint *brk)
 {
 	preempt_disable();
-	__set_breakpoint(brk);
+	__set_breakpoint(brk, 0);
 	preempt_enable();
 }
 
@@ -793,13 +793,13 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
 	return 0;
 }
 
-void __set_breakpoint(struct arch_hw_breakpoint *brk)
+void __set_breakpoint(struct arch_hw_breakpoint *brk, int nr)
 {
-	memcpy(this_cpu_ptr(&current_brk), brk, sizeof(*brk));
+	memcpy(this_cpu_ptr(&current_brk[nr]), brk, sizeof(*brk));
 
 	if (dawr_enabled())
 		// Power8 or later
-		set_dawr(brk, 0);
+		set_dawr(brk, nr);
 	else if (IS_ENABLED(CONFIG_PPC_8xx))
 		set_breakpoint_8xx(brk);
 	else if (!cpu_has_feature(CPU_FTR_ARCH_207S))
@@ -1173,8 +1173,8 @@ struct task_struct *__switch_to(struct task_struct *prev,
  * schedule DABR
  */
 #ifndef CONFIG_HAVE_HW_BREAKPOINT
-	if (unlikely(!hw_brk_match(this_cpu_ptr(&current_brk), &new->thread.hw_brk)))
-		__set_breakpoint(&new->thread.hw_brk);
+	if (unlikely(!hw_brk_match(this_cpu_ptr(&current_brk[0]), &new->thread.hw_brk)))
+		__set_breakpoint(&new->thread.hw_brk, 0);
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #endif
 
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index e6c30cee6abf..8bc6cc55420a 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -129,7 +129,7 @@ static void do_signal(struct task_struct *tsk)
 	 * triggered inside the kernel.
 	 */
 	if (tsk->thread.hw_brk.address && tsk->thread.hw_brk.type)
-		__set_breakpoint(&tsk->thread.hw_brk);
+		__set_breakpoint(&tsk->thread.hw_brk, 0);
 #endif
 	/* Re-enable the breakpoints for the signal stack */
 	thread_change_pc(tsk, tsk->thread.regs);
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 6c4a8f8c0bd8..0ca0d29f99c6 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -934,7 +934,7 @@ static void insert_cpu_bpts(void)
 		brk.address = dabr.address;
 		brk.type = (dabr.enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
 		brk.len = DABR_MAX_LEN;
-		__set_breakpoint(&brk);
+		__set_breakpoint(&brk, 0);
 	}
 
 	if (iabr)
-- 
2.21.1

[PATCH 07/15] powerpc/watchpoint: Get watchpoint count dynamically while disabling them

[Ravi Bangoria]

Instead of disabling only one watchpooint, get num of available
watchpoints dynamically and disable all of them.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/hw_breakpoint.h | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 980ac7d9f267..ec61e2b7195c 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -75,14 +75,13 @@ extern void ptrace_triggered(struct perf_event *bp,
 			struct perf_sample_data *data, struct pt_regs *regs);
 static inline void hw_breakpoint_disable(void)
 {
-	struct arch_hw_breakpoint brk;
-
-	brk.address = 0;
-	brk.type = 0;
-	brk.len = 0;
-	brk.hw_len = 0;
-	if (ppc_breakpoint_available())
-		__set_breakpoint(&brk, 0);
+	int i;
+	struct arch_hw_breakpoint null_brk = {0};
+
+	if (ppc_breakpoint_available()) {
+		for (i = 0; i < nr_wp_slots(); i++)
+			__set_breakpoint(&null_brk, i);
+	}
 }
 extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs);
 int hw_breakpoint_handler(struct die_args *args);
-- 
2.21.1

Re: [PATCH 07/15] powerpc/watchpoint: Get watchpoint count dynamically while disabling them

[Christophe Leroy]

Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
> Instead of disabling only one watchpooint, get num of available
> watchpoints dynamically and disable all of them.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/hw_breakpoint.h | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index 980ac7d9f267..ec61e2b7195c 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -75,14 +75,13 @@ extern void ptrace_triggered(struct perf_event *bp,
>   			struct perf_sample_data *data, struct pt_regs *regs);
>   static inline void hw_breakpoint_disable(void)
>   {
> -	struct arch_hw_breakpoint brk;
> -
> -	brk.address = 0;
> -	brk.type = 0;
> -	brk.len = 0;
> -	brk.hw_len = 0;
> -	if (ppc_breakpoint_available())
> -		__set_breakpoint(&brk, 0);
> +	int i;
> +	struct arch_hw_breakpoint null_brk = {0};
> +
> +	if (ppc_breakpoint_available()) {

I think this test should go into nr_wp_slots() which should return zero 
when no breakpoint is available. This would simplify at least here and 
patch 4

Christophe

> +		for (i = 0; i < nr_wp_slots(); i++)
> +			__set_breakpoint(&null_brk, i);
> +	}
>   }
>   extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs);
>   int hw_breakpoint_handler(struct die_args *args);
> 

Re: [PATCH 07/15] powerpc/watchpoint: Get watchpoint count dynamically while disabling them

[Ravi Bangoria]

On 3/17/20 4:02 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>> Instead of disabling only one watchpooint, get num of available
>> watchpoints dynamically and disable all of them.
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/hw_breakpoint.h | 15 +++++++--------
>>   1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
>> index 980ac7d9f267..ec61e2b7195c 100644
>> --- a/arch/powerpc/include/asm/hw_breakpoint.h
>> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
>> @@ -75,14 +75,13 @@ extern void ptrace_triggered(struct perf_event *bp,
>>               struct perf_sample_data *data, struct pt_regs *regs);
>>   static inline void hw_breakpoint_disable(void)
>>   {
>> -    struct arch_hw_breakpoint brk;
>> -
>> -    brk.address = 0;
>> -    brk.type = 0;
>> -    brk.len = 0;
>> -    brk.hw_len = 0;
>> -    if (ppc_breakpoint_available())
>> -        __set_breakpoint(&brk, 0);
>> +    int i;
>> +    struct arch_hw_breakpoint null_brk = {0};
>> +
>> +    if (ppc_breakpoint_available()) {
> 
> I think this test should go into nr_wp_slots() which should return zero when no breakpoint is available.

Seems possible. Will change it in next version.

Thanks,
Ravi

Re: [PATCH 07/15] powerpc/watchpoint: Get watchpoint count dynamically while disabling them

[Ravi Bangoria]

On 3/18/20 12:27 PM, Ravi Bangoria wrote:
> 
> 
> On 3/17/20 4:02 PM, Christophe Leroy wrote:
>>
>>
>> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>>> Instead of disabling only one watchpooint, get num of available
>>> watchpoints dynamically and disable all of them.
>>>
>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>> ---
>>>   arch/powerpc/include/asm/hw_breakpoint.h | 15 +++++++--------
>>>   1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
>>> index 980ac7d9f267..ec61e2b7195c 100644
>>> --- a/arch/powerpc/include/asm/hw_breakpoint.h
>>> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
>>> @@ -75,14 +75,13 @@ extern void ptrace_triggered(struct perf_event *bp,
>>>               struct perf_sample_data *data, struct pt_regs *regs);
>>>   static inline void hw_breakpoint_disable(void)
>>>   {
>>> -    struct arch_hw_breakpoint brk;
>>> -
>>> -    brk.address = 0;
>>> -    brk.type = 0;
>>> -    brk.len = 0;
>>> -    brk.hw_len = 0;
>>> -    if (ppc_breakpoint_available())
>>> -        __set_breakpoint(&brk, 0);
>>> +    int i;
>>> +    struct arch_hw_breakpoint null_brk = {0};
>>> +
>>> +    if (ppc_breakpoint_available()) {
>>
>> I think this test should go into nr_wp_slots() which should return zero when no breakpoint is available.
> 
> Seems possible. Will change it in next version.

Once we move ppc_breakpoint_available() logic into nr_wp_slots(),
'dawr_force_enable' variable is checked in nr_wp_slots() before
it gets initialized in dawr_force_setup():

     start_kernel()
     |-> perf_event_init()
     |     init_hw_breakpoint()
     |       hw_breakpoint_slots()
     |         nr_wp_slots()
     |           /* Check dawr_force_enable variable */
     |
     |-> arch_call_rest_init()
           rest_init()
             kernel_thread(kernel_init, ...)
               ...
                 kernel_init()
                   kernel_init_freeable()
                     ...
                     do_one_initcall()
                       dawr_force_setup()
                         /* Set dawr_force_enable = true */

Because of this, hw-breakpoint infrastructure is initialized with
no DAWRs. So I'm thinking to keep the code as it is i.e. not moving
ppc_breakpoint_available() test inside nr_wp_slots().

Ravi

[PATCH 08/15] powerpc/watchpoint: Disable all available watchpoints when !dawr_force_enable

[Ravi Bangoria]

Instead of disabling only first watchpoint, disable all available
watchpoints while clearing dawr_force_enable.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/kernel/dawr.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
index 311e51ee09f4..5c882f07ac7d 100644
--- a/arch/powerpc/kernel/dawr.c
+++ b/arch/powerpc/kernel/dawr.c
@@ -50,9 +50,13 @@ int set_dawr(struct arch_hw_breakpoint *brk, int nr)
 	return 0;
 }
 
-static void set_dawr_cb(void *info)
+static void disable_dawrs(void *info)
 {
-	set_dawr(info, 0);
+	struct arch_hw_breakpoint null_brk = {0};
+	int i;
+
+	for (i = 0; i < nr_wp_slots(); i++)
+		set_dawr(&null_brk, i);
 }
 
 static ssize_t dawr_write_file_bool(struct file *file,
@@ -74,7 +78,7 @@ static ssize_t dawr_write_file_bool(struct file *file,
 
 	/* If we are clearing, make sure all CPUs have the DAWR cleared */
 	if (!dawr_force_enable)
-		smp_call_function(set_dawr_cb, &null_brk, 0);
+		smp_call_function(disable_dawrs, NULL, 0);
 
 	return rc;
 }
-- 
2.21.1

Re: [PATCH 08/15] powerpc/watchpoint: Disable all available watchpoints when !dawr_force_enable

[Christophe Leroy]

Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
> Instead of disabling only first watchpoint, disable all available
> watchpoints while clearing dawr_force_enable.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/kernel/dawr.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
> index 311e51ee09f4..5c882f07ac7d 100644
> --- a/arch/powerpc/kernel/dawr.c
> +++ b/arch/powerpc/kernel/dawr.c
> @@ -50,9 +50,13 @@ int set_dawr(struct arch_hw_breakpoint *brk, int nr)
>   	return 0;
>   }
>   
> -static void set_dawr_cb(void *info)
> +static void disable_dawrs(void *info)

Can you explain a bit more what you do exactly ? Why do you change the 
name of the function and why the parameter becomes NULL ? And why it 
doens't take into account the parameter anymore ?

>   {
> -	set_dawr(info, 0);
> +	struct arch_hw_breakpoint null_brk = {0};
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++)
> +		set_dawr(&null_brk, i);
>   }
>   
>   static ssize_t dawr_write_file_bool(struct file *file,
> @@ -74,7 +78,7 @@ static ssize_t dawr_write_file_bool(struct file *file,
>   
>   	/* If we are clearing, make sure all CPUs have the DAWR cleared */
>   	if (!dawr_force_enable)
> -		smp_call_function(set_dawr_cb, &null_brk, 0);
> +		smp_call_function(disable_dawrs, NULL, 0);
>   
>   	return rc;
>   }
> 

Christophe

Re: [PATCH 08/15] powerpc/watchpoint: Disable all available watchpoints when !dawr_force_enable

[Ravi Bangoria]

On 3/17/20 4:05 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>> Instead of disabling only first watchpoint, disable all available
>> watchpoints while clearing dawr_force_enable.
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> ---
>>   arch/powerpc/kernel/dawr.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
>> index 311e51ee09f4..5c882f07ac7d 100644
>> --- a/arch/powerpc/kernel/dawr.c
>> +++ b/arch/powerpc/kernel/dawr.c
>> @@ -50,9 +50,13 @@ int set_dawr(struct arch_hw_breakpoint *brk, int nr)
>>       return 0;
>>   }
>> -static void set_dawr_cb(void *info)
>> +static void disable_dawrs(void *info)
> 
> Can you explain a bit more what you do exactly ? Why do you change the name of the function and why the parameter becomes NULL ? And why it doens't take into account the parameter anymore ?

Before:
   static void set_dawr_cb(void *info)
   {
           set_dawr(info);
   }
   
   static ssize_t dawr_write_file_bool(...)
   {
   	...
           /* If we are clearing, make sure all CPUs have the DAWR cleared */
           if (!dawr_force_enable)
                   smp_call_function(set_dawr_cb, &null_brk, 0);
   }

After:
   static void disable_dawrs(void *info)
   {
           struct arch_hw_breakpoint null_brk = {0};
           int i;
   
           for (i = 0; i < nr_wp_slots(); i++)
                   set_dawr(&null_brk, i);
   }
   
   static ssize_t dawr_write_file_bool(...)
   {
   	...
           /* If we are clearing, make sure all CPUs have the DAWR cleared */
           if (!dawr_force_enable)
                   smp_call_function(disable_dawrs, NULL, 0);
   }

We use callback function only for disabling watchpoint. Thus I renamed
it to disable_dawrs(). And we are passing null_brk as parameter which
is not really required while disabling watchpoint. So removed it.

Thanks,
Ravi

[PATCH 09/15] powerpc/watchpoint: Convert thread_struct->hw_brk to an array

[Ravi Bangoria]

So far powerpc hw supported only one watchpoint. But Future Power
architecture is introducing 2nd DAWR. Convert thread_struct->hw_brk
into an array.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/processor.h |  2 +-
 arch/powerpc/kernel/process.c        | 43 ++++++++++++++++++++--------
 arch/powerpc/kernel/ptrace.c         | 42 ++++++++++++++++++++-------
 arch/powerpc/kernel/ptrace32.c       |  4 +--
 arch/powerpc/kernel/signal.c         |  9 ++++--
 5 files changed, 72 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 666b2825278c..57a8fac2e72b 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -183,7 +183,7 @@ struct thread_struct {
 	 */
 	struct perf_event *last_hit_ubp;
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
-	struct arch_hw_breakpoint hw_brk; /* info on the hardware breakpoint */
+	struct arch_hw_breakpoint hw_brk[HBP_NUM_MAX]; /* hardware breakpoint info */
 	unsigned long	trap_nr;	/* last trap # on this thread */
 	u8 load_slb;			/* Ages out SLB preload cache entries */
 	u8 load_fp;
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index f6bb2586fa5d..42ff62ef749c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -704,21 +704,25 @@ void switch_booke_debug_regs(struct debug_reg *new_debug)
 EXPORT_SYMBOL_GPL(switch_booke_debug_regs);
 #else	/* !CONFIG_PPC_ADV_DEBUG_REGS */
 #ifndef CONFIG_HAVE_HW_BREAKPOINT
-static void set_breakpoint(struct arch_hw_breakpoint *brk)
+static void set_breakpoint(struct arch_hw_breakpoint *brk, int i)
 {
 	preempt_disable();
-	__set_breakpoint(brk, 0);
+	__set_breakpoint(brk, i);
 	preempt_enable();
 }
 
 static void set_debug_reg_defaults(struct thread_struct *thread)
 {
-	thread->hw_brk.address = 0;
-	thread->hw_brk.type = 0;
-	thread->hw_brk.len = 0;
-	thread->hw_brk.hw_len = 0;
-	if (ppc_breakpoint_available())
-		set_breakpoint(&thread->hw_brk);
+	int i;
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		thread->hw_brk[i].address = 0;
+		thread->hw_brk[i].type = 0;
+		thread->hw_brk[i].len = 0;
+		thread->hw_brk[i].hw_len = 0;
+		if (ppc_breakpoint_available())
+			set_breakpoint(&thread->hw_brk[i], i);
+	}
 }
 #endif /* !CONFIG_HAVE_HW_BREAKPOINT */
 #endif	/* CONFIG_PPC_ADV_DEBUG_REGS */
@@ -1141,6 +1145,24 @@ static inline void restore_sprs(struct thread_struct *old_thread,
 	thread_pkey_regs_restore(new_thread, old_thread);
 }
 
+#ifndef CONFIG_HAVE_HW_BREAKPOINT
+static void switch_hw_breakpoint(struct task_struct *new)
+{
+	int i;
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (unlikely(!hw_brk_match(this_cpu_ptr(&current_brk[i]),
+					   &new->thread.hw_brk[i]))) {
+			__set_breakpoint(&new->thread.hw_brk[i], i);
+		}
+	}
+}
+#else
+static void switch_hw_breakpoint(struct task_struct *new)
+{
+}
+#endif
+
 struct task_struct *__switch_to(struct task_struct *prev,
 	struct task_struct *new)
 {
@@ -1172,10 +1194,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
  * For PPC_BOOK3S_64, we use the hw-breakpoint interfaces that would
  * schedule DABR
  */
-#ifndef CONFIG_HAVE_HW_BREAKPOINT
-	if (unlikely(!hw_brk_match(this_cpu_ptr(&current_brk[0]), &new->thread.hw_brk)))
-		__set_breakpoint(&new->thread.hw_brk, 0);
-#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+	switch_hw_breakpoint(new);
 #endif
 
 	/*
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index dd46e174dbe7..f6d7955fc61e 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2382,6 +2382,11 @@ void ptrace_triggered(struct perf_event *bp,
 }
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
+/*
+ * ptrace_set_debugreg() fakes DABR and DABR is only one. So even if
+ * internal hw supports more than one watchpoint, we support only one
+ * watchpoint with this interface.
+ */
 static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
@@ -2451,7 +2456,7 @@ static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			return ret;
 		}
 		thread->ptrace_bps[0] = bp;
-		thread->hw_brk = hw_brk;
+		thread->hw_brk[0] = hw_brk;
 		return 0;
 	}
 
@@ -2473,7 +2478,7 @@ static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 	if (set_bp && (!ppc_breakpoint_available()))
 		return -ENODEV;
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
-	task->thread.hw_brk = hw_brk;
+	task->thread.hw_brk[0] = hw_brk;
 #else /* CONFIG_PPC_ADV_DEBUG_REGS */
 	/* As described above, it was assumed 3 bits were passed with the data
 	 *  address, but we will assume only the mode bits will be passed
@@ -2824,9 +2829,23 @@ static int set_dac_range(struct task_struct *child,
 }
 #endif /* CONFIG_PPC_ADV_DEBUG_DAC_RANGE */
 
+#ifndef CONFIG_PPC_ADV_DEBUG_REGS
+static int empty_hw_brk(struct thread_struct *thread)
+{
+	int i;
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (!thread->hw_brk[i].address)
+			return i;
+	}
+	return -1;
+}
+#endif
+
 static long ppc_set_hwdebug(struct task_struct *child,
 		     struct ppc_hw_breakpoint *bp_info)
 {
+	int i;
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 	int len = 0;
 	struct thread_struct *thread = &(child->thread);
@@ -2919,15 +2938,16 @@ static long ppc_set_hwdebug(struct task_struct *child,
 	if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
 		return -EINVAL;
 
-	if (child->thread.hw_brk.address)
+	i = empty_hw_brk(&child->thread);
+	if (i < 0)
 		return -ENOSPC;
 
 	if (!ppc_breakpoint_available())
 		return -ENODEV;
 
-	child->thread.hw_brk = brk;
+	child->thread.hw_brk[i] = brk;
 
-	return 1;
+	return i + 1;
 #endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */
 }
 
@@ -2955,7 +2975,7 @@ static long ppc_del_hwdebug(struct task_struct *child, long data)
 	}
 	return rc;
 #else
-	if (data != 1)
+	if (data < 1 || data > nr_wp_slots())
 		return -EINVAL;
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
@@ -2967,11 +2987,11 @@ static long ppc_del_hwdebug(struct task_struct *child, long data)
 		ret = -ENOENT;
 	return ret;
 #else /* CONFIG_HAVE_HW_BREAKPOINT */
-	if (child->thread.hw_brk.address == 0)
+	if (child->thread.hw_brk[data - 1].address == 0)
 		return -ENOENT;
 
-	child->thread.hw_brk.address = 0;
-	child->thread.hw_brk.type = 0;
+	child->thread.hw_brk[data - 1].address = 0;
+	child->thread.hw_brk[data - 1].type = 0;
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	return 0;
@@ -3124,8 +3144,8 @@ long arch_ptrace(struct task_struct *child, long request,
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 		ret = put_user(child->thread.debug.dac1, datalp);
 #else
-		dabr_fake = ((child->thread.hw_brk.address & (~HW_BRK_TYPE_DABR)) |
-			     (child->thread.hw_brk.type & HW_BRK_TYPE_DABR));
+		dabr_fake = ((child->thread.hw_brk[0].address & (~HW_BRK_TYPE_DABR)) |
+			     (child->thread.hw_brk[0].type & HW_BRK_TYPE_DABR));
 		ret = put_user(dabr_fake, datalp);
 #endif
 		break;
diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c
index f37eb53de1a1..e227cd320b46 100644
--- a/arch/powerpc/kernel/ptrace32.c
+++ b/arch/powerpc/kernel/ptrace32.c
@@ -270,8 +270,8 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 		ret = put_user(child->thread.debug.dac1, (u32 __user *)data);
 #else
 		dabr_fake = (
-			(child->thread.hw_brk.address & (~HW_BRK_TYPE_DABR)) |
-			(child->thread.hw_brk.type & HW_BRK_TYPE_DABR));
+			(child->thread.hw_brk[0].address & (~HW_BRK_TYPE_DABR)) |
+			(child->thread.hw_brk[0].type & HW_BRK_TYPE_DABR));
 		ret = put_user(dabr_fake, (u32 __user *)data);
 #endif
 		break;
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index 8bc6cc55420a..3116896e89a6 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -107,6 +107,9 @@ static void do_signal(struct task_struct *tsk)
 	struct ksignal ksig = { .sig = 0 };
 	int ret;
 	int is32 = is_32bit_task();
+#ifndef CONFIG_PPC_ADV_DEBUG_REGS
+	int i;
+#endif
 
 	BUG_ON(tsk != current);
 
@@ -128,8 +131,10 @@ static void do_signal(struct task_struct *tsk)
 	 * user space. The DABR will have been cleared if it
 	 * triggered inside the kernel.
 	 */
-	if (tsk->thread.hw_brk.address && tsk->thread.hw_brk.type)
-		__set_breakpoint(&tsk->thread.hw_brk, 0);
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (tsk->thread.hw_brk[i].address && tsk->thread.hw_brk[i].type)
+			__set_breakpoint(&tsk->thread.hw_brk[i], i);
+	}
 #endif
 	/* Re-enable the breakpoints for the signal stack */
 	thread_change_pc(tsk, tsk->thread.regs);
-- 
2.21.1

Re: [PATCH 09/15] powerpc/watchpoint: Convert thread_struct->hw_brk to an array

[Christophe Leroy]

Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
> So far powerpc hw supported only one watchpoint. But Future Power
> architecture is introducing 2nd DAWR. Convert thread_struct->hw_brk
> into an array.

Looks like you are doing a lot more than that in this patch.

Should this patch be splitted in two parts ?

Christophe

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/processor.h |  2 +-
>   arch/powerpc/kernel/process.c        | 43 ++++++++++++++++++++--------
>   arch/powerpc/kernel/ptrace.c         | 42 ++++++++++++++++++++-------
>   arch/powerpc/kernel/ptrace32.c       |  4 +--
>   arch/powerpc/kernel/signal.c         |  9 ++++--
>   5 files changed, 72 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index 666b2825278c..57a8fac2e72b 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -183,7 +183,7 @@ struct thread_struct {
>   	 */
>   	struct perf_event *last_hit_ubp;
>   #endif /* CONFIG_HAVE_HW_BREAKPOINT */
> -	struct arch_hw_breakpoint hw_brk; /* info on the hardware breakpoint */
> +	struct arch_hw_breakpoint hw_brk[HBP_NUM_MAX]; /* hardware breakpoint info */
>   	unsigned long	trap_nr;	/* last trap # on this thread */
>   	u8 load_slb;			/* Ages out SLB preload cache entries */
>   	u8 load_fp;
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index f6bb2586fa5d..42ff62ef749c 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -704,21 +704,25 @@ void switch_booke_debug_regs(struct debug_reg *new_debug)
>   EXPORT_SYMBOL_GPL(switch_booke_debug_regs);
>   #else	/* !CONFIG_PPC_ADV_DEBUG_REGS */
>   #ifndef CONFIG_HAVE_HW_BREAKPOINT
> -static void set_breakpoint(struct arch_hw_breakpoint *brk)
> +static void set_breakpoint(struct arch_hw_breakpoint *brk, int i)
>   {
>   	preempt_disable();
> -	__set_breakpoint(brk, 0);
> +	__set_breakpoint(brk, i);
>   	preempt_enable();
>   }
>   
>   static void set_debug_reg_defaults(struct thread_struct *thread)
>   {
> -	thread->hw_brk.address = 0;
> -	thread->hw_brk.type = 0;
> -	thread->hw_brk.len = 0;
> -	thread->hw_brk.hw_len = 0;
> -	if (ppc_breakpoint_available())
> -		set_breakpoint(&thread->hw_brk);
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		thread->hw_brk[i].address = 0;
> +		thread->hw_brk[i].type = 0;
> +		thread->hw_brk[i].len = 0;
> +		thread->hw_brk[i].hw_len = 0;
> +		if (ppc_breakpoint_available())
> +			set_breakpoint(&thread->hw_brk[i], i);
> +	}
>   }
>   #endif /* !CONFIG_HAVE_HW_BREAKPOINT */
>   #endif	/* CONFIG_PPC_ADV_DEBUG_REGS */
> @@ -1141,6 +1145,24 @@ static inline void restore_sprs(struct thread_struct *old_thread,
>   	thread_pkey_regs_restore(new_thread, old_thread);
>   }
>   
> +#ifndef CONFIG_HAVE_HW_BREAKPOINT
> +static void switch_hw_breakpoint(struct task_struct *new)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (unlikely(!hw_brk_match(this_cpu_ptr(&current_brk[i]),
> +					   &new->thread.hw_brk[i]))) {
> +			__set_breakpoint(&new->thread.hw_brk[i], i);
> +		}
> +	}
> +}
> +#else
> +static void switch_hw_breakpoint(struct task_struct *new)
> +{
> +}
> +#endif
> +
>   struct task_struct *__switch_to(struct task_struct *prev,
>   	struct task_struct *new)
>   {
> @@ -1172,10 +1194,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
>    * For PPC_BOOK3S_64, we use the hw-breakpoint interfaces that would
>    * schedule DABR
>    */
> -#ifndef CONFIG_HAVE_HW_BREAKPOINT
> -	if (unlikely(!hw_brk_match(this_cpu_ptr(&current_brk[0]), &new->thread.hw_brk)))
> -		__set_breakpoint(&new->thread.hw_brk, 0);
> -#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> +	switch_hw_breakpoint(new);
>   #endif
>   
>   	/*
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index dd46e174dbe7..f6d7955fc61e 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -2382,6 +2382,11 @@ void ptrace_triggered(struct perf_event *bp,
>   }
>   #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>   
> +/*
> + * ptrace_set_debugreg() fakes DABR and DABR is only one. So even if
> + * internal hw supports more than one watchpoint, we support only one
> + * watchpoint with this interface.
> + */
>   static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
>   			       unsigned long data)
>   {
> @@ -2451,7 +2456,7 @@ static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
>   			return ret;
>   		}
>   		thread->ptrace_bps[0] = bp;
> -		thread->hw_brk = hw_brk;
> +		thread->hw_brk[0] = hw_brk;
>   		return 0;
>   	}
>   
> @@ -2473,7 +2478,7 @@ static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
>   	if (set_bp && (!ppc_breakpoint_available()))
>   		return -ENODEV;
>   #endif /* CONFIG_HAVE_HW_BREAKPOINT */
> -	task->thread.hw_brk = hw_brk;
> +	task->thread.hw_brk[0] = hw_brk;
>   #else /* CONFIG_PPC_ADV_DEBUG_REGS */
>   	/* As described above, it was assumed 3 bits were passed with the data
>   	 *  address, but we will assume only the mode bits will be passed
> @@ -2824,9 +2829,23 @@ static int set_dac_range(struct task_struct *child,
>   }
>   #endif /* CONFIG_PPC_ADV_DEBUG_DAC_RANGE */
>   
> +#ifndef CONFIG_PPC_ADV_DEBUG_REGS
> +static int empty_hw_brk(struct thread_struct *thread)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!thread->hw_brk[i].address)
> +			return i;
> +	}
> +	return -1;
> +}
> +#endif
> +
>   static long ppc_set_hwdebug(struct task_struct *child,
>   		     struct ppc_hw_breakpoint *bp_info)
>   {
> +	int i;
>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
>   	int len = 0;
>   	struct thread_struct *thread = &(child->thread);
> @@ -2919,15 +2938,16 @@ static long ppc_set_hwdebug(struct task_struct *child,
>   	if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
>   		return -EINVAL;
>   
> -	if (child->thread.hw_brk.address)
> +	i = empty_hw_brk(&child->thread);
> +	if (i < 0)
>   		return -ENOSPC;
>   
>   	if (!ppc_breakpoint_available())
>   		return -ENODEV;
>   
> -	child->thread.hw_brk = brk;
> +	child->thread.hw_brk[i] = brk;
>   
> -	return 1;
> +	return i + 1;
>   #endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */
>   }
>   
> @@ -2955,7 +2975,7 @@ static long ppc_del_hwdebug(struct task_struct *child, long data)
>   	}
>   	return rc;
>   #else
> -	if (data != 1)
> +	if (data < 1 || data > nr_wp_slots())
>   		return -EINVAL;
>   
>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
> @@ -2967,11 +2987,11 @@ static long ppc_del_hwdebug(struct task_struct *child, long data)
>   		ret = -ENOENT;
>   	return ret;
>   #else /* CONFIG_HAVE_HW_BREAKPOINT */
> -	if (child->thread.hw_brk.address == 0)
> +	if (child->thread.hw_brk[data - 1].address == 0)
>   		return -ENOENT;
>   
> -	child->thread.hw_brk.address = 0;
> -	child->thread.hw_brk.type = 0;
> +	child->thread.hw_brk[data - 1].address = 0;
> +	child->thread.hw_brk[data - 1].type = 0;
>   #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>   
>   	return 0;
> @@ -3124,8 +3144,8 @@ long arch_ptrace(struct task_struct *child, long request,
>   #ifdef CONFIG_PPC_ADV_DEBUG_REGS
>   		ret = put_user(child->thread.debug.dac1, datalp);
>   #else
> -		dabr_fake = ((child->thread.hw_brk.address & (~HW_BRK_TYPE_DABR)) |
> -			     (child->thread.hw_brk.type & HW_BRK_TYPE_DABR));
> +		dabr_fake = ((child->thread.hw_brk[0].address & (~HW_BRK_TYPE_DABR)) |
> +			     (child->thread.hw_brk[0].type & HW_BRK_TYPE_DABR));
>   		ret = put_user(dabr_fake, datalp);
>   #endif
>   		break;
> diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c
> index f37eb53de1a1..e227cd320b46 100644
> --- a/arch/powerpc/kernel/ptrace32.c
> +++ b/arch/powerpc/kernel/ptrace32.c
> @@ -270,8 +270,8 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
>   		ret = put_user(child->thread.debug.dac1, (u32 __user *)data);
>   #else
>   		dabr_fake = (
> -			(child->thread.hw_brk.address & (~HW_BRK_TYPE_DABR)) |
> -			(child->thread.hw_brk.type & HW_BRK_TYPE_DABR));
> +			(child->thread.hw_brk[0].address & (~HW_BRK_TYPE_DABR)) |
> +			(child->thread.hw_brk[0].type & HW_BRK_TYPE_DABR));
>   		ret = put_user(dabr_fake, (u32 __user *)data);
>   #endif
>   		break;
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index 8bc6cc55420a..3116896e89a6 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -107,6 +107,9 @@ static void do_signal(struct task_struct *tsk)
>   	struct ksignal ksig = { .sig = 0 };
>   	int ret;
>   	int is32 = is_32bit_task();
> +#ifndef CONFIG_PPC_ADV_DEBUG_REGS
> +	int i;
> +#endif
>   
>   	BUG_ON(tsk != current);
>   
> @@ -128,8 +131,10 @@ static void do_signal(struct task_struct *tsk)
>   	 * user space. The DABR will have been cleared if it
>   	 * triggered inside the kernel.
>   	 */
> -	if (tsk->thread.hw_brk.address && tsk->thread.hw_brk.type)
> -		__set_breakpoint(&tsk->thread.hw_brk, 0);
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (tsk->thread.hw_brk[i].address && tsk->thread.hw_brk[i].type)
> +			__set_breakpoint(&tsk->thread.hw_brk[i], i);
> +	}
>   #endif
>   	/* Re-enable the breakpoints for the signal stack */
>   	thread_change_pc(tsk, tsk->thread.regs);
> 

Re: [PATCH 09/15] powerpc/watchpoint: Convert thread_struct->hw_brk to an array

[Ravi Bangoria]

On 3/17/20 4:07 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> So far powerpc hw supported only one watchpoint. But Future Power
>> architecture is introducing 2nd DAWR. Convert thread_struct->hw_brk
>> into an array.
> 
> Looks like you are doing a lot more than that in this patch.
> 
> Should this patch be splitted in two parts ?

So far thread_struct->hw_brk was a normal variable so accessing it was simple.
Once it gets converted into an array, loop needs to be used to access it. So
all misc changes are basically converting simple access into loops.

I don't see how this can be splitted.

Thanks,
Ravi

Re: [PATCH 09/15] powerpc/watchpoint: Convert thread_struct->hw_brk to an array

[Christophe Leroy]

Le 18/03/2020 à 09:36, Ravi Bangoria a écrit :
> 
> 
> On 3/17/20 4:07 PM, Christophe Leroy wrote:
>>
>>
>> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>>> So far powerpc hw supported only one watchpoint. But Future Power
>>> architecture is introducing 2nd DAWR. Convert thread_struct->hw_brk
>>> into an array.
>>
>> Looks like you are doing a lot more than that in this patch.
>>
>> Should this patch be splitted in two parts ?
> 
> So far thread_struct->hw_brk was a normal variable so accessing it was 
> simple.
> Once it gets converted into an array, loop needs to be used to access 
> it. So
> all misc changes are basically converting simple access into loops.
> 
> I don't see how this can be splitted.
> 

You could first change all thread_struct->hw_brk to 
thread_struct->hw_brk[0] or thread_struct->hw_brk[i] when you know that 
i can only be 0 for now. Then add the loops and new functions in a 
second patch.

Christophe

Re: [PATCH 09/15] powerpc/watchpoint: Convert thread_struct->hw_brk to an array

[Ravi Bangoria]

On 3/18/20 2:26 PM, Christophe Leroy wrote:
> 
> 
> Le 18/03/2020 à 09:36, Ravi Bangoria a écrit :
>>
>>
>> On 3/17/20 4:07 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>>>> So far powerpc hw supported only one watchpoint. But Future Power
>>>> architecture is introducing 2nd DAWR. Convert thread_struct->hw_brk
>>>> into an array.
>>>
>>> Looks like you are doing a lot more than that in this patch.
>>>
>>> Should this patch be splitted in two parts ?
>>
>> So far thread_struct->hw_brk was a normal variable so accessing it was simple.
>> Once it gets converted into an array, loop needs to be used to access it. So
>> all misc changes are basically converting simple access into loops.
>>
>> I don't see how this can be splitted.
>>
> 
> You could first change all thread_struct->hw_brk to thread_struct->hw_brk[0] or thread_struct->hw_brk[i] when you know that i can only be 0 for now. Then add the loops and new functions in a second patch.

Ok. I've already tried that :) But it looked unnecessary split to
me because _all_ hw_brk => hw_brk[0] changes will again need to be
changed to use loop in 2nd patch. So I thought it's better to do
both changes in one patch.

Thanks,
Ravi

[PATCH 10/15] powerpc/watchpoint: Use loop for thread_struct->ptrace_bps

[Ravi Bangoria]

ptrace_bps is already an array of size HBP_NUM_MAX. But we use
hardcoded index 0 while fetching/updating it. Convert such code
to loop over array.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/kernel/hw_breakpoint.c |  7 +++++--
 arch/powerpc/kernel/process.c       |  6 +++++-
 arch/powerpc/kernel/ptrace.c        | 28 +++++++++++++++++++++-------
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index f4d48f87dcb8..b27aca623267 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -419,10 +419,13 @@ NOKPROBE_SYMBOL(hw_breakpoint_exceptions_notify);
  */
 void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
 {
+	int i;
 	struct thread_struct *t = &tsk->thread;
 
-	unregister_hw_breakpoint(t->ptrace_bps[0]);
-	t->ptrace_bps[0] = NULL;
+	for (i = 0; i < nr_wp_slots(); i++) {
+		unregister_hw_breakpoint(t->ptrace_bps[i]);
+		t->ptrace_bps[i] = NULL;
+	}
 }
 
 void hw_breakpoint_pmu_read(struct perf_event *bp)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 42ff62ef749c..b9ab740fcacf 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1628,6 +1628,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
 	void (*f)(void);
 	unsigned long sp = (unsigned long)task_stack_page(p) + THREAD_SIZE;
 	struct thread_info *ti = task_thread_info(p);
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	int i;
+#endif
 
 	klp_init_thread_info(p);
 
@@ -1687,7 +1690,8 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
 	p->thread.ksp_limit = (unsigned long)end_of_stack(p);
 #endif
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
-	p->thread.ptrace_bps[0] = NULL;
+	for (i = 0; i < nr_wp_slots(); i++)
+		p->thread.ptrace_bps[i] = NULL;
 #endif
 
 	p->thread.fp_save_area = NULL;
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index f6d7955fc61e..e2651f86d56f 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2829,6 +2829,19 @@ static int set_dac_range(struct task_struct *child,
 }
 #endif /* CONFIG_PPC_ADV_DEBUG_DAC_RANGE */
 
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+static int empty_ptrace_bp(struct thread_struct *thread)
+{
+	int i;
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (!thread->ptrace_bps[i])
+			return i;
+	}
+	return -1;
+}
+#endif
+
 #ifndef CONFIG_PPC_ADV_DEBUG_REGS
 static int empty_hw_brk(struct thread_struct *thread)
 {
@@ -2915,8 +2928,9 @@ static long ppc_set_hwdebug(struct task_struct *child,
 		len = 1;
 	else
 		return -EINVAL;
-	bp = thread->ptrace_bps[0];
-	if (bp)
+
+	i = empty_ptrace_bp(thread);
+	if (i < 0)
 		return -ENOSPC;
 
 	/* Create a new breakpoint request if one doesn't exist already */
@@ -2925,14 +2939,14 @@ static long ppc_set_hwdebug(struct task_struct *child,
 	attr.bp_len = len;
 	arch_bp_generic_fields(brk.type, &attr.bp_type);
 
-	thread->ptrace_bps[0] = bp = register_user_hw_breakpoint(&attr,
+	thread->ptrace_bps[i] = bp = register_user_hw_breakpoint(&attr,
 					       ptrace_triggered, NULL, child);
 	if (IS_ERR(bp)) {
-		thread->ptrace_bps[0] = NULL;
+		thread->ptrace_bps[i] = NULL;
 		return PTR_ERR(bp);
 	}
 
-	return 1;
+	return i + 1;
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
@@ -2979,10 +2993,10 @@ static long ppc_del_hwdebug(struct task_struct *child, long data)
 		return -EINVAL;
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
-	bp = thread->ptrace_bps[0];
+	bp = thread->ptrace_bps[data - 1];
 	if (bp) {
 		unregister_hw_breakpoint(bp);
-		thread->ptrace_bps[0] = NULL;
+		thread->ptrace_bps[data - 1] = NULL;
 	} else
 		ret = -ENOENT;
 	return ret;
-- 
2.21.1

Re: [PATCH 10/15] powerpc/watchpoint: Use loop for thread_struct->ptrace_bps

[Christophe Leroy]

Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
> ptrace_bps is already an array of size HBP_NUM_MAX. But we use
> hardcoded index 0 while fetching/updating it. Convert such code
> to loop over array.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/kernel/hw_breakpoint.c |  7 +++++--
>   arch/powerpc/kernel/process.c       |  6 +++++-
>   arch/powerpc/kernel/ptrace.c        | 28 +++++++++++++++++++++-------
>   3 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index f4d48f87dcb8..b27aca623267 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -419,10 +419,13 @@ NOKPROBE_SYMBOL(hw_breakpoint_exceptions_notify);
>    */
>   void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
>   {
> +	int i;
>   	struct thread_struct *t = &tsk->thread;
>   
> -	unregister_hw_breakpoint(t->ptrace_bps[0]);
> -	t->ptrace_bps[0] = NULL;
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		unregister_hw_breakpoint(t->ptrace_bps[i]);
> +		t->ptrace_bps[i] = NULL;
> +	}
>   }
>   
>   void hw_breakpoint_pmu_read(struct perf_event *bp)
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 42ff62ef749c..b9ab740fcacf 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1628,6 +1628,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
>   	void (*f)(void);
>   	unsigned long sp = (unsigned long)task_stack_page(p) + THREAD_SIZE;
>   	struct thread_info *ti = task_thread_info(p);
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	int i;
> +#endif

Could we avoid all those #ifdefs ?

I think if we make p->thread.ptrace_bps[] exist all the time, with a 
size of 0 when CONFIG_HAVE_HW_BREAKPOINT is not set, then we can drop a 
lot of #ifdefs.

>   
>   	klp_init_thread_info(p);
>   
> @@ -1687,7 +1690,8 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
>   	p->thread.ksp_limit = (unsigned long)end_of_stack(p);
>   #endif
>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
> -	p->thread.ptrace_bps[0] = NULL;
> +	for (i = 0; i < nr_wp_slots(); i++)
> +		p->thread.ptrace_bps[i] = NULL;
>   #endif
>   
>   	p->thread.fp_save_area = NULL;
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index f6d7955fc61e..e2651f86d56f 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c

You'll have to rebase all this on the series 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=161356 
which is about to go into powerpc-next

> @@ -2829,6 +2829,19 @@ static int set_dac_range(struct task_struct *child,
>   }
>   #endif /* CONFIG_PPC_ADV_DEBUG_DAC_RANGE */
>   
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +static int empty_ptrace_bp(struct thread_struct *thread)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!thread->ptrace_bps[i])
> +			return i;
> +	}
> +	return -1;
> +}
> +#endif

What does this function do exactly ? I seems to do more than what its 
name suggests.

> +
>   #ifndef CONFIG_PPC_ADV_DEBUG_REGS
>   static int empty_hw_brk(struct thread_struct *thread)
>   {
> @@ -2915,8 +2928,9 @@ static long ppc_set_hwdebug(struct task_struct *child,
>   		len = 1;
>   	else
>   		return -EINVAL;
> -	bp = thread->ptrace_bps[0];
> -	if (bp)
> +
> +	i = empty_ptrace_bp(thread);
> +	if (i < 0)
>   		return -ENOSPC;
>   
>   	/* Create a new breakpoint request if one doesn't exist already */
> @@ -2925,14 +2939,14 @@ static long ppc_set_hwdebug(struct task_struct *child,
>   	attr.bp_len = len;
>   	arch_bp_generic_fields(brk.type, &attr.bp_type);
>   
> -	thread->ptrace_bps[0] = bp = register_user_hw_breakpoint(&attr,
> +	thread->ptrace_bps[i] = bp = register_user_hw_breakpoint(&attr,
>   					       ptrace_triggered, NULL, child);
>   	if (IS_ERR(bp)) {
> -		thread->ptrace_bps[0] = NULL;
> +		thread->ptrace_bps[i] = NULL;
>   		return PTR_ERR(bp);
>   	}
>   
> -	return 1;
> +	return i + 1;
>   #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>   
>   	if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
> @@ -2979,10 +2993,10 @@ static long ppc_del_hwdebug(struct task_struct *child, long data)
>   		return -EINVAL;
>   
>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
> -	bp = thread->ptrace_bps[0];
> +	bp = thread->ptrace_bps[data - 1];

Is data checked somewhere to ensure it is not out of boundaries ? Or are 
we sure it is always within ?

>   	if (bp) {
>   		unregister_hw_breakpoint(bp);
> -		thread->ptrace_bps[0] = NULL;
> +		thread->ptrace_bps[data - 1] = NULL;
>   	} else
>   		ret = -ENOENT;
>   	return ret;
> 


Christophe

Re: [PATCH 10/15] powerpc/watchpoint: Use loop for thread_struct->ptrace_bps

[Ravi Bangoria]

>> @@ -1628,6 +1628,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
>>       void (*f)(void);
>>       unsigned long sp = (unsigned long)task_stack_page(p) + THREAD_SIZE;
>>       struct thread_info *ti = task_thread_info(p);
>> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
>> +    int i;
>> +#endif
> 
> Could we avoid all those #ifdefs ?
> 
> I think if we make p->thread.ptrace_bps[] exist all the time, with a size of 0 when CONFIG_HAVE_HW_BREAKPOINT is not set, then we can drop a lot of #ifdefs.

Hmm.. what you are saying seems possible. But IMO it should be done as
independent series. Will work on it.

> 
>>       klp_init_thread_info(p);
>> @@ -1687,7 +1690,8 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
>>       p->thread.ksp_limit = (unsigned long)end_of_stack(p);
>>   #endif
>>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
>> -    p->thread.ptrace_bps[0] = NULL;
>> +    for (i = 0; i < nr_wp_slots(); i++)
>> +        p->thread.ptrace_bps[i] = NULL;
>>   #endif
>>       p->thread.fp_save_area = NULL;
>> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
>> index f6d7955fc61e..e2651f86d56f 100644
>> --- a/arch/powerpc/kernel/ptrace.c
>> +++ b/arch/powerpc/kernel/ptrace.c
> 
> You'll have to rebase all this on the series https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=161356 which is about to go into powerpc-next

Sure. Thanks for heads up.

> 
>> @@ -2829,6 +2829,19 @@ static int set_dac_range(struct task_struct *child,
>>   }
>>   #endif /* CONFIG_PPC_ADV_DEBUG_DAC_RANGE */
>> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
>> +static int empty_ptrace_bp(struct thread_struct *thread)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < nr_wp_slots(); i++) {
>> +        if (!thread->ptrace_bps[i])
>> +            return i;
>> +    }
>> +    return -1;
>> +}
>> +#endif
> 
> What does this function do exactly ? I seems to do more than what its name suggests.

It finds an empty breakpoint in ptrace_bps[]. But yeah, function name is
misleading. I'll rename it to find_empty_ptrace_bp().

...

>> @@ -2979,10 +2993,10 @@ static long ppc_del_hwdebug(struct task_struct *child, long data)
>>           return -EINVAL;
>>   #ifdef CONFIG_HAVE_HW_BREAKPOINT
>> -    bp = thread->ptrace_bps[0];
>> +    bp = thread->ptrace_bps[data - 1];
> 
> Is data checked somewhere to ensure it is not out of boundaries ? Or are we sure it is always within ?

Yes. it's checked. See patch #9:

   @@ -2955,7 +2975,7 @@ static long ppc_del_hwdebug(struct task_struct *child, long data)
    	}
    	return rc;
    #else
   -	if (data != 1)
   +	if (data < 1 || data > nr_wp_slots())
    		return -EINVAL;
    
    #ifdef CONFIG_HAVE_HW_BREAKPOINT

Thanks,
Ravi

[PATCH 11/15] powerpc/watchpoint: Introduce is_ptrace_bp() function

[Ravi Bangoria]

Introduce is_ptrace_bp() function and move the check inside the
function. We will utilize it more in later set of patches.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/kernel/hw_breakpoint.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index b27aca623267..0e35ff372d8e 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -90,6 +90,11 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 	hw_breakpoint_disable();
 }
 
+static bool is_ptrace_bp(struct perf_event *bp)
+{
+	return (bp->overflow_handler == ptrace_triggered);
+}
+
 /*
  * Perform cleanup of arch-specific counters during unregistration
  * of the perf-event
@@ -324,7 +329,7 @@ int hw_breakpoint_handler(struct die_args *args)
 	 * one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
 	 * generated in do_dabr().
 	 */
-	if (bp->overflow_handler == ptrace_triggered) {
+	if (is_ptrace_bp(bp)) {
 		perf_bp_event(bp, regs);
 		rc = NOTIFY_DONE;
 		goto out;
-- 
2.21.1

Re: [PATCH 11/15] powerpc/watchpoint: Introduce is_ptrace_bp() function

[Christophe Leroy]

Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
> Introduce is_ptrace_bp() function and move the check inside the
> function. We will utilize it more in later set of patches.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/kernel/hw_breakpoint.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index b27aca623267..0e35ff372d8e 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -90,6 +90,11 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
>   	hw_breakpoint_disable();
>   }
>   
> +static bool is_ptrace_bp(struct perf_event *bp)
> +{
> +	return (bp->overflow_handler == ptrace_triggered);

You don't need parenthesis here.

> +}
> +
>   /*
>    * Perform cleanup of arch-specific counters during unregistration
>    * of the perf-event
> @@ -324,7 +329,7 @@ int hw_breakpoint_handler(struct die_args *args)
>   	 * one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
>   	 * generated in do_dabr().
>   	 */
> -	if (bp->overflow_handler == ptrace_triggered) {
> +	if (is_ptrace_bp(bp)) {
>   		perf_bp_event(bp, regs);
>   		rc = NOTIFY_DONE;
>   		goto out;
> 

Christophe

[PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

[Ravi Bangoria]

Currently we assume that we have only one watchpoint supported by hw.
Get rid of that assumption and use dynamic loop instead. This should
make supporting more watchpoints very easy.

So far, with only one watchpoint, the handler was simple. But with
multiple watchpoints, we need a mechanism to detect which watchpoint
caused the exception. HW doesn't provide that information and thus
we need software logic for this. This makes exception handling bit
more complex.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/processor.h |   2 +-
 arch/powerpc/include/asm/sstep.h     |   2 +
 arch/powerpc/kernel/hw_breakpoint.c  | 396 +++++++++++++++++++++------
 arch/powerpc/kernel/process.c        |   3 -
 4 files changed, 313 insertions(+), 90 deletions(-)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 57a8fac2e72b..1609ee75ee74 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -181,7 +181,7 @@ struct thread_struct {
 	 * Helps identify source of single-step exception and subsequent
 	 * hw-breakpoint enablement
 	 */
-	struct perf_event *last_hit_ubp;
+	struct perf_event *last_hit_ubp[HBP_NUM_MAX];
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 	struct arch_hw_breakpoint hw_brk[HBP_NUM_MAX]; /* hardware breakpoint info */
 	unsigned long	trap_nr;	/* last trap # on this thread */
diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 769f055509c9..38919b27a6fa 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -48,6 +48,8 @@ enum instruction_type {
 
 #define INSTR_TYPE_MASK	0x1f
 
+#define OP_IS_LOAD(type)	((LOAD <= (type) && (type) <= LOAD_VSX) || (type) == LARX)
+#define OP_IS_STORE(type)	((STORE <= (type) && (type) <= STORE_VSX) || (type) == STCX)
 #define OP_IS_LOAD_STORE(type)	(LOAD <= (type) && (type) <= STCX)
 
 /* Compute flags, ORed in with type */
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 0e35ff372d8e..2ac89b92590f 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -30,7 +30,7 @@
  * Stores the breakpoints currently in use on each breakpoint address
  * register for every cpu
  */
-static DEFINE_PER_CPU(struct perf_event *, bp_per_reg);
+static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
 
 /*
  * Returns total number of data or instruction breakpoints available.
@@ -42,6 +42,17 @@ int hw_breakpoint_slots(int type)
 	return 0;		/* no instruction breakpoints available */
 }
 
+static bool single_step_pending(void)
+{
+	int i;
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (current->thread.last_hit_ubp[i])
+			return true;
+	}
+	return false;
+}
+
 /*
  * Install a perf counter breakpoint.
  *
@@ -54,16 +65,26 @@ int hw_breakpoint_slots(int type)
 int arch_install_hw_breakpoint(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
-	struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
+	struct perf_event **slot;
+	int i;
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		slot = this_cpu_ptr(&bp_per_reg[i]);
+		if (!*slot) {
+			*slot = bp;
+			break;
+		}
+	}
 
-	*slot = bp;
+	if (WARN_ONCE(i == nr_wp_slots(), "Can't find any breakpoint slot"))
+		return -EBUSY;
 
 	/*
 	 * Do not install DABR values if the instruction must be single-stepped.
 	 * If so, DABR will be populated in single_step_dabr_instruction().
 	 */
-	if (current->thread.last_hit_ubp != bp)
-		__set_breakpoint(info, 0);
+	if (!single_step_pending())
+		__set_breakpoint(info, i);
 
 	return 0;
 }
@@ -79,15 +100,22 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
  */
 void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 {
-	struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
+	struct arch_hw_breakpoint null_brk = {0};
+	struct perf_event **slot;
+	int i;
 
-	if (*slot != bp) {
-		WARN_ONCE(1, "Can't find the breakpoint");
-		return;
+	for (i = 0; i < nr_wp_slots(); i++) {
+		slot = this_cpu_ptr(&bp_per_reg[i]);
+		if (*slot == bp) {
+			*slot = NULL;
+			break;
+		}
 	}
 
-	*slot = NULL;
-	hw_breakpoint_disable();
+	if (WARN_ONCE(i == nr_wp_slots(), "Can't find any breakpoint slot"))
+		return;
+
+	__set_breakpoint(&null_brk, i);
 }
 
 static bool is_ptrace_bp(struct perf_event *bp)
@@ -101,14 +129,20 @@ static bool is_ptrace_bp(struct perf_event *bp)
  */
 void arch_unregister_hw_breakpoint(struct perf_event *bp)
 {
+	int i;
+
 	/*
 	 * If the breakpoint is unregistered between a hw_breakpoint_handler()
 	 * and the single_step_dabr_instruction(), then cleanup the breakpoint
 	 * restoration variables to prevent dangling pointers.
 	 * FIXME, this should not be using bp->ctx at all! Sayeth peterz.
 	 */
-	if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L))
-		bp->ctx->task->thread.last_hit_ubp = NULL;
+	if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L)) {
+		for (i = 0; i < nr_wp_slots(); i++) {
+			if (bp->ctx->task->thread.last_hit_ubp[i] == bp)
+				bp->ctx->task->thread.last_hit_ubp[i] = NULL;
+		}
+	}
 }
 
 /*
@@ -220,90 +254,215 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
 {
 	struct arch_hw_breakpoint *info;
+	int i;
 
-	if (likely(!tsk->thread.last_hit_ubp))
-		return;
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (unlikely(tsk->thread.last_hit_ubp[i]))
+			goto reset;
+	}
+	return;
 
-	info = counter_arch_bp(tsk->thread.last_hit_ubp);
+reset:
 	regs->msr &= ~MSR_SE;
-	__set_breakpoint(info, 0);
-	tsk->thread.last_hit_ubp = NULL;
+	for (i = 0; i < nr_wp_slots(); i++) {
+		info = counter_arch_bp(__this_cpu_read(bp_per_reg[i]));
+		__set_breakpoint(info, i);
+		tsk->thread.last_hit_ubp[i] = NULL;
+	}
 }
 
-static bool dar_within_range(unsigned long dar, struct arch_hw_breakpoint *info)
+static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
 {
 	return ((info->address <= dar) && (dar - info->address < info->len));
 }
 
-static bool
-dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
+static bool dar_user_range_overlaps(unsigned long dar, int size,
+				    struct arch_hw_breakpoint *info)
 {
 	return ((dar <= info->address + info->len - 1) &&
 		(dar + size - 1 >= info->address));
 }
 
+static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
+{
+	unsigned long hw_start_addr, hw_end_addr;
+
+	hw_start_addr = info->address & ~HW_BREAKPOINT_ALIGN;
+	hw_end_addr =  hw_start_addr + info->hw_len - 1;
+
+	return ((hw_start_addr <= dar) && (hw_end_addr >= dar));
+}
+
+static bool dar_hw_range_overlaps(unsigned long dar, int size,
+				  struct arch_hw_breakpoint *info)
+{
+	unsigned long hw_start_addr, hw_end_addr;
+
+	hw_start_addr = info->address & ~HW_BREAKPOINT_ALIGN;
+	hw_end_addr =  hw_start_addr + info->hw_len - 1;
+
+	return ((dar <= hw_end_addr) && (dar + size - 1 >= hw_start_addr));
+}
+
 /*
- * Handle debug exception notifications.
+ * If hw has multiple DAWR registers, we also need to check all
+ * dawrx constraint bits to confirm this is _really_ a valid event.
  */
-static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
-			     struct arch_hw_breakpoint *info)
+static bool check_dawrx_constraints(struct pt_regs *regs, int type,
+				    struct arch_hw_breakpoint *info)
 {
-	unsigned int instr = 0;
-	int ret, type, size;
-	struct instruction_op op;
-	unsigned long addr = info->address;
+	if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
+		return false;
 
-	if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
-		goto fail;
+	if (OP_IS_STORE(type) && !(info->type & HW_BRK_TYPE_WRITE))
+		return false;
 
-	ret = analyse_instr(&op, regs, instr);
-	type = GETTYPE(op.type);
-	size = GETSIZE(op.type);
+	if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
+		return false;
 
-	if (!ret && (type == LARX || type == STCX)) {
-		printk_ratelimited("Breakpoint hit on instruction that can't be emulated."
-				   " Breakpoint at 0x%lx will be disabled.\n", addr);
-		goto disable;
-	}
+	if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
+		return false;
+
+	return true;
+}
+
+/*
+ * Returns true if the event is valid wrt dawr configuration,
+ * including extraneous exception. Otherwise return false.
+ */
+static bool check_constraints(struct pt_regs *regs, unsigned int instr,
+			      int type, int size,
+			      struct arch_hw_breakpoint *info)
+{
+	bool in_user_range = dar_in_user_range(regs->dar, info);
+	bool dawrx_constraints;
 
 	/*
-	 * If it's extraneous event, we still need to emulate/single-
-	 * step the instruction, but we don't generate an event.
+	 * 8xx supports only one breakpoint and thus we can
+	 * unconditionally return true.
 	 */
-	if (size && !dar_range_overlaps(regs->dar, size, info))
-		info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+	if (IS_ENABLED(CONFIG_PPC_8xx)) {
+		if (!in_user_range)
+			info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+		return true;
+	}
 
-	/* Do not emulate user-space instructions, instead single-step them */
-	if (user_mode(regs)) {
-		current->thread.last_hit_ubp = bp;
-		regs->msr |= MSR_SE;
+	if (unlikely(instr == -1)) {
+		if (in_user_range)
+			return true;
+
+		if (dar_in_hw_range(regs->dar, info)) {
+			info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+			return true;
+		}
 		return false;
 	}
 
-	if (!emulate_step(regs, instr))
-		goto fail;
+	dawrx_constraints = check_dawrx_constraints(regs, type, info);
 
-	return true;
+	if (dar_user_range_overlaps(regs->dar, size, info))
+		return dawrx_constraints;
+
+	if (dar_hw_range_overlaps(regs->dar, size, info)) {
+		if (dawrx_constraints) {
+			info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+			return true;
+		}
+	}
+	return false;
+}
+
+static int get_instr_detail(struct pt_regs *regs, int *type, int *size,
+			    bool *larx_stcx)
+{
+	unsigned int instr = 0;
+	struct instruction_op op;
+
+	if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
+		return -1;
+
+	analyse_instr(&op, regs, instr);
 
-fail:
 	/*
-	 * We've failed in reliably handling the hw-breakpoint. Unregister
-	 * it and throw a warning message to let the user know about it.
+	 * Set size = 8 if analyse_instr() fails. If it's a userspace
+	 * watchpoint(valid or extraneous), we can notify user about it.
+	 * If it's a kernel watchpoint, instruction  emulation will fail
+	 * in stepping_handler() and watchpoint will be disabled.
 	 */
-	WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
-		"0x%lx will be disabled.", addr);
+	*type = GETTYPE(op.type);
+	*size = !(*type == UNKNOWN) ? GETSIZE(op.type) : 8;
+	*larx_stcx = (*type == LARX || *type == STCX);
 
-disable:
+	return instr;
+}
+
+/*
+ * We've failed in reliably handling the hw-breakpoint. Unregister
+ * it and throw a warning message to let the user know about it.
+ */
+static void handler_error(struct perf_event *bp, struct arch_hw_breakpoint *info)
+{
+	WARN(1, "Unable to handle hardware breakpoint."
+		"Breakpoint at 0x%lx will be disabled.",
+		info->address);
+	perf_event_disable_inatomic(bp);
+}
+
+static void larx_stcx_err(struct perf_event *bp, struct arch_hw_breakpoint *info)
+{
+	printk_ratelimited("Breakpoint hit on instruction that can't "
+			   "be emulated. Breakpoint at 0x%lx will be "
+			   "disabled.\n", info->address);
 	perf_event_disable_inatomic(bp);
-	return false;
+}
+
+static bool stepping_handler(struct pt_regs *regs, struct perf_event **bp,
+			     struct arch_hw_breakpoint **info, int *hit,
+			     unsigned int instr)
+{
+	int i;
+	int stepped;
+
+	/* Do not emulate user-space instructions, instead single-step them */
+	if (user_mode(regs)) {
+		for (i = 0; i < nr_wp_slots(); i++) {
+			if (!hit[i])
+				continue;
+			current->thread.last_hit_ubp[i] = bp[i];
+			info[i] = NULL;
+		}
+		regs->msr |= MSR_SE;
+		return false;
+	}
+
+	stepped = emulate_step(regs, instr);
+	if (!stepped) {
+		for (i = 0; i < nr_wp_slots(); i++) {
+			if (!hit[i])
+				continue;
+			handler_error(bp[i], info[i]);
+			info[i] = NULL;
+		}
+		return false;
+	}
+	return true;
 }
 
 int hw_breakpoint_handler(struct die_args *args)
 {
+	bool err = false;
 	int rc = NOTIFY_STOP;
-	struct perf_event *bp;
+	struct perf_event *bp[HBP_NUM_MAX] = {0};
 	struct pt_regs *regs = args->regs;
-	struct arch_hw_breakpoint *info;
+	struct arch_hw_breakpoint *info[HBP_NUM_MAX] = {0};
+	int i;
+	int hit[HBP_NUM_MAX] = {0};
+	int nr_hit = 0;
+	bool ptrace_bp = false;
+	unsigned int instr = 0;
+	int type = 0;
+	int size = 0;
+	bool larx_stcx = false;
 
 	/* Disable breakpoints during exception handling */
 	hw_breakpoint_disable();
@@ -316,12 +475,39 @@ int hw_breakpoint_handler(struct die_args *args)
 	 */
 	rcu_read_lock();
 
-	bp = __this_cpu_read(bp_per_reg);
-	if (!bp) {
+	if (!IS_ENABLED(CONFIG_PPC_8xx))
+		instr = get_instr_detail(regs, &type, &size, &larx_stcx);
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		bp[i] = __this_cpu_read(bp_per_reg[i]);
+		if (!bp[i])
+			continue;
+
+		info[i] = counter_arch_bp(bp[i]);
+		info[i]->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
+
+		if (check_constraints(regs, instr, type, size, info[i])) {
+			if (!IS_ENABLED(CONFIG_PPC_8xx) && instr == -1) {
+				handler_error(bp[i], info[i]);
+				info[i] = NULL;
+				err = 1;
+				continue;
+			}
+
+			if (is_ptrace_bp(bp[i]))
+				ptrace_bp = true;
+			hit[i] = 1;
+			nr_hit++;
+		}
+	}
+
+	if (err)
+		goto reset;
+
+	if (!nr_hit) {
 		rc = NOTIFY_DONE;
 		goto out;
 	}
-	info = counter_arch_bp(bp);
 
 	/*
 	 * Return early after invoking user-callback function without restoring
@@ -329,29 +515,50 @@ int hw_breakpoint_handler(struct die_args *args)
 	 * one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
 	 * generated in do_dabr().
 	 */
-	if (is_ptrace_bp(bp)) {
-		perf_bp_event(bp, regs);
+	if (ptrace_bp) {
+		for (i = 0; i < nr_wp_slots(); i++) {
+			if (!hit[i])
+				continue;
+			perf_bp_event(bp[i], regs);
+			info[i] = NULL;
+		}
 		rc = NOTIFY_DONE;
-		goto out;
+		goto reset;
 	}
 
-	info->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
-	if (IS_ENABLED(CONFIG_PPC_8xx)) {
-		if (!dar_within_range(regs->dar, info))
-			info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
-	} else {
-		if (!stepping_handler(regs, bp, info))
-			goto out;
+	if (!IS_ENABLED(CONFIG_PPC_8xx)) {
+		if (larx_stcx) {
+			for (i = 0; i < nr_wp_slots(); i++) {
+				if (!hit[i])
+					continue;
+				larx_stcx_err(bp[i], info[i]);
+				info[i] = NULL;
+			}
+			goto reset;
+		}
+
+		if (!stepping_handler(regs, bp, info, hit, instr))
+			goto reset;
 	}
 
 	/*
 	 * As a policy, the callback is invoked in a 'trigger-after-execute'
 	 * fashion
 	 */
-	if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
-		perf_bp_event(bp, regs);
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (!hit[i])
+			continue;
+		if (!(info[i]->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
+			perf_bp_event(bp[i], regs);
+	}
+
+reset:
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (!info[i])
+			continue;
+		__set_breakpoint(info[i], i);
+	}
 
-	__set_breakpoint(info, 0);
 out:
 	rcu_read_unlock();
 	return rc;
@@ -366,26 +573,43 @@ static int single_step_dabr_instruction(struct die_args *args)
 	struct pt_regs *regs = args->regs;
 	struct perf_event *bp = NULL;
 	struct arch_hw_breakpoint *info;
+	int i;
+	bool found = false;
 
-	bp = current->thread.last_hit_ubp;
 	/*
 	 * Check if we are single-stepping as a result of a
 	 * previous HW Breakpoint exception
 	 */
-	if (!bp)
-		return NOTIFY_DONE;
+	for (i = 0; i < nr_wp_slots(); i++) {
+		bp = current->thread.last_hit_ubp[i];
+
+		if (!bp)
+			continue;
+
+		found = true;
+		info = counter_arch_bp(bp);
+
+		/*
+		 * We shall invoke the user-defined callback function in the
+		 * single stepping handler to confirm to 'trigger-after-execute'
+		 * semantics
+		 */
+		if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
+			perf_bp_event(bp, regs);
+		current->thread.last_hit_ubp[i] = NULL;
+	}
 
-	info = counter_arch_bp(bp);
+	if (!found)
+		return NOTIFY_DONE;
 
-	/*
-	 * We shall invoke the user-defined callback function in the single
-	 * stepping handler to confirm to 'trigger-after-execute' semantics
-	 */
-	if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
-		perf_bp_event(bp, regs);
+	for (i = 0; i < nr_wp_slots(); i++) {
+		bp = __this_cpu_read(bp_per_reg[i]);
+		if (!bp)
+			continue;
 
-	__set_breakpoint(info, 0);
-	current->thread.last_hit_ubp = NULL;
+		info = counter_arch_bp(bp);
+		__set_breakpoint(info, i);
+	}
 
 	/*
 	 * If the process was being single-stepped by ptrace, let the
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index b9ab740fcacf..c21bf367136b 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -622,9 +622,6 @@ void do_break (struct pt_regs *regs, unsigned long address,
 	if (debugger_break_match(regs))
 		return;
 
-	/* Clear the breakpoint */
-	hw_breakpoint_disable();
-
 	/* Deliver the signal to userspace */
 	force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
 }
-- 
2.21.1

Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

[Christophe Leroy]

Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
> Currently we assume that we have only one watchpoint supported by hw.
> Get rid of that assumption and use dynamic loop instead. This should
> make supporting more watchpoints very easy.

I think using 'we' is to be avoided in commit message.

Could be something like:

"Currently the handler assumes there is only one watchpoint supported by hw"

> 
> So far, with only one watchpoint, the handler was simple. But with
> multiple watchpoints, we need a mechanism to detect which watchpoint
> caused the exception. HW doesn't provide that information and thus
> we need software logic for this. This makes exception handling bit
> more complex.

Same here, the 'we' should be avoided.


This patch is pretty big. I think you should explain a bit more what is 
done. Otherwise it is difficult to review.

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/processor.h |   2 +-
>   arch/powerpc/include/asm/sstep.h     |   2 +
>   arch/powerpc/kernel/hw_breakpoint.c  | 396 +++++++++++++++++++++------
>   arch/powerpc/kernel/process.c        |   3 -
>   4 files changed, 313 insertions(+), 90 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index 57a8fac2e72b..1609ee75ee74 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -181,7 +181,7 @@ struct thread_struct {
>   	 * Helps identify source of single-step exception and subsequent
>   	 * hw-breakpoint enablement
>   	 */
> -	struct perf_event *last_hit_ubp;
> +	struct perf_event *last_hit_ubp[HBP_NUM_MAX];
>   #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>   	struct arch_hw_breakpoint hw_brk[HBP_NUM_MAX]; /* hardware breakpoint info */
>   	unsigned long	trap_nr;	/* last trap # on this thread */
> diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> index 769f055509c9..38919b27a6fa 100644
> --- a/arch/powerpc/include/asm/sstep.h
> +++ b/arch/powerpc/include/asm/sstep.h
> @@ -48,6 +48,8 @@ enum instruction_type {
>   
>   #define INSTR_TYPE_MASK	0x1f
>   
> +#define OP_IS_LOAD(type)	((LOAD <= (type) && (type) <= LOAD_VSX) || (type) == LARX)
> +#define OP_IS_STORE(type)	((STORE <= (type) && (type) <= STORE_VSX) || (type) == STCX)
>   #define OP_IS_LOAD_STORE(type)	(LOAD <= (type) && (type) <= STCX)
>   
>   /* Compute flags, ORed in with type */
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 0e35ff372d8e..2ac89b92590f 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -30,7 +30,7 @@
>    * Stores the breakpoints currently in use on each breakpoint address
>    * register for every cpu
>    */
> -static DEFINE_PER_CPU(struct perf_event *, bp_per_reg);
> +static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
>   
>   /*
>    * Returns total number of data or instruction breakpoints available.
> @@ -42,6 +42,17 @@ int hw_breakpoint_slots(int type)
>   	return 0;		/* no instruction breakpoints available */
>   }
>   
> +static bool single_step_pending(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (current->thread.last_hit_ubp[i])
> +			return true;
> +	}
> +	return false;
> +}
> +
>   /*
>    * Install a perf counter breakpoint.
>    *
> @@ -54,16 +65,26 @@ int hw_breakpoint_slots(int type)
>   int arch_install_hw_breakpoint(struct perf_event *bp)
>   {
>   	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> -	struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
> +	struct perf_event **slot;
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		slot = this_cpu_ptr(&bp_per_reg[i]);
> +		if (!*slot) {
> +			*slot = bp;
> +			break;
> +		}
> +	}
>   
> -	*slot = bp;
> +	if (WARN_ONCE(i == nr_wp_slots(), "Can't find any breakpoint slot"))
> +		return -EBUSY;
>   
>   	/*
>   	 * Do not install DABR values if the instruction must be single-stepped.
>   	 * If so, DABR will be populated in single_step_dabr_instruction().
>   	 */
> -	if (current->thread.last_hit_ubp != bp)
> -		__set_breakpoint(info, 0);
> +	if (!single_step_pending())
> +		__set_breakpoint(info, i);
>   
>   	return 0;
>   }
> @@ -79,15 +100,22 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
>    */
>   void arch_uninstall_hw_breakpoint(struct perf_event *bp)
>   {
> -	struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
> +	struct arch_hw_breakpoint null_brk = {0};
> +	struct perf_event **slot;
> +	int i;
>   
> -	if (*slot != bp) {
> -		WARN_ONCE(1, "Can't find the breakpoint");
> -		return;
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		slot = this_cpu_ptr(&bp_per_reg[i]);
> +		if (*slot == bp) {
> +			*slot = NULL;
> +			break;
> +		}
>   	}
>   
> -	*slot = NULL;
> -	hw_breakpoint_disable();
> +	if (WARN_ONCE(i == nr_wp_slots(), "Can't find any breakpoint slot"))
> +		return;
> +
> +	__set_breakpoint(&null_brk, i);
>   }
>   
>   static bool is_ptrace_bp(struct perf_event *bp)
> @@ -101,14 +129,20 @@ static bool is_ptrace_bp(struct perf_event *bp)
>    */
>   void arch_unregister_hw_breakpoint(struct perf_event *bp)
>   {
> +	int i;
> +
>   	/*
>   	 * If the breakpoint is unregistered between a hw_breakpoint_handler()
>   	 * and the single_step_dabr_instruction(), then cleanup the breakpoint
>   	 * restoration variables to prevent dangling pointers.
>   	 * FIXME, this should not be using bp->ctx at all! Sayeth peterz.
>   	 */
> -	if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L))
> -		bp->ctx->task->thread.last_hit_ubp = NULL;
> +	if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L)) {
> +		for (i = 0; i < nr_wp_slots(); i++) {
> +			if (bp->ctx->task->thread.last_hit_ubp[i] == bp)
> +				bp->ctx->task->thread.last_hit_ubp[i] = NULL;
> +		}
> +	}
>   }
>   
>   /*
> @@ -220,90 +254,215 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>   void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
>   {
>   	struct arch_hw_breakpoint *info;
> +	int i;
>   
> -	if (likely(!tsk->thread.last_hit_ubp))
> -		return;
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (unlikely(tsk->thread.last_hit_ubp[i]))
> +			goto reset;
> +	}
> +	return;
>   
> -	info = counter_arch_bp(tsk->thread.last_hit_ubp);
> +reset:
>   	regs->msr &= ~MSR_SE;
> -	__set_breakpoint(info, 0);
> -	tsk->thread.last_hit_ubp = NULL;
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		info = counter_arch_bp(__this_cpu_read(bp_per_reg[i]));
> +		__set_breakpoint(info, i);
> +		tsk->thread.last_hit_ubp[i] = NULL;
> +	}
>   }
>   
> -static bool dar_within_range(unsigned long dar, struct arch_hw_breakpoint *info)
> +static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)

Is this name change directly related to the patch ?

>   {
>   	return ((info->address <= dar) && (dar - info->address < info->len));
>   }
>   
> -static bool
> -dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
> +static bool dar_user_range_overlaps(unsigned long dar, int size,
> +				    struct arch_hw_breakpoint *info)

Same question.

>   {
>   	return ((dar <= info->address + info->len - 1) &&
>   		(dar + size - 1 >= info->address));
>   }
>   
> +static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
> +{
> +	unsigned long hw_start_addr, hw_end_addr;
> +
> +	hw_start_addr = info->address & ~HW_BREAKPOINT_ALIGN;
> +	hw_end_addr =  hw_start_addr + info->hw_len - 1;
> +
> +	return ((hw_start_addr <= dar) && (hw_end_addr >= dar));
> +}
> +
> +static bool dar_hw_range_overlaps(unsigned long dar, int size,
> +				  struct arch_hw_breakpoint *info)
> +{
> +	unsigned long hw_start_addr, hw_end_addr;
> +
> +	hw_start_addr = info->address & ~HW_BREAKPOINT_ALIGN;
> +	hw_end_addr =  hw_start_addr + info->hw_len - 1;
> +
> +	return ((dar <= hw_end_addr) && (dar + size - 1 >= hw_start_addr));
> +}
> +
>   /*
> - * Handle debug exception notifications.
> + * If hw has multiple DAWR registers, we also need to check all
> + * dawrx constraint bits to confirm this is _really_ a valid event.
>    */
> -static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
> -			     struct arch_hw_breakpoint *info)
> +static bool check_dawrx_constraints(struct pt_regs *regs, int type,
> +				    struct arch_hw_breakpoint *info)
>   {
> -	unsigned int instr = 0;
> -	int ret, type, size;
> -	struct instruction_op op;
> -	unsigned long addr = info->address;
> +	if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
> +		return false;
>   
> -	if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
> -		goto fail;
> +	if (OP_IS_STORE(type) && !(info->type & HW_BRK_TYPE_WRITE))
> +		return false;
>   
> -	ret = analyse_instr(&op, regs, instr);
> -	type = GETTYPE(op.type);
> -	size = GETSIZE(op.type);
> +	if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
> +		return false;
>   
> -	if (!ret && (type == LARX || type == STCX)) {
> -		printk_ratelimited("Breakpoint hit on instruction that can't be emulated."
> -				   " Breakpoint at 0x%lx will be disabled.\n", addr);
> -		goto disable;
> -	}
> +	if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * Returns true if the event is valid wrt dawr configuration,
> + * including extraneous exception. Otherwise return false.
> + */
> +static bool check_constraints(struct pt_regs *regs, unsigned int instr,
> +			      int type, int size,
> +			      struct arch_hw_breakpoint *info)
> +{
> +	bool in_user_range = dar_in_user_range(regs->dar, info);
> +	bool dawrx_constraints;
>   
>   	/*
> -	 * If it's extraneous event, we still need to emulate/single-
> -	 * step the instruction, but we don't generate an event.
> +	 * 8xx supports only one breakpoint and thus we can
> +	 * unconditionally return true.
>   	 */
> -	if (size && !dar_range_overlaps(regs->dar, size, info))
> -		info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> +	if (IS_ENABLED(CONFIG_PPC_8xx)) {
> +		if (!in_user_range)
> +			info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> +		return true;
> +	}
>   
> -	/* Do not emulate user-space instructions, instead single-step them */
> -	if (user_mode(regs)) {
> -		current->thread.last_hit_ubp = bp;
> -		regs->msr |= MSR_SE;
> +	if (unlikely(instr == -1)) {
> +		if (in_user_range)
> +			return true;
> +
> +		if (dar_in_hw_range(regs->dar, info)) {
> +			info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> +			return true;
> +		}
>   		return false;
>   	}
>   
> -	if (!emulate_step(regs, instr))
> -		goto fail;
> +	dawrx_constraints = check_dawrx_constraints(regs, type, info);
>   
> -	return true;
> +	if (dar_user_range_overlaps(regs->dar, size, info))
> +		return dawrx_constraints;
> +
> +	if (dar_hw_range_overlaps(regs->dar, size, info)) {
> +		if (dawrx_constraints) {
> +			info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +
> +static int get_instr_detail(struct pt_regs *regs, int *type, int *size,
> +			    bool *larx_stcx)
> +{
> +	unsigned int instr = 0;
> +	struct instruction_op op;
> +
> +	if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
> +		return -1;
> +
> +	analyse_instr(&op, regs, instr);
>   
> -fail:
>   	/*
> -	 * We've failed in reliably handling the hw-breakpoint. Unregister
> -	 * it and throw a warning message to let the user know about it.
> +	 * Set size = 8 if analyse_instr() fails. If it's a userspace
> +	 * watchpoint(valid or extraneous), we can notify user about it.
> +	 * If it's a kernel watchpoint, instruction  emulation will fail
> +	 * in stepping_handler() and watchpoint will be disabled.
>   	 */
> -	WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
> -		"0x%lx will be disabled.", addr);
> +	*type = GETTYPE(op.type);
> +	*size = !(*type == UNKNOWN) ? GETSIZE(op.type) : 8;
> +	*larx_stcx = (*type == LARX || *type == STCX);
>   
> -disable:
> +	return instr;
> +}
> +
> +/*
> + * We've failed in reliably handling the hw-breakpoint. Unregister
> + * it and throw a warning message to let the user know about it.
> + */
> +static void handler_error(struct perf_event *bp, struct arch_hw_breakpoint *info)
> +{
> +	WARN(1, "Unable to handle hardware breakpoint."
> +		"Breakpoint at 0x%lx will be disabled.",
> +		info->address);
> +	perf_event_disable_inatomic(bp);
> +}
> +
> +static void larx_stcx_err(struct perf_event *bp, struct arch_hw_breakpoint *info)
> +{
> +	printk_ratelimited("Breakpoint hit on instruction that can't "
> +			   "be emulated. Breakpoint at 0x%lx will be "
> +			   "disabled.\n", info->address);
>   	perf_event_disable_inatomic(bp);
> -	return false;
> +}
> +
> +static bool stepping_handler(struct pt_regs *regs, struct perf_event **bp,
> +			     struct arch_hw_breakpoint **info, int *hit,
> +			     unsigned int instr)
> +{
> +	int i;
> +	int stepped;
> +
> +	/* Do not emulate user-space instructions, instead single-step them */
> +	if (user_mode(regs)) {
> +		for (i = 0; i < nr_wp_slots(); i++) {
> +			if (!hit[i])
> +				continue;
> +			current->thread.last_hit_ubp[i] = bp[i];
> +			info[i] = NULL;
> +		}
> +		regs->msr |= MSR_SE;
> +		return false;
> +	}
> +
> +	stepped = emulate_step(regs, instr);
> +	if (!stepped) {
> +		for (i = 0; i < nr_wp_slots(); i++) {
> +			if (!hit[i])
> +				continue;
> +			handler_error(bp[i], info[i]);
> +			info[i] = NULL;
> +		}
> +		return false;
> +	}
> +	return true;
>   }
>   
>   int hw_breakpoint_handler(struct die_args *args)
>   {
> +	bool err = false;
>   	int rc = NOTIFY_STOP;
> -	struct perf_event *bp;
> +	struct perf_event *bp[HBP_NUM_MAX] = {0};
>   	struct pt_regs *regs = args->regs;
> -	struct arch_hw_breakpoint *info;
> +	struct arch_hw_breakpoint *info[HBP_NUM_MAX] = {0};
> +	int i;
> +	int hit[HBP_NUM_MAX] = {0};
> +	int nr_hit = 0;
> +	bool ptrace_bp = false;
> +	unsigned int instr = 0;
> +	int type = 0;
> +	int size = 0;
> +	bool larx_stcx = false;
>   
>   	/* Disable breakpoints during exception handling */
>   	hw_breakpoint_disable();
> @@ -316,12 +475,39 @@ int hw_breakpoint_handler(struct die_args *args)
>   	 */
>   	rcu_read_lock();
>   
> -	bp = __this_cpu_read(bp_per_reg);
> -	if (!bp) {
> +	if (!IS_ENABLED(CONFIG_PPC_8xx))
> +		instr = get_instr_detail(regs, &type, &size, &larx_stcx);
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		bp[i] = __this_cpu_read(bp_per_reg[i]);
> +		if (!bp[i])
> +			continue;
> +
> +		info[i] = counter_arch_bp(bp[i]);
> +		info[i]->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
> +
> +		if (check_constraints(regs, instr, type, size, info[i])) {
> +			if (!IS_ENABLED(CONFIG_PPC_8xx) && instr == -1) {
> +				handler_error(bp[i], info[i]);
> +				info[i] = NULL;
> +				err = 1;
> +				continue;
> +			}
> +
> +			if (is_ptrace_bp(bp[i]))
> +				ptrace_bp = true;
> +			hit[i] = 1;
> +			nr_hit++;
> +		}
> +	}
> +
> +	if (err)
> +		goto reset;
> +
> +	if (!nr_hit) {
>   		rc = NOTIFY_DONE;
>   		goto out;
>   	}
> -	info = counter_arch_bp(bp);
>   
>   	/*
>   	 * Return early after invoking user-callback function without restoring
> @@ -329,29 +515,50 @@ int hw_breakpoint_handler(struct die_args *args)
>   	 * one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
>   	 * generated in do_dabr().
>   	 */
> -	if (is_ptrace_bp(bp)) {
> -		perf_bp_event(bp, regs);
> +	if (ptrace_bp) {
> +		for (i = 0; i < nr_wp_slots(); i++) {
> +			if (!hit[i])
> +				continue;
> +			perf_bp_event(bp[i], regs);
> +			info[i] = NULL;
> +		}
>   		rc = NOTIFY_DONE;
> -		goto out;
> +		goto reset;
>   	}
>   
> -	info->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
> -	if (IS_ENABLED(CONFIG_PPC_8xx)) {
> -		if (!dar_within_range(regs->dar, info))
> -			info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> -	} else {
> -		if (!stepping_handler(regs, bp, info))
> -			goto out;
> +	if (!IS_ENABLED(CONFIG_PPC_8xx)) {
> +		if (larx_stcx) {
> +			for (i = 0; i < nr_wp_slots(); i++) {
> +				if (!hit[i])
> +					continue;
> +				larx_stcx_err(bp[i], info[i]);
> +				info[i] = NULL;
> +			}
> +			goto reset;
> +		}
> +
> +		if (!stepping_handler(regs, bp, info, hit, instr))
> +			goto reset;
>   	}
>   
>   	/*
>   	 * As a policy, the callback is invoked in a 'trigger-after-execute'
>   	 * fashion
>   	 */
> -	if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
> -		perf_bp_event(bp, regs);
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!hit[i])
> +			continue;
> +		if (!(info[i]->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
> +			perf_bp_event(bp[i], regs);
> +	}
> +
> +reset:
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!info[i])
> +			continue;
> +		__set_breakpoint(info[i], i);
> +	}
>   
> -	__set_breakpoint(info, 0);
>   out:
>   	rcu_read_unlock();
>   	return rc;
> @@ -366,26 +573,43 @@ static int single_step_dabr_instruction(struct die_args *args)
>   	struct pt_regs *regs = args->regs;
>   	struct perf_event *bp = NULL;
>   	struct arch_hw_breakpoint *info;
> +	int i;
> +	bool found = false;
>   
> -	bp = current->thread.last_hit_ubp;
>   	/*
>   	 * Check if we are single-stepping as a result of a
>   	 * previous HW Breakpoint exception
>   	 */
> -	if (!bp)
> -		return NOTIFY_DONE;
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		bp = current->thread.last_hit_ubp[i];
> +
> +		if (!bp)
> +			continue;
> +
> +		found = true;
> +		info = counter_arch_bp(bp);
> +
> +		/*
> +		 * We shall invoke the user-defined callback function in the
> +		 * single stepping handler to confirm to 'trigger-after-execute'
> +		 * semantics
> +		 */
> +		if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
> +			perf_bp_event(bp, regs);
> +		current->thread.last_hit_ubp[i] = NULL;
> +	}
>   
> -	info = counter_arch_bp(bp);
> +	if (!found)
> +		return NOTIFY_DONE;
>   
> -	/*
> -	 * We shall invoke the user-defined callback function in the single
> -	 * stepping handler to confirm to 'trigger-after-execute' semantics
> -	 */
> -	if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
> -		perf_bp_event(bp, regs);
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		bp = __this_cpu_read(bp_per_reg[i]);
> +		if (!bp)
> +			continue;
>   
> -	__set_breakpoint(info, 0);
> -	current->thread.last_hit_ubp = NULL;
> +		info = counter_arch_bp(bp);
> +		__set_breakpoint(info, i);
> +	}
>   
>   	/*
>   	 * If the process was being single-stepped by ptrace, let the
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index b9ab740fcacf..c21bf367136b 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -622,9 +622,6 @@ void do_break (struct pt_regs *regs, unsigned long address,
>   	if (debugger_break_match(regs))
>   		return;
>   
> -	/* Clear the breakpoint */
> -	hw_breakpoint_disable();
> -
>   	/* Deliver the signal to userspace */
>   	force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
>   }
> 

Christophe

Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

[Michael Ellerman]

Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> Currently we assume that we have only one watchpoint supported by hw.
>> Get rid of that assumption and use dynamic loop instead. This should
>> make supporting more watchpoints very easy.
>
> I think using 'we' is to be avoided in commit message.

Hmm, is it?

I use 'we' all the time. Which doesn't mean it's correct, but I think it
reads OK.

cheers

Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

[Christophe Leroy]

Le 18/03/2020 à 12:35, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>>> Currently we assume that we have only one watchpoint supported by hw.
>>> Get rid of that assumption and use dynamic loop instead. This should
>>> make supporting more watchpoints very easy.
>>
>> I think using 'we' is to be avoided in commit message.
> 
> Hmm, is it?
> 
> I use 'we' all the time. Which doesn't mean it's correct, but I think it
> reads OK.
> 
> cheers
> 

 From 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html :

Describe your changes in imperative mood, e.g. “make xyzzy do frotz” 
instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to 
do frotz”, as if you are giving orders to the codebase to change its 
behaviour.

Christophe

Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

[Segher Boessenkool]

On Wed, Mar 18, 2020 at 12:44:52PM +0100, Christophe Leroy wrote:
> Le 18/03/2020 à 12:35, Michael Ellerman a écrit :
> >Christophe Leroy <christophe.leroy@c-s.fr> writes:
> >>Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
> >>>Currently we assume that we have only one watchpoint supported by hw.
> >>>Get rid of that assumption and use dynamic loop instead. This should
> >>>make supporting more watchpoints very easy.
> >>
> >>I think using 'we' is to be avoided in commit message.
> >
> >Hmm, is it?
> >
> >I use 'we' all the time. Which doesn't mean it's correct, but I think it
> >reads OK.
> >
> >cheers
> 
> From 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html :
> 
> Describe your changes in imperative mood, e.g. “make xyzzy do frotz” 
> instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy 
> to do frotz”, as if you are giving orders to the codebase to change its 
> behaviour.

That is what is there already?  "Get rid of ...".

You cannot describe the current situation with an imperative.


Segher

Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

[Michael Ellerman]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Wed, Mar 18, 2020 at 12:44:52PM +0100, Christophe Leroy wrote:
>> Le 18/03/2020 à 12:35, Michael Ellerman a écrit :
>> >Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> >>Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> >>>Currently we assume that we have only one watchpoint supported by hw.
>> >>>Get rid of that assumption and use dynamic loop instead. This should
>> >>>make supporting more watchpoints very easy.
>> >>
>> >>I think using 'we' is to be avoided in commit message.
>> >
>> >Hmm, is it?
>> >
>> >I use 'we' all the time. Which doesn't mean it's correct, but I think it
>> >reads OK.
>> >
>> >cheers
>> 
>> From 
>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html :
>> 
>> Describe your changes in imperative mood, e.g. “make xyzzy do frotz” 
>> instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy 
>> to do frotz”, as if you are giving orders to the codebase to change its 
>> behaviour.
>
> That is what is there already?  "Get rid of ...".
>
> You cannot describe the current situation with an imperative.

Yeah, I think the use of 'we' and the imperative mood are separate
things.

ie. this uses 'we' to describe the current behaviour and then the
imperative mood to describe the change that's being made:

  Currently we assume xyzzy always does bar, which is incorrect.

  Change it to do frotz.


cheers

Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint

[Ravi Bangoria]

On 3/17/20 4:29 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> Currently we assume that we have only one watchpoint supported by hw.
>> Get rid of that assumption and use dynamic loop instead. This should
>> make supporting more watchpoints very easy.
> 
> I think using 'we' is to be avoided in commit message.
> 
> Could be something like:
> 
> "Currently the handler assumes there is only one watchpoint supported by hw"
> 
>>
>> So far, with only one watchpoint, the handler was simple. But with
>> multiple watchpoints, we need a mechanism to detect which watchpoint
>> caused the exception. HW doesn't provide that information and thus
>> we need software logic for this. This makes exception handling bit
>> more complex.
> 
> Same here, the 'we' should be avoided.
> 
> 
> This patch is pretty big. I think you should explain a bit more what is done. Otherwise it is difficult to review.

I understand, and the diff is also messy. But splitting this into granular
patches looks difficult to me.

The patch enables core hw-breakpoint subsystem to install/uninstall more than one
watchpoint, basically converting direct accesses of data structures in a loop.
Similarly changes in thread_chage_pc(), single_step_dabr_instruction() etc.

Now, with more than one watchpoints, exception handler need to know which DAWR
caused the exception, and hw currently does not provide it. So we need sw logic
for the same. To figure out which DAWR caused the exception, we check all
different combinations of user specified range, dawr address range, actual
access range and dawrx constrains. For ex, if user specified range and actual
access range overlaps but dawrx is configured for readonly watchpoint and the
instruction is store, this DAWR must not have caused exception. This logic is
implemented by check_constraints() function.

...

>> -static bool dar_within_range(unsigned long dar, struct arch_hw_breakpoint *info)
>> +static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
> 
> Is this name change directly related to the patch ?

Yes. now we have two functions: dar_in_user_range() and dar_in_hw_range().
More detail below...

> 
>>   {
>>       return ((info->address <= dar) && (dar - info->address < info->len));
>>   }
>> -static bool
>> -dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
>> +static bool dar_user_range_overlaps(unsigned long dar, int size,
>> +                    struct arch_hw_breakpoint *info)
> 
> Same question.

For single DAWR, hw_breakpoint_handler() logic is:

	if (user range overlaps with actual access range)
		valid event;
	else
		valid but extraneous;

With multiple breakpoints, this logic can't work when hw is not telling us which
DAWR caused exception. So the logic is like:

	for (i = 0; i < nr_wps; i++) {
		if (user range overlaps with actual access range)
			valid event;
		else if (hw range overlaps with actual access range)
			valid but extraneous;
		else
			exception cause by some other dawr;
	}

So we have two functions: dar_user_range_overlaps() and dar_hw_range_overlaps().
If reading instruction fails, we can't get actual access range an in that case
we use: dar_in_user_range() and dar_in_hw_range().

Thanks,
Ravi

[PATCH 13/15] powerpc/watchpoint: Don't allow concurrent perf and ptrace events

[Ravi Bangoria]

ptrace and perf watchpoints on powerpc behaves differently. Ptrace
watchpoint works in one-shot mode and generates signal before executing
instruction. It's ptrace user's job to single-step the instruction and
re-enable the watchpoint. OTOH, in case of perf watchpoint, kernel
emulates/single-steps the instruction and then generates event. If perf
and ptrace creates two events with same or overlapping address ranges,
it's ambiguous to decide who should single-step the instruction. Because
of this issue ptrace and perf event can't coexist when the address range
overlaps.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/include/asm/hw_breakpoint.h |   2 +
 arch/powerpc/kernel/hw_breakpoint.c      | 220 +++++++++++++++++++++++
 kernel/events/hw_breakpoint.c            |  16 ++
 3 files changed, 238 insertions(+)

diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index ec61e2b7195c..6e1a19af5177 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -66,6 +66,8 @@ extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 						unsigned long val, void *data);
 int arch_install_hw_breakpoint(struct perf_event *bp);
 void arch_uninstall_hw_breakpoint(struct perf_event *bp);
+int arch_reserve_bp_slot(struct perf_event *bp);
+void arch_release_bp_slot(struct perf_event *bp);
 void arch_unregister_hw_breakpoint(struct perf_event *bp);
 void hw_breakpoint_pmu_read(struct perf_event *bp);
 extern void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 2ac89b92590f..d8529d9151e8 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -123,6 +123,226 @@ static bool is_ptrace_bp(struct perf_event *bp)
 	return (bp->overflow_handler == ptrace_triggered);
 }
 
+struct breakpoint {
+	struct list_head list;
+	struct perf_event *bp;
+	bool ptrace_bp;
+};
+
+static DEFINE_PER_CPU(struct breakpoint *, cpu_bps[HBP_NUM_MAX]);
+static LIST_HEAD(task_bps);
+
+static struct breakpoint *alloc_breakpoint(struct perf_event *bp)
+{
+	struct breakpoint *tmp;
+
+	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
+	if (!tmp)
+		return ERR_PTR(-ENOMEM);
+	tmp->bp = bp;
+	tmp->ptrace_bp = is_ptrace_bp(bp);
+	return tmp;
+}
+
+static bool bp_addr_range_overlap(struct perf_event *bp1, struct perf_event *bp2)
+{
+	__u64 bp1_saddr, bp1_eaddr, bp2_saddr, bp2_eaddr;
+
+	bp1_saddr = bp1->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
+	bp1_eaddr = (bp1->attr.bp_addr + bp1->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
+	bp2_saddr = bp2->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
+	bp2_eaddr = (bp2->attr.bp_addr + bp2->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
+
+	return (bp1_saddr <= bp2_eaddr && bp1_eaddr >= bp2_saddr);
+}
+
+static bool alternate_infra_bp(struct breakpoint *b, struct perf_event *bp)
+{
+	return is_ptrace_bp(bp) ? !b->ptrace_bp : b->ptrace_bp;
+}
+
+static bool can_co_exist(struct breakpoint *b, struct perf_event *bp)
+{
+	return !(alternate_infra_bp(b, bp) && bp_addr_range_overlap(b->bp, bp));
+}
+
+static int task_bps_add(struct perf_event *bp)
+{
+	struct breakpoint *tmp;
+
+	tmp = alloc_breakpoint(bp);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+
+	list_add(&tmp->list, &task_bps);
+	return 0;
+}
+
+static void task_bps_remove(struct perf_event *bp)
+{
+	struct list_head *pos, *q;
+	struct breakpoint *tmp;
+
+	list_for_each_safe(pos, q, &task_bps) {
+		tmp = list_entry(pos, struct breakpoint, list);
+
+		if (tmp->bp == bp) {
+			list_del(&tmp->list);
+			kfree(tmp);
+			break;
+		}
+	}
+}
+
+/*
+ * If any task has breakpoint from alternate infrastructure,
+ * return true. Otherwise return false.
+ */
+static bool all_task_bps_check(struct perf_event *bp)
+{
+	struct breakpoint *tmp;
+
+	list_for_each_entry(tmp, &task_bps, list) {
+		if (!can_co_exist(tmp, bp))
+			return true;
+	}
+	return false;
+}
+
+/*
+ * If same task has breakpoint from alternate infrastructure,
+ * return true. Otherwise return false.
+ */
+static bool same_task_bps_check(struct perf_event *bp)
+{
+	struct breakpoint *tmp;
+
+	list_for_each_entry(tmp, &task_bps, list) {
+		if (tmp->bp->hw.target == bp->hw.target &&
+		    !can_co_exist(tmp, bp))
+			return true;
+	}
+	return false;
+}
+
+static int cpu_bps_add(struct perf_event *bp)
+{
+	struct breakpoint **cpu_bp;
+	struct breakpoint *tmp;
+	int i = 0;
+
+	tmp = alloc_breakpoint(bp);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+
+	cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (!cpu_bp[i]) {
+			cpu_bp[i] = tmp;
+			break;
+		}
+	}
+	return 0;
+}
+
+static void cpu_bps_remove(struct perf_event *bp)
+{
+	struct breakpoint **cpu_bp;
+	int i = 0;
+
+	cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (!cpu_bp[i])
+			continue;
+
+		if (cpu_bp[i]->bp == bp) {
+			kfree(cpu_bp[i]);
+			cpu_bp[i] = NULL;
+			break;
+		}
+	}
+}
+
+static bool cpu_bps_check(int cpu, struct perf_event *bp)
+{
+	struct breakpoint **cpu_bp;
+	int i;
+
+	cpu_bp = per_cpu_ptr(cpu_bps, cpu);
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (cpu_bp[i] && !can_co_exist(cpu_bp[i], bp))
+			return true;
+	}
+	return false;
+}
+
+static bool all_cpu_bps_check(struct perf_event *bp)
+{
+	int cpu;
+
+	for_each_online_cpu(cpu) {
+		if (cpu_bps_check(cpu, bp))
+			return true;
+	}
+	return false;
+}
+
+/*
+ * We don't use any locks to serialize accesses to cpu_bps or task_bps
+ * because are already inside nr_bp_mutex.
+ */
+int arch_reserve_bp_slot(struct perf_event *bp)
+{
+	int ret;
+
+	if (is_ptrace_bp(bp)) {
+		if (all_cpu_bps_check(bp))
+			return -ENOSPC;
+
+		if (same_task_bps_check(bp))
+			return -ENOSPC;
+
+		return task_bps_add(bp);
+	} else {
+		if (is_kernel_addr(bp->attr.bp_addr))
+			return 0;
+
+		if (bp->hw.target && bp->cpu == -1) {
+			if (same_task_bps_check(bp))
+				return -ENOSPC;
+
+			return task_bps_add(bp);
+		} else if (!bp->hw.target && bp->cpu != -1) {
+			if (all_task_bps_check(bp))
+				return -ENOSPC;
+
+			return cpu_bps_add(bp);
+		} else {
+			if (same_task_bps_check(bp))
+				return -ENOSPC;
+
+			ret = cpu_bps_add(bp);
+			if (ret)
+				return ret;
+			ret = task_bps_add(bp);
+			if (ret)
+				cpu_bps_remove(bp);
+
+			return ret;
+		}
+	}
+}
+
+void arch_release_bp_slot(struct perf_event *bp)
+{
+	if (!is_kernel_addr(bp->attr.bp_addr)) {
+		if (bp->hw.target)
+			task_bps_remove(bp);
+		if (bp->cpu != -1)
+			cpu_bps_remove(bp);
+	}
+}
+
 /*
  * Perform cleanup of arch-specific counters during unregistration
  * of the perf-event
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 3cc8416ec844..b48d7039a015 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -213,6 +213,15 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
 		list_del(&bp->hw.bp_list);
 }
 
+__weak int arch_reserve_bp_slot(struct perf_event *bp)
+{
+	return 0;
+}
+
+__weak void arch_release_bp_slot(struct perf_event *bp)
+{
+}
+
 /*
  * Function to perform processor-specific cleanup during unregistration
  */
@@ -270,6 +279,7 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
 	struct bp_busy_slots slots = {0};
 	enum bp_type_idx type;
 	int weight;
+	int ret;
 
 	/* We couldn't initialize breakpoint constraints on boot */
 	if (!constraints_initialized)
@@ -294,6 +304,10 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
 	if (slots.pinned + (!!slots.flexible) > nr_slots[type])
 		return -ENOSPC;
 
+	ret = arch_reserve_bp_slot(bp);
+	if (ret)
+		return ret;
+
 	toggle_bp_slot(bp, true, type, weight);
 
 	return 0;
@@ -317,6 +331,8 @@ static void __release_bp_slot(struct perf_event *bp, u64 bp_type)
 	enum bp_type_idx type;
 	int weight;
 
+	arch_release_bp_slot(bp);
+
 	type = find_slot_idx(bp_type);
 	weight = hw_breakpoint_weight(bp);
 	toggle_bp_slot(bp, false, type, weight);
-- 
2.21.1

Re: [PATCH 13/15] powerpc/watchpoint: Don't allow concurrent perf and ptrace events

[Christophe Leroy]

Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
> ptrace and perf watchpoints on powerpc behaves differently. Ptrace

On the 8xx, ptrace generates signal after executing the instruction.

> watchpoint works in one-shot mode and generates signal before executing
> instruction. It's ptrace user's job to single-step the instruction and
> re-enable the watchpoint. OTOH, in case of perf watchpoint, kernel
> emulates/single-steps the instruction and then generates event. If perf
> and ptrace creates two events with same or overlapping address ranges,
> it's ambiguous to decide who should single-step the instruction. Because
> of this issue ptrace and perf event can't coexist when the address range
> overlaps.

Ok, and then ? What's the purpose of this (big) patch ?

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/hw_breakpoint.h |   2 +
>   arch/powerpc/kernel/hw_breakpoint.c      | 220 +++++++++++++++++++++++
>   kernel/events/hw_breakpoint.c            |  16 ++
>   3 files changed, 238 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index ec61e2b7195c..6e1a19af5177 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -66,6 +66,8 @@ extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
>   						unsigned long val, void *data);
>   int arch_install_hw_breakpoint(struct perf_event *bp);
>   void arch_uninstall_hw_breakpoint(struct perf_event *bp);
> +int arch_reserve_bp_slot(struct perf_event *bp);
> +void arch_release_bp_slot(struct perf_event *bp);
>   void arch_unregister_hw_breakpoint(struct perf_event *bp);
>   void hw_breakpoint_pmu_read(struct perf_event *bp);
>   extern void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 2ac89b92590f..d8529d9151e8 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -123,6 +123,226 @@ static bool is_ptrace_bp(struct perf_event *bp)
>   	return (bp->overflow_handler == ptrace_triggered);
>   }
>   
> +struct breakpoint {
> +	struct list_head list;
> +	struct perf_event *bp;
> +	bool ptrace_bp;
> +};

Don't we have an equivalent struct already ?

> +
> +static DEFINE_PER_CPU(struct breakpoint *, cpu_bps[HBP_NUM_MAX]);
> +static LIST_HEAD(task_bps);
> +
> +static struct breakpoint *alloc_breakpoint(struct perf_event *bp)
> +{
> +	struct breakpoint *tmp;
> +
> +	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
> +	if (!tmp)
> +		return ERR_PTR(-ENOMEM);
> +	tmp->bp = bp;
> +	tmp->ptrace_bp = is_ptrace_bp(bp);
> +	return tmp;
> +}
> +
> +static bool bp_addr_range_overlap(struct perf_event *bp1, struct perf_event *bp2)
> +{
> +	__u64 bp1_saddr, bp1_eaddr, bp2_saddr, bp2_eaddr;
> +
> +	bp1_saddr = bp1->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
> +	bp1_eaddr = (bp1->attr.bp_addr + bp1->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
> +	bp2_saddr = bp2->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
> +	bp2_eaddr = (bp2->attr.bp_addr + bp2->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
> +
> +	return (bp1_saddr <= bp2_eaddr && bp1_eaddr >= bp2_saddr);

Would be better with something like (HW_BREAKPOINT_SIZE needs to be 
defined).

	bp1_saddr = ALIGN_DOWN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE);
	bp1_eaddr = ALIGN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE);
	bp2_saddr = ALIGN_DOWN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE);
	bp2_eaddr = ALIGN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE);

	return (bp1_saddr < bp2_eaddr && bp1_eaddr > bp2_saddr);

> +}
> +
> +static bool alternate_infra_bp(struct breakpoint *b, struct perf_event *bp)
> +{
> +	return is_ptrace_bp(bp) ? !b->ptrace_bp : b->ptrace_bp;
> +}
> +
> +static bool can_co_exist(struct breakpoint *b, struct perf_event *bp)
> +{
> +	return !(alternate_infra_bp(b, bp) && bp_addr_range_overlap(b->bp, bp));
> +}
> +
> +static int task_bps_add(struct perf_event *bp)
> +{
> +	struct breakpoint *tmp;
> +
> +	tmp = alloc_breakpoint(bp);
> +	if (IS_ERR(tmp))
> +		return PTR_ERR(tmp);
> +
> +	list_add(&tmp->list, &task_bps);
> +	return 0;
> +}
> +
> +static void task_bps_remove(struct perf_event *bp)
> +{
> +	struct list_head *pos, *q;
> +	struct breakpoint *tmp;
> +
> +	list_for_each_safe(pos, q, &task_bps) {
> +		tmp = list_entry(pos, struct breakpoint, list);
> +
> +		if (tmp->bp == bp) {
> +			list_del(&tmp->list);
> +			kfree(tmp);
> +			break;
> +		}
> +	}
> +}
> +
> +/*
> + * If any task has breakpoint from alternate infrastructure,
> + * return true. Otherwise return false.
> + */
> +static bool all_task_bps_check(struct perf_event *bp)
> +{
> +	struct breakpoint *tmp;
> +
> +	list_for_each_entry(tmp, &task_bps, list) {
> +		if (!can_co_exist(tmp, bp))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +/*
> + * If same task has breakpoint from alternate infrastructure,
> + * return true. Otherwise return false.
> + */
> +static bool same_task_bps_check(struct perf_event *bp)
> +{
> +	struct breakpoint *tmp;
> +
> +	list_for_each_entry(tmp, &task_bps, list) {
> +		if (tmp->bp->hw.target == bp->hw.target &&
> +		    !can_co_exist(tmp, bp))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static int cpu_bps_add(struct perf_event *bp)
> +{
> +	struct breakpoint **cpu_bp;
> +	struct breakpoint *tmp;
> +	int i = 0;
> +
> +	tmp = alloc_breakpoint(bp);
> +	if (IS_ERR(tmp))
> +		return PTR_ERR(tmp);
> +
> +	cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!cpu_bp[i]) {
> +			cpu_bp[i] = tmp;
> +			break;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void cpu_bps_remove(struct perf_event *bp)
> +{
> +	struct breakpoint **cpu_bp;
> +	int i = 0;
> +
> +	cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!cpu_bp[i])
> +			continue;
> +
> +		if (cpu_bp[i]->bp == bp) {
> +			kfree(cpu_bp[i]);
> +			cpu_bp[i] = NULL;
> +			break;
> +		}
> +	}
> +}
> +
> +static bool cpu_bps_check(int cpu, struct perf_event *bp)
> +{
> +	struct breakpoint **cpu_bp;
> +	int i;
> +
> +	cpu_bp = per_cpu_ptr(cpu_bps, cpu);
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (cpu_bp[i] && !can_co_exist(cpu_bp[i], bp))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static bool all_cpu_bps_check(struct perf_event *bp)
> +{
> +	int cpu;
> +
> +	for_each_online_cpu(cpu) {
> +		if (cpu_bps_check(cpu, bp))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +/*
> + * We don't use any locks to serialize accesses to cpu_bps or task_bps
> + * because are already inside nr_bp_mutex.
> + */
> +int arch_reserve_bp_slot(struct perf_event *bp)
> +{
> +	int ret;
> +
> +	if (is_ptrace_bp(bp)) {
> +		if (all_cpu_bps_check(bp))
> +			return -ENOSPC;
> +
> +		if (same_task_bps_check(bp))
> +			return -ENOSPC;
> +
> +		return task_bps_add(bp);
> +	} else {
> +		if (is_kernel_addr(bp->attr.bp_addr))
> +			return 0;
> +
> +		if (bp->hw.target && bp->cpu == -1) {
> +			if (same_task_bps_check(bp))
> +				return -ENOSPC;
> +
> +			return task_bps_add(bp);
> +		} else if (!bp->hw.target && bp->cpu != -1) {
> +			if (all_task_bps_check(bp))
> +				return -ENOSPC;
> +
> +			return cpu_bps_add(bp);
> +		} else {
> +			if (same_task_bps_check(bp))
> +				return -ENOSPC;
> +
> +			ret = cpu_bps_add(bp);
> +			if (ret)
> +				return ret;
> +			ret = task_bps_add(bp);
> +			if (ret)
> +				cpu_bps_remove(bp);
> +
> +			return ret;
> +		}
> +	}
> +}
> +
> +void arch_release_bp_slot(struct perf_event *bp)
> +{
> +	if (!is_kernel_addr(bp->attr.bp_addr)) {
> +		if (bp->hw.target)
> +			task_bps_remove(bp);
> +		if (bp->cpu != -1)
> +			cpu_bps_remove(bp);
> +	}
> +}
> +
>   /*
>    * Perform cleanup of arch-specific counters during unregistration
>    * of the perf-event
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 3cc8416ec844..b48d7039a015 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -213,6 +213,15 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
>   		list_del(&bp->hw.bp_list);
>   }
>   
> +__weak int arch_reserve_bp_slot(struct perf_event *bp)
> +{
> +	return 0;
> +}
> +
> +__weak void arch_release_bp_slot(struct perf_event *bp)
> +{
> +}
> +
>   /*
>    * Function to perform processor-specific cleanup during unregistration
>    */
> @@ -270,6 +279,7 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
>   	struct bp_busy_slots slots = {0};
>   	enum bp_type_idx type;
>   	int weight;
> +	int ret;
>   
>   	/* We couldn't initialize breakpoint constraints on boot */
>   	if (!constraints_initialized)
> @@ -294,6 +304,10 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
>   	if (slots.pinned + (!!slots.flexible) > nr_slots[type])
>   		return -ENOSPC;
>   
> +	ret = arch_reserve_bp_slot(bp);
> +	if (ret)
> +		return ret;
> +
>   	toggle_bp_slot(bp, true, type, weight);
>   
>   	return 0;
> @@ -317,6 +331,8 @@ static void __release_bp_slot(struct perf_event *bp, u64 bp_type)
>   	enum bp_type_idx type;
>   	int weight;
>   
> +	arch_release_bp_slot(bp);
> +
>   	type = find_slot_idx(bp_type);
>   	weight = hw_breakpoint_weight(bp);
>   	toggle_bp_slot(bp, false, type, weight);
> 


Christophe

Re: [PATCH 13/15] powerpc/watchpoint: Don't allow concurrent perf and ptrace events

[Ravi Bangoria]

On 3/17/20 4:38 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> ptrace and perf watchpoints on powerpc behaves differently. Ptrace
> 
> On the 8xx, ptrace generates signal after executing the instruction.

8xx logic is unchanged. I should have mentioned "Book3s DAWR".

> 
>> watchpoint works in one-shot mode and generates signal before executing
>> instruction. It's ptrace user's job to single-step the instruction and
>> re-enable the watchpoint. OTOH, in case of perf watchpoint, kernel
>> emulates/single-steps the instruction and then generates event. If perf
>> and ptrace creates two events with same or overlapping address ranges,
>> it's ambiguous to decide who should single-step the instruction. Because
>> of this issue ptrace and perf event can't coexist when the address range
>> overlaps.
> 
> Ok, and then ? What's the purpose of this (big) patch ?

Don't allow perf and ptrace watchpoint at the same time if their address
range overlaps.

...

>> +struct breakpoint {
>> +    struct list_head list;
>> +    struct perf_event *bp;
>> +    bool ptrace_bp;
>> +};
> 
> Don't we have an equivalent struct already ?

No. Using this we track percpu and perthread watchpoints for both perf and
ptrace. This problems is powerpc(DAWR) specific and thus we need to hook arch
specific logic in watchopint installation/uninstallation path.

...

>> +static bool bp_addr_range_overlap(struct perf_event *bp1, struct perf_event *bp2)
>> +{
>> +    __u64 bp1_saddr, bp1_eaddr, bp2_saddr, bp2_eaddr;
>> +
>> +    bp1_saddr = bp1->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
>> +    bp1_eaddr = (bp1->attr.bp_addr + bp1->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
>> +    bp2_saddr = bp2->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
>> +    bp2_eaddr = (bp2->attr.bp_addr + bp2->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
>> +
>> +    return (bp1_saddr <= bp2_eaddr && bp1_eaddr >= bp2_saddr);
> 
> Would be better with something like (HW_BREAKPOINT_SIZE needs to be defined).
> 
>      bp1_saddr = ALIGN_DOWN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE);
>      bp1_eaddr = ALIGN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE);
>      bp2_saddr = ALIGN_DOWN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE);
>      bp2_eaddr = ALIGN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE);
> 
>      return (bp1_saddr < bp2_eaddr && bp1_eaddr > bp2_saddr);

Ok.

Thanks,
Ravi

[PATCH 14/15] powerpc/watchpoint/xmon: Don't allow breakpoint overwriting

[Ravi Bangoria]

Xmon allows overwriting breakpoints because it's supported by only
one dawr. But with multiple dawrs, overwriting becomes ambiguous
or unnecessary complicated. So let's not allow it.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/xmon/xmon.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 0ca0d29f99c6..ac18fe3e4295 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1381,6 +1381,10 @@ bpt_cmds(void)
 			printf("Hardware data breakpoint not supported on this cpu\n");
 			break;
 		}
+		if (dabr.enabled) {
+			printf("Couldn't find free breakpoint register\n");
+			break;
+		}
 		mode = 7;
 		cmd = inchar();
 		if (cmd == 'r')
-- 
2.21.1

Re: [PATCH 14/15] powerpc/watchpoint/xmon: Don't allow breakpoint overwriting

[Christophe Leroy]

Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
> Xmon allows overwriting breakpoints because it's supported by only
> one dawr. But with multiple dawrs, overwriting becomes ambiguous
> or unnecessary complicated. So let's not allow it.

Could we drop this completely (I mean the functionnality, not the patch).

Christophe

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/xmon/xmon.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 0ca0d29f99c6..ac18fe3e4295 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -1381,6 +1381,10 @@ bpt_cmds(void)
>   			printf("Hardware data breakpoint not supported on this cpu\n");
>   			break;
>   		}
> +		if (dabr.enabled) {
> +			printf("Couldn't find free breakpoint register\n");
> +			break;
> +		}
>   		mode = 7;
>   		cmd = inchar();
>   		if (cmd == 'r')
> 

Re: [PATCH 14/15] powerpc/watchpoint/xmon: Don't allow breakpoint overwriting

[Ravi Bangoria]

On 3/17/20 4:40 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> Xmon allows overwriting breakpoints because it's supported by only
>> one dawr. But with multiple dawrs, overwriting becomes ambiguous
>> or unnecessary complicated. So let's not allow it.
> 
> Could we drop this completely (I mean the functionnality, not the patch).

Not sure I follow. Isn't the same thing I'm doing?

-Ravi

Re: [PATCH 14/15] powerpc/watchpoint/xmon: Don't allow breakpoint overwriting

[Christophe Leroy]

Le 18/03/2020 à 13:37, Ravi Bangoria a écrit :
> 
> 
> On 3/17/20 4:40 PM, Christophe Leroy wrote:
>>
>>
>> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>>> Xmon allows overwriting breakpoints because it's supported by only
>>> one dawr. But with multiple dawrs, overwriting becomes ambiguous
>>> or unnecessary complicated. So let's not allow it.
>>
>> Could we drop this completely (I mean the functionnality, not the patch).
> 
> Not sure I follow. Isn't the same thing I'm doing?
> 

Yes, I think I misunderstood the patch. That seems ok.

Christophe

[PATCH 15/15] powerpc/watchpoint/xmon: Support 2nd dawr

[Ravi Bangoria]

Add support for 2nd DAWR in xmon. With this, we can have two
simultaneous breakpoints from xmon.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/xmon/xmon.c | 101 ++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 32 deletions(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index ac18fe3e4295..20adc83404c8 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -110,7 +110,7 @@ struct bpt {
 
 #define NBPTS	256
 static struct bpt bpts[NBPTS];
-static struct bpt dabr;
+static struct bpt dabr[HBP_NUM_MAX];
 static struct bpt *iabr;
 static unsigned bpinstr = 0x7fe00008;	/* trap */
 
@@ -786,10 +786,17 @@ static int xmon_sstep(struct pt_regs *regs)
 
 static int xmon_break_match(struct pt_regs *regs)
 {
+	int i;
+
 	if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) != (MSR_IR|MSR_64BIT))
 		return 0;
-	if (dabr.enabled == 0)
-		return 0;
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (dabr[i].enabled)
+			goto found;
+	}
+	return 0;
+
+found:
 	xmon_core(regs, 0);
 	return 1;
 }
@@ -928,13 +935,16 @@ static void insert_bpts(void)
 
 static void insert_cpu_bpts(void)
 {
+	int i;
 	struct arch_hw_breakpoint brk;
 
-	if (dabr.enabled) {
-		brk.address = dabr.address;
-		brk.type = (dabr.enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
-		brk.len = DABR_MAX_LEN;
-		__set_breakpoint(&brk, 0);
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (dabr[i].enabled) {
+			brk.address = dabr[i].address;
+			brk.type = (dabr[i].enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
+			brk.len = 8;
+			__set_breakpoint(&brk, i);
+		}
 	}
 
 	if (iabr)
@@ -1348,6 +1358,35 @@ static long check_bp_loc(unsigned long addr)
 	return 1;
 }
 
+static int free_data_bpt(void)
+{
+	int i;
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (!dabr[i].enabled)
+			return i;
+	}
+	printf("Couldn't find free breakpoint register\n");
+	return -1;
+}
+
+static void print_data_bpts(void)
+{
+	int i;
+
+	for (i = 0; i < nr_wp_slots(); i++) {
+		if (!dabr[i].enabled)
+			continue;
+
+		printf("   data   "REG"  [", dabr[i].address);
+		if (dabr[i].enabled & 1)
+			printf("r");
+		if (dabr[i].enabled & 2)
+			printf("w");
+		printf("]\n");
+	}
+}
+
 static char *breakpoint_help_string =
     "Breakpoint command usage:\n"
     "b                show breakpoints\n"
@@ -1381,10 +1420,9 @@ bpt_cmds(void)
 			printf("Hardware data breakpoint not supported on this cpu\n");
 			break;
 		}
-		if (dabr.enabled) {
-			printf("Couldn't find free breakpoint register\n");
+		i = free_data_bpt();
+		if (i < 0)
 			break;
-		}
 		mode = 7;
 		cmd = inchar();
 		if (cmd == 'r')
@@ -1393,15 +1431,15 @@ bpt_cmds(void)
 			mode = 6;
 		else
 			termch = cmd;
-		dabr.address = 0;
-		dabr.enabled = 0;
-		if (scanhex(&dabr.address)) {
-			if (!is_kernel_addr(dabr.address)) {
+		dabr[i].address = 0;
+		dabr[i].enabled = 0;
+		if (scanhex(&dabr[i].address)) {
+			if (!is_kernel_addr(dabr[i].address)) {
 				printf(badaddr);
 				break;
 			}
-			dabr.address &= ~HW_BRK_TYPE_DABR;
-			dabr.enabled = mode | BP_DABR;
+			dabr[i].address &= ~HW_BRK_TYPE_DABR;
+			dabr[i].enabled = mode | BP_DABR;
 		}
 
 		force_enable_xmon();
@@ -1440,7 +1478,9 @@ bpt_cmds(void)
 			for (i = 0; i < NBPTS; ++i)
 				bpts[i].enabled = 0;
 			iabr = NULL;
-			dabr.enabled = 0;
+			for (i = 0; i < nr_wp_slots(); i++)
+				dabr[i].enabled = 0;
+
 			printf("All breakpoints cleared\n");
 			break;
 		}
@@ -1474,14 +1514,7 @@ bpt_cmds(void)
 		if (xmon_is_ro || !scanhex(&a)) {
 			/* print all breakpoints */
 			printf("   type            address\n");
-			if (dabr.enabled) {
-				printf("   data   "REG"  [", dabr.address);
-				if (dabr.enabled & 1)
-					printf("r");
-				if (dabr.enabled & 2)
-					printf("w");
-				printf("]\n");
-			}
+			print_data_bpts();
 			for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
 				if (!bp->enabled)
 					continue;
@@ -1941,8 +1974,13 @@ static void dump_207_sprs(void)
 
 	printf("hfscr  = %.16lx  dhdes = %.16lx rpr    = %.16lx\n",
 		mfspr(SPRN_HFSCR), mfspr(SPRN_DHDES), mfspr(SPRN_RPR));
-	printf("dawr   = %.16lx  dawrx = %.16lx ciabr  = %.16lx\n",
-		mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0), mfspr(SPRN_CIABR));
+	printf("dawr0  = %.16lx dawrx0 = %.16lx\n",
+		mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0));
+	if (nr_wp_slots() > 1) {
+		printf("dawr1  = %.16lx dawrx1 = %.16lx\n",
+			mfspr(SPRN_DAWR1), mfspr(SPRN_DAWRX1));
+	}
+	printf("ciabr  = %.16lx\n", mfspr(SPRN_CIABR));
 #endif
 }
 
@@ -3862,10 +3900,9 @@ static void clear_all_bpt(void)
 		bpts[i].enabled = 0;
 
 	/* Clear any data or iabr breakpoints */
-	if (iabr || dabr.enabled) {
-		iabr = NULL;
-		dabr.enabled = 0;
-	}
+	iabr = NULL;
+	for (i = 0; i < nr_wp_slots(); i++)
+		dabr[i].enabled = 0;
 }
 
 #ifdef CONFIG_DEBUG_FS
-- 
2.21.1

Re: [PATCH 15/15] powerpc/watchpoint/xmon: Support 2nd dawr

[Christophe Leroy]

Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
> Add support for 2nd DAWR in xmon. With this, we can have two
> simultaneous breakpoints from xmon.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/xmon/xmon.c | 101 ++++++++++++++++++++++++++-------------
>   1 file changed, 69 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index ac18fe3e4295..20adc83404c8 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -110,7 +110,7 @@ struct bpt {
>   
>   #define NBPTS	256
>   static struct bpt bpts[NBPTS];
> -static struct bpt dabr;
> +static struct bpt dabr[HBP_NUM_MAX];
>   static struct bpt *iabr;
>   static unsigned bpinstr = 0x7fe00008;	/* trap */
>   
> @@ -786,10 +786,17 @@ static int xmon_sstep(struct pt_regs *regs)
>   
>   static int xmon_break_match(struct pt_regs *regs)
>   {
> +	int i;
> +
>   	if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) != (MSR_IR|MSR_64BIT))
>   		return 0;
> -	if (dabr.enabled == 0)
> -		return 0;
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (dabr[i].enabled)
> +			goto found;
> +	}
> +	return 0;
> +
> +found:
>   	xmon_core(regs, 0);
>   	return 1;
>   }
> @@ -928,13 +935,16 @@ static void insert_bpts(void)
>   
>   static void insert_cpu_bpts(void)
>   {
> +	int i;
>   	struct arch_hw_breakpoint brk;
>   
> -	if (dabr.enabled) {
> -		brk.address = dabr.address;
> -		brk.type = (dabr.enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
> -		brk.len = DABR_MAX_LEN;
> -		__set_breakpoint(&brk, 0);
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (dabr[i].enabled) {
> +			brk.address = dabr[i].address;
> +			brk.type = (dabr[i].enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
> +			brk.len = 8;
> +			__set_breakpoint(&brk, i);
> +		}
>   	}
>   
>   	if (iabr)
> @@ -1348,6 +1358,35 @@ static long check_bp_loc(unsigned long addr)
>   	return 1;
>   }
>   
> +static int free_data_bpt(void)

This names suggests the function frees a breakpoint.
I guess it should be find_free_data_bpt()

> +{
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!dabr[i].enabled)
> +			return i;
> +	}
> +	printf("Couldn't find free breakpoint register\n");
> +	return -1;
> +}
> +
> +static void print_data_bpts(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!dabr[i].enabled)
> +			continue;
> +
> +		printf("   data   "REG"  [", dabr[i].address);
> +		if (dabr[i].enabled & 1)
> +			printf("r");
> +		if (dabr[i].enabled & 2)
> +			printf("w");
> +		printf("]\n");
> +	}
> +}
> +
>   static char *breakpoint_help_string =
>       "Breakpoint command usage:\n"
>       "b                show breakpoints\n"
> @@ -1381,10 +1420,9 @@ bpt_cmds(void)
>   			printf("Hardware data breakpoint not supported on this cpu\n");
>   			break;
>   		}
> -		if (dabr.enabled) {
> -			printf("Couldn't find free breakpoint register\n");
> +		i = free_data_bpt();
> +		if (i < 0)
>   			break;
> -		}
>   		mode = 7;
>   		cmd = inchar();
>   		if (cmd == 'r')
> @@ -1393,15 +1431,15 @@ bpt_cmds(void)
>   			mode = 6;
>   		else
>   			termch = cmd;
> -		dabr.address = 0;
> -		dabr.enabled = 0;
> -		if (scanhex(&dabr.address)) {
> -			if (!is_kernel_addr(dabr.address)) {
> +		dabr[i].address = 0;
> +		dabr[i].enabled = 0;
> +		if (scanhex(&dabr[i].address)) {
> +			if (!is_kernel_addr(dabr[i].address)) {
>   				printf(badaddr);
>   				break;
>   			}
> -			dabr.address &= ~HW_BRK_TYPE_DABR;
> -			dabr.enabled = mode | BP_DABR;
> +			dabr[i].address &= ~HW_BRK_TYPE_DABR;
> +			dabr[i].enabled = mode | BP_DABR;
>   		}
>   
>   		force_enable_xmon();
> @@ -1440,7 +1478,9 @@ bpt_cmds(void)
>   			for (i = 0; i < NBPTS; ++i)
>   				bpts[i].enabled = 0;
>   			iabr = NULL;
> -			dabr.enabled = 0;
> +			for (i = 0; i < nr_wp_slots(); i++)
> +				dabr[i].enabled = 0;
> +
>   			printf("All breakpoints cleared\n");
>   			break;
>   		}
> @@ -1474,14 +1514,7 @@ bpt_cmds(void)
>   		if (xmon_is_ro || !scanhex(&a)) {
>   			/* print all breakpoints */
>   			printf("   type            address\n");
> -			if (dabr.enabled) {
> -				printf("   data   "REG"  [", dabr.address);
> -				if (dabr.enabled & 1)
> -					printf("r");
> -				if (dabr.enabled & 2)
> -					printf("w");
> -				printf("]\n");
> -			}
> +			print_data_bpts();
>   			for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
>   				if (!bp->enabled)
>   					continue;
> @@ -1941,8 +1974,13 @@ static void dump_207_sprs(void)
>   
>   	printf("hfscr  = %.16lx  dhdes = %.16lx rpr    = %.16lx\n",
>   		mfspr(SPRN_HFSCR), mfspr(SPRN_DHDES), mfspr(SPRN_RPR));
> -	printf("dawr   = %.16lx  dawrx = %.16lx ciabr  = %.16lx\n",
> -		mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0), mfspr(SPRN_CIABR));
> +	printf("dawr0  = %.16lx dawrx0 = %.16lx\n",
> +		mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0));
> +	if (nr_wp_slots() > 1) {
> +		printf("dawr1  = %.16lx dawrx1 = %.16lx\n",
> +			mfspr(SPRN_DAWR1), mfspr(SPRN_DAWRX1));
> +	}
> +	printf("ciabr  = %.16lx\n", mfspr(SPRN_CIABR));
>   #endif
>   }
>   
> @@ -3862,10 +3900,9 @@ static void clear_all_bpt(void)
>   		bpts[i].enabled = 0;
>   
>   	/* Clear any data or iabr breakpoints */
> -	if (iabr || dabr.enabled) {
> -		iabr = NULL;
> -		dabr.enabled = 0;
> -	}
> +	iabr = NULL;
> +	for (i = 0; i < nr_wp_slots(); i++)
> +		dabr[i].enabled = 0;
>   }
>   
>   #ifdef CONFIG_DEBUG_FS
> 

Christophe

Re: [PATCH 15/15] powerpc/watchpoint/xmon: Support 2nd dawr

[Ravi Bangoria]

>> +static int free_data_bpt(void)
> 
> This names suggests the function frees a breakpoint.
> I guess it should be find_free_data_bpt()

Sure.

Thanks,
Ravi

Re: [PATCH 00/15] powerpc/watchpoint: Preparation for more than one watchpoint

[Christophe Leroy]

Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
> So far, powerpc Book3S code has been written with an assumption of only
> one watchpoint. But future power architecture is introducing second
> watchpoint register (DAWR). Even though this patchset does not enable
> 2nd DAWR, it make the infrastructure ready so that enabling 2nd DAWR
> should just be a matter of changing count.

Some book3s (e300 family for instance, I think G2 as well) already have 
a DABR2 in addition to DABR.
Will this series allow to use it as well ?

Christophe

Re: [PATCH 00/15] powerpc/watchpoint: Preparation for more than one watchpoint

[Segher Boessenkool]

On Mon, Mar 16, 2020 at 04:05:01PM +0100, Christophe Leroy wrote:
> Some book3s (e300 family for instance, I think G2 as well) already have 
> a DABR2 in addition to DABR.

The original "G2" (meaning 603 and 604) do not have DABR2.  The newer
"G2" (meaning e300) does have it.  e500 and e600 do not have it either.

Hope I got that right ;-)


Segher

Re: [PATCH 00/15] powerpc/watchpoint: Preparation for more than one watchpoint

[Christophe Leroy]

Le 16/03/2020 à 19:43, Segher Boessenkool a écrit :
> On Mon, Mar 16, 2020 at 04:05:01PM +0100, Christophe Leroy wrote:
>> Some book3s (e300 family for instance, I think G2 as well) already have
>> a DABR2 in addition to DABR.
> 
> The original "G2" (meaning 603 and 604) do not have DABR2.  The newer
> "G2" (meaning e300) does have it.  e500 and e600 do not have it either.
> 
> Hope I got that right ;-)
> 
> 

G2 core reference manual says:

Features specific to the G2 core not present on the original MPC603e 
(PID6-603e) processors follow:
...
  Enhanced debug features
  — Addition of three breakpoint registers—IABR2, DABR, and DABR2
  — Two new breakpoint control registers—DBCR and IBCR


e500 has DAC1 and DAC2 instead for breakpoints iaw e500 core reference 
manual.

Christophe

Re: [PATCH 00/15] powerpc/watchpoint: Preparation for more than one watchpoint

[Ravi Bangoria]

On 3/16/20 8:35 PM, Christophe Leroy wrote:
> 
> 
> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>> So far, powerpc Book3S code has been written with an assumption of only
>> one watchpoint. But future power architecture is introducing second
>> watchpoint register (DAWR). Even though this patchset does not enable
>> 2nd DAWR, it make the infrastructure ready so that enabling 2nd DAWR
>> should just be a matter of changing count.
> 
> Some book3s (e300 family for instance, I think G2 as well) already have a DABR2 in addition to DABR.
> Will this series allow to use it as well ?

I wasn't aware of that. I'll take a look at their specs and check if they
can piggyback on this series for 2nd DABR.

Thanks,
Ravi

Re: [PATCH 00/15] powerpc/watchpoint: Preparation for more than one watchpoint

[Ravi Bangoria]

On 3/18/20 6:22 PM, Ravi Bangoria wrote:
> 
> 
> On 3/16/20 8:35 PM, Christophe Leroy wrote:
>>
>>
>> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>>> So far, powerpc Book3S code has been written with an assumption of only
>>> one watchpoint. But future power architecture is introducing second
>>> watchpoint register (DAWR). Even though this patchset does not enable
>>> 2nd DAWR, it make the infrastructure ready so that enabling 2nd DAWR
>>> should just be a matter of changing count.
>>
>> Some book3s (e300 family for instance, I think G2 as well) already have a DABR2 in addition to DABR.
>> Will this series allow to use it as well ?
> 
> I wasn't aware of that. I'll take a look at their specs and check if they
> can piggyback on this series for 2nd DABR.

There are some differences between G2/e300 DABRs and Book3S DAWRs. G2/e300
DABRs provides some functionalities like "Match if EA less/greater than DABR",
"combined mode" etc. are not present with DAWRs. DBCR on G2/e300 also provides
which DABR caused the exception. So this series might not directly allow to
use DABR2 but, I guess, it should work as base infrastructure.

Ravi