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
