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

Florian Bezdeka <[email protected]> writes:

> 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.
>

Um, no. I would not do that. Such systemic change is prone to create
regressions due to unwanted side-effects as new call sites may go
unnoticed as they are added over time. I would introduce a specific
pipeline-aware predicate instead, like interrupt_virtually_enabled() or
whatever fits. That way we could do sweeping reviews of
interrupt_enabled() usage periodically, checking whether conversion to
interrupt_virtually_enabled() is needed for those new call sites.

>
> 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?
>

Hw bp/wp handling is not supported on arm ATM, x86 is the only
architecture where the pipeline would officially support this (Jan
worked on this to enable kgdb/x86 many moons ago IIRC). So yes, we need
to mark trap entries if we'd want to enable this for arm as well.

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

There are pros and cons to report the hw vs virtual interrupt state
here. We may need both for debugging actually.

> arch/arm/mm/alignment.c - Memory alignment exception
> Seems this one was wrongly migrated already. mark_trap_entry() called
> too late

mark_trap_entry() is expected to cause a stage switch to in-band if
running oob, possibly switching to other tasks, in addition to
re-enabling hw IRQ, all in the same move.

Since the branch predictor hardening code mitigates Spectre attacks,
running it early - prior to switching back to a user context - looks
right.

In fact, the current scope of the mark_trap_entry/exit section may be
too broad, since the alignment fixup code looks like oob-safe. Such
section might only be required around force_sig_fault().

> and enabling hard IRQs instead of inband unstall.
> Seems easy to fix.

We want hw interrupts to be enabled while fixing up an alignment trap to
decrease latency for oob. Enabling them virtually would help in
maintaining fine-grained native preemption for in-band exclusively.

>
> 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?
>

gic_handle_irq() is called only once for each event, from entry-armv.S
(irq_service_handler set to handle_arch_irq_pipelined -> ... ->
handle_irq_pipelined -> ... -> gic_handle_irq).

irq_desc->handle_irq() is the one which may be called multiple times via
generic_handle_irq_desc(), if we have to propagate an event to the
in-band stage.

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

Yes, that one must be left unchanged. We need to check the hw state in
order to figure out whether a NMI was taken.

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

Are you still referring to gic_handle_irq()? If so, that part merely
decodes the hardware event.

-- 
Philippe.