Re: [PATCH 0/2] arm: Review irq pipeline

On Wed, 2026-06-03 at 20:38 +0200, Philippe Gerum wrote:
> Florian Bezdeka <[email protected]> writes:
> 
> > Hi Philippe,
> > 
> > This is the result of a review of the arm implementation of the IRQ
> > pipeline. Triggered by some risc-v reviews.
> > 
> > I'm sure about the two easy fixes, targeting different Dovetail
> > versions, so split into different patches.
> > 
> > In addition there is one more, I need your input here:
> > 
> > We have two calls to interrupts_enabled() in arch/arm/mm/fault.c,
> > both in the do_page_fault() path.
> > 
> > Assuming that we are handling a page fault over kernel space,
> > inband stage stalled, we would leave the kernel with the inband stage
> > unstalled, as interrupts_enabled() is checking the hardware state (as
> > pushed to the stack on entry) instead of the inband stage stall bit.
> > 
> > Is that correct?
> > 
> 
> Nope.
> 
> > I'm quite sure we have more real problems that in this case, but
> > maybe we should fix that up?
> > 
> > We would have to call something that does the HW check in case
> > CONFIG_IRQ_PIPELINE is off, but checks the stall bit in case pipelining
> > is enabled.
> > 
> 
> Instead of checking for interrupts_enabled(regs) in do_page_fault() and
> do_kernel_address_page_fault(), we should rather check for
> !(arch_kentry_get_irqstate(regs) & KENTRY_STALL_BIT) when pipelining,
> which would give us the virtual interrupt state on entry, provided we
> run in-band.
> 
> Inner issue: since do_kernel_address_page_fault() skipped fault_entry()
> on trap from supervisor mode, we might still be running oob, calling
> local_irq_enable() in such a case would be wrong.

Agreed.

I was starting to change the interrupts_enabled() implementation for the
pipelining case, which would affect all users. So I had to check them
first.


I found the following:

arch/arm/kernel/hw_breakpoint.c - hardware break/watchpoint exception:
Are we able to handle that exception from the oob stage?
The implementation would unconditionally change the inband IRQ state.
Missing fault_{entry,exit} with new trap type?

arch/arm/kernel/process.c - reporting
I guess we should report the inband IRQ state, no?
Would be fixed "automatically".

arch/arm/mm/alignment.c - Memory alignment exception
Seems this one was wrongly migrated already. mark_trap_entry() called
too late and enabling hard IRQs instead of inband unstall.
Seems easy to fix.

arch/arm/mm/fault.c - As discussed
Would be fixed "automatically".

drivers/irqchip/irq-gic-v3.c - global IRQ handler
That's the one that bothers me most as it is not limited to arm (used by
arm64 as well) and should be called twice during IRQ flow. Once for
handling the HW IRQ, once for IRQ log sync. Correct?

That user has to stay untouched, always checking the hardware state,
right?

My understanding is that we "fix" or prepare the IRQ states accordingly
before starting IRQ log synchronization.