[PATCH 0/3] LoongArch: ptrace: Fix some watchpoint function

[PATCH 0/3] LoongArch: ptrace: Fix some watchpoint function

[Hui Li]

LoongArch defines hardware watchpoint functions for fetch and load/store
operations. After the software configures the watchpoints for fetch and
load/store, the processor hardware will monitor the access addresses of
the fetch and load/store operations and trigger a watchpoint exception
when the watchpoint setting conditions are met.

Fix some hardware watchpoint issues on user-space and kernel interface.

All changes are made according to the LoongArch Reference Manual:
https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints

Testing for this series can use this gdb patch:
https://sourceware.org/pipermail/gdb-patches/2024-May/209371.html


Hui Li (3):
  LoongArch: ptrace: Fix watchpoint setting error
  LoongArch: Trigger user-space watchpoints correctly
  LoongArch: Fix multiple hardware watchpoint issues

 arch/loongarch/include/asm/hw_breakpoint.h |  2 +-
 arch/loongarch/kernel/hw_breakpoint.c      | 82 +++++++++++-----------
 arch/loongarch/kernel/ptrace.c             | 48 +++++++------
 3 files changed, 71 insertions(+), 61 deletions(-)

-- 
2.38.1

[PATCH 1/3] LoongArch: ptrace: Fix watchpoint setting error

[Hui Li]

In the current code, when debugging the following code using gdb,
"invalid argument ..." message will be displayed.

lihui@bogon:~$ cat test.c
  #include <stdio.h>
  int a = 0;
  int main()
  {
    printf("start test\n");
    a = 1;
    printf("a = %d\n", a);
    printf("end test\n");
    return 0;
  }
lihui@bogon:~$ gcc -g test.c -o test
lihui@bogon:~$ gdb test
...
(gdb) start
...
Temporary breakpoint 1, main () at test.c:5
5        printf("start test\n");
(gdb) watch a
Hardware watchpoint 2: a
(gdb) c
Continuing.
Invalid argument setting hardware debug registers

The root cause is that the existing kernel ptrace interface
has some issues on watchpoint argument parsing and setting.
It mainly includes the two types of issues.

1. Some incorrect judgment condition existed in ptrace watchpoints
   argument parsing, causing -EINVAL to be returned.

2. The watchpoint argument was not set correctly due to unnecessary
   addr offset.

Modify the relevant code to solve the above problem. Ensure the watchpont
argument is set correctly.

All changes according to the LoongArch Reference Manual:
https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints

with this patch:

lihui@bogon:~$ gdb test
...
(gdb) start
...
Temporary breakpoint 1, main () at test.c:5
5        printf("start test\n");
(gdb) watch a
Hardware watchpoint 2: a
(gdb) c
Continuing.
start test
a = 1
...

Signed-off-by: Hui Li <lihui@loongson.cn>
---
 arch/loongarch/include/asm/hw_breakpoint.h |  2 +-
 arch/loongarch/kernel/hw_breakpoint.c      | 20 ++++----------
 arch/loongarch/kernel/ptrace.c             | 32 ++++++++++------------
 3 files changed, 21 insertions(+), 33 deletions(-)

diff --git a/arch/loongarch/include/asm/hw_breakpoint.h b/arch/loongarch/include/asm/hw_breakpoint.h
index 21447fb1efc7..a8ce580f4fc6 100644
--- a/arch/loongarch/include/asm/hw_breakpoint.h
+++ b/arch/loongarch/include/asm/hw_breakpoint.h
@@ -101,7 +101,7 @@ struct perf_event;
 struct perf_event_attr;
 
 extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
-				  int *gen_len, int *gen_type, int *offset);
+				  int *gen_len, int *gen_type);
 extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
 extern int hw_breakpoint_arch_parse(struct perf_event *bp,
 				    const struct perf_event_attr *attr,
diff --git a/arch/loongarch/kernel/hw_breakpoint.c b/arch/loongarch/kernel/hw_breakpoint.c
index fc55c4de2a11..2d311aff91af 100644
--- a/arch/loongarch/kernel/hw_breakpoint.c
+++ b/arch/loongarch/kernel/hw_breakpoint.c
@@ -283,7 +283,7 @@ int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
  * to generic breakpoint descriptions.
  */
 int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
-			   int *gen_len, int *gen_type, int *offset)
+			   int *gen_len, int *gen_type)
 {
 	/* Type */
 	switch (ctrl.type) {
@@ -303,11 +303,6 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 		return -EINVAL;
 	}
 
-	if (!ctrl.len)
-		return -EINVAL;
-
-	*offset = __ffs(ctrl.len);
-
 	/* Len */
 	switch (ctrl.len) {
 	case LOONGARCH_BREAKPOINT_LEN_1:
@@ -386,22 +381,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 			     struct arch_hw_breakpoint *hw)
 {
 	int ret;
-	u64 alignment_mask, offset;
+	u64 alignment_mask;
 
 	/* Build the arch_hw_breakpoint. */
 	ret = arch_build_bp_info(bp, attr, hw);
 	if (ret)
 		return ret;
 
-	if (hw->ctrl.type != LOONGARCH_BREAKPOINT_EXECUTE)
-		alignment_mask = 0x7;
-	else
+	if (hw->ctrl.type == LOONGARCH_BREAKPOINT_EXECUTE) {
 		alignment_mask = 0x3;
-	offset = hw->address & alignment_mask;
-
-	hw->address &= ~alignment_mask;
-	hw->ctrl.len <<= offset;
-
+		hw->address &= ~alignment_mask;
+	}
 	return 0;
 }
 
diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
index c114c5ef1332..16b756c6049b 100644
--- a/arch/loongarch/kernel/ptrace.c
+++ b/arch/loongarch/kernel/ptrace.c
@@ -494,28 +494,14 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
 				     struct arch_hw_breakpoint_ctrl ctrl,
 				     struct perf_event_attr *attr)
 {
-	int err, len, type, offset;
+	int err, len, type;
 
-	err = arch_bp_generic_fields(ctrl, &len, &type, &offset);
+	err = arch_bp_generic_fields(ctrl, &len, &type);
 	if (err)
 		return err;
 
-	switch (note_type) {
-	case NT_LOONGARCH_HW_BREAK:
-		if ((type & HW_BREAKPOINT_X) != type)
-			return -EINVAL;
-		break;
-	case NT_LOONGARCH_HW_WATCH:
-		if ((type & HW_BREAKPOINT_RW) != type)
-			return -EINVAL;
-		break;
-	default:
-		return -EINVAL;
-	}
-
 	attr->bp_len	= len;
 	attr->bp_type	= type;
-	attr->bp_addr	+= offset;
 
 	return 0;
 }
