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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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

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

> > --- /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  = ""
> >       
> 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 = "" in init_one_irq_desc().
But considering that irq_desc[] is zero-initialized then perhaps there is no any
sense for desc->action = "" at all.
> 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(). Or could ->irq be used in arch_init_one_irq_desc()
for something?

~ Oleksii