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

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

On 11/14/26 2:35 PM, Anup Patel wrote:
> On Fri, Nov 13, 2026 at 11:49 PM Oleksii Kurochko
> <oleksii.kurochko@gmail.com> wrote:
>>
>>
>>
>> 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)
> 
> Over here the MMIO write targets hart C IMSIC
> 
>>     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.
> 
> I am still not on-board adding a barrier in hot-path. Also, any barrier
> we add must be paired with a barrier else where with proper comments
> so this single barrier anyways does not satify this requirement.

Oh, right, I missed that.

> 
> Instead of this, I think a much cleaner approach is to convert vsfile_lock
> to raw_spinlock_t. The current vsfile_lock being a rwlock_t is create other
> issues as well [1]. This will also allow us to use writel_relaxed() in the
> kvm_riscv_vcpu_aia_imsic_inject() since raw_spin_unlock() will take
> care of draining the write buffer.

Totally agree, raw_spinlock_t will work here and will be cleaner.

> 
> I will post something along these lines.

Thanks!

> 
> [1] https://lore.kernel.org/r/20260925155116.465203-1-andrew.jones@oss.qualcomm.com

~ Oleksii