1. Patchew
  2. linux
  3. [PATCH v4 0/2] arm64: Fix pending single-step debugging issues
  4. View patch

[PATCH v4 1/2] arm64: entry: Skip single stepping into interrupt handlers

Sumit Garg posted 2 patches 2 years, 9 months ago
There is a newer version of this series
[PATCH v4 1/2] arm64: entry: Skip single stepping into interrupt handlers
Posted by Sumit Garg 2 years, 9 months ago 
Currently on systems where the timer interrupt (or any other
fast-at-human-scale periodic interrupt) is active then it is impossible
to step any code with interrupts unlocked because we will always end up
stepping into the timer interrupt instead of stepping the user code.

The common user's goal while single stepping is that when they step then
the system will stop at PC+4 or PC+I for a branch that gets taken
relative to the instruction they are stepping. So, fix broken single step
implementation via skipping single stepping into 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.

Suggested-by: Will Deacon <will@kernel.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Tested-by: Douglas Anderson <dianders@chromium.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 cce1167199e3..53bcb1902f2f 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -471,19 +471,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.34.1
Re: [PATCH v4 1/2] arm64: entry: Skip single stepping into interrupt handlers
Posted by Mark Rutland 2 years, 9 months ago 
On Thu, Dec 15, 2022 at 07:59:02PM +0530, Sumit Garg wrote:
> Currently on systems where the timer interrupt (or any other
> fast-at-human-scale periodic interrupt) is active then it is impossible
> to step any code with interrupts unlocked because we will always end up
> stepping into the timer interrupt instead of stepping the user code.
> 
> The common user's goal while single stepping is that when they step then
> the system will stop at PC+4 or PC+I for a branch that gets taken
> relative to the instruction they are stepping. So, fix broken single step
> implementation via skipping single stepping into 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.
> 
> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> Tested-by: Douglas Anderson <dianders@chromium.org>

FWIW, this looks reasonable to me; I have a couple of minor style/structure
comments below.

> ---
>  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 cce1167199e3..53bcb1902f2f 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -471,19 +471,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();

Please change arm64_preempt_schedule_irq() to take the regs as an argument, and
put this test inside arm64_preempt_schedule_irq(). That way all the
decision-making about whether to preempt is in one place.

That can go immediately after the need_irq_preemption() test.

>  
>  	exit_to_kernel_mode(regs);
>  }
> +
>  static void noinstr el1_interrupt(struct pt_regs *regs,
>  				  void (*handler)(struct pt_regs *))
>  {
> +	unsigned long reg;

Please s/reg/mdscr/. That way it's harder to confuse with 'regs', it's clearer
that it's the MDSCR_ELx value, and if we have to save/restore any other
registers in future it'll be obvious how to name things.

Thanks,
Mark.

> +
> +	/* 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.34.1
>
Re: [PATCH v4 1/2] arm64: entry: Skip single stepping into interrupt handlers
Posted by Sumit Garg 2 years, 9 months ago 
 On Fri, 16 Dec 2022 at 22:14, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Thu, Dec 15, 2022 at 07:59:02PM +0530, Sumit Garg wrote:
> > Currently on systems where the timer interrupt (or any other
> > fast-at-human-scale periodic interrupt) is active then it is impossible
> > to step any code with interrupts unlocked because we will always end up
> > stepping into the timer interrupt instead of stepping the user code.
> >
> > The common user's goal while single stepping is that when they step then
> > the system will stop at PC+4 or PC+I for a branch that gets taken
> > relative to the instruction they are stepping. So, fix broken single step
> > implementation via skipping single stepping into 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.
> >
> > Suggested-by: Will Deacon <will@kernel.org>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > Tested-by: Douglas Anderson <dianders@chromium.org>
>
> FWIW, this looks reasonable to me; I have a couple of minor style/structure
> comments below.

Thanks Mark for your review.

>
> > ---
> >  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 cce1167199e3..53bcb1902f2f 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -471,19 +471,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();
>
> Please change arm64_preempt_schedule_irq() to take the regs as an argument, and
> put this test inside arm64_preempt_schedule_irq(). That way all the
> decision-making about whether to preempt is in one place.
>
> That can go immediately after the need_irq_preemption() test.

Okay, I will change arm64_preempt_schedule_irq() accordingly.

>
> >
> >       exit_to_kernel_mode(regs);
> >  }
> > +
> >  static void noinstr el1_interrupt(struct pt_regs *regs,
> >                                 void (*handler)(struct pt_regs *))
> >  {
> > +     unsigned long reg;
>
> Please s/reg/mdscr/. That way it's harder to confuse with 'regs', it's clearer
> that it's the MDSCR_ELx value, and if we have to save/restore any other
> registers in future it'll be obvious how to name things.
>

Ack.

-Sumit

> Thanks,
> Mark.
>
> > +
> > +     /* 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.34.1
> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.