@@ -609,7 +595,19 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
 		return PTR_ERR(bp);
 
 	attr = bp->attr;
-	decode_ctrl_reg(uctrl, &ctrl);
+
+	switch (note_type) {
+	case NT_LOONGARCH_HW_BREAK:
+		ctrl.type = LOONGARCH_BREAKPOINT_EXECUTE;
+		ctrl.len = LOONGARCH_BREAKPOINT_LEN_4;
+		break;
+	case NT_LOONGARCH_HW_WATCH:
+		decode_ctrl_reg(uctrl, &ctrl);
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
 	if (err)
 		return err;
-- 
2.38.1

Re: [PATCH 1/3] LoongArch: ptrace: Fix watchpoint setting error

[Jinyang He]

On 2024-05-29 09:30, Hui Li wrote:
> In the current code, when debugging the following code using gdb,
> "invalid argument ..." message will be displayed.
>
> lihui@bogon:~$ cat test.c
>    #include <stdio.h>
>    int a = 0;
>    int main()
>    {
>      printf("start test\n");
>      a = 1;
>      printf("a = %d\n", a);
>      printf("end test\n");
>      return 0;
>    }
> lihui@bogon:~$ gcc -g test.c -o test
> lihui@bogon:~$ gdb test
> ...
> (gdb) start
> ...
> Temporary breakpoint 1, main () at test.c:5
> 5        printf("start test\n");
> (gdb) watch a
> Hardware watchpoint 2: a
> (gdb) c
> Continuing.
> Invalid argument setting hardware debug registers
>
> The root cause is that the existing kernel ptrace interface
> has some issues on watchpoint argument parsing and setting.
> It mainly includes the two types of issues.
>
> 1. Some incorrect judgment condition existed in ptrace watchpoints
>     argument parsing, causing -EINVAL to be returned.

Which judgment? I see we return -EINVAL if !ctrl.len while 
LOONGARCH_BREAKPOINT_LEN_8  is zero.

I think the commit message should clearly point out this/these judgment.

And I think the test log is too complex.


>
> 2. The watchpoint argument was not set correctly due to unnecessary
>     addr offset.

Agree. The address is incorrect by add offset in ptrace_hbp_fill_attr_ctrl.


>
> Modify the relevant code to solve the above problem. Ensure the watchpont
> argument is set correctly.
>
> All changes according to the LoongArch Reference Manual:
> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints
>
> with this patch:
>
> lihui@bogon:~$ gdb test
> ...
> (gdb) start
> ...
> Temporary breakpoint 1, main () at test.c:5
> 5        printf("start test\n");
> (gdb) watch a
> Hardware watchpoint 2: a
> (gdb) c
> Continuing.
> start test
> a = 1
> ...
>
> Signed-off-by: Hui Li <lihui@loongson.cn>
> ---
>   arch/loongarch/include/asm/hw_breakpoint.h |  2 +-
>   arch/loongarch/kernel/hw_breakpoint.c      | 20 ++++----------
>   arch/loongarch/kernel/ptrace.c             | 32 ++++++++++------------
>   3 files changed, 21 insertions(+), 33 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/hw_breakpoint.h b/arch/loongarch/include/asm/hw_breakpoint.h
> index 21447fb1efc7..a8ce580f4fc6 100644
> --- a/arch/loongarch/include/asm/hw_breakpoint.h
> +++ b/arch/loongarch/include/asm/hw_breakpoint.h
> @@ -101,7 +101,7 @@ struct perf_event;
>   struct perf_event_attr;
>   
>   extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
> -				  int *gen_len, int *gen_type, int *offset);
> +				  int *gen_len, int *gen_type);
>   extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
>   extern int hw_breakpoint_arch_parse(struct perf_event *bp,
>   				    const struct perf_event_attr *attr,
> diff --git a/arch/loongarch/kernel/hw_breakpoint.c b/arch/loongarch/kernel/hw_breakpoint.c
> index fc55c4de2a11..2d311aff91af 100644
> --- a/arch/loongarch/kernel/hw_breakpoint.c
> +++ b/arch/loongarch/kernel/hw_breakpoint.c
> @@ -283,7 +283,7 @@ int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
>    * to generic breakpoint descriptions.
>    */
>   int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
> -			   int *gen_len, int *gen_type, int *offset)
> +			   int *gen_len, int *gen_type)
>   {
>   	/* Type */
>   	switch (ctrl.type) {
> @@ -303,11 +303,6 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>   		return -EINVAL;
>   	}
>   
> -	if (!ctrl.len)
> -		return -EINVAL;
> -
> -	*offset = __ffs(ctrl.len);
> -
>   	/* Len */
>   	switch (ctrl.len) {
>   	case LOONGARCH_BREAKPOINT_LEN_1:
> @@ -386,22 +381,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>   			     struct arch_hw_breakpoint *hw)
>   {
>   	int ret;
> -	u64 alignment_mask, offset;
> +	u64 alignment_mask;
>   
>   	/* Build the arch_hw_breakpoint. */
>   	ret = arch_build_bp_info(bp, attr, hw);
>   	if (ret)
>   		return ret;
>   
> -	if (hw->ctrl.type != LOONGARCH_BREAKPOINT_EXECUTE)
> -		alignment_mask = 0x7;
> -	else
> +	if (hw->ctrl.type == LOONGARCH_BREAKPOINT_EXECUTE) {
>   		alignment_mask = 0x3;
> -	offset = hw->address & alignment_mask;
> -
> -	hw->address &= ~alignment_mask;
> -	hw->ctrl.len <<= offset;
> -
> +		hw->address &= ~alignment_mask;

hw->address &= ~0x3ul; ?

How about other types? The hw->address can fixed by hw->ctrl.len I think.

I suspect that LoongArch only support aligned load or store hw breakpoints.


Thanks,

Jinyang

> +	}
>   	return 0;
>   }
>   
> diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
> index c114c5ef1332..16b756c6049b 100644
> --- a/arch/loongarch/kernel/ptrace.c
> +++ b/arch/loongarch/kernel/ptrace.c
> @@ -494,28 +494,14 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
>   				     struct arch_hw_breakpoint_ctrl ctrl,
>   				     struct perf_event_attr *attr)
>   {
> -	int err, len, type, offset;
> +	int err, len, type;
>   
> -	err = arch_bp_generic_fields(ctrl, &len, &type, &offset);
> +	err = arch_bp_generic_fields(ctrl, &len, &type);
>   	if (err)
>   		return err;
>   
> -	switch (note_type) {
> -	case NT_LOONGARCH_HW_BREAK:
> -		if ((type & HW_BREAKPOINT_X) != type)
> -			return -EINVAL;
> -		break;
> -	case NT_LOONGARCH_HW_WATCH:
> -		if ((type & HW_BREAKPOINT_RW) != type)
> -			return -EINVAL;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -
>   	attr->bp_len	= len;
>   	attr->bp_type	= type;
> -	attr->bp_addr	+= offset;
>   
>   	return 0;
>   }
> @@ -609,7 +595,19 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
>   		return PTR_ERR(bp);
>   
>   	attr = bp->attr;
> -	decode_ctrl_reg(uctrl, &ctrl);
> +
> +	switch (note_type) {
> +	case NT_LOONGARCH_HW_BREAK:
> +		ctrl.type = LOONGARCH_BREAKPOINT_EXECUTE;
> +		ctrl.len = LOONGARCH_BREAKPOINT_LEN_4;
> +		break;
> +	case NT_LOONGARCH_HW_WATCH:
> +		decode_ctrl_reg(uctrl, &ctrl);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
>   	err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
>   	if (err)
>   		return err;

Re: [PATCH 1/3] LoongArch: ptrace: Fix watchpoint setting error

[Hui Li]

On 2024/5/29 下午5:56, Jinyang He wrote:
> 
> On 2024-05-29 09:30, Hui Li wrote:
>> In the current code, when debugging the following code using gdb,
>> "invalid argument ..." message will be displayed.
>>
>> lihui@bogon:~$ cat test.c
>>    #include <stdio.h>
>>    int a = 0;
>>    int main()
>>    {
>>      printf("start test\n");
>>      a = 1;
>>      printf("a = %d\n", a);
>>      printf("end test\n");
>>      return 0;
>>    }
>> lihui@bogon:~$ gcc -g test.c -o test
>> lihui@bogon:~$ gdb test
>> ...
>> (gdb) start
>> ...
>> Temporary breakpoint 1, main () at test.c:5
>> 5        printf("start test\n");
>> (gdb) watch a
>> Hardware watchpoint 2: a
>> (gdb) c
>> Continuing.
>> Invalid argument setting hardware debug registers
>>
>> The root cause is that the existing kernel ptrace interface
>> has some issues on watchpoint argument parsing and setting.
>> It mainly includes the two types of issues.
>>
>> 1. Some incorrect judgment condition existed in ptrace watchpoints
>>     argument parsing, causing -EINVAL to be returned.
> 
> Which judgment? I see we return -EINVAL if !ctrl.len while 
> LOONGARCH_BREAKPOINT_LEN_8  is zero.
> 
> I think the commit message should clearly point out this/these judgment.
> 
> And I think the test log is too complex.
> 
Thanks for your reviewing.

I will modify the commit message to describe the problem more clearly in v2

> 
>>
>> 2. The watchpoint argument was not set correctly due to unnecessary
>>     addr offset.
> 
> Agree. The address is incorrect by add offset in ptrace_hbp_fill_attr_ctrl.
> 
>
>>
>> Modify the relevant code to solve the above problem. Ensure the watchpont
>> argument is set correctly.
>>
>> All changes according to the LoongArch Reference Manual:
>> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints 
>>
>>
>> with this patch:
>>
>> lihui@bogon:~$ gdb test
>> ...
>> (gdb) start
>> ...
>> Temporary breakpoint 1, main () at test.c:5
>> 5        printf("start test\n");
>> (gdb) watch a
>> Hardware watchpoint 2: a
>> (gdb) c
>> Continuing.
>> start test
>> a = 1
>> ...
>>
>> Signed-off-by: Hui Li <lihui@loongson.cn>
>> ---
>>   arch/loongarch/include/asm/hw_breakpoint.h |  2 +-
>>   arch/loongarch/kernel/hw_breakpoint.c      | 20 ++++----------
>>   arch/loongarch/kernel/ptrace.c             | 32 ++++++++++------------
>>   3 files changed, 21 insertions(+), 33 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/hw_breakpoint.h 
>> b/arch/loongarch/include/asm/hw_breakpoint.h
>> index 21447fb1efc7..a8ce580f4fc6 100644
>> --- a/arch/loongarch/include/asm/hw_breakpoint.h
>> +++ b/arch/loongarch/include/asm/hw_breakpoint.h
>> @@ -101,7 +101,7 @@ struct perf_event;
>>   struct perf_event_attr;
>>   extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>> -                  int *gen_len, int *gen_type, int *offset);
>> +                  int *gen_len, int *gen_type);
>>   extern int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
>>   extern int hw_breakpoint_arch_parse(struct perf_event *bp,
>>                       const struct perf_event_attr *attr,
>> diff --git a/arch/loongarch/kernel/hw_breakpoint.c 
>> b/arch/loongarch/kernel/hw_breakpoint.c
>> index fc55c4de2a11..2d311aff91af 100644
>> --- a/arch/loongarch/kernel/hw_breakpoint.c
>> +++ b/arch/loongarch/kernel/hw_breakpoint.c
>> @@ -283,7 +283,7 @@ int arch_check_bp_in_kernelspace(struct 
>> arch_hw_breakpoint *hw)
>>    * to generic breakpoint descriptions.
>>    */
>>   int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
>> -               int *gen_len, int *gen_type, int *offset)
>> +               int *gen_len, int *gen_type)
>>   {
>>       /* Type */
>>       switch (ctrl.type) {
>> @@ -303,11 +303,6 @@ int arch_bp_generic_fields(struct 
>> arch_hw_breakpoint_ctrl ctrl,
>>           return -EINVAL;
>>       }
>> -    if (!ctrl.len)
>> -        return -EINVAL;
>> -
>> -    *offset = __ffs(ctrl.len);
>> -
>>       /* Len */
>>       switch (ctrl.len) {
>>       case LOONGARCH_BREAKPOINT_LEN_1:
>> @@ -386,22 +381,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>>                    struct arch_hw_breakpoint *hw)
>>   {
>>       int ret;
>> -    u64 alignment_mask, offset;
>> +    u64 alignment_mask;
>>       /* Build the arch_hw_breakpoint. */
>>       ret = arch_build_bp_info(bp, attr, hw);
>>       if (ret)
>>           return ret;
>> -    if (hw->ctrl.type != LOONGARCH_BREAKPOINT_EXECUTE)
>> -        alignment_mask = 0x7;
>> -    else
>> +    if (hw->ctrl.type == LOONGARCH_BREAKPOINT_EXECUTE) {
>>           alignment_mask = 0x3;
>> -    offset = hw->address & alignment_mask;
>> -
>> -    hw->address &= ~alignment_mask;
>> -    hw->ctrl.len <<= offset;
>> -
>> +        hw->address &= ~alignment_mask;
> 
> hw->address &= ~0x3ul; ?
> 
> How about other types? The hw->address can fixed by hw->ctrl.len I think.
> 
> I suspect that LoongArch only support aligned load or store hw breakpoints.
> 
> 
> Thanks,
> 
> Jinyang

After more test and discussion, the values in MWPCFG1.Vaddr is not must 
have to be aligned.

Thanks,

Hui

> 
>> +    }
>>       return 0;
>>   }
>> diff --git a/arch/loongarch/kernel/ptrace.c 
>> b/arch/loongarch/kernel/ptrace.c
>> index c114c5ef1332..16b756c6049b 100644
>> --- a/arch/loongarch/kernel/ptrace.c
>> +++ b/arch/loongarch/kernel/ptrace.c
>> @@ -494,28 +494,14 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned 
>> int note_type,
>>                        struct arch_hw_breakpoint_ctrl ctrl,
>>                        struct perf_event_attr *attr)
>>   {
>> -    int err, len, type, offset;
>> +    int err, len, type;
>> -    err = arch_bp_generic_fields(ctrl, &len, &type, &offset);
>> +    err = arch_bp_generic_fields(ctrl, &len, &type);
>>       if (err)
>>           return err;
>> -    switch (note_type) {
>> -    case NT_LOONGARCH_HW_BREAK:
>> -        if ((type & HW_BREAKPOINT_X) != type)
>> -            return -EINVAL;
>> -        break;
>> -    case NT_LOONGARCH_HW_WATCH:
>> -        if ((type & HW_BREAKPOINT_RW) != type)
>> -            return -EINVAL;
>> -        break;
>> -    default:
>> -        return -EINVAL;
>> -    }
>> -
>>       attr->bp_len    = len;
>>       attr->bp_type    = type;
>> -    attr->bp_addr    += offset;
>>       return 0;
>>   }
>> @@ -609,7 +595,19 @@ static int ptrace_hbp_set_ctrl(unsigned int 
>> note_type,
>>           return PTR_ERR(bp);
>>       attr = bp->attr;
>> -    decode_ctrl_reg(uctrl, &ctrl);
>> +
>> +    switch (note_type) {
>> +    case NT_LOONGARCH_HW_BREAK:
>> +        ctrl.type = LOONGARCH_BREAKPOINT_EXECUTE;
>> +        ctrl.len = LOONGARCH_BREAKPOINT_LEN_4;
>> +        break;
>> +    case NT_LOONGARCH_HW_WATCH:
>> +        decode_ctrl_reg(uctrl, &ctrl);
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>>       err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
>>       if (err)
>>           return err;
> 

[PATCH 2/3] LoongArch: Trigger user-space watchpoints correctly

[Hui Li]

In the current code, gdb can successfully set the watchpoint through
ptrace interface, But watchpoint will not be triggered.

When debugging the following code using gdb,

lihui@bogon:~$ cat test.c
  #include <stdio.h>
  int a = 0;
  int main()
  {
    printf("start test\n");
    a = 1;
    printf("a = %d\n", a);
    printf("end test\n");
    return 0;
  }
lihui@bogon:~$ gcc -g test.c -o test
lihui@bogon:~$ gdb test
...
(gdb) start
...
Temporary breakpoint 1, main () at test.c:5
5        printf("start test\n");
(gdb) watch a
Hardware watchpoint 2: a
(gdb) c
Continuing.
start test
a = 1
end test
[Inferior 1 (process 1154) exited normally]

No watchpoints were triggered, the root causes are:

1. The kernel uses perf_event and hw_breakpoint framework to control
   watchpoint. But the perf_event corresponding to watchpoint is not
   enabled, it needs to be enabled according to watchpoint control register.

2. The global enable control for all watchpoints is the WE bit of CSR.CRMD,
   and hardware sets the value to 0 when an exception is triggered. When
   the ERTN instruction is executed to return, the hardware restores the
   value of the PWE field of CSR.PRMD here. So, before a thread containing
   watchpoints be scheduled, the PWE field of CSR.PRMD needs to be set to 1.

Modify the relevant code to solve the above problem. Enable the watchpoints in
the user-space thread.

All changes according to the LoongArch Reference Manual:
https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints
https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#basic-control-and-status-registers

with this patch:

lihui@bogon:~$ gdb test
...
(gdb) start
...
Temporary breakpoint 1, main () at test.c:5
5        printf("start test\n");
(gdb) watch a
Hardware watchpoint 2: a
(gdb) c
Continuing.
start test

Hardware watchpoint 2: a

Old value = 0
New value = 1
main () at test.c:7
7        printf("a = %d\n", a);
(gdb) c
Continuing.
a = 1
end test

Signed-off-by: Hui Li <lihui@loongson.cn>
---
 arch/loongarch/kernel/hw_breakpoint.c |  4 +++-
 arch/loongarch/kernel/ptrace.c        | 16 +++++++++++++---
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/arch/loongarch/kernel/hw_breakpoint.c b/arch/loongarch/kernel/hw_breakpoint.c
index 2d311aff91af..01cd0b051225 100644
--- a/arch/loongarch/kernel/hw_breakpoint.c
+++ b/arch/loongarch/kernel/hw_breakpoint.c
@@ -517,7 +517,9 @@ void hw_breakpoint_thread_switch(struct task_struct *next)
 		if (!((regs->csr_era ^ addr) & ~mask))
 			csr_write32(CSR_FWPC_SKIP, LOONGARCH_CSR_FWPS);
 		regs->csr_prmd |= CSR_PRMD_PWE;
-	} else {
+	} else if (test_tsk_thread_flag(next, TIF_LOAD_WATCH))
+                regs->csr_prmd |= CSR_PRMD_PWE;
+	else {
 		/* Update breakpoints */
 		update_bp_registers(regs, 1, 0);
 		/* Update watchpoints */
diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
index 16b756c6049b..328510bf60e1 100644
--- a/arch/loongarch/kernel/ptrace.c
+++ b/arch/loongarch/kernel/ptrace.c
@@ -589,6 +589,8 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
 	struct perf_event *bp;
 	struct perf_event_attr attr;
 	struct arch_hw_breakpoint_ctrl ctrl;
+        struct thread_info *ti = task_thread_info(tsk);
+        int hbp_enable = uctrl & CTRL_PLV_ENABLE;
 
 	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
 	if (IS_ERR(bp))
@@ -608,9 +610,17 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
 		return -EINVAL;
 	}
 
-	err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
-	if (err)
-		return err;
+       if (hbp_enable == CTRL_PLV_ENABLE) {
+		err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
+		if (err)
+			return err;
+		attr.disabled = 0;
+		set_ti_thread_flag(ti, TIF_LOAD_WATCH);
+	}
+	else {
+		attr.disabled = 1;
+		clear_ti_thread_flag(ti, TIF_LOAD_WATCH);
+	}
 
 	return modify_user_hw_breakpoint(bp, &attr);
 }
-- 
2.38.1

Re: [PATCH 2/3] LoongArch: Trigger user-space watchpoints correctly

[Jinyang He]

On 2024-05-29 09:30, Hui Li wrote:

> In the current code, gdb can successfully set the watchpoint through
> ptrace interface, But watchpoint will not be triggered.
>
> When debugging the following code using gdb,
>
> lihui@bogon:~$ cat test.c
>    #include <stdio.h>
>    int a = 0;
>    int main()
>    {
>      printf("start test\n");
>      a = 1;
>      printf("a = %d\n", a);
>      printf("end test\n");
>      return 0;
>    }
> lihui@bogon:~$ gcc -g test.c -o test
> lihui@bogon:~$ gdb test
> ...
> (gdb) start
> ...
> Temporary breakpoint 1, main () at test.c:5
> 5        printf("start test\n");
> (gdb) watch a
> Hardware watchpoint 2: a
> (gdb) c
> Continuing.
> start test
> a = 1
> end test
> [Inferior 1 (process 1154) exited normally]
>
> No watchpoints were triggered, the root causes are:
>
> 1. The kernel uses perf_event and hw_breakpoint framework to control
>     watchpoint. But the perf_event corresponding to watchpoint is not
>     enabled, it needs to be enabled according to watchpoint control register.
>
> 2. The global enable control for all watchpoints is the WE bit of CSR.CRMD,
>     and hardware sets the value to 0 when an exception is triggered. When
>     the ERTN instruction is executed to return, the hardware restores the
>     value of the PWE field of CSR.PRMD here. So, before a thread containing
>     watchpoints be scheduled, the PWE field of CSR.PRMD needs to be set to 1.
>
> Modify the relevant code to solve the above problem. Enable the watchpoints in
> the user-space thread.
>
> All changes according to the LoongArch Reference Manual:
> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints
> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#basic-control-and-status-registers
>
> with this patch:
>
> lihui@bogon:~$ gdb test
> ...
> (gdb) start
> ...
> Temporary breakpoint 1, main () at test.c:5
> 5        printf("start test\n");
> (gdb) watch a
> Hardware watchpoint 2: a
> (gdb) c
> Continuing.
> start test
>
> Hardware watchpoint 2: a
>
> Old value = 0
> New value = 1
> main () at test.c:7
> 7        printf("a = %d\n", a);
> (gdb) c
> Continuing.
> a = 1
> end test
>
> Signed-off-by: Hui Li <lihui@loongson.cn>
> ---
>   arch/loongarch/kernel/hw_breakpoint.c |  4 +++-
>   arch/loongarch/kernel/ptrace.c        | 16 +++++++++++++---
>   2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/loongarch/kernel/hw_breakpoint.c b/arch/loongarch/kernel/hw_breakpoint.c
> index 2d311aff91af..01cd0b051225 100644
> --- a/arch/loongarch/kernel/hw_breakpoint.c
> +++ b/arch/loongarch/kernel/hw_breakpoint.c
> @@ -517,7 +517,9 @@ void hw_breakpoint_thread_switch(struct task_struct *next)
>   		if (!((regs->csr_era ^ addr) & ~mask))
>   			csr_write32(CSR_FWPC_SKIP, LOONGARCH_CSR_FWPS);
>   		regs->csr_prmd |= CSR_PRMD_PWE;
> -	} else {
> +	} else if (test_tsk_thread_flag(next, TIF_LOAD_WATCH))
> +                regs->csr_prmd |= CSR_PRMD_PWE;

