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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4167 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 5864 bytes --]