| Message ID | 1685488b8c1b48149e94bd3625c7b46b78c72e8e.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: > Implement functions necessarry to have working external interrupts in > hypervisor mode. The following changes are done: > - Add a common function intc_handle_external_irq() to call APLIC specific > function to handle an interrupt. > - Update do_trap() function to handle IRQ_S_EXT case; add the check to catch > case when cause of trap is an interrupt. > - Add handle_interrrupt() member to intc_hw_operations structure. > - Enable local interrupt delivery for IMSIC by implementation and calling of > imsic_ids_local_delivery() in imsic_init(); Ah, here is where that call really belongs (see my question on the earlier patch). Please make sure you series builds okay at every patch boundary. > --- a/xen/arch/riscv/aplic.c > +++ b/xen/arch/riscv/aplic.c > @@ -261,6 +261,21 @@ static void aplic_set_irq_affinity(struct irq_desc *desc, const cpumask_t *mask) > spin_unlock(&aplic.lock); > } > > +static void aplic_handle_interrupt(unsigned long cause, struct cpu_user_regs *regs) > +{ > + /* disable to avoid more external interrupts */ > + csr_clear(CSR_SIE, 1UL << IRQ_S_EXT); Didn't I see you use BIT() elsewhere? Would be nice to be overall consistent at least within related code. > + /* clear the pending bit */ > + csr_clear(CSR_SIP, 1UL << IRQ_S_EXT); > + > + /* dispatch the interrupt */ > + do_IRQ(regs, csr_swap(CSR_STOPEI, 0) >> TOPI_IID_SHIFT); > + > + /* enable external interrupts */ > + csr_set(CSR_SIE, 1UL << IRQ_S_EXT); > +} Why does "cause" need passing into here? I realize the function is used ... > @@ -278,6 +293,7 @@ static const struct intc_hw_operations aplic_ops = { > .host_irq_type = &aplic_host_irq_type, > .set_irq_priority = aplic_set_irq_priority, > .set_irq_type = aplic_set_irq_type, > + .handle_interrupt = aplic_handle_interrupt, > }; ... as a hook, yet it's still unclear whether (why) any other such hook would need the cause to be passed in. > @@ -33,6 +44,20 @@ do { \ > csr_clear(CSR_SIREG, v); \ > } while (0) > > +void imsic_ids_local_delivery(bool enable) __init as long as the sole caller is such? > --- a/xen/arch/riscv/include/asm/intc.h > +++ b/xen/arch/riscv/include/asm/intc.h > @@ -34,6 +34,8 @@ struct intc_hw_operations { > /* Set IRQ priority */ > void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority); > > + /* handle external interrupt */ > + void (*handle_interrupt)(unsigned long cause, struct cpu_user_regs *regs); > }; > > void intc_preinit(void); > @@ -45,4 +47,7 @@ void register_intc_ops(const struct intc_hw_operations *ops); > struct irq_desc; > void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority); > > +struct cpu_user_regs; This is too late - you've used it above already. It either can be dropped, or needs to move up. > --- a/xen/arch/riscv/intc.c > +++ b/xen/arch/riscv/intc.c > @@ -51,6 +51,13 @@ static void intc_set_irq_priority(struct irq_desc *desc, unsigned int priority) > intc_hw_ops->set_irq_priority(desc, priority); > } > > +void intc_handle_external_irqs(unsigned long cause, struct cpu_user_regs *regs) > +{ > + ASSERT(intc_hw_ops && intc_hw_ops->handle_interrupt); I don't view such checks (on every interrupt) as very useful. If you checked once early on - okay. But here you gain nothing at all ... > + intc_hw_ops->handle_interrupt(cause, regs); ... towards the use here, when considering a release build. > @@ -83,3 +87,42 @@ void __init init_IRQ(void) > if ( init_irq_data() < 0 ) > panic("initialization of IRQ data failed\n"); > } > + > +/* Dispatch an interrupt */ > +void do_IRQ(struct cpu_user_regs *regs, unsigned int irq) > +{ > + struct irq_desc *desc = irq_to_desc(irq); > + struct irqaction *action; > + > + irq_enter(); > + > + spin_lock(&desc->lock); > + desc->handler->ack(desc); > + > + if ( test_bit(_IRQ_DISABLED, &desc->status) ) > + goto out; > + > + set_bit(_IRQ_INPROGRESS, &desc->status); Same comment as on the earlier patch - atomic bitop when in a suitably locked region? > + action = desc->action; > + > + spin_unlock_irq(&desc->lock); > + > +#ifndef CONFIG_IRQ_HAS_MULTIPLE_ACTION Stolen from Arm? What's this about? > + action->handler(irq, action->dev_id); > +#else > + do { > + action->handler(irq, action->dev_id); > + action = action->next; > + } while ( action ); > +#endif /* CONFIG_IRQ_HAS_MULTIPLE_ACTION */ > + > + spin_lock_irq(&desc->lock); > + > + clear_bit(_IRQ_INPROGRESS, &desc->status); See above. > +out: Nit (you know what). > @@ -128,6 +129,23 @@ void do_trap(struct cpu_user_regs *cpu_regs) > } > fallthrough; > default: > + if ( cause & CAUSE_IRQ_FLAG ) > + { > + /* Handle interrupt */ > + unsigned long icause = cause & ~CAUSE_IRQ_FLAG; > + > + switch ( icause ) > + { > + case IRQ_S_EXT: > + intc_handle_external_irqs(cause, cpu_regs); > + break; > + default: Nit: Blank line please between non-fall-through case blocks. Jan
On 4/15/25 4:42 PM, Jan Beulich wrote: > >> + /* clear the pending bit */ >> + csr_clear(CSR_SIP, 1UL << IRQ_S_EXT); >> + >> + /* dispatch the interrupt */ >> + do_IRQ(regs, csr_swap(CSR_STOPEI, 0) >> TOPI_IID_SHIFT); >> + >> + /* enable external interrupts */ >> + csr_set(CSR_SIE, 1UL << IRQ_S_EXT); >> +} > Why does "cause" need passing into here? I realize the function is used ... > >> @@ -278,6 +293,7 @@ static const struct intc_hw_operations aplic_ops = { >> .host_irq_type = &aplic_host_irq_type, >> .set_irq_priority = aplic_set_irq_priority, >> .set_irq_type = aplic_set_irq_type, >> + .handle_interrupt = aplic_handle_interrupt, >> }; > ... as a hook, yet it's still unclear whether (why) any other such hook > would need the cause to be passed in. I don't remember a particular reason, but it could have been dropped. If, for some reason, the cause is needed in|handle_interrupt()|, it can be retrieved explicitly from a register. > >> @@ -33,6 +44,20 @@ do { \ >> csr_clear(CSR_SIREG, v); \ >> } while (0) >> >> +void imsic_ids_local_delivery(bool enable) > __init as long as the sole caller is such? Yes, it make sense. Also, I noticed some other functions that could be __init in imsic.c (but likely you mentioned that in the previous patches). >> --- a/xen/arch/riscv/intc.c >> +++ b/xen/arch/riscv/intc.c >> @@ -51,6 +51,13 @@ static void intc_set_irq_priority(struct irq_desc *desc, unsigned int priority) >> intc_hw_ops->set_irq_priority(desc, priority); >> } >> >> +void intc_handle_external_irqs(unsigned long cause, struct cpu_user_regs *regs) >> +{ >> + ASSERT(intc_hw_ops && intc_hw_ops->handle_interrupt); > I don't view such checks (on every interrupt) as very useful. If you checked > once early on - okay. But here you gain nothing at all ... > >> + intc_hw_ops->handle_interrupt(cause, regs); > ... towards the use here, when considering a release build. I will try to find a better place then. > > >> @@ -83,3 +87,42 @@ void __init init_IRQ(void) >> if ( init_irq_data() < 0 ) >> panic("initialization of IRQ data failed\n"); >> } >> + >> +/* Dispatch an interrupt */ >> +void do_IRQ(struct cpu_user_regs *regs, unsigned int irq) >> +{ >> + struct irq_desc *desc = irq_to_desc(irq); >> + struct irqaction *action; >> + >> + irq_enter(); >> + >> + spin_lock(&desc->lock); >> + desc->handler->ack(desc); >> + >> + if ( test_bit(_IRQ_DISABLED, &desc->status) ) >> + goto out; >> + >> + set_bit(_IRQ_INPROGRESS, &desc->status); > Same comment as on the earlier patch - atomic bitop when in a suitably > locked region? Agree, it could be used non-atomic bitop. > >> + action = desc->action; >> + >> + spin_unlock_irq(&desc->lock); >> + >> +#ifndef CONFIG_IRQ_HAS_MULTIPLE_ACTION > Stolen from Arm? What's this about? Yes, it is stolen from Arm. I thought that it is a generic one, but the config is defined inside Arm's config.h. Then it could be dropped now as I don't know, at the moment, the cases when it is neeeded to exectute several handler for an irq for RISC-V. Thanks. ~ Oleksii
On 4/17/25 10:44 AM, Oleksii Kurochko wrote: >>> + action = desc->action; >>> + >>> + spin_unlock_irq(&desc->lock); >>> + >>> +#ifndef CONFIG_IRQ_HAS_MULTIPLE_ACTION >> Stolen from Arm? What's this about? > Yes, it is stolen from Arm. I thought that it is a generic one, but the config is defined > inside Arm's config.h. > Then it could be dropped now as I don't know, at the moment, the cases when it is neeeded > to exectute several handler for an irq for RISC-V. Probably, IOMMU may setup multiple handler for the same interrupt. I'll double check that. ~ Oleksii
diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c index 4b60cb9a77..38b57ed1ac 100644 --- a/xen/arch/riscv/aplic.c +++ b/xen/arch/riscv/aplic.c @@ -261,6 +261,21 @@ static void aplic_set_irq_affinity(struct irq_desc *desc, const cpumask_t *mask) spin_unlock(&aplic.lock); } +static void aplic_handle_interrupt(unsigned long cause, struct cpu_user_regs *regs) +{ + /* disable to avoid more external interrupts */ + csr_clear(CSR_SIE, 1UL << IRQ_S_EXT); + + /* clear the pending bit */ + csr_clear(CSR_SIP, 1UL << IRQ_S_EXT); + + /* dispatch the interrupt */ + do_IRQ(regs, csr_swap(CSR_STOPEI, 0) >> TOPI_IID_SHIFT); + + /* enable external interrupts */ + csr_set(CSR_SIE, 1UL << IRQ_S_EXT); +} + static hw_irq_controller aplic_host_irq_type = { .typename = "aplic", .startup = aplic_irq_startup, @@ -278,6 +293,7 @@ static const struct intc_hw_operations aplic_ops = { .host_irq_type = &aplic_host_irq_type, .set_irq_priority = aplic_set_irq_priority, .set_irq_type = aplic_set_irq_type, + .handle_interrupt = aplic_handle_interrupt, }; static int aplic_irq_xlate(const uint32_t *intspec, unsigned int intsize, @@ -318,6 +334,9 @@ static int __init aplic_preinit(struct dt_device_node *node, const void *dat) register_intc_ops(&aplic_ops); + /* Enable supervisor external interrupt */ + csr_set(CSR_SIE, 1UL << IRQ_S_EXT); + return 0; } diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c index 8198d008ef..e00f2d69df 100644 --- a/xen/arch/riscv/imsic.c +++ b/xen/arch/riscv/imsic.c @@ -19,8 +19,19 @@ #include <asm/imsic.h> +#define IMSIC_DISABLE_EIDELIVERY 0 +#define IMSIC_ENABLE_EIDELIVERY 1 +#define IMSIC_DISABLE_EITHRESHOLD 1 +#define IMSIC_ENABLE_EITHRESHOLD 0 + static struct imsic_config imsic_cfg; +#define imsic_csr_write(c, v) \ +do { \ + csr_write(CSR_SISELECT, c); \ + csr_write(CSR_SIREG, v); \ +} while (0) + #define imsic_csr_set(c, v) \ do { \ csr_write(CSR_SISELECT, c); \ @@ -33,6 +44,20 @@ do { \ csr_clear(CSR_SIREG, v); \ } while (0) +void imsic_ids_local_delivery(bool enable) +{ + if ( enable ) + { + imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_ENABLE_EITHRESHOLD); + imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_ENABLE_EIDELIVERY); + } + else + { + imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_DISABLE_EITHRESHOLD); + imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_DISABLE_EIDELIVERY); + } +} + static void imsic_local_eix_update(unsigned long base_id, unsigned long num_id, bool pend, bool val) { diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h index d2c0178529..b2c674f271 100644 --- a/xen/arch/riscv/include/asm/imsic.h +++ b/xen/arch/riscv/include/asm/imsic.h @@ -12,6 +12,7 @@ #define ASM__RISCV__IMSIC_H #include <xen/spinlock.h> +#include <xen/stdbool.h> #include <xen/types.h> #define IMSIC_MMIO_PAGE_SHIFT 12 @@ -20,6 +21,10 @@ #define IMSIC_MIN_ID 63 #define IMSIC_MAX_ID 2048 +#define IMSIC_EIDELIVERY 0x70 + +#define IMSIC_EITHRESHOLD 0x72 + #define IMSIC_EIP0 0x80 #define IMSIC_EIPx_BITS 32 @@ -78,4 +83,6 @@ const struct imsic_config *imsic_get_config(void); void imsic_irq_enable(unsigned int hwirq); void imsic_irq_disable(unsigned int hwirq); +void imsic_ids_local_delivery(bool enable); + #endif /* ASM__RISCV__IMSIC_H */ diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h index db53caa07b..e4363af87d 100644 --- a/xen/arch/riscv/include/asm/intc.h +++ b/xen/arch/riscv/include/asm/intc.h @@ -34,6 +34,8 @@ struct intc_hw_operations { /* Set IRQ priority */ void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority); + /* handle external interrupt */ + void (*handle_interrupt)(unsigned long cause, struct cpu_user_regs *regs); }; void intc_preinit(void); @@ -45,4 +47,7 @@ void register_intc_ops(const struct intc_hw_operations *ops); struct irq_desc; void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority); +struct cpu_user_regs; +void intc_handle_external_irqs(unsigned long cause, struct cpu_user_regs *regs); + #endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */ diff --git a/xen/arch/riscv/include/asm/irq.h b/xen/arch/riscv/include/asm/irq.h index 163a478d78..9558d3fa61 100644 --- a/xen/arch/riscv/include/asm/irq.h +++ b/xen/arch/riscv/include/asm/irq.h @@ -51,6 +51,9 @@ int platform_get_irq(const struct dt_device_node *device, int index); void init_IRQ(void); +struct cpu_user_regs; +void do_IRQ(struct cpu_user_regs *regs, unsigned int irq); + #endif /* ASM__RISCV__IRQ_H */ /* diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c index 8274897d8c..41a4310ead 100644 --- a/xen/arch/riscv/intc.c +++ b/xen/arch/riscv/intc.c @@ -51,6 +51,13 @@ static void intc_set_irq_priority(struct irq_desc *desc, unsigned int priority) intc_hw_ops->set_irq_priority(desc, priority); } +void intc_handle_external_irqs(unsigned long cause, struct cpu_user_regs *regs) +{ + ASSERT(intc_hw_ops && intc_hw_ops->handle_interrupt); + + intc_hw_ops->handle_interrupt(cause, regs); +} + void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority) { ASSERT(test_bit(_IRQ_DISABLED, &desc->status)); diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c index c332e000c4..3c0b95220a 100644 --- a/xen/arch/riscv/irq.c +++ b/xen/arch/riscv/irq.c @@ -11,6 +11,10 @@ #include <xen/errno.h> #include <xen/init.h> #include <xen/irq.h> +#include <xen/spinlock.h> + +#include <asm/hardirq.h> +#include <asm/intc.h> static irq_desc_t irq_desc[NR_IRQS]; @@ -83,3 +87,42 @@ void __init init_IRQ(void) if ( init_irq_data() < 0 ) panic("initialization of IRQ data failed\n"); } + +/* Dispatch an interrupt */ +void do_IRQ(struct cpu_user_regs *regs, unsigned int irq) +{ + struct irq_desc *desc = irq_to_desc(irq); + struct irqaction *action; + + irq_enter(); + + spin_lock(&desc->lock); + desc->handler->ack(desc); + + if ( test_bit(_IRQ_DISABLED, &desc->status) ) + goto out; + + set_bit(_IRQ_INPROGRESS, &desc->status); + + action = desc->action; + + spin_unlock_irq(&desc->lock); + +#ifndef CONFIG_IRQ_HAS_MULTIPLE_ACTION + action->handler(irq, action->dev_id); +#else + do { + action->handler(irq, action->dev_id); + action = action->next; + } while ( action ); +#endif /* CONFIG_IRQ_HAS_MULTIPLE_ACTION */ + + spin_lock_irq(&desc->lock); + + clear_bit(_IRQ_INPROGRESS, &desc->status); + +out: + desc->handler->end(desc); + spin_unlock(&desc->lock); + irq_exit(); +} diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c index ea3638a54f..da5813e34a 100644 --- a/xen/arch/riscv/traps.c +++ b/xen/arch/riscv/traps.c @@ -11,6 +11,7 @@ #include <xen/nospec.h> #include <xen/sched.h> +#include <asm/intc.h> #include <asm/processor.h> #include <asm/riscv_encoding.h> #include <asm/traps.h> @@ -128,6 +129,23 @@ void do_trap(struct cpu_user_regs *cpu_regs) } fallthrough; default: + if ( cause & CAUSE_IRQ_FLAG ) + { + /* Handle interrupt */ + unsigned long icause = cause & ~CAUSE_IRQ_FLAG; + + switch ( icause ) + { + case IRQ_S_EXT: + intc_handle_external_irqs(cause, cpu_regs); + break; + default: + break; + } + + break; + } + do_unexpected_trap(cpu_regs); break; }
Implement functions necessarry to have working external interrupts in hypervisor mode. The following changes are done: - Add a common function intc_handle_external_irq() to call APLIC specific function to handle an interrupt. - Update do_trap() function to handle IRQ_S_EXT case; add the check to catch case when cause of trap is an interrupt. - Add handle_interrrupt() member to intc_hw_operations structure. - Enable local interrupt delivery for IMSIC by implementation and calling of imsic_ids_local_delivery() in imsic_init(); additionally introduce helper imsic_csr_write() to update IMSIC_EITHRESHOLD and IMSIC_EITHRESHOLD. - Enable hypervisor external interrupts. - Implement aplic_handler_interrupt() and use it to init ->handle_interrupt member of intc_hw_operations for APLIC. - Add implementation of do_IRQ() to dispatch the interrupt. The current patch is based on the code from [1]. [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/aplic.c | 19 +++++++++++++ xen/arch/riscv/imsic.c | 25 +++++++++++++++++ xen/arch/riscv/include/asm/imsic.h | 7 +++++ xen/arch/riscv/include/asm/intc.h | 5 ++++ xen/arch/riscv/include/asm/irq.h | 3 +++ xen/arch/riscv/intc.c | 7 +++++ xen/arch/riscv/irq.c | 43 ++++++++++++++++++++++++++++++ xen/arch/riscv/traps.c | 18 +++++++++++++ 8 files changed, 127 insertions(+)