It means enable PWE without update bp regs when switch to another task.

Will there be any critical situations with other task having set HW 
breakpoints before?


> +	else {
>   		/* Update breakpoints */
>   		update_bp_registers(regs, 1, 0);
>   		/* Update watchpoints */
> diff --git a/arch/loongarch/kernel/ptrace.c b/arch/loongarch/kernel/ptrace.c
> index 16b756c6049b..328510bf60e1 100644
> --- a/arch/loongarch/kernel/ptrace.c
> +++ b/arch/loongarch/kernel/ptrace.c
> @@ -589,6 +589,8 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
>   	struct perf_event *bp;
>   	struct perf_event_attr attr;
>   	struct arch_hw_breakpoint_ctrl ctrl;
> +        struct thread_info *ti = task_thread_info(tsk);
> +        int hbp_enable = uctrl & CTRL_PLV_ENABLE;

./scripts/checkpatch.pl


>   
>   	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
>   	if (IS_ERR(bp))
> @@ -608,9 +610,17 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
>   		return -EINVAL;
>   	}
>   
> -	err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
> -	if (err)
> -		return err;
> +       if (hbp_enable == CTRL_PLV_ENABLE) {

The uctrl is comes form user_buf, here means the PLV{0,1,2,3} should all 
be 1?

That looks strange.


Thanks,

Jinyang

> +		err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
> +		if (err)
> +			return err;
> +		attr.disabled = 0;
> +		set_ti_thread_flag(ti, TIF_LOAD_WATCH);
> +	}
> +	else {
> +		attr.disabled = 1;
> +		clear_ti_thread_flag(ti, TIF_LOAD_WATCH);
> +	}
>   
>   	return modify_user_hw_breakpoint(bp, &attr);
>   }

