Introduce the `intc_hw_operations` structure to encapsulate interrupt
controller-specific data and operations. This structure includes:
- A pointer to interrupt controller information (`intc_info`)
- Callbacks to initialize the controller and set IRQ type/priority
- A reference to an interupt controller descriptor (`host_irq_type`)

Also introduce generic helper functions:
- `intc_init()`: Initializes the interrupt controller
- `register_intc_ops()`: Registers the `intc_hw_operations` implementation
- `intc_route_irq_to_xen()`: Configures IRQ routing to Xen, setting handler,
  type, and priority

Most of these functions act as thin wrappers around the corresponding
callbacks in `intc_hw_operations`.

This abstraction lays the groundwork for supporting multiple interrupt
controller types (e.g., PLIC, APLIC) in extensible way.

This patch is based on the changes from [1].

[1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/7cfb4bd4748ca268142497ac5c327d2766fb342d

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/intc.h | 23 ++++++++++++++
 xen/arch/riscv/intc.c             | 51 +++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index 52ba196d87..0d498b10f4 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -17,6 +17,29 @@ struct intc_info {
     const struct dt_device_node *node;
 };
 
+struct intc_hw_operations {
+    /* Hold intc hw information */
+    const struct intc_info *info;
+    /* Initialize the intc and the boot CPU */
+    int (*init)(void);
+
+    /* hw_irq_controller to enable/disable/eoi host irq */
+    hw_irq_controller *host_irq_type;
+
+    /* Set IRQ type */
+    void (*set_irq_type)(struct irq_desc *desc, unsigned int type);
+    /* Set IRQ priority */
+    void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
+
+};
+
 void intc_preinit(void);
 
+void intc_init(void);
+
+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);
+
 #endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */
diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
index 4061a3c457..8274897d8c 100644
--- a/xen/arch/riscv/intc.c
+++ b/xen/arch/riscv/intc.c
@@ -1,9 +1,21 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
 #include <xen/acpi.h>
+#include <xen/bug.h>
 #include <xen/device_tree.h>
 #include <xen/init.h>
+#include <xen/irq.h>
 #include <xen/lib.h>
+#include <xen/spinlock.h>
+
+#include <asm/intc.h>
+
+static const struct intc_hw_operations *intc_hw_ops;
+
+void register_intc_ops(const struct intc_hw_operations *ops)
+{
+    intc_hw_ops = ops;
+}
 
 void __init intc_preinit(void)
 {
@@ -12,3 +24,42 @@ void __init intc_preinit(void)
     else
         panic("ACPI interrupt controller preinit() isn't implemented\n");
 }
+
+void __init intc_init(void)
+{
+    ASSERT(intc_hw_ops);
+
+    if ( intc_hw_ops->init() )
+        panic("Failed to initialize the interrupt controller drivers\n");
+}
+
+/* desc->irq needs to be disabled before calling this function */
+static void intc_set_irq_type(struct irq_desc *desc, unsigned int type)
+{
+    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
+    ASSERT(spin_is_locked(&desc->lock));
+    ASSERT(type != IRQ_TYPE_INVALID);
+    ASSERT(intc_hw_ops && intc_hw_ops->set_irq_type);
+
+    intc_hw_ops->set_irq_type(desc, type);
+}
+
+static void intc_set_irq_priority(struct irq_desc *desc, unsigned int priority)
+{
+    ASSERT(intc_hw_ops && intc_hw_ops->set_irq_priority);
+
+    intc_hw_ops->set_irq_priority(desc, priority);
+}
+
+void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
+{
+    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
+    ASSERT(spin_is_locked(&desc->lock));
+    /* Can't route interrupts that don't exist */
+    ASSERT(intc_hw_ops && desc->irq < intc_hw_ops->info->nr_irqs);
+
+    desc->handler = intc_hw_ops->host_irq_type;
+
+    intc_set_irq_type(desc, desc->arch.type);
+    intc_set_irq_priority(desc, priority);
+}
-- 
2.49.0

On 08.04.2025 17:57, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/intc.h
> +++ b/xen/arch/riscv/include/asm/intc.h
> @@ -17,6 +17,29 @@ struct intc_info {
>      const struct dt_device_node *node;
>  };
>  
> +struct intc_hw_operations {
> +    /* Hold intc hw information */
> +    const struct intc_info *info;
> +    /* Initialize the intc and the boot CPU */
> +    int (*init)(void);
> +
> +    /* hw_irq_controller to enable/disable/eoi host irq */
> +    hw_irq_controller *host_irq_type;

Pointer-to-const perhaps?

> +    /* Set IRQ type */
> +    void (*set_irq_type)(struct irq_desc *desc, unsigned int type);
> +    /* Set IRQ priority */
> +    void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
> +
> +};
> +
>  void intc_preinit(void);
>  
> +void intc_init(void);
> +
> +void register_intc_ops(const struct intc_hw_operations *ops);
> +
> +struct irq_desc;

If it's needed here at all, it needs to move up, as some of the hook pointers
already use the type.

> --- a/xen/arch/riscv/intc.c
> +++ b/xen/arch/riscv/intc.c
> @@ -1,9 +1,21 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
>  #include <xen/acpi.h>
> +#include <xen/bug.h>
>  #include <xen/device_tree.h>
>  #include <xen/init.h>
> +#include <xen/irq.h>
>  #include <xen/lib.h>
> +#include <xen/spinlock.h>
> +
> +#include <asm/intc.h>
> +
> +static const struct intc_hw_operations *intc_hw_ops;

__ro_after_init perhaps?

> +
> +void register_intc_ops(const struct intc_hw_operations *ops)

__init perhaps?

> +{
> +    intc_hw_ops = ops;
> +}
>  
>  void __init intc_preinit(void)
>  {
> @@ -12,3 +24,42 @@ void __init intc_preinit(void)
>      else
>          panic("ACPI interrupt controller preinit() isn't implemented\n");
>  }
> +
> +void __init intc_init(void)
> +{
> +    ASSERT(intc_hw_ops);
> +
> +    if ( intc_hw_ops->init() )
> +        panic("Failed to initialize the interrupt controller drivers\n");
> +}
> +
> +/* desc->irq needs to be disabled before calling this function */
> +static void intc_set_irq_type(struct irq_desc *desc, unsigned int type)
> +{
> +    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(type != IRQ_TYPE_INVALID);
> +    ASSERT(intc_hw_ops && intc_hw_ops->set_irq_type);
> +
> +    intc_hw_ops->set_irq_type(desc, type);
> +}
> +
> +static void intc_set_irq_priority(struct irq_desc *desc, unsigned int priority)
> +{
> +    ASSERT(intc_hw_ops && intc_hw_ops->set_irq_priority);
> +
> +    intc_hw_ops->set_irq_priority(desc, priority);
> +}
> +
> +void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
> +{
> +    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
> +    ASSERT(spin_is_locked(&desc->lock));
> +    /* Can't route interrupts that don't exist */
> +    ASSERT(intc_hw_ops && desc->irq < intc_hw_ops->info->nr_irqs);
> +
> +    desc->handler = intc_hw_ops->host_irq_type;
> +
> +    intc_set_irq_type(desc, desc->arch.type);
> +    intc_set_irq_priority(desc, priority);

If these are going to remain the sole callers of the two functions, I'd question
the need for the separate functions. Some of the assertions done there would then
actually be redundant.

If not, is there a reason intc_set_irq_priority() doesn't have a lock-held check?

Jan

On 4/10/25 6:02 PM, Jan Beulich wrote:
> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/include/asm/intc.h
>> +++ b/xen/arch/riscv/include/asm/intc.h
>> @@ -17,6 +17,29 @@ struct intc_info {
>>       const struct dt_device_node *node;
>>   };
>>   
>> +struct intc_hw_operations {
>> +    /* Hold intc hw information */
>> +    const struct intc_info *info;
>> +    /* Initialize the intc and the boot CPU */
>> +    int (*init)(void);
>> +
>> +    /* hw_irq_controller to enable/disable/eoi host irq */
>> +    hw_irq_controller *host_irq_type;
> Pointer-to-const perhaps?

It could be pointer-to-const. I'll updat

>
>> +    /* Set IRQ type */
>> +    void (*set_irq_type)(struct irq_desc *desc, unsigned int type);
>> +    /* Set IRQ priority */
>> +    void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
>> +
>> +};
>> +
>>   void intc_preinit(void);
>>   
>> +void intc_init(void);
>> +
>> +void register_intc_ops(const struct intc_hw_operations *ops);
>> +
>> +struct irq_desc;
> If it's needed here at all, it needs to move up, as some of the hook pointers
> already use the type.

It could be dropped, but then we would need to include|<xen/irq.h>|, and I figured
the fewer includes we have, the better.

>
>> --- a/xen/arch/riscv/intc.c
>> +++ b/xen/arch/riscv/intc.c
>> @@ -1,9 +1,21 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>>   
>>   #include <xen/acpi.h>
>> +#include <xen/bug.h>
>>   #include <xen/device_tree.h>
>>   #include <xen/init.h>
>> +#include <xen/irq.h>
>>   #include <xen/lib.h>
>> +#include <xen/spinlock.h>
>> +
>> +#include <asm/intc.h>
>> +
>> +static const struct intc_hw_operations *intc_hw_ops;
> __ro_after_init perhaps?
>
>> +
>> +void register_intc_ops(const struct intc_hw_operations *ops)
> __init perhaps?

For both, yes, it should be __ro_after_init and __init.

>
>> +{
>> +    intc_hw_ops = ops;
>> +}
>>   
>>   void __init intc_preinit(void)
>>   {
>> @@ -12,3 +24,42 @@ void __init intc_preinit(void)
>>       else
>>           panic("ACPI interrupt controller preinit() isn't implemented\n");
>>   }
>> +
>> +void __init intc_init(void)
>> +{
>> +    ASSERT(intc_hw_ops);
>> +
>> +    if ( intc_hw_ops->init() )
>> +        panic("Failed to initialize the interrupt controller drivers\n");
>> +}
>> +
>> +/* desc->irq needs to be disabled before calling this function */
>> +static void intc_set_irq_type(struct irq_desc *desc, unsigned int type)
>> +{
>> +    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>> +    ASSERT(spin_is_locked(&desc->lock));
>> +    ASSERT(type != IRQ_TYPE_INVALID);
>> +    ASSERT(intc_hw_ops && intc_hw_ops->set_irq_type);
>> +
>> +    intc_hw_ops->set_irq_type(desc, type);
>> +}
>> +
>> +static void intc_set_irq_priority(struct irq_desc *desc, unsigned int priority)
>> +{
>> +    ASSERT(intc_hw_ops && intc_hw_ops->set_irq_priority);
>> +
>> +    intc_hw_ops->set_irq_priority(desc, priority);
>> +}
>> +
>> +void intc_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
>> +{
>> +    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>> +    ASSERT(spin_is_locked(&desc->lock));
>> +    /* Can't route interrupts that don't exist */
>> +    ASSERT(intc_hw_ops && desc->irq < intc_hw_ops->info->nr_irqs);
>> +
>> +    desc->handler = intc_hw_ops->host_irq_type;
>> +
>> +    intc_set_irq_type(desc, desc->arch.type);
>> +    intc_set_irq_priority(desc, priority);
> If these are going to remain the sole callers of the two functions, I'd question
> the need for the separate functions. Some of the assertions done there would then
> actually be redundant.

Likely they will be reused for similar function but for guest to route an interrupt
to a guest. And, perhaps, only intc_set_irq_type() will be re-used outside
intc_route_irq_to_{xen,guest}...

>
> If not, is there a reason intc_set_irq_priority() doesn't have a lock-held check?

Do you mean this one: ASSERT(spin_is_locked(&desc->lock));

...
And it is the reason why intc_set_irq_priority() doesn't have this check as it is checked in the
caller.

But I think just to be on a safe side, it could be better to add this check to
intc_set_irq_priority().

~ Oleksii