| Message ID | 2bc37c3996978129a5b2c888917307ea32448651.1744126720.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: introduce basic UART support and interrupts for hypervisor mode | expand |
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
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;
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(+)