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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/15/25 4:42 PM, Jan Beulich wrote:

> > +    /* 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.
>     
I don't remember a particular reason, but it could have been dropped. If, for some reason,
the cause is needed in handle_interrupt(), it can be retrieved explicitly from a register.
> > @@ -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?
>     
Yes, it make sense. Also, I noticed some other functions that could be __init in imsic.c (but likely
you mentioned that in the previous patches).
> > --- 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.
>     
I will try to find a better place then.
> > @@ -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?
>     
Agree, it could be used non-atomic bitop.

> > +    action = ""
> > +
> > +    spin_unlock_irq(&desc->lock);
> > +
> > +#ifndef CONFIG_IRQ_HAS_MULTIPLE_ACTION
> >       
> Stolen from Arm? What's this about?
>     
Yes, it is stolen from Arm. I thought that it is a generic one, but the config is defined
inside Arm's config.h.
Then it could be dropped now as I don't know, at the moment, the cases when it is neeeded
to exectute several handler for an irq for RISC-V.

Thanks.

~ Oleksii