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

[Jinyang He <hejinyang@loongson.cn>]

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);
>   }