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
