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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/10/25 6:02 PM, Jan Beulich wrote:

> 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?
>     
It could be pointer-to-const. I'll updat
> > +    /* 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.
>     
It could be dropped, but then we would need to include <xen/irq.h>, and I figured
the fewer includes we have, the better.

> > --- 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?
>     
For both, yes, it should be __ro_after_init and __init. 
> > +{
> > +    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.
>     
Likely they will be reused for similar function but for guest to route an interrupt
to a guest. And, perhaps, only intc_set_irq_type() will be re-used outside
intc_route_irq_to_{xen,guest}...
> If not, is there a reason intc_set_irq_priority() doesn't have a lock-held check?
>     
Do you mean this one: ASSERT(spin_is_locked(&desc->lock));

...
And it is the reason why intc_set_irq_priority() doesn't have this check as it is checked in the
caller.

But I think just to be on a safe side, it could be better to add this check to 
intc_set_irq_priority().

~ Oleksii