Re: [PATCH v1 07/14] xen/riscv: Introduce intc_hw_operations abstraction

[Jan Beulich <jbeulich@xxxxxxxx>]

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