Introduce the intc_hw_operations structure to encapsulate interrupt
controller-specific data and operations. This structure includes:
- A pointer to interrupt controller information (`intc_info`)
- Callbacks to initialize the controller and set IRQ type/priority
- A reference to an interupt controller descriptor (`host_irq_type`)
- number of interrupt controller irqs.

Add function register_intc_ops() to mentioned above structure.

Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - This patch was part of "xen/riscv: Introduce intc_hw_operations abstraction"
   and splitted to have ability to merge some patches from this patch series
   into one patch.
 - Declare host_irq_type member of intc_hw_operations as pointer-to-const.
 - Moved up forward declaration of irq_desc.
 - Use attribute __init for register_intc_ops().
 - Use attribute __ro_after_init for intc_hw_ops variable.
 - Update prototype of register_intc_ops() because of what mention in the
   previous point.
---
 xen/arch/riscv/include/asm/intc.h | 22 ++++++++++++++++++++++
 xen/arch/riscv/intc.c             |  9 +++++++++
 2 files changed, 31 insertions(+)

diff --git a/xen/arch/riscv/include/asm/intc.h b/xen/arch/riscv/include/asm/intc.h
index 52ba196d87..e72d5fd9d3 100644
--- 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>
+
 enum intc_version {
     INTC_APLIC,
 };
@@ -17,6 +19,26 @@ struct intc_info {
     const struct dt_device_node *node;
 };
 
+struct irq_desc;
+
+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 */
+    const hw_irq_controller *host_irq_type;
+
+    /* 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 register_intc_ops(struct intc_hw_operations *ops);
+
 #endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */
diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
index 4061a3c457..122e7b32b5 100644
--- 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;
+
+void __init register_intc_ops(struct intc_hw_operations *ops)
+{
+    intc_hw_ops = ops;
+}
+
 void __init intc_preinit(void)
 {
     if ( acpi_disabled )
-- 
2.49.0

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

> +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 */
> +    const hw_irq_controller *host_irq_type;
> +
> +    /* 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 register_intc_ops(struct intc_hw_operations *ops);
> +
>  #endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */

... throughout here.

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

Jan

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

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.

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

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

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

Jan

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

On 20.05.2025 16:04, Oleksii Kurochko wrote:
> 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?

The above decl merely introduces the type into (global) scope. The same is
achieved by ...

>>>     struct intc_hw_operations {
>>>         ...
>>>         // const hw_irq_controller *host_irq_type;
>>>         const struct hw_interrupt_type *host_irq_type;

... this. The case where the earlier decl matters is when a type is used
as a function parameter in a prototype. There, if not previously introduced
into global scope, the scope would be limited to that of the function decl
(and hence a type conflict would result when later the same type is
introduced into global scope).

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

You need to separate properties of the (pointer type) variable, and what
that pointer points to. __ro_after_init applies to the variable, whereas
the const asked for is to apply to the pointed-to type. (This is more
obvious when you place the attribute as requested. Hence why we want that
particular placement.)

Jan