Re: [PATCH 2/3] LoongArch: Trigger user-space watchpoints correctly

[Hui Li]

On 2024/5/29 下午7:33, Jinyang He wrote:
> On 2024-05-29 09:30, Hui Li wrote:
> 
>> In the current code, gdb can successfully set the watchpoint through
>> ptrace interface, But watchpoint will not be triggered.
>>
>> When debugging the following code using gdb,
>>
>> lihui@bogon:~$ cat test.c
>>    #include <stdio.h>
>>    int a = 0;
>>    int main()
>>    {
>>      printf("start test\n");
>>      a = 1;
>>      printf("a = %d\n", a);
>>      printf("end test\n");
>>      return 0;
>>    }
>> lihui@bogon:~$ gcc -g test.c -o test
>> lihui@bogon:~$ gdb test
>> ...
>> (gdb) start
>> ...
>> Temporary breakpoint 1, main () at test.c:5
>> 5        printf("start test\n");
>> (gdb) watch a
>> Hardware watchpoint 2: a
>> (gdb) c
>> Continuing.
>> start test
>> a = 1
>> end test
>> [Inferior 1 (process 1154) exited normally]
>>
>> No watchpoints were triggered, the root causes are:
>>
>> 1. The kernel uses perf_event and hw_breakpoint framework to control
>>     watchpoint. But the perf_event corresponding to watchpoint is not
>>     enabled, it needs to be enabled according to watchpoint control 
>> register.
>>
>> 2. The global enable control for all watchpoints is the WE bit of 
>> CSR.CRMD,
>>     and hardware sets the value to 0 when an exception is triggered. When
>>     the ERTN instruction is executed to return, the hardware restores the
>>     value of the PWE field of CSR.PRMD here. So, before a thread 
>> containing
>>     watchpoints be scheduled, the PWE field of CSR.PRMD needs to be 
>> set to 1.
>>
>> Modify the relevant code to solve the above problem. Enable the 
>> watchpoints in
>> the user-space thread.
>>
>> All changes according to the LoongArch Reference Manual:
>> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints 
>>
>> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#basic-control-and-status-registers 
>>
>>
>> with this patch:
>>
>> lihui@bogon:~$ gdb test
>> ...
>> (gdb) start
>> ...
>> Temporary breakpoint 1, main () at test.c:5
>> 5        printf("start test\n");
>> (gdb) watch a
>> Hardware watchpoint 2: a
>> (gdb) c
>> Continuing.
>> start test
>>
>> Hardware watchpoint 2: a
>>
>> Old value = 0
>> New value = 1
>> main () at test.c:7
>> 7        printf("a = %d\n", a);
>> (gdb) c
>> Continuing.
>> a = 1
>> end test
>>
>> Signed-off-by: Hui Li <lihui@loongson.cn>
>> ---
>>   arch/loongarch/kernel/hw_breakpoint.c |  4 +++-
>>   arch/loongarch/kernel/ptrace.c        | 16 +++++++++++++---
>>   2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/hw_breakpoint.c 
>> b/arch/loongarch/kernel/hw_breakpoint.c
>> index 2d311aff91af..01cd0b051225 100644
>> --- a/arch/loongarch/kernel/hw_breakpoint.c
>> +++ b/arch/loongarch/kernel/hw_breakpoint.c
>> @@ -517,7 +517,9 @@ void hw_breakpoint_thread_switch(struct 
>> task_struct *next)
>>           if (!((regs->csr_era ^ addr) & ~mask))
>>               csr_write32(CSR_FWPC_SKIP, LOONGARCH_CSR_FWPS);
>>           regs->csr_prmd |= CSR_PRMD_PWE;
>> -    } else {
>> +    } else if (test_tsk_thread_flag(next, TIF_LOAD_WATCH))
>> +                regs->csr_prmd |= CSR_PRMD_PWE;
> 
> It means enable PWE without update bp regs when switch to another task.
> 
> Will there be any critical situations with other task having set HW 
> breakpoints before?


Thanks for your reviewing.

Because enabling the PWE operation and setting watch regs operation not 
in one function.

I will move the enable PWE operation to hw_breakpoint_control().


> 
> 
>> +    else {
>>           /* Update breakpoints */
>>           update_bp_registers(regs, 1, 0);
>>           /* Update watchpoints */
>> diff --git a/arch/loongarch/kernel/ptrace.c 
>> b/arch/loongarch/kernel/ptrace.c
>> index 16b756c6049b..328510bf60e1 100644
>> --- a/arch/loongarch/kernel/ptrace.c
>> +++ b/arch/loongarch/kernel/ptrace.c
>> @@ -589,6 +589,8 @@ static int ptrace_hbp_set_ctrl(unsigned int 
>> note_type,
>>       struct perf_event *bp;
>>       struct perf_event_attr attr;
>>       struct arch_hw_breakpoint_ctrl ctrl;
>> +        struct thread_info *ti = task_thread_info(tsk);
>> +        int hbp_enable = uctrl & CTRL_PLV_ENABLE;
> 
> ./scripts/checkpatch.pl
> 
> 
>>       bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
>>       if (IS_ERR(bp))
>> @@ -608,9 +610,17 @@ static int ptrace_hbp_set_ctrl(unsigned int 
>> note_type,
>>           return -EINVAL;
>>       }
>> -    err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
>> -    if (err)
>> -        return err;
>> +       if (hbp_enable == CTRL_PLV_ENABLE) {
> 
> The uctrl is comes form user_buf, here means the PLV{0,1,2,3} should all 
> be 1?
> 
> That looks strange.
> 
> 
> Thanks,
> 
> Jinyang


This is modified to use PLV3 as a constraint.

And a comments from our internal review, a simple judgment is added in 
ptrace_hbp_set_addr()

	/* Kernel-space address cannot be monitored by user-space */
	if ((addr & XKPRANGE) == XKPRANGE)
		return -EINVAL;

to ensure that kernel-space address cannot be monitored in user mode.


I will send the v2 version to make these changes next week.


Thanks,

Hui


> 
>> +        err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
>> +        if (err)
>> +            return err;
>> +        attr.disabled = 0;
>> +        set_ti_thread_flag(ti, TIF_LOAD_WATCH);
>> +    }
>> +    else {
>> +        attr.disabled = 1;
>> +        clear_ti_thread_flag(ti, TIF_LOAD_WATCH);
>> +    }
>>       return modify_user_hw_breakpoint(bp, &attr);
>>   }
> 

[PATCH 3/3] LoongArch: Fix multiple hardware watchpoint issues

[Hui Li]

In the current code, multiple hardware breakpoint/watchpoint in
a user-space thread, some hardware breakpoint/watchpoint will not
be triggered.

When debugging the following code using gdb

lihui@bogon:~$ cat test.c
  #include <stdio.h>
  int a = 0;
  int main()
  {
    printf("start test\n");
    a = 1;
    printf("a = %d\n", a);
    printf("end test\n");
    return 0;
  }
lihui@bogon:~$ gcc -g test.c -o test
lihui@bogon:~$ gdb test
...
(gdb) start
...
Temporary breakpoint 1, main () at test.c:5
5        printf("start test\n");
(gdb) watch a
Hardware watchpoint 2: a
(gdb) hbreak 8
Hardware assisted breakpoint 3 at 0x1200006ec: file test.c, line 8.
(gdb) c
Continuing.
start test
a = 1

Breakpoint 3, main () at test.c:8
8        printf("end test\n");
...

The first hardware watchpoint is not triggered, the root causes are:

1. In hw_breakpoint_control(), The FWPnCFG1.2.4/MWPnCFG1.2.4 register setting
   are not distinguished. they should be set based on hardware watchpoint
   functions (fetch or load/store operations).

2. In breakpoint_handler() and watchpoint_handler(), not identifying which
   watchpoint was triggered, and all watchpoin-related perf_event callbacks
   are called and send siginfo to the user space. The user-space unable to
   determine which watchpoint is triggered currently. So, the kernel need
   to identity which watchpoint is triggered via MWPS/FWPS register and call
   the corresponding perf event callback to report siginfo to the user-space.

Modify the relevant code to solve above issues. All changes according to the
LoongArch Reference Manual:
https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints

with this patch:

lihui@bogon:~$ gdb test
...
(gdb) start
...
Temporary breakpoint 1, main () at test.c:5
5        printf("start test\n");
(gdb) watch a
Hardware watchpoint 2: a
(gdb) hbreak 8
Hardware assisted breakpoint 3 at 0x1200006ec: file test.c, line 8.
(gdb) c
Continuing.
start test

Hardware watchpoint 2: a

Old value = 0
New value = 1
main () at test.c:7
7        printf("a = %d\n", a);
(gdb) c
Continuing.
a = 1

Breakpoint 3, main () at test.c:8
8        printf("end test\n");
(gdb) c
Continuing.
end test
[Inferior 1 (process 778) exited normally]

Signed-off-by: Hui Li <lihui@loongson.cn>
---
 arch/loongarch/kernel/hw_breakpoint.c | 58 ++++++++++++++++-----------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/arch/loongarch/kernel/hw_breakpoint.c b/arch/loongarch/kernel/hw_breakpoint.c
index 01cd0b051225..143a45a4c7da 100644
--- a/arch/loongarch/kernel/hw_breakpoint.c
+++ b/arch/loongarch/kernel/hw_breakpoint.c
@@ -197,15 +197,15 @@ static int hw_breakpoint_control(struct perf_event *bp,
 	switch (ops) {
 	case HW_BREAKPOINT_INSTALL:
 		/* Set the FWPnCFG/MWPnCFG 1~4 register. */
-		write_wb_reg(CSR_CFG_ADDR, i, 0, info->address);
-		write_wb_reg(CSR_CFG_ADDR, i, 1, info->address);
-		write_wb_reg(CSR_CFG_MASK, i, 0, info->mask);
-		write_wb_reg(CSR_CFG_MASK, i, 1, info->mask);
-		write_wb_reg(CSR_CFG_ASID, i, 0, 0);
-		write_wb_reg(CSR_CFG_ASID, i, 1, 0);
 		if (info->ctrl.type == LOONGARCH_BREAKPOINT_EXECUTE) {
+			write_wb_reg(CSR_CFG_ADDR, i, 0, info->address);
+			write_wb_reg(CSR_CFG_MASK, i, 0, info->mask);
+			write_wb_reg(CSR_CFG_ASID, i, 0, 0);
 			write_wb_reg(CSR_CFG_CTRL, i, 0, CTRL_PLV_ENABLE);
 		} else {
+			write_wb_reg(CSR_CFG_ADDR, i, 1, info->address);
+			write_wb_reg(CSR_CFG_MASK, i, 1, info->mask);
+			write_wb_reg(CSR_CFG_ASID, i, 1, 0);
 			ctrl = encode_ctrl_reg(info->ctrl);
 			write_wb_reg(CSR_CFG_CTRL, i, 1, ctrl | CTRL_PLV_ENABLE);
 		}
@@ -214,14 +214,18 @@ static int hw_breakpoint_control(struct perf_event *bp,
 		break;
 	case HW_BREAKPOINT_UNINSTALL:
 		/* Reset the FWPnCFG/MWPnCFG 1~4 register. */
-		write_wb_reg(CSR_CFG_ADDR, i, 0, 0);
-		write_wb_reg(CSR_CFG_ADDR, i, 1, 0);
-		write_wb_reg(CSR_CFG_MASK, i, 0, 0);
-		write_wb_reg(CSR_CFG_MASK, i, 1, 0);
-		write_wb_reg(CSR_CFG_CTRL, i, 0, 0);
-		write_wb_reg(CSR_CFG_CTRL, i, 1, 0);
-		write_wb_reg(CSR_CFG_ASID, i, 0, 0);
-		write_wb_reg(CSR_CFG_ASID, i, 1, 0);
+		if (info->ctrl.type == LOONGARCH_BREAKPOINT_EXECUTE) {
+			write_wb_reg(CSR_CFG_ADDR, i, 0, 0);
+			write_wb_reg(CSR_CFG_MASK, i, 0, 0);
+			write_wb_reg(CSR_CFG_CTRL, i, 0, 0);
+			write_wb_reg(CSR_CFG_ASID, i, 0, 0);
+		}
+		else {
+			write_wb_reg(CSR_CFG_ADDR, i, 1, 0);
+			write_wb_reg(CSR_CFG_MASK, i, 1, 0);
+			write_wb_reg(CSR_CFG_CTRL, i, 1, 0);
+			write_wb_reg(CSR_CFG_ASID, i, 1, 0);
+		}
 		break;
 	}
 
@@ -461,12 +465,15 @@ void breakpoint_handler(struct pt_regs *regs)
 	slots = this_cpu_ptr(bp_on_reg);
 
 	for (i = 0; i < boot_cpu_data.watch_ireg_count; ++i) {
-		bp = slots[i];
-		if (bp == NULL)
-			continue;
-		perf_bp_event(bp, regs);
+		if ((csr_read32(LOONGARCH_CSR_FWPS) & (0x1 << i))) {
+			bp = slots[i];
+			if (bp == NULL)
+				continue;
+			perf_bp_event(bp, regs);
+			csr_write32(0x1 << i, LOONGARCH_CSR_FWPS);
+			update_bp_registers(regs, 0, 0);
+		}
 	}
-	update_bp_registers(regs, 0, 0);
 }
 NOKPROBE_SYMBOL(breakpoint_handler);
 
@@ -478,12 +485,15 @@ void watchpoint_handler(struct pt_regs *regs)
 	slots = this_cpu_ptr(wp_on_reg);
 
 	for (i = 0; i < boot_cpu_data.watch_dreg_count; ++i) {
-		wp = slots[i];
-		if (wp == NULL)
-			continue;
-		perf_bp_event(wp, regs);
+		if ((csr_read32(LOONGARCH_CSR_MWPS) & (0x1 << i))) {
+			wp = slots[i];
+			if (wp == NULL)
+				continue;
+			perf_bp_event(wp, regs);
+			csr_write32(0x1 << i, LOONGARCH_CSR_MWPS);
+			update_bp_registers(regs, 0, 1);
+		}
 	}
-	update_bp_registers(regs, 0, 1);
 }
 NOKPROBE_SYMBOL(watchpoint_handler);
 
-- 
2.38.1

Re: [PATCH 0/3] LoongArch: ptrace: Fix some watchpoint function

[Huacai Chen]

Hi, Qing and Jinyang,

Could you please help to review this patchset?

And Binbin,

Could you please help to test kgdb with this patchset?

Thank you,
Huacai


On Wed, May 29, 2024 at 9:30 AM Hui Li <lihui@loongson.cn> wrote:
>
> LoongArch defines hardware watchpoint functions for fetch and load/store
> operations. After the software configures the watchpoints for fetch and
> load/store, the processor hardware will monitor the access addresses of
> the fetch and load/store operations and trigger a watchpoint exception
> when the watchpoint setting conditions are met.
>
> Fix some hardware watchpoint issues on user-space and kernel interface.
>
> All changes are made according to the LoongArch Reference Manual:
> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#control-and-status-registers-related-to-watchpoints
>
> Testing for this series can use this gdb patch:
> https://sourceware.org/pipermail/gdb-patches/2024-May/209371.html
>
>
> Hui Li (3):
>   LoongArch: ptrace: Fix watchpoint setting error
>   LoongArch: Trigger user-space watchpoints correctly
>   LoongArch: Fix multiple hardware watchpoint issues
>
>  arch/loongarch/include/asm/hw_breakpoint.h |  2 +-
>  arch/loongarch/kernel/hw_breakpoint.c      | 82 +++++++++++-----------
>  arch/loongarch/kernel/ptrace.c             | 48 +++++++------
>  3 files changed, 71 insertions(+), 61 deletions(-)
>
> --
> 2.38.1
>
>