Re: [PATCH v1 11/14] xen/riscv: add external interrupt handling for hypervisor mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 17:57, Oleksii Kurochko wrote:
> Implement functions necessarry to have working external interrupts in
> hypervisor mode. The following changes are done:
>  - Add a common function intc_handle_external_irq() to call APLIC specific
>    function to handle an interrupt.
>  - Update do_trap() function to handle IRQ_S_EXT case; add the check to catch
>    case when cause of trap is an interrupt.
>  - Add handle_interrrupt() member to intc_hw_operations structure.
>  - Enable local interrupt delivery for IMSIC by implementation and calling of
>    imsic_ids_local_delivery() in imsic_init();

Ah, here is where that call really belongs (see my question on the earlier
patch). Please make sure you series builds okay at every patch boundary.

> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -261,6 +261,21 @@ static void aplic_set_irq_affinity(struct irq_desc 
> *desc, const cpumask_t *mask)
>      spin_unlock(&aplic.lock);
>  }
>  
> +static void aplic_handle_interrupt(unsigned long cause, struct cpu_user_regs 
> *regs)
> +{
> +    /* disable to avoid more external interrupts */
> +    csr_clear(CSR_SIE, 1UL << IRQ_S_EXT);

Didn't I see you use BIT() elsewhere? Would be nice to be overall consistent
at least within related code.

> +    /* clear the pending bit */
> +    csr_clear(CSR_SIP, 1UL << IRQ_S_EXT);
> +
> +    /* dispatch the interrupt */
> +    do_IRQ(regs, csr_swap(CSR_STOPEI, 0) >> TOPI_IID_SHIFT);
> +
> +    /* enable external interrupts */
> +    csr_set(CSR_SIE, 1UL << IRQ_S_EXT);
> +}

Why does "cause" need passing into here? I realize the function is used ...

> @@ -278,6 +293,7 @@ static const struct intc_hw_operations aplic_ops = {
>      .host_irq_type       = &aplic_host_irq_type,
>      .set_irq_priority    = aplic_set_irq_priority,
>      .set_irq_type        = aplic_set_irq_type,
> +    .handle_interrupt    = aplic_handle_interrupt,
>  };

... as a hook, yet it's still unclear whether (why) any other such hook
would need the cause to be passed in.

> @@ -33,6 +44,20 @@ do {                            \
>      csr_clear(CSR_SIREG, v);    \
>  } while (0)
>  
> +void imsic_ids_local_delivery(bool enable)

__init as long as the sole caller is such?

> --- a/xen/arch/riscv/include/asm/intc.h
> +++ b/xen/arch/riscv/include/asm/intc.h
> @@ -34,6 +34,8 @@ struct intc_hw_operations {
>      /* Set IRQ priority */
>      void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
>  
> +    /* handle external interrupt */
> +    void (*handle_interrupt)(unsigned long cause, struct cpu_user_regs 
> *regs);
>  };
>  
>  void intc_preinit(void);
> @@ -45,4 +47,7 @@ void register_intc_ops(const struct intc_hw_operations 
> *ops);
>  struct irq_desc;
>  void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority);
>  
> +struct cpu_user_regs;

This is too late - you've used it above already. It either can be dropped,
or needs to move up.

> --- a/xen/arch/riscv/intc.c
> +++ b/xen/arch/riscv/intc.c
> @@ -51,6 +51,13 @@ static void intc_set_irq_priority(struct irq_desc *desc, 
> unsigned int priority)
>      intc_hw_ops->set_irq_priority(desc, priority);
>  }
>  
> +void intc_handle_external_irqs(unsigned long cause, struct cpu_user_regs 
> *regs)
> +{
> +    ASSERT(intc_hw_ops && intc_hw_ops->handle_interrupt);

I don't view such checks (on every interrupt) as very useful. If you checked
once early on - okay. But here you gain nothing at all ...

> +    intc_hw_ops->handle_interrupt(cause, regs);

... towards the use here, when considering a release build.


> @@ -83,3 +87,42 @@ void __init init_IRQ(void)
>      if ( init_irq_data() < 0 )
>          panic("initialization of IRQ data failed\n");
>  }
> +
> +/* Dispatch an interrupt */
> +void do_IRQ(struct cpu_user_regs *regs, unsigned int irq)
> +{
> +    struct irq_desc *desc = irq_to_desc(irq);
> +    struct irqaction *action;
> +
> +    irq_enter();
> +
> +    spin_lock(&desc->lock);
> +    desc->handler->ack(desc);
> +
> +    if ( test_bit(_IRQ_DISABLED, &desc->status) )
> +        goto out;
> +
> +    set_bit(_IRQ_INPROGRESS, &desc->status);

Same comment as on the earlier patch - atomic bitop when in a suitably
locked region?

> +    action = desc->action;
> +
> +    spin_unlock_irq(&desc->lock);
> +
> +#ifndef CONFIG_IRQ_HAS_MULTIPLE_ACTION

Stolen from Arm? What's this about?

> +    action->handler(irq, action->dev_id);
> +#else
> +    do {
> +        action->handler(irq, action->dev_id);
> +        action = action->next;
> +    } while ( action );
> +#endif /* CONFIG_IRQ_HAS_MULTIPLE_ACTION */
> +
> +    spin_lock_irq(&desc->lock);
> +
> +    clear_bit(_IRQ_INPROGRESS, &desc->status);

See above.

> +out:

Nit (you know what).

> @@ -128,6 +129,23 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>          }
>          fallthrough;
>      default:
> +        if ( cause & CAUSE_IRQ_FLAG )
> +        {
> +            /* Handle interrupt */
> +            unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
> +
> +            switch ( icause )
> +            {
> +            case IRQ_S_EXT:
> +                intc_handle_external_irqs(cause, cpu_regs);
> +                break;
> +            default:

Nit: Blank line please between non-fall-through case blocks.

Jan