Re: [PATCH v2 09/16] xen/riscv: introduce register_intc_ops() and intc_hw_ops.

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/15/25 10:06 AM, Jan Beulich wrote:

> On 06.05.2025 18:51, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/intc.h
> > +++ b/xen/arch/riscv/include/asm/intc.h
> > @@ -8,6 +8,8 @@
> >  #ifndef ASM__RISCV__INTERRUPT_CONTOLLER_H
> >  #define ASM__RISCV__INTERRUPT_CONTOLLER_H
> >  
> > +#include <xen/irq.h>
> >       
> If you need this include anyway, why ...
>
> > @@ -17,6 +19,26 @@ struct intc_info {
> >      const struct dt_device_node *node;
> >  };
> >  
> > +struct irq_desc;
> >       
> ... this "forward" decl for something that's then already fully defined?
> I can't, however, spot why xen/irq.h would be needed for anything ...
>     
forward decl for irq_desc could be really dropped.

Inclusion of xen/irq.h was added because of hw_irq_controller which is defined as:
  typedef const struct hw_interrupt_type hw_irq_controller;

And I'm not sure how to do forward declaration properly in this case. Just use
an explicit definition of hw_irq_controller for host_irq_type member of struct
intc_hw_operations seems as not the best one option:
  struct hw_interrupt_type;
 
  struct intc_hw_operations {
      ...
      // const hw_irq_controller *host_irq_type;
      const struct hw_interrupt_type *host_irq_type;

It seems like the best one option is to do the following:
  typedef const struct hw_interrupt_type hw_irq_controller; in asm/intc.h.
I will follow it then.

> > --- a/xen/arch/riscv/intc.c
> > +++ b/xen/arch/riscv/intc.c
> > @@ -5,6 +5,15 @@
> >  #include <xen/init.h>
> >  #include <xen/lib.h>
> >  
> > +#include <asm/intc.h>
> > +
> > +static struct __ro_after_init intc_hw_operations *intc_hw_ops;
> >       
> Nit: Attributes between type and identifier please. Also shouldn't both
> this and ...
>
> > +void __init register_intc_ops(struct intc_hw_operations *ops)
> >       
> ... the parameter here be pointer-to-const?
>     
Then intc_hw_ops should also be marked as const (with the removal of __ro_after_init),
otherwise a compilation error will occur (something like "assignment discards 'const'
qualifier").
Additionally, __ro_after_init should be replaced with const for aplic_ops in future
patches.
Let me know which approach you prefer. I prefer using const, as
intc_hw_operations and aplic_ops are not expected to change after initialization.
~ Oleksii