[PATCH 0/2] arm: Review irq pipeline

[PATCH 0/2] arm: Review irq pipeline

[Florian Bezdeka]

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?

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.

To: xenomai@lists.linux.dev
Cc: Philippe Gerum <rpm@xenomai.org>
Cc: Tobias Schaffner <tobias.schaffner@siemens.com>

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
Florian Bezdeka (2):
      arm: irq_pipeline: Fix fault_{entry,exit} imbalance
      arm: irq_pipeline: Fix fault_{entry,exit} imbalance

 arch/arm/mm/fault.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
---
base-commit: 5c4243abc449083b74c94fcd353d360b6ce78b5b
change-id: 20260603-wip-flo-v7-1-arm-fixups-25d58fb72f3e

Best regards,
-- 
Florian Bezdeka <florian.bezdeka@siemens.com>

[PATCH 1/2] arm: irq_pipeline: Fix fault_{entry,exit} imbalance

[Florian Bezdeka]

upstream commit
d92725256b4f ("mm: avoid unnecessary page fault retires on shared memory types")
introduced a new exit path to do_page_fault(), so that an imbalance
of fault_{entry,exit} can happen.

The commit was introduced with Linux 6.0, so all newer Dovetail
versions are affected.

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
 arch/arm/mm/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index b4bb33e1638cd67ce2b6c0b90a4a9693e784bb3e..4beae304c0269d11173cd3b360cec05e59f30d82 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -539,7 +539,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 
 	/* The fault is fully completed (including releasing mmap lock) */
 	if (fault & VM_FAULT_COMPLETED)
-		return 0;
+		goto out;
 
 	if (!(fault & VM_FAULT_ERROR)) {
 		if (fault & VM_FAULT_RETRY) {

-- 
2.54.0

Re: [PATCH 1/2] arm: irq_pipeline: Fix fault_{entry,exit} imbalance

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> writes:

> upstream commit
> d92725256b4f ("mm: avoid unnecessary page fault retires on shared memory types")
> introduced a new exit path to do_page_fault(), so that an imbalance
> of fault_{entry,exit} can happen.
>
> The commit was introduced with Linux 6.0, so all newer Dovetail
> versions are affected.
>
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
>  arch/arm/mm/fault.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index b4bb33e1638cd67ce2b6c0b90a4a9693e784bb3e..4beae304c0269d11173cd3b360cec05e59f30d82 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -539,7 +539,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  
>  	/* The fault is fully completed (including releasing mmap lock) */
>  	if (fault & VM_FAULT_COMPLETED)
> -		return 0;
> +		goto out;
>  
>  	if (!(fault & VM_FAULT_ERROR)) {
>  		if (fault & VM_FAULT_RETRY) {

Merged, thanks.

-- 
Philippe.

[PATCH 2/2] arm: irq_pipeline: Fix fault_{entry,exit} imbalance

[Florian Bezdeka]

upstream commit
c16af1212479 ("ARM: 9328/1: mm: try VMA lock-based page fault handling first")
introduced a new exit path to do_page_fault(), so that an imbalance
of fault_{entry,exit} can happen.

The commit was introduced with Linux 6.8, so all newer Dovetail
versions are affected.

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
 arch/arm/mm/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 4beae304c0269d11173cd3b360cec05e59f30d82..49f5d628c4c9a91a3e879af0d541fd76665d78fe 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -502,7 +502,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	if (fault_signal_pending(fault, regs)) {
 		if (!user_mode(regs))
 			goto no_context;
-		return 0;
+		goto out;
 	}
 lock_mmap:
 

-- 
2.54.0

Re: [PATCH 2/2] arm: irq_pipeline: Fix fault_{entry,exit} imbalance

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> writes:

> upstream commit
> c16af1212479 ("ARM: 9328/1: mm: try VMA lock-based page fault handling first")
> introduced a new exit path to do_page_fault(), so that an imbalance
> of fault_{entry,exit} can happen.
>
> The commit was introduced with Linux 6.8, so all newer Dovetail
> versions are affected.
>
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
>  arch/arm/mm/fault.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index 4beae304c0269d11173cd3b360cec05e59f30d82..49f5d628c4c9a91a3e879af0d541fd76665d78fe 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -502,7 +502,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  	if (fault_signal_pending(fault, regs)) {
>  		if (!user_mode(regs))
>  			goto no_context;
> -		return 0;
> +		goto out;
>  	}
>  lock_mmap:

Merged, thanks.

-- 
Philippe.

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

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> 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.

-- 
Philippe.

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

[Florian Bezdeka]

On Wed, 2026-06-03 at 20:38 +0200, Philippe Gerum wrote:
> Florian Bezdeka <florian.bezdeka@siemens.com> 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.

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

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> writes:

> On Wed, 2026-06-03 at 20:38 +0200, Philippe Gerum wrote:
>> Florian Bezdeka <florian.bezdeka@siemens.com> 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.