Re: [PATCH v2 06/16] xen/riscv: introduce init_IRQ()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.05.2025 13:53, Oleksii Kurochko wrote:
> On 5/14/25 4:49 PM, Jan Beulich wrote:
>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/irq.h
>>> +++ b/xen/arch/riscv/include/asm/irq.h
>>> @@ -3,6 +3,11 @@
>>>   #define ASM__RISCV__IRQ_H
>>>   
>>>   #include <xen/bug.h>
>>> +#include <xen/device_tree.h>
>>> +
>>> +#include <asm/irq-dt.h>
>>> +
>>> +#define NR_IRQS 1024
>> Is this arbitrary or arch-induced? Imo it wants saying in a brief comment 
>> either
>> way. Then again maybe it's entirely obvious for a RISC-V person ...
> 
> 1024 it is number of interrupt sources for PLIC and APLIC. I will add the 
> comment above:
> /*
>   * According to the AIA spec:
>   *   The maximum number of interrupt sources an APLIC may support is 1023.
>   *
>   * The same is true for PLIC.
>   *
>   * Interrupt Source 0 is reserved and shall never generate an interrupt.
>   */
> #define NR_CPUS 1024

s/CPU/IRQ I suppose.

> Probably, it makes sense to change it to 1023 as interrupt 0 isn't really 
> used.

I'd recommend against that. It would likely make things more difficult when
range-checking IRQ numbers.

>>> --- /dev/null
>>> +++ b/xen/arch/riscv/irq.c
>>> @@ -0,0 +1,45 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +
>>> +/*
>>> + * RISC-V Trap handlers
>>> + *
>>> + * Copyright (c) 2024 Vates
>>> + */
>>> +
>>> +#include <xen/bug.h>
>>> +#include <xen/init.h>
>>> +#include <xen/irq.h>
>>> +
>>> +static irq_desc_t irq_desc[NR_IRQS];
>>> +
>>> +int arch_init_one_irq_desc(struct irq_desc *desc)
>>> +{
>>> +    desc->arch.type = IRQ_TYPE_INVALID;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int __init init_irq_data(void)
>>> +{
>>> +    unsigned int irq;
>>> +
>>> +    for ( irq = 0; irq < NR_IRQS; irq++ )
>>> +    {
>>> +        struct irq_desc *desc = irq_to_desc(irq);
>>> +        int rc = init_one_irq_desc(desc);
>>> +
>>> +        if ( rc )
>>> +            return rc;
>>> +
>>> +        desc->irq = irq;
>>> +        desc->action  = NULL;
>> Nit: You copied a stray blank from Arm code. Actually I wonder why it isn't
>> init_one_irq_desc() that clears this field.
> 
> I can come up with the patch which does desc->action = NULL in 
> init_one_irq_desc().
> But considering that irq_desc[] is zero-initialized then perhaps there is no 
> any
> sense for desc->action = NULL, at all.

Oh, yes, indeed.

>> It also feels like ->irq would
>> better be set ahead of calling that function, like x86 has it.
> 
> But what is the difference with an order of writing a value to ->irq? It isn't
> really used in init_one_irq_desc().

That's the present state of things, yes. There or ...

> Or could ->irq be used in arch_init_one_irq_desc()
> for something?

... there it could be used e.g. for a log message. Maybe even just a transient
debugging one. And there's no (proper) way to re-establish the IRQ number from
the desc pointer, at least not in arch-independent code.

Jan