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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 17:57, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/irq.h
> +++ b/xen/arch/riscv/include/asm/irq.h
> @@ -26,6 +26,8 @@
>  #define IRQ_TYPE_SENSE_MASK     DT_IRQ_TYPE_SENSE_MASK
>  #define IRQ_TYPE_INVALID        DT_IRQ_TYPE_INVALID
>  
> +#define IRQ_NO_PRIORITY 0
> +
>  /* TODO */
>  #define nr_static_irqs 0
>  #define arch_hwdom_irqs(domid) 0U
> @@ -54,6 +56,10 @@ void init_IRQ(void);
>  struct cpu_user_regs;

Seeing this and ...

>  void do_IRQ(struct cpu_user_regs *regs, unsigned int irq);
>  
> +struct irq_desc;
> +struct cpumask_t;

... now these - all of such forward decls may want to collectively live in a
central place higher up in the file.

> @@ -57,6 +59,99 @@ int platform_get_irq(const struct dt_device_node *device, 
> int index)
>      return dt_irq.irq;
>  }
>  
> +static int __setup_irq(struct irq_desc *desc, unsigned int irqflags,
> +                       struct irqaction *new)

Irrespective of you possibly having found it like this elsewhere, may I
suggest that in new code we avoid leading double underscores? A single one
will do here.

> +{
> +    bool shared = irqflags & IRQF_SHARED;
> +
> +    ASSERT(new != NULL);
> +
> +    /* Sanity checks:

Nit: Comment style (and there are many more issues below).

> +     *  - if the IRQ is marked as shared
> +     *  - dev_id is not NULL when IRQF_SHARED is set
> +     */
> +    if ( desc->action != NULL && (!test_bit(_IRQF_SHARED, &desc->status)
> +         || !shared) )

Nit: Operator placement and indentation.

You're probably better off this way anyway:

    if ( desc->action != NULL &&
         (!test_bit(_IRQF_SHARED, &desc->status) || !shared) )

> +        return -EINVAL;
> +    if ( shared && new->dev_id == NULL )
> +        return -EINVAL;
> +
> +    if ( shared )
> +        set_bit(_IRQF_SHARED, &desc->status);

See comments on earlier patches.

> +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
> +    new->next = desc->action;
> +    smp_mb();
> +#endif
> +
> +    desc->action = new;
> +    smp_mb();

Aren't smp_wmb() sufficient on both places? If not, I think comments
want adding.

> +    return 0;
> +}
> +
> +void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
> +{
> +    if ( desc != NULL )

Can desc really be NULL here? Isn't desc->lock required to be held?

> +        desc->handler->set_affinity(desc, cpu_mask);
> +}
> +
> +int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
> +{
> +    int rc;
> +    unsigned long flags;
> +    struct irq_desc *desc;
> +    bool disabled;
> +
> +    desc = irq_to_desc(irq);

Make this the variable's initializer?

> +    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?

> +        /* 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.

> +    }
> +
> +err:

Nit (yet once again).

Jan