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

Tobias Schaffner <[email protected]> writes:

> +extern int NR_CALL_FUNCTION_OOB_IPI;
> +extern int NR_RESCHEDULE_OOB_IPI;
> +extern int NR_TIMER_OOB_IPI;

Instead of defining these pseudo-variables which are basically
duplicating the out-of-band IPI indices, couldn't we move the definition
of ipi_message_type to <asm/smp.h>?

> +
>  int riscv_hartid_to_cpuid(unsigned long hartid)
>  {
>  	int i;
> @@ -95,7 +134,7 @@ static inline void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
>  
>  	atomic_dec(&waiting_for_crash_ipi);
>  
> -	local_irq_disable();
> +	hard_local_irq_disable();
>

local_irq_disable_full() may be safer in case hotplug handlers have
assertions checking for irqs_disabled().

> @@ -176,9 +176,19 @@ 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();
> +
> +		if (running_inband())
> +			local_irq_enable_full();
> +		else
> +			hard_local_irq_enable();
>

This is basically what local_irq_enable_full() does, keeping
CONFIG_TRACE_IRQFLAGS happy too.

This said, I'm unsure that we really want the in-band stage to be
unstalled in the illegal instruction handler, although we do want the
oob stage to keep receiving interrupts as long as possible while
handling such trap. As a rule of thumb, unless the in-band code
explicitly enables interrupts (e.g. on page fault), we should assume
that it might expect not to receive any interrupt while handling such
trap.

Would hard_local_irq_enable() unconditionally suffice here?

>  		handled = riscv_v_first_use_handler(regs);
> +
> +		if (running_inband())
> +			local_irq_disable_full();
> +		else
> +			hard_local_irq_disable();
> +

Ditto (local_irq_disable_full()).

>  		if (!handled)
>  			do_trap_error(regs, SIGILL, ILL_ILLOPC, regs->epc,
>  				      "Oops - illegal instruction");
> @@ -308,11 +318,19 @@ 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();
> +
> +		if (running_inband())
> +			local_irq_enable_full();
> +		else
> +			hard_local_irq_enable();
>

Ditto.

>  		handle_break(regs);
>  
> -		local_irq_disable();
> +		if (running_inband())
> +			local_irq_disable_full();
> +		else
> +			hard_local_irq_disable();
> +

Ditto.

>  		irqentry_exit_to_user_mode(regs);
>  	} else {
>  		irqentry_state_t state = irqentry_nmi_enter(regs);
> @@ -429,12 +447,72 @@ asmlinkage __visible noinstr void do_page_fault(struct pt_regs *regs)
>  
>  	handle_page_fault(regs);
>  
> -	local_irq_disable();
> +	if (running_inband())
> +		local_irq_disable_full();
> +	else
> +		hard_local_irq_disable();
>  

Ditto.

-- 
Philippe.