Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 6 May 2022 17:12:58 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     linux-arm-kernel@lists.infradead.org, dianders@chromium.org,
        will@kernel.org, liwei391@huawei.com, catalin.marinas@arm.com,
        mark.rutland@arm.com, mhiramat@kernel.org,
        jason.wessel@windriver.com, maz@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable
 single-step
Message-ID: <20220506161258.p5jztdy33hlua3bu@maple.lan>
References: <20220413065458.88541-1-sumit.garg@linaro.org>
 <20220413065458.88541-3-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220413065458.88541-3-sumit.garg@linaro.org>
Precedence: bulk

On Wed, Apr 13, 2022 at 12:24:58PM +0530, Sumit Garg wrote:
> After fixing wrongly single-stepping into the irq handler, when we execute
> single-step in kdb/kgdb, we can see only the first step can work.

I might be nitpicking since, again, I've no problems with the code
but... I'd rather this patch description focused on what this patch
does rather than what the patch before it does!

Something more like:

  Currently only the first attempt to single-step has any effect.
  After that all further stepping remains "stuck" at the same program
  counter value.


Daniel.


> Refer to the ARM Architecture Reference Manual (ARM DDI 0487E.a) D2.12,
> i think PSTATE.SS=1 should be set each step for transferring the PE to the
> 'Active-not-pending' state. The problem here is PSTATE.SS=1 is not set
> since the second single-step.
> 
> After the first single-step, the PE transferes to the 'Inactive' state,
> with PSTATE.SS=0 and MDSCR.SS=1, thus PSTATE.SS won't be set to 1 due to
> kernel_active_single_step()=true. Then the PE transferes to the
> 'Active-pending' state when ERET and returns to the debugger by step
> exception.
> 
> Before this patch:
> ==================
> Entering kdb (current=0xffff3376039f0000, pid 1) on processor 0 due to Keyboard Entry
> [0]kdb>
> 
> [0]kdb>
> [0]kdb> bp write_sysrq_trigger
> Instruction(i) BP #0 at 0xffffa45c13d09290 (write_sysrq_trigger)
>     is enabled   addr at ffffa45c13d09290, hardtype=0 installed=0
> 
> [0]kdb> go
> $ echo h > /proc/sysrq-trigger
> 
> Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to Breakpoint @ 0xffffad651a309290
> [1]kdb> ss
> 
> Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294
> [1]kdb> ss
> 
> Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294
> [1]kdb>
> 
> After this patch:
> =================
> Entering kdb (current=0xffff6851c39f0000, pid 1) on processor 0 due to Keyboard Entry
> [0]kdb> bp write_sysrq_trigger
> Instruction(i) BP #0 at 0xffffc02d2dd09290 (write_sysrq_trigger)
>     is enabled   addr at ffffc02d2dd09290, hardtype=0 installed=0
> 
> [0]kdb> go
> $ echo h > /proc/sysrq-trigger
> 
> Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to Breakpoint @ 0xffffc02d2dd09290
> [1]kdb> ss
> 
> Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09294
> [1]kdb> ss
> 
> Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09298
> [1]kdb> ss
> 
> Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd0929c
> [1]kdb>
> 
> Fixes: 44679a4f142b ("arm64: KGDB: Add step debugging support")
> Co-developed-by: Wei Li <liwei391@huawei.com>
> Signed-off-by: Wei Li <liwei391@huawei.com>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  arch/arm64/include/asm/debug-monitors.h | 1 +
>  arch/arm64/kernel/debug-monitors.c      | 5 +++++
>  arch/arm64/kernel/kgdb.c                | 2 ++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 00c291067e57..9e1e864d6440 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -104,6 +104,7 @@ void user_regs_reset_single_step(struct user_pt_regs *regs,
>  void kernel_enable_single_step(struct pt_regs *regs);
>  void kernel_disable_single_step(void);
>  int kernel_active_single_step(void);
> +void kernel_regs_reset_single_step(struct pt_regs *regs);
>  
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>  int reinstall_suspended_bps(struct pt_regs *regs);
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 4f3661eeb7ec..ea3f410aa385 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -438,6 +438,11 @@ int kernel_active_single_step(void)
>  }
>  NOKPROBE_SYMBOL(kernel_active_single_step);
>  
> +void kernel_regs_reset_single_step(struct pt_regs *regs)
> +{
> +	set_regs_spsr_ss(regs);
> +}
> +
>  /* ptrace API */
>  void user_enable_single_step(struct task_struct *task)
>  {
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 2aede780fb80..acf2196b1e9b 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -224,6 +224,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
>  		 */
>  		if (!kernel_active_single_step())
>  			kernel_enable_single_step(linux_regs);
> +		else
> +			kernel_regs_reset_single_step(linux_regs);
>  		err = 0;
>  		break;
>  	default:
> -- 
> 2.25.1