Re: [PATCH dovetail 02/11] riscv: irq_pipeline: add IRQ pipelining core

Tobias Schaffner <[email protected]> writes:

> This patchset integrates IRQ pipelining into the RISC-V architecture,
> bringing it in line with the Dovetail/IRQ pipeline model used on other
> architectures. It adds the core pipelining infrastructure and adapts
> low-level primitives to cleanly separate in-band and out-of-band
> interrupt handling.
>
> @@ -176,9 +204,12 @@ asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *re
>  
>  	if (user_mode(regs)) {
>  		irqentry_enter_from_user_mode(regs);
> -		local_irq_enable();
> +		hard_local_irq_enable();
>

irqentry_enter_from_user_mode() has just stalled the in-band stage, so
we end up here with hard irqs on, but irqs_disabled() now yields true.
Maybe riscv_v_first_use_handler() would never complain about running
with irqs (virtually) disabled although the caller explicitly enables
them, but maybe it would, right now or as a result of a future upstream
change.

As a rule of thumb, I would always keep the assumption the original
kernel logic may make about the interrupt state when running in-band,
in this case (*):

-	local_irq_enable();
+	unstall_inband_nocheck();
+	hard_local_irq_enable();

(*) hard irqs must be off when manipulating the stall bit, so the proper
sequence is always unstall+enable when entering with hard irqs off,
disable+stall when entering with hard irqs on.

>  		handled = riscv_v_first_use_handler(regs);
> +
> +		hard_local_irq_disable();
> +

and conversely,

+	hard_local_irq_disable();
+	stall_inband_nocheck();

>  		if (!handled)
>  			do_trap_error(regs, SIGILL, ILL_ILLOPC, regs->epc,
>  				      "Oops - illegal instruction");
> @@ -308,11 +339,12 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
>  {
>  	if (user_mode(regs)) {
>  		irqentry_enter_from_user_mode(regs);
> -		local_irq_enable();
> +		hard_local_irq_enable();
>

Ditto.

>  		handle_break(regs);
>  
> -		local_irq_disable();
> +		hard_local_irq_disable();
> +
>  		irqentry_exit_to_user_mode(regs);
>  	} else {
>  		irqentry_state_t state = irqentry_nmi_enter(regs);
> @@ -428,13 +460,69 @@ asmlinkage __visible noinstr void do_page_fault(struct pt_regs *regs)
>  	irqentry_state_t state = irqentry_enter(regs);
>  
>  	handle_page_fault(regs);
> -
> -	local_irq_disable();
> +	hard_local_irq_disable();
>

Ditto.

>  	irqentry_exit(regs, state);
>  }
>  #endif
>  
> +#ifdef CONFIG_IRQ_PIPELINE
> +
> +extern void (*handle_arch_irq)(struct pt_regs *);
> +
> +static void noinstr handle_riscv_irq_pipelined(struct pt_regs *regs)
> +{
> +	struct pt_regs *old_regs = set_irq_regs(regs);
> +	handle_arch_irq(regs);
> +	set_irq_regs(old_regs);
> +}
> +
> +DEFINE_PER_CPU(int, irq_nesting);
> +
> +static void noinstr handle_riscv_irq_pipelined_on_stack(struct pt_regs *regs)
> +{
> +	if (IS_ENABLED(CONFIG_IRQ_STACKS) && this_cpu_inc_return(irq_nesting) == 1)
> +		call_on_irq_stack(regs, handle_riscv_irq_pipelined);
> +	else
> +		handle_riscv_irq_pipelined(regs);
> +	this_cpu_dec(irq_nesting);

What if CONFIG_IRQ_STACKS is off? Imbalance?

-- 
Philippe.