Re: [PATCH v2 09/26] xen/riscv: introduce init interrupt controller operations

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 5/21/26 3:25 PM, Jan Beulich wrote:
> On 08.05.2026 16:43, Oleksii Kurochko wrote:
>> Introduce intc_hw_init_ops structure to avoid risky mix of init
>> function and non-init function.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>> Changes in v2:
>>   - New patch.
>> ---
>>   xen/arch/riscv/aplic.c            |  7 +++++--
>>   xen/arch/riscv/include/asm/intc.h | 10 +++++++---
>>   xen/arch/riscv/intc.c             | 10 ++++++++--
>>   3 files changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
>> index 739e8dab3498..97dc0ef731f0 100644
>> --- a/xen/arch/riscv/aplic.c
>> +++ b/xen/arch/riscv/aplic.c
>> @@ -306,12 +306,15 @@ static const hw_irq_controller aplic_xen_irq_type = {
>>   
>>   static const struct intc_hw_operations aplic_ops = {
>>       .info                = &aplic_info,
>> -    .init                = aplic_init,
>>       .host_irq_type       = &aplic_xen_irq_type,
>>       .handle_interrupt    = aplic_handle_interrupt,
>>       .set_irq_type        = aplic_set_irq_type,
>>   };
>>   
>> +static const struct intc_hw_init_ops __initdata aplic_init_ops = {
>> +    .init                = aplic_init,
>> +};
> 
> const wants to pair with __initconst. Then:
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
Thanks.

> However, I have another comment for consideration:
> 
>> --- a/xen/arch/riscv/intc.c
>> +++ b/xen/arch/riscv/intc.c
>> @@ -12,9 +12,13 @@
>>   
>>   static const struct intc_hw_operations *__ro_after_init intc_hw_ops;
>>   
>> -void __init register_intc_ops(const struct intc_hw_operations *ops)
>> +static const struct intc_hw_init_ops *__initdata intc_hw_init_ops;
>> +
>> +void __init register_intc_ops(const struct intc_hw_operations *ops,
>> +                              const struct intc_hw_init_ops *init_ops)
>>   {
>>       intc_hw_ops = ops;
>> +    intc_hw_init_ops = init_ops;
>>   }
> 
> Again following what we do e.g. in x86'es IOMMU code, instead of passing
> two pointers to the function, have struct intc_hw_init_ops have a
> const struct intc_hw_operations * member which then can be used to
> set intc_hw_ops here? Both will always come in pairs anyway.

It makes sense to me.

So I will do the following:

@@ -350,7 +351,7 @@ static int __init aplic_preinit(struct 
dt_device_node *node, const void *dat)

      dt_irq_xlate = aplic_irq_xlate;

-    register_intc_ops(&aplic_ops, &aplic_init_ops);
+    register_intc_ops(&aplic_init_ops);

      /* Enable supervisor external interrupt */
      csr_set(CSR_SIE, BIT(IRQ_S_EXT, UL));
diff --git a/xen/arch/riscv/include/asm/intc.h 
b/xen/arch/riscv/include/asm/intc.h
index 8b498e43b33f..3d84fcc51d1a 100644
--- a/xen/arch/riscv/include/asm/intc.h
+++ b/xen/arch/riscv/include/asm/intc.h
@@ -42,14 +42,14 @@ struct intc_hw_operations {
  };

  struct intc_hw_init_ops {
+    const struct intc_hw_operations *ops;
      /* Initialize the intc and the boot CPU */
      int (*init)(void);
  };

  void intc_preinit(void);

-void register_intc_ops(const struct intc_hw_operations *ops,
-                       const struct intc_hw_init_ops *init_ops);
+void register_intc_ops(const struct intc_hw_init_ops *init_ops);

  void intc_init(void);

diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
index 8649160403f7..3600d23bdb5b 100644
--- a/xen/arch/riscv/intc.c
+++ b/xen/arch/riscv/intc.c
@@ -14,10 +14,9 @@ static const struct intc_hw_operations 
*__ro_after_init intc_hw_ops;

  static const struct intc_hw_init_ops *__initdata intc_hw_init_ops;

-void __init register_intc_ops(const struct intc_hw_operations *ops,
-                              const struct intc_hw_init_ops *init_ops)
+void __init register_intc_ops(const struct intc_hw_init_ops *init_ops)
  {
-    intc_hw_ops = ops;
+    intc_hw_ops = init_ops->ops;
      intc_hw_init_ops = init_ops;
  }

Thanks.

~ Oleksii