Introduce support for IRQ setup on RISC-V by implementing `setup_irq()` and
`__setup_irq()`, adapted and extended from an initial implementation by [1].
__setup_irq() does the following:
- Sets up an IRQ action.
- Validates that shared IRQs have non-NULL `dev_id` and are only used when
existing handlers allow sharing.
- Uses `smp_mb()` to enforce memory ordering after assigning `desc->action`
to ensure visibility before enabling the IRQ.
- Supports multi-action setups via `CONFIG_IRQ_HAS_MULTIPLE_ACTION`.
setup_irq() does the following:
- Converts IRQ number to descriptor and acquires its lock.
- Rejects registration if the IRQ is already assigned to a guest domain,
printing an error.
- Delegates the core setup to `__setup_irq()`.
- On first-time setup, disables the IRQ, routes it to Xen using
`intc_route_irq_to_xen()`, sets default CPU affinity (current CPU),
calls the handler’s startup routine, and finally enables the IRQ.
irq_set_affinity() invokes `set_affinity` callback from the IRQ handler
if present.
Defined `IRQ_NO_PRIORITY` as default priority used when routing IRQs to Xen.
[1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/7390e2365828b83e27ead56b03114a56e3699dd5
Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/riscv/include/asm/irq.h | 6 ++
xen/arch/riscv/irq.c | 95 ++++++++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h
index 9558d3fa61..bba3a97e3e 100644
--- 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;
void do_IRQ(struct cpu_user_regs *regs, unsigned int irq);
+struct irq_desc;
+struct cpumask_t;
+void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
+
#endif /* ASM__RISCV__IRQ_H */
/*
diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
index 3c0b95220a..1e937d4306 100644
--- a/xen/arch/riscv/irq.c
+++ b/xen/arch/riscv/irq.c
@@ -6,7 +6,9 @@
* Copyright (c) 2024 Vates
*/
+#include <xen/bitops.h>
#include <xen/bug.h>
+#include <xen/cpumask.h>
#include <xen/device_tree.h>
#include <xen/errno.h>
#include <xen/init.h>
@@ -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)
+{
+ bool shared = irqflags & IRQF_SHARED;
+
+ ASSERT(new != NULL);
+
+ /* Sanity checks:
+ * - 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) )
+ return -EINVAL;
+ if ( shared && new->dev_id == NULL )
+ return -EINVAL;
+
+ if ( shared )
+ set_bit(_IRQF_SHARED, &desc->status);
+
+#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
+ new->next = desc->action;
+ smp_mb();
+#endif
+
+ desc->action = new;
+ smp_mb();
+
+ return 0;
+}
+
+void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
+{
+ if ( desc != NULL )
+ 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);
+
+ 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);
+
+ /* 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);
+ }
+
+err:
+ spin_unlock_irqrestore(&desc->lock, flags);
+
+ return rc;
+}
+
int arch_init_one_irq_desc(struct irq_desc *desc)
{
desc->arch.type = IRQ_TYPE_INVALID;
--
2.49.0
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
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 = new;
>> + 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
On 17.04.2025 12:10, Oleksii Kurochko wrote: > On 4/15/25 5:55 PM, Jan Beulich wrote >>> + /* >>> + * 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. Wait - see my comment there. I think it belongs here, not there. Jan
© 2016 - 2025 Red Hat, Inc.