[PATCH RFC Dovetail 6.16 0/5] Dovetail: Minor cleanups found while debugging the pipeline entry code

[PATCH RFC Dovetail 6.16 0/5] Dovetail: Minor cleanups found while debugging the pipeline entry code

[Florian Bezdeka]

Hi Philippe,

this is now the end of my queue. Nothing more pending.

All of that was found / noticed while debugging the pipeline entry code
some weeks back. Maybe you can scan that and let me know which parts you
like to have / take.

Cc: Philippe Gerum <rpm@xenomai.org>

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
Florian Bezdeka (5):
      kernel/entry: irq_pipeline: Do not leave .noinstr.text section
      kernel/irq/chip: Change return value of get_flow_step() to int
      kernel/irq/chip: Simplify may_start_flow()
      kernel/irq/chip: Simplify should_feed_pipeline()
      kernel/irq/chip: Do not call low level irq chip hooks directly

 kernel/entry/common.c |  4 ++--
 kernel/irq/chip.c     | 29 ++++++++++++-----------------
 2 files changed, 14 insertions(+), 19 deletions(-)
---
base-commit: 138f1183deed300fd1f03abb6511635a916b98d0
change-id: 20250925-wip-flo-cleanups-based-on-6-16-89989cc95468

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

[PATCH RFC Dovetail 6.16 1/5] kernel/entry: irq_pipeline: Do not leave .noinstr.text section

[Florian Bezdeka]

Fixing the following warning by moving the call to irqentry_syncstage()
into a instrumentation_begin() section.

vmlinux.o: warning: objtool: irqentry_exit+0xab: call to synchronize_pipeline() leaves .noinstr.text section

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
 kernel/entry/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 583dcef13c7c0..284d41cb1c918 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -393,6 +393,7 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
 		return;
 	}
 
