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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxx>
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