Re: [PATCH Dovetail 2/2] arm64: irq_pipeline: Fix the demotion checks for el0 and el1 IRQs

Florian Bezdeka <[email protected]> writes:

> On Tue, 2026-02-17 at 09:12 +0100, Florian Bezdeka wrote:
>> On Tue, 2026-02-17 at 09:00 +0100, Philippe Gerum wrote:
>> > Florian Bezdeka <[email protected]> writes:
>> > 
>> > > While reviewing some of the low level pipeline code I realized that
>> > > the checks for task demotion are wrong on arm64.
>> > > 
>> > > el0: The demotion check was missing in the oob case. We have to check
>> > >      for running_inband() only as user_mode(regs) will always be true.
>> > >      We are serving an IRQ over el0, so application / user mode.
>> > > 
>> > > el1: The demotion check was unnecessary and "inactive" as
>> > >      user_mode(regs) is never true on el1, we are serving an IRQ over
>> > >      kernel mode.
>> > > 
>> > 
>> > If the demotion happens from el1, you still want to check for a
>> > rescheduling opportunity (i.e. kernel preemption case).
>> > 
>> > > Signed-off-by: Florian Bezdeka <[email protected]>
>> > > ---
>> > >  arch/arm64/kernel/entry-common.c | 8 ++++----
>> > >  1 file changed, 4 insertions(+), 4 deletions(-)
>> > > 
>> > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
>> > > index 07fa70713ce04eaf3df9223354babcecde923280..9c99eb3d18c459a90e1f7ce4e4c307e235e457a2 100644
>> > > --- a/arch/arm64/kernel/entry-common.c
>> > > +++ b/arch/arm64/kernel/entry-common.c
>> > > @@ -73,6 +73,10 @@ static noinstr void arm64_pipeline_el0_irq(struct pt_regs *regs,
>> > >  	do_interrupt_handler(regs, handler);
>> > >  	/* Done, unwind now. */
>> > >  	handle_irq_pipelined_finish(prevd, regs);
>> > > +	if (running_inband()) {
>> > > +		stall_inband_nocheck();
>> > > +		irqentry_exit_to_user_mode(regs);
>> > > +	}
>> > >  	instrumentation_end();
>> > >  	arm64_exit_to_user_mode(regs);
>> > >  }
>> > > @@ -90,10 +94,6 @@ static noinstr void arm64_pipeline_el1_irq(struct pt_regs *regs,
>> > >  		prevd = handle_irq_pipelined_prepare(regs);
>> > >  		do_interrupt_handler(regs, handler);
>> > >  		handle_irq_pipelined_finish(prevd, regs);
>> > > -		if (running_inband() && user_mode(regs)) {
>> > > -			stall_inband_nocheck();
>> > > -			irqentry_exit_to_user_mode(regs);
>> > > -		}
>> > >  		instrumentation_end();
>> > >  		mte_check_tfsr_exit();
>> > >  		return;
>> > 
>> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
>> > index e0227d467cef4..770fc4059e083 100644
>> > --- a/arch/arm64/kernel/entry-common.c
>> > +++ b/arch/arm64/kernel/entry-common.c
>> > @@ -90,10 +90,8 @@ static noinstr void arm64_pipeline_el1_irq(struct pt_regs *regs,
>> >  		prevd = handle_irq_pipelined_prepare(regs);
>> >  		do_interrupt_handler(regs, handler);
>> >  		handle_irq_pipelined_finish(prevd, regs);
>> > -		if (running_inband() && user_mode(regs)) {
>> > -			stall_inband_nocheck();
>> > -			irqentry_exit_to_user_mode(regs);
>> > -		}
>> > +		if (running_inband())
>> > +			goto out_irqentry;
>> >  		instrumentation_end();
>> >  		mte_check_tfsr_exit();
>> >  		return;
>> > @@ -109,6 +107,7 @@ static noinstr void arm64_pipeline_el1_irq(struct pt_regs *regs,
>> >  	trace_hardirqs_on();
>> >  	unstall_inband_nocheck();
>> >  	handle_irq_pipelined_finish(prevd, regs);
>> > +out_irqentry:
>> >  	stall_inband_nocheck();
>> >  	trace_hardirqs_off();
>> >  	instrumentation_end();
>> > 
>> 
>> That would look a bit imbalanced. We would have one additional
>> trace_hardirqs_off() and one irqentry_exit() call without counterparts.
>> 
>> Is that really OK? Testing...
>
> Nope, doesn't boot up on my arm64 qemu.
>

This patch runs fine on my end, survives the full evl test suite,
including the hectic test. Which kernel preemption model are you using?
mine is this one:

Linux qemu-arm64-evl 6.18.7-00979-gb2cf536d0898-dirty #286 SMP PREEMPT_RT EVL Tue Feb 17 09:45:50 CET 2026 aarch64 GNU/Linux

-- 
Philippe.