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:49 +0100, Philippe Gerum wrote:
>> 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
>
> Linux demo 6.19.0-00150-gcb2e9689c529-dirty #232 SMP PREEMPT DOVETAIL Tue Feb 17 09:55:00 CET 2026 aarch64 GNU/Linux
>
> Let me schedule a CI run to see what real devices do.

Nah, this can't work. My patch is giving trash to irqentry_exit()
instead of a proper state.

-- 
Philippe.