Re: [PATCH] RISC-V: KVM: Order IMSIC VS-file write before releasing vsfile_lock

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 11/13/26 9:00 AM, Anup Patel wrote:
> On Fri, Oct 23, 2026 at 7:56 PM Oleksii Kurochko
> <oleksii.kurochko@gmail.com> wrote:
>>
>> kvm_riscv_vcpu_aia_imsic_inject() writes the interrupt identity to the
>> SETIPNUM register of the vCPU's IMSIC VS-file while holding vsfile_lock
>> for reading. The lock keeps the VS-file from being switched under the
>> injector: kvm_riscv_vcpu_aia_imsic_update() and
>> kvm_riscv_vcpu_aia_imsic_release() take it for writing to move the vCPU
>> to a new VS-file or to the SW-file, then read and clear the old VS-file
>> and free its HGEI line.
>>
>> On RISC-V, writel() is followed only by mmiowb_set_pending(). The
>> trailing "fence o,w" is deferred to spin_unlock() by the mmiowb()
>> tracking, and rwlocks don't take part in that tracking:
>> queued_read_unlock() issues just "fence rw,w" before the AMO, which
>> doesn't order device output. So the SETIPNUM write can still be in
>> flight after another hart has observed vsfile_lock being released. If
>> the old VS-file is read and cleared before the write lands, the
>> interrupt is lost. If the HGEI line has already been handed out
>> again, the interrupt ends up in another vCPU's VS-file.
>>
>> Add an explicit RISCV_FENCE(o, w) after the write to the VS-file, which
>> is what spin_unlock() would issue via mmiowb().
>>
>> Fixes: db8b7e97d613 ("RISC-V: KVM: Add in-kernel virtualization of AIA IMSIC")
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>> Why RISCV_FENCE(o, w) and not mmiowb():
>>
>> - Since the mmiowb() rework in v5.2 (e.g. commit 60ca1e5a200c
>>    ("mmiowb: Hook up mmiowb helpers to spinlocks and generic I/O
>>    accessors")), mmiowb() is an implementation detail of the spinlock
>>    code. Explicit invocations were removed from drivers by commit
>>    fb24ea52f78e ("drivers: Remove explicit invocations of mmiowb()") and
>>    from the documentation by commit 915530396c78 ("Documentation: Kill
>>    all references to mmiowb()"), so adding a new direct caller would go
>>    against that.
>>
>> - RISCV_FENCE(o, w) states exactly the ordering needed here (device
>>    output before the store that releases the lock), which is fine for
>>    arch-specific code, and it is the same instruction mmiowb() expands to
>>    on RISC-V. wmb() ("fence ow,ow") would also work but is stronger than
>>    required.
>>
>> Scope: this only orders the write before the lock release. Whether a
>> write that is already in global memory order is guaranteed to have
>> taken effect in the old VS-file before kvm_riscv_vcpu_aia_imsic_update()
>> or kvm_riscv_vcpu_aia_imsic_release() reads and clears it is a separate
>> question, see https://github.com/riscv/riscv-aia/issues/172. If it
>> isn't, these paths will also need to drain the old VS-file, and such a
>> drain depends on the ordering added here.
>> ---
>>   arch/riscv/kvm/aia_imsic.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
>> index f17dc559a709..807bb73a129a 100644
>> --- a/arch/riscv/kvm/aia_imsic.c
>> +++ b/arch/riscv/kvm/aia_imsic.c
>> @@ -1056,6 +1056,13 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
>>
>>          if (imsic->vsfile_cpu >= 0) {
>>                  writel(iid, imsic->vsfile_va + IMSIC_MMIO_SETIPNUM_LE);
>> +               /*
>> +                * rwlocks don't participate in mmiowb() tracking, so order
>> +                * the write to the VS-file before releasing vsfile_lock,
>> +                * i.e. before the vCPU can be moved off this VS-file and
>> +                * the VS-file read and cleared.
>> +                */
>> +               RISCV_FENCE(o, w);
> 
> Another fence after every writel() is a big hammer in the path.
> 
> If the intention is to have correct contents when VS-file is read then
> better place to add a fence is at the start of imsic_vsfile_local_read().
Thanks for the review.

I don't think a fence in imsic_vsfile_local_read() helps here. The
problem is on the writer's side, and imsic_vsfile_local_read() runs on
a different hart.

The SETIPNUM write is done by whichever hart calls
kvm_riscv_vcpu_aia_imsic_inject(), which can be any CPU (irqfd,
ioctl, ...). imsic_vsfile_local_read() runs on the old vsfile_cpu, via
the IPI from imsic_vsfile_read(). A fence only orders the accesses of
the hart that executes it, so a fence on the reading hart can't make
another hart's MMIO store land.

Without the fence, this can happen:

   hart A (inject)            hart B (update/release)    hart C (old 
vsfile_cpu)
   read_lock()
   writel(SETIPNUM)
     (store still in flight)
   read_unlock()
                              write_lock()
                              switch VS-file
                              write_unlock()
                              IPI ------------------->
                                                        fence  <-proposed
                                                        read+clear VSfile
                              free HGEI
   SETIPNUM lands: the interrupt
   is lost, or ends up in another
   vCPU's VS-file if the HGEI was
   already reused

Hart A's unlock can become visible before its SETIPNUM write has
reached the IMSIC, because read_unlock() doesn't order device output.
Nothing that hart B or hart C does afterwards can wait for that write.
Only hart A can hold back its unlock until the write is ordered, and
that is what the fence o,w after writel() does.

With the fence:

   hart A (inject)            hart B (update/release)    hart C (old 
vsfile_cpu)
   read_lock()
   writel(SETIPNUM)
   fence o,w
   read_unlock()
                              write_lock()
                              switch VS-file
                              write_unlock()
                              IPI ------------------->
                                                       read+clear VS-file
                              free HGEI

The SETIPNUM write is now ordered before the unlock, so it is also
ordered before everything hart B does after taking the lock, and,
through the IPI, before hart C reads the old VS-file. It can no longer
arrive after the read or after the HGEI is freed.

About the cost: this is the same fence that every writel() under a
spinlock already gets. On RISC-V, writel() only sets mmiowb pending and
spin_unlock() then issues "fence o,w". We need it explicitly here only
because vsfile_lock is an rwlock. It runs once per injected MSI, and
only when the vCPU has a VS-file.

The only way I see to keep it out of the inject path is for
update/release to IPI every CPU that might be injecting and run
"fence o,w" there before the old VS-file is read. That is much heavier
and moves the cost to vCPU migration, so I'd rather keep the fence in
inject. Let me know if you'd still prefer the other way.

One more thing, not directly connected to this patch: could you please
also take a look at https://github.com/riscv/riscv-aia/issues/172?
The fence makes sure that the SETIPNUM write is ordered before hart C
reads the old VS-file, but hart C reads it through CSRs
(vsiselect/vsireg), not through memory. As far as I can tell, the spec
doesn't say whether a write that is already in global memory order is
guaranteed to be visible in eip at that point. If it isn't,
update/release will also need to drain the old VS-file before reading
it. Such a drain would build on this fence rather than replace it, since
it only works if the write is ordered before the unlock.

~ Oleksii