+	instrumentation_begin();
 	synchronized = irqentry_syncstage(state);
 
 	if (irqexit_may_preempt_schedule(state, regs)) {
@@ -402,7 +403,6 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
 		 * and RCU as the return to user mode path.
 		 */
 		if (state.exit_rcu) {
-			instrumentation_begin();
 			/* Tell the tracer that IRET will enable interrupts */
 			trace_hardirqs_on_prepare();
 			lockdep_hardirqs_on_prepare();
@@ -412,7 +412,6 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
 			goto out;
 		}
 
-		instrumentation_begin();
 		if (IS_ENABLED(CONFIG_PREEMPTION))
 			irqentry_exit_cond_resched();
 
@@ -420,6 +419,7 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
 		trace_hardirqs_on();
 		instrumentation_end();
 	} else {
+		instrumentation_end();
 		/*
 		 * IRQ flags state is correct already. Just tell RCU if it
 		 * was not watching on entry.

-- 
2.39.5

Re: [PATCH RFC Dovetail 6.16 1/5] kernel/entry: irq_pipeline: Do not leave .noinstr.text section

[Philippe Gerum]

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

> Fixing the following warning by moving the call to irqentry_syncstage()
> into a instrumentation_begin() section.
>
> vmlinux.o: warning: objtool: irqentry_exit+0xab: call to synchronize_pipeline() leaves .noinstr.text section
>
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
>  kernel/entry/common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index 583dcef13c7c0..284d41cb1c918 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -393,6 +393,7 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
>  		return;
>  	}
>  
> +	instrumentation_begin();
>  	synchronized = irqentry_syncstage(state);
>

This means that we won't have ftracing or anything that depends on
instrumentation in any inband irq handler triggered during stage sync
from this code path.

-- 
Philippe.

Re: [PATCH RFC Dovetail 6.16 1/5] kernel/entry: irq_pipeline: Do not leave .noinstr.text section

[Florian Bezdeka]

On Fri, 2025-09-26 at 10:49 +0200, Philippe Gerum wrote:
> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> 
> > Fixing the following warning by moving the call to irqentry_syncstage()
> > into a instrumentation_begin() section.
> > 
> > vmlinux.o: warning: objtool: irqentry_exit+0xab: call to synchronize_pipeline() leaves .noinstr.text section
> > 
> > Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> > ---
> >  kernel/entry/common.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> > index 583dcef13c7c0..284d41cb1c918 100644
> > --- a/kernel/entry/common.c
> > +++ b/kernel/entry/common.c
> > @@ -393,6 +393,7 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
> >  		return;
> >  	}
> >  
> > +	instrumentation_begin();
> >  	synchronized = irqentry_syncstage(state);
> > 
> 
> This means that we won't have ftracing or anything that depends on
> instrumentation in any inband irq handler triggered during stage sync
> from this code path.

Hm. Really? The instrumentation is now enabled before running into
irqendtry_syncstage(). No?

Seems I have some issues to fully understand that instrumentation
thingy. Let me double check...

> 
> -- 
> Philippe.

Re: [PATCH RFC Dovetail 6.16 1/5] kernel/entry: irq_pipeline: Do not leave .noinstr.text section

[Philippe Gerum]

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

> On Fri, 2025-09-26 at 10:49 +0200, Philippe Gerum wrote:
>> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>> 
>> > Fixing the following warning by moving the call to irqentry_syncstage()
>> > into a instrumentation_begin() section.
>> > 
>> > vmlinux.o: warning: objtool: irqentry_exit+0xab: call to synchronize_pipeline() leaves .noinstr.text section
>> > 
>> > Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
>> > ---
>> >  kernel/entry/common.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/kernel/entry/common.c b/kernel/entry/common.c
>> > index 583dcef13c7c0..284d41cb1c918 100644
>> > --- a/kernel/entry/common.c
>> > +++ b/kernel/entry/common.c
>> > @@ -393,6 +393,7 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
>> >  		return;
>> >  	}
>> >  
>> > +	instrumentation_begin();
>> >  	synchronized = irqentry_syncstage(state);
>> > 
>> 
>> This means that we won't have ftracing or anything that depends on
>> instrumentation in any inband irq handler triggered during stage sync
>> from this code path.
>
> Hm. Really? The instrumentation is now enabled before running into
> irqendtry_syncstage(). No?
>
> Seems I have some issues to fully understand that instrumentation
> thingy. Let me double check...

Nope, I got this backwards, your code is right. /me multiplexing way too
much ATM.

-- 
Philippe.

[PATCH RFC Dovetail 6.16 2/5] kernel/irq/chip: Change return value of get_flow_step() to int

[Florian Bezdeka]

IRQ_FLOW_* is modeled as enum, so the underlying type is int.
All callers were already using an int typed variable to store
the result.

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

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 12f520f6a1972..5774fd96ec6c5 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -578,7 +578,7 @@ static irqreturn_t forward_irq_event(struct irq_desc *desc)
  *   IRQ_FLOW_REPLAY since only the oob handler may request
  *   forwarding.
  */
-static inline unsigned int get_flow_step(struct irq_desc *desc)
+static inline int get_flow_step(struct irq_desc *desc)
 {
 	if (likely(!irqs_pipelined() || in_pipeline()))
 		return IRQ_FLOW_START;

-- 
2.39.5

[PATCH RFC Dovetail 6.16 3/5] kernel/irq/chip: Simplify may_start_flow()

[Florian Bezdeka]

There was something like IRQ_FLOW_PILEUP in the past which does no
longer exist. We can now clean up and simplify may_start_flow().
No functional change.

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

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 5774fd96ec6c5..8593cdfb6e2ca 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -479,7 +479,7 @@ enum {
 
 static inline bool may_start_flow(int flow)
 {
-	return flow != IRQ_FLOW_REPLAY && flow != IRQ_FLOW_FORWARD;
+	return flow == IRQ_FLOW_START;
 }
 
 void irq_clear_deferral(struct irq_desc *desc)

-- 
2.39.5

Re: [PATCH RFC Dovetail 6.16 3/5] kernel/irq/chip: Simplify may_start_flow()

[Philippe Gerum]

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

> There was something like IRQ_FLOW_PILEUP in the past which does no
> longer exist. We can now clean up and simplify may_start_flow().
> No functional change.
>
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
>  kernel/irq/chip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 5774fd96ec6c5..8593cdfb6e2ca 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -479,7 +479,7 @@ enum {
>  
>  static inline bool may_start_flow(int flow)
>  {
> -	return flow != IRQ_FLOW_REPLAY && flow != IRQ_FLOW_FORWARD;
> +	return flow == IRQ_FLOW_START;
>  }
>  
>  void irq_clear_deferral(struct irq_desc *desc)

Ack.

-- 
Philippe.

[PATCH RFC Dovetail 6.16 4/5] kernel/irq/chip: Simplify should_feed_pipeline()

[Florian Bezdeka]

No functional change.

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
 kernel/irq/chip.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 8593cdfb6e2ca..f18f513e20c7d 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -628,15 +628,13 @@ static inline bool should_feed_pipeline(struct irq_desc *desc, int state)
 	switch (state) {
 	case IRQ_FLOW_REPLAY:	/* Deferred to in-band. */
 		irq_clear_deferral(desc);
-		break;
+		return false;
 	case IRQ_FLOW_FORWARD:	/* Handled oob, forwarded in-band. */
 		irq_clear_forward(desc);
-		break;
+		return false;
 	default:
 		return true;
 	}
-
-	return false;
 }
 
 #ifdef CONFIG_IRQ_PIPELINE

-- 
2.39.5

[PATCH RFC Dovetail 6.16 5/5] kernel/irq/chip: Do not call low level irq chip hooks directly

[Florian Bezdeka]

irq_mask() and irq_unmask() are tracking a software IRQ state that
might run out of sync when bypassing them.

No functional change.

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
 kernel/irq/chip.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index f18f513e20c7d..280f1526f0f8f 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1125,8 +1125,8 @@ void handle_percpu_irq(struct irq_desc *desc)
 			handled = handle_oob_irq(desc);
 			if (chip->irq_eoi)
 				chip->irq_eoi(&desc->irq_data);
-			if (!handled && chip->irq_mask)
-				chip->irq_mask(&desc->irq_data);
+			if (!handled)
+				mask_irq(desc);
 		} else if (flow == IRQ_FLOW_FORWARD) {
 			handle_irq_event_percpu(desc);
 		} else {
@@ -1137,11 +1137,9 @@ void handle_percpu_irq(struct irq_desc *desc)
 			 * off, which is not the case on replay of
 			 * unserialized percpu irqs: fix this up.
 			 */
-			if (chip->irq_unmask) {
-				flags = hard_cond_local_irq_save();
-				chip->irq_unmask(&desc->irq_data);
-				hard_cond_local_irq_restore(flags);
-			}
+			flags = hard_cond_local_irq_save();
+			unmask_irq(desc);
+			hard_cond_local_irq_restore(flags);
 		}
 		return;
 	}
@@ -1190,8 +1188,8 @@ void handle_percpu_devid_irq(struct irq_desc *desc)
 			handled = handle_oob_irq(desc);
 			if (chip->irq_eoi)
 				chip->irq_eoi(&desc->irq_data);
-			if (!handled && chip->irq_mask)
-				chip->irq_mask(&desc->irq_data);
+			if (!handled)
+				mask_irq(desc);
 			return;
 		} else if (flow == IRQ_FLOW_FORWARD) {
 			handle_irq_event_percpu(desc);
@@ -1229,8 +1227,7 @@ void handle_percpu_devid_irq(struct irq_desc *desc)
 		 * percpu irqs: fix this up.
 		 */
 		flags = hard_cond_local_irq_save();
-		if (chip->irq_unmask)
-			chip->irq_unmask(&desc->irq_data);
+		unmask_irq(desc);
 		hard_cond_local_irq_restore(flags);
 	} else if (chip->irq_eoi) {
 		chip->irq_eoi(&desc->irq_data);

-- 
2.39.5

Re: [PATCH RFC Dovetail 6.16 5/5] kernel/irq/chip: Do not call low level irq chip hooks directly

[Philippe Gerum]

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

> irq_mask() and irq_unmask() are tracking a software IRQ state that
> might run out of sync when bypassing them.
>
> No functional change.
>

Actually, there is. Percpu IRQs are not serialized by the desc->lock, so
calling mask_irq/unmask_irq in these handlers is unsafe since we may end
up flipping bits from the irqd state concurrently on multiple CPUs for
the same descriptor. This is the reason why masking/unmasking was
open-coded there.  This patch typically breaks my kvm/arm64 fixture at
boot, not observed on kvm/x86 so far though.

-- 
Philippe.

Re: [PATCH RFC Dovetail 6.16 5/5] kernel/irq/chip: Do not call low level irq chip hooks directly

[Philippe Gerum]

Philippe Gerum <rpm@xenomai.org> writes:

> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>
>> irq_mask() and irq_unmask() are tracking a software IRQ state that
>> might run out of sync when bypassing them.
>>
>> No functional change.
>>
>
> Actually, there is. Percpu IRQs are not serialized by the desc->lock, so
> calling mask_irq/unmask_irq in these handlers is unsafe since we may end
> up flipping bits from the irqd state concurrently on multiple CPUs for
> the same descriptor. This is the reason why masking/unmasking was
> open-coded there.  This patch typically breaks my kvm/arm64 fixture at
> boot, not observed on kvm/x86 so far though.

With respect to the IRQ state consistency, the logic assumes that the
percpu IRQ is unmasked in the descriptor while its handler runs, so not
mirroring the hardware state into the irqd state word is correct.

-- 
Philippe.

Re: [PATCH RFC Dovetail 6.16 5/5] kernel/irq/chip: Do not call low level irq chip hooks directly

[Florian Bezdeka]

On Sun, 2025-09-28 at 10:12 +0200, Philippe Gerum wrote:
> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> 
> > irq_mask() and irq_unmask() are tracking a software IRQ state that
> > might run out of sync when bypassing them.
> > 
> > No functional change.
> > 
> 
> Actually, there is. Percpu IRQs are not serialized by the desc->lock, so
> calling mask_irq/unmask_irq in these handlers is unsafe since we may end
> up flipping bits from the irqd state concurrently on multiple CPUs for
> the same descriptor. This is the reason why masking/unmasking was
> open-coded there.  This patch typically breaks my kvm/arm64 fixture at
> boot, not observed on kvm/x86 so far though.
> 

I clearly missed the serialization part. Wondering why this did not
trigger on any board / kvm test...

Anyway, it's clear that this patch should be dropped.

Do you want me to resend the series with 5/5 dropped?

Florian

Re: [PATCH RFC Dovetail 6.16 5/5] kernel/irq/chip: Do not call low level irq chip hooks directly

[Philippe Gerum]

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

> On Sun, 2025-09-28 at 10:12 +0200, Philippe Gerum wrote:
>> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>> 
>> > irq_mask() and irq_unmask() are tracking a software IRQ state that
>> > might run out of sync when bypassing them.
>> > 
>> > No functional change.
>> > 
>> 
>> Actually, there is. Percpu IRQs are not serialized by the desc->lock, so
>> calling mask_irq/unmask_irq in these handlers is unsafe since we may end
>> up flipping bits from the irqd state concurrently on multiple CPUs for
>> the same descriptor. This is the reason why masking/unmasking was
>> open-coded there.  This patch typically breaks my kvm/arm64 fixture at
>> boot, not observed on kvm/x86 so far though.
>> 
>
> I clearly missed the serialization part. Wondering why this did not
> trigger on any board / kvm test...
>
> Anyway, it's clear that this patch should be dropped.
>
> Do you want me to resend the series with 5/5 dropped?
>

No need for this. I'll pick the rest of the series directly. Thanks.

-- 
Philippe.

Re: [PATCH RFC Dovetail 6.16 0/5] Dovetail: Minor cleanups found while debugging the pipeline entry code

[Philippe Gerum]

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

> Hi Philippe,
>
> this is now the end of my queue. Nothing more pending.
>
> All of that was found / noticed while debugging the pipeline entry code
> some weeks back. Maybe you can scan that and let me know which parts you
> like to have / take.
>
> Cc: Philippe Gerum <rpm@xenomai.org>
>
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
> Florian Bezdeka (5):
>       kernel/entry: irq_pipeline: Do not leave .noinstr.text section
>       kernel/irq/chip: Change return value of get_flow_step() to int
>       kernel/irq/chip: Simplify may_start_flow()
>       kernel/irq/chip: Simplify should_feed_pipeline()
>       kernel/irq/chip: Do not call low level irq chip hooks directly
>
>  kernel/entry/common.c |  4 ++--
>  kernel/irq/chip.c     | 29 ++++++++++++-----------------
>  2 files changed, 14 insertions(+), 19 deletions(-)
> ---
> base-commit: 138f1183deed300fd1f03abb6511635a916b98d0
> change-id: 20250925-wip-flo-cleanups-based-on-6-16-89989cc95468
>
> Best regards,

All patches but 5/5 are fine with me.

-- 
Philippe.