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

WFlorian 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.
>
> Comparing with x86 again, I think that my proposal is "correct" in terms
> of identical to what x86 does. Do all architectures have a gap here?

x86 has a single implementation for both user and kernel preemption
paths, arm64 has two since the privilege level is explicitly stated by
the irq handler being called, but this still must translate identically
logically speaking. Your implementation is missing the kernel preemption
path after demotion.

i.e. when checking for running_oob() || irqs_disabled(), the cases
covered are:

(1) in-band user path on entry (implies !irqs_disabled())
(2) oob user path on entry (might be demoted)
(3) (virtually) stalled in-band kernel path on entry (implies no reschedule,
  filtered out by irqentry_exit())
(4) oob kernel path on entry (might be demoted)

Therefore, with your patch in, el1 is now missing (4).

-- 
Philippe.