Re: [PATCH v1 12/14] xen/riscv: implement setup_irq()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/15/25 5:55 PM, Jan Beulich wrote

> > +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
> > +    new->next = desc->action;
> > +    smp_mb();
> > +#endif
> > +
> > +    desc->action = ""
> > +    smp_mb();
> >       
> Aren't smp_wmb() sufficient on both places? If not, I think comments
> want adding.
>     
smp_wmb() will be sufficient but I think the barrier could be dropped at all
as __setup_irq() is called only in setup_irq() and __setup_irq() call is wrapped
by spinlock_{un}lock_irqsave() where spinlock_unlock_*() will put barrier.

> > +    return 0;
> > +}
> > +
> > +void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
> > +{
> > +    if ( desc != NULL )
> >       
> Can desc really be NULL here?
>     
It can't as irq_desc[] isn't dynamically allocated.

>  Isn't desc->lock required to be held?
>     
It is and it is called in setup_irq() which calls spin_lock_irqsave().
Anyway, I think it could be dropped at all and use 'desc->handler->set_affinity(desc, cpu_mask);'
explicitly in setup_irq().
> > +    spin_lock_irqsave(&desc->lock, flags);
> > +
> > +    disabled = (desc->action == NULL);
> > +
> > +    if ( test_bit(_IRQ_GUEST, &desc->status) )
> > +    {
> > +        spin_unlock_irqrestore(&desc->lock, flags);
> > +        /*
> > +         * TODO: would be nice to have functionality to print which domain owns
> > +         *       an IRQ.
> > +         */
> > +        printk(XENLOG_ERR "ERROR: IRQ %u is already in use by a domain\n", irq);
> > +        return -EBUSY;
> > +    }
> > +
> > +    rc = __setup_irq(desc, irqflags, new);
> > +    if ( rc )
> > +        goto err;
> > +
> > +    /* First time the IRQ is setup */
> > +    if ( disabled )
> > +    {
> > +        /* disable irq by default */
> > +        set_bit(_IRQ_DISABLED, &desc->status);
> >       
> Shouldn't this be set when we make it here?
>     
It should be. I'll drop the setting of _IRQ_DISABLED.
> > +        /* route interrupt to xen */
> > +        intc_route_irq_to_xen(desc, IRQ_NO_PRIORITY);
> > +
> > +        /*
> > +         * we don't care for now which CPU will receive the
> > +         * interrupt
> > +         *
> > +         * TODO: Handle case where IRQ is setup on different CPU than
> > +         * the targeted CPU and the priority.
> > +         */
> > +        irq_set_affinity(desc, cpumask_of(smp_processor_id()));
> > +        desc->handler->startup(desc);
> > +        /* enable irq */
> > +        clear_bit(_IRQ_DISABLED, &desc->status);
> >       
> Now it turns out this is really done twice: Once in aplic_irq_enable(),
> and once here.
>     
Agree, this is a job of *_startup()->*_aplic_irq_enable(). I'll drop that too.

~ Oleksii