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(+)
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;
}
--
2.49.0On 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
© 2016 - 2025 Red Hat, Inc.