Re: [PATCH v2 12/16] xen/riscv: introduce intc_init() and helpers

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/15/25 11:29 AM, Jan Beulich wrote:

> On 06.05.2025 18:51, Oleksii Kurochko wrote:
> > Introduce intc_init() to initialize the interrupt controller using the
> > registered hardware ops.
> > Also add intc_route_irq_to_xen() to route IRQs to Xen, with support for
> > setting IRQ type and priority via new internal helpers intc_set_irq_type()
> > and intc_set_irq_priority().
> >
> > Call intc_init() to do basic initialization steps for APLIC and IMSIC.
> >
> > Co-developed-by: Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in V2:
> >  - This patch was part of "xen/riscv: Introduce intc_hw_operations abstraction"
> >    and splitted to have ability to merge patch "xen/riscv: initialize interrupt
> >    controller" to the current patch (where intc_init() call is actually
> >    introduced).
> >  - Add checks of that callbacks aren't set to NULL in intc_set_irq_priority()
> >    and intc_set_irq_type().
> >  - add num_irqs member to struct intc_info as it is used now in
> >    intc_route_irq_to_xen().
> >  - Add ASSERT(spin_is_locked(&desc->lock)) to intc_set_irq_priority() in
> >    the case this function will be called outside intc_route_irq_to_xen().
> > ---
> >  xen/arch/riscv/include/asm/intc.h |  4 +++
> >  xen/arch/riscv/intc.c             | 45 +++++++++++++++++++++++++++++++
> >  xen/arch/riscv/setup.c            |  2 ++
> >  3 files changed, 51 insertions(+)
> >
> > diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
> > index 2d55d74a2e..45a41147a6 100644
> > --- a/xen/arch/riscv/include/asm/intc.h
> > +++ b/xen/arch/riscv/include/asm/intc.h
> > @@ -44,4 +44,8 @@ void intc_preinit(void);
> >  
> >  void register_intc_ops(struct intc_hw_operations *ops);
> >  
> > +void intc_init(void);
> > +
> > +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 122e7b32b5..15f358601d 100644
> > --- a/xen/arch/riscv/intc.c
> > +++ b/xen/arch/riscv/intc.c
> > @@ -1,9 +1,12 @@
> >  /* 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>
> >  
> > @@ -21,3 +24,45 @@ void __init intc_preinit(void)
> >      else
> >          panic("ACPI interrupt controller preinit() isn't implemented\n");
> >  }
> > +
> > +void __init intc_init(void)
> > +{
> > +    ASSERT(intc_hw_ops);
> >       
> What's the goal of this check (also elsewhere below)? You'll crash anyway ...
>     
Agree, that it could be dropped.

The goal was that it will a little bit easier to find a place where NULL
pointer de-reference  will/could happen.

Then it make sense to drop ASSERT(intc_hw_ops) in intc_set_irq_type() and
intc_set_irq_priority() as intc_init() will be called first and so if it
won't crash, then intc_hw_ops is registered.
> > +    if ( intc_hw_ops->init() )
> >       
> ... here if the point is still NULL, just like you will if the function
> pointer is unpopulated (and hence NULL).
>     
Here, I think it would be better to rewrite to:
  void __init intc_init(void)
  {
      if ( !intc_hw_ops->init )
          return;

      if ( intc_hw_ops->init() )
          panic("Failed to initialize the interrupt controller drivers\n");
  }
For the case, if an interrupt controller doesn't want to do some initialization.
(but I am not sure if it makes sense, likely an interrupt controller will want
to do some initialization. Then it makes do drop the first if (...)).

> Preferably with all of these (but not the other assertions) dropped
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>     
Thanks.

~ Oleksii