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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/16/26 12:24 PM, Oleksii Kurochko wrote:
> 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.
After inspecting of RISC-V spec again I found that:
To enforce ordering in all other cases, software should execute a FENCE 
instruction between the relevant accesses. For the purposes of the FENCE 
instruction, CSR read accesses are classified as device input (I), and 
CSR write accesses are classified as device output (O).
So wmb() should be enough in the current case as wmb is RISCV_FENCE(ow, 
ow). I will add the following comment:
    /*
     * wmb() (fence ow,ow) orders the ->status memory write (w) before the
     * CSR write inside imsic_irq_enable() (device output, o on RISC-V).
     * arch_lock_release_barrier() uses fence rw,rw which does not cover
     * device output (o), so wmb() is required to close that gap.
     */
    wmb();

> > > @@ -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.
And here I re-read your comment and IIUC that we want to guarantee that
desc->status |= IRQ_DISABLED; isn't globally visible until 
imsic_irq_disable() is executed. Then we need the following here:
+    /*
+     * wmb() (fence ow,ow) ensures the CSR write (device output, o) inside
+     * imsic_irq_disable() is globally visible before ->status is marked
+     * IRQ_DISABLED. imsic_irq_disable()'s spin_unlock uses fence rw,rw
+     * which does not order device output (o) writes before subsequent
+     * memory writes (w), so an explicit wmb() is needed here.
+     */
+    wmb();

Would you be okay with such barriers or you still think it is needed mb()?

Also I think considering that I'll added the comments above wmb() I 
could drop what I mentioned in commit message regarding barriers().
~ Oleksii