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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/15/26 5:53 PM, Jan Beulich wrote:
> 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?
Good point. The RISC-V ISA spec is clear that fence orders memory 
predecessor and successor operations. CSR accesses are architecturally a 
distinct category. fence w,w carries no formal guarantee that a 
preceding memory store to desc->status becomes visible before a 
subsequent CSR write.
Also, according to the spec:

Each RISC-V hart normally observes its own CSR accesses, including its 
implicit CSR accesses, as performed in program order. In particular, 
unless specified otherwise, a CSR access is performed after the 
execution of any prior instructions in program order whose behavior 
modifies or is modified by the CSR state and before the execution of any 
subsequent instructions in program order whose behavior modifies or is 
modified by the CSR state. Furthermore, an explicit CSR read returns the 
CSR state before the execution of the instruction, while an explicit CSR 
write suppresses and overrides any implicit writes or modifications to 
the same CSR by the same instruction.
and ...

 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.)
...

After re-looking at imsic_irq_enable() it is calling 
imsic_local_eix_update() which targets the local hart's IMSIC interrupt 
file (note the function name), and IRQs are disabled at this point (the 
ASSERT(!local_irq_is_enabled())). So do_IRQ() can only fire on the same 
hart, after IRQs are re-enabled on return from aplic_irq_enable(). 
Same-hart program order already guarantees desc->status is committed 
(before of the mentioned above) before the CSR write retires, making the 
barrier redundant.
So barrier could be dropped here.

On the other side if some code will check on different CPU what is 
desc->status without a barrier it will see a stale value. But then in 
this case we have to use or atomic set_bit() here or 
spinlock(desc->lock) around an update of desc->status. Then the similar 
is needed for aplic_irq_disable().
> > @@ -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.
The ordering is intentional: desc->status |= IRQ_DISABLED is placed 
after imsic_irq_disable() so that do_IRQ() can never observe 
IRQ_DISABLED while the interrupt is still enabled in hardware.
Regarding the fence/CSR ordering concern: imsic_irq_disable() writes to 
the local hart's IMSIC interrupt file, so do_IRQ() for this irq can only 
run on the same hart. Same-hart program order already guarantees that 
both the CSR write and the desc->status update are visible in the 
correct sequence before any subsequent interrupt handler runs.
> > --- 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.
I will add the following to commit description:

The removal of desc->status &= ~IRQ_DISABLED from setup_irq() is safe 
for RISC-V because aplic_irq_startup() calls aplic_irq_enable() 
directly, which now clears the flag itself. The explicit clear in 
setup_irq() is therefore redundant.
Thanks.

~ Oleksii