Re: [PATCH v2 26/26] xen/riscv: manage IRQ_DISABLED flag in APLIC irq enable/disable callbacks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.05.2026 16:43, Oleksii Kurochko wrote:
> desc->status is only set once during setup_irq(), but interrupts can be
> enabled/disabled at runtime, so update it in the corresponding callbacks.
> 
> wmb() in aplic_irq_enable() ensures do_IRQ(), which can fire immediately
> after the interrupt is enabled, sees the updated desc->status.

This doesn't look entirely correct. Aiui ...

> No rmb() is
> needed on the do_IRQ() side because desc->status is read under a spinlock,
> which implies an acquire barrier.
> 
> No barrier is needed in aplic_irq_disable() because the hardware disables
> the interrupt before the status is updated, so do_IRQ() cannot fire, and
> spin_unlock() makes the updated value visible.
> 
> Fixes: d4676a1398bc5 ("xen/riscv: implementation of aplic and imsic 
> operations")
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in v2:
>  - New patch.
> ---
>  xen/arch/riscv/aplic.c | 5 +++++
>  xen/arch/riscv/irq.c   | 3 ---
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -161,6 +161,9 @@ static void cf_check aplic_irq_enable(struct irq_desc 
> *desc)
>  
>      spin_lock(&aplic.lock);
>  
> +    desc->status &= ~IRQ_DISABLED;
> +    wmb();
> +
>      /* Enable interrupt in IMSIC */
>      imsic_irq_enable(desc->irq);

... you want to order the ->status update ahead of the imsic_irq_enable()
operation. Yet that's a CSR write. Do fences really cover that? If not,
you may need to resort to mb(), to order the ->status write against the
->irq read, in lieu of being able to order against a CSR write.

Of course with imsic_irq_enable() itself acquiring a lock (which
necessarily has a memory write), you could then further argue that wmb()
is indeed sufficient, bot for a reason different from the one presently
stated in the description. (I would also strongly suggest to annotate the
wmb() with an explanatory comment.)

> @@ -189,6 +192,8 @@ static void cf_check aplic_irq_disable(struct irq_desc 
> *desc)
>      /* Disable interrupt in IMSIC */
>      imsic_irq_disable(desc->irq);
>  
> +    desc->status |= IRQ_DISABLED;
> +
>      spin_unlock(&aplic.lock);
>  }

The ->status write becoming globally visible ahead of imsic_irq_disable()
doing its work is not an issue? In the paragraph in the description you
again appear to assume that the CSR write and the memory write of the
spin-unlock are ordered wrt one another.

> --- a/xen/arch/riscv/irq.c
> +++ b/xen/arch/riscv/irq.c
> @@ -145,9 +145,6 @@ int setup_irq(unsigned int irq, unsigned int irqflags, 
> struct irqaction *new)
>          desc->handler->set_affinity(desc, cpumask_of(smp_processor_id()));
>  
>          desc->handler->startup(desc);
> -
> -        /* Enable irq */
> -        desc->status &= ~IRQ_DISABLED;
>      }

Arm and x86 are different in this regard, so it's hard to tell which way
it ought to be for RISC-V. This removal surely wants a sentence or two in
the description.

Jan