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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 ...

> +    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).

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

Jan