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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/19/25 3:16 PM, Jan Beulich wrote:

> On 19.05.2025 11:16, Oleksii Kurochko wrote:
> > 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;
> >       
> This isn't needed for the use below.
>     
Shouldn't I tell the compiler that definition of hw_interrupt_type struct exist
somewhere else?
> >    struct intc_hw_operations {
> >        ...
> >        // const hw_irq_controller *host_irq_type;
> >        const struct hw_interrupt_type *host_irq_type;
> >       
> It might be that something like this is already done elsewhere in the tree,
> so not really an issue imo if a 2nd instance appeared.
>     
It is really happing for several places in x86 code.
> > 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.
> >       
> Misra may dislike this.
>     
Then this is not an option. I will use then the option above.

> > > > --- 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|),
> >       
> Why remove the attribute?
>     
My understanding is that if it is marked as 'const' then it automatically mean that it is read-only
always before and after init, so '__ro_after_init' is useless.
> > 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.
> >       
> Same question here then.
>     
Just wanted to be in sync. If I have intc_hw_ops marked as const, then the thing which will be used
to set intc_hw_ops wants to be also const.

~ Oleksii