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 11:11 +0100, Philippe Gerum wrote:
>> Philippe Gerum <[email protected]> writes:
>> 
>> > Florian Bezdeka <[email protected]> writes:
>> > 
>> > > On Tue, 2026-02-17 at 09:41 +0100, Philippe Gerum wrote:
>> > > > > 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).
>> > > 
>> > > Right, but I'm wondering if x86 ignores this case as well.
>> > > 
>> > > After demotion of a oob kernel path entry, user_mode() should still be
>> > > false - bypassing the call to irqentry_exit_to_user_mode() - No?
>> > 
>> > Yes, x86 assumes that a kernel path demoted to in-band is going to cross
>> > an IRQ synchronization point shortly after on return to the preempted
>> > context. Now, with hindsight, the question is: are we 100% certain of
>> > that? Any real (hw) IRQ over the in-band stage would trigger the
>> > synchronization as expected, but a synthetic one posted from the oob
>> > stage might linger if this assumption ends up being wrong. I need to
>> > have a second look at this code.
>> 
>> Which means that your latest patch series is ok and complete. The change
>> if any would most likely happen in the generic pipeline bits.
>
> I think (2) is still wrong on arm64 now, so:
>
> @@ -65,15 +65,17 @@ static noinstr void arm64_pipeline_el0_irq(struct
> pt_regs *regs,
>                                 void (*handler)(struct pt_regs *))
>  {
>         struct irq_stage_data *prevd;
> +       bool oob_on_enter;
>  
>         arm64_enter_from_user_mode(regs);
>         instrumentation_begin();
>         /* Prep for handling, switching oob if needed. */
> +       oob_on_enter = running_oob();
>         prevd = handle_irq_pipelined_prepare(regs);
>         do_interrupt_handler(regs, handler);
>         /* Done, unwind now. */
>         handle_irq_pipelined_finish(prevd, regs);
> -       if (running_inband()) {
> +       if (oob_on_enter && running_inband()) {
>                 stall_inband_nocheck();
>                 irqentry_exit_to_user_mode(regs);
>         }
>
> Do you agree?
>

On a second look, _el0 needs no particular handling of the demotion
case, because arm64_exit_to_user_mode() already does what is required.

-- 
Philippe.