Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 6 May 2022 17:06:49 +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 1/2] arm64: entry: Skip single stepping interrupt
 handlers
Message-ID: <20220506160649.ojwc4rqpyhxqyrdo@maple.lan>
References: <20220413065458.88541-1-sumit.garg@linaro.org>
 <20220413065458.88541-2-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220413065458.88541-2-sumit.garg@linaro.org>
Precedence: bulk

On Wed, Apr 13, 2022 at 12:24:57PM +0530, Sumit Garg wrote:
> Current implementation allows single stepping into interrupt handlers
> for interrupts that were received during single stepping. But interrupt
> handlers aren't something that the user expect to debug. Moreover single
> stepping interrupt handlers is risky as it may sometimes leads to
> unbalanced locking when we resume from single-step debug.

Why does single stepping interrupt handlers cause unbalanced locking
(with the current code)?


> Fix broken single-step implementation via skipping single-step over
> interrupt handlers. The methodology is when we receive an interrupt from
> EL1, check if we are single stepping (pstate.SS). If yes then we save
> MDSCR_EL1.SS and clear the register bit if it was set. Then unmask only
> D and leave I set. On return from the interrupt, set D and restore
> MDSCR_EL1.SS. Along with this skip reschedule if we were stepping.

Does this description really explains the motivation here.

Isn't the goal to meet the user's expectation that when they step then
the system will stop at PC+4 relative the instruction they are stepping
(or PC+I for a branch that gets taken)?

To be clear, I've no objections to leaving interrupt handling enabled
when single stepping (AFAIK it is similar to what x86 does) but I think
this patch description will be difficult for future adventurers to
comprehend.


Daniel.


> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  arch/arm64/kernel/entry-common.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 878c65aa7206..dd2d3af615de 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -458,19 +458,35 @@ static __always_inline void __el1_irq(struct pt_regs *regs,
>  	do_interrupt_handler(regs, handler);
>  	irq_exit_rcu();
>  
> -	arm64_preempt_schedule_irq();
> +	/* Don't reschedule in case we are single stepping */
> +	if (!(regs->pstate & DBG_SPSR_SS))
> +		arm64_preempt_schedule_irq();
>  
>  	exit_to_kernel_mode(regs);
>  }
> +
>  static void noinstr el1_interrupt(struct pt_regs *regs,
>  				  void (*handler)(struct pt_regs *))
>  {
> +	unsigned long reg;
> +
> +	/* Disable single stepping within interrupt handler */
> +	if (regs->pstate & DBG_SPSR_SS) {
> +		reg = read_sysreg(mdscr_el1);
> +		write_sysreg(reg & ~DBG_MDSCR_SS, mdscr_el1);
> +	}
> +
>  	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
>  
>  	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
>  		__el1_pnmi(regs, handler);
>  	else
>  		__el1_irq(regs, handler);
> +
> +	if (regs->pstate & DBG_SPSR_SS) {
> +		write_sysreg(DAIF_PROCCTX_NOIRQ | PSR_D_BIT, daif);
> +		write_sysreg(reg, mdscr_el1);
> +	}
>  }
>  
>  asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
> -- 
> 2.25.1