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
