* [PATCH v4] RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
@ 2026-11-13 10:13 ` BillXiang
0 siblings, 0 replies; 7+ messages in thread
From: BillXiang @ 2026-11-13 10:13 UTC (permalink / raw)
To: anup
Cc: xb, atish.patra, pjw, palmer, aou, alex, pbonzini, graf,
kvm-riscv, kvm, linux-riscv, linux-kernel, xiangwencheng
The TOPEI read-and-clear must be atomic with respect to
interrupt injection (set_bit in EIP). Otherwise an
interrupt injected between the topei read and the
clear_bit could be silently dropped.
Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com>
---
v4:
- pass topei value to __imsic_swfile_extirq_update()
v3:
- fix undefined eflags
v2:
- fix patch file curruption
v1: https://lore.kernel.org/kvm-riscv/20260923070332.1376132-1-xiangwencheng@lanxincomputing.com/
---
arch/riscv/kvm/aia_imsic.c | 47 +++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
index c1af23e79ae0..0a6b20f3a28d 100644
--- a/arch/riscv/kvm/aia_imsic.c
+++ b/arch/riscv/kvm/aia_imsic.c
@@ -607,26 +607,25 @@ static void imsic_vsfile_cleanup(struct imsic *imsic)
kvm_riscv_aia_free_hgei(old_vsfile_cpu, old_vsfile_hgei);
}
-static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
+static void __imsic_swfile_extirq_update(struct kvm_vcpu *vcpu, u32 topei)
{
struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
struct imsic_mrif *mrif = imsic->swfile;
- unsigned long flags;
-
- /*
- * The critical section is necessary during external interrupt
- * updates to avoid the risk of losing interrupts due to potential
- * interruptions between reading topei and updating pending status.
- */
-
- raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
- if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) &&
- imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis))
+ if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) && topei)
kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT);
else
kvm_riscv_vcpu_unset_interrupt(vcpu, IRQ_VS_EXT);
+}
+
+static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
+{
+ struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
+ struct imsic_mrif *mrif = imsic->swfile;
+ unsigned long flags;
+ raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
+ __imsic_swfile_extirq_update(vcpu, imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis));
raw_spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags);
}
@@ -912,12 +911,15 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
struct imsic_mrif_eix *eix;
int r, rc = KVM_INSN_CONTINUE_NEXT_SEPC;
struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
+ unsigned long flags;
/* If IMSIC vCPU state not initialized then forward to user space */
if (!imsic)
return KVM_INSN_EXIT_TO_USER_SPACE;
if (isel == KVM_RISCV_AIA_IMSIC_TOPEI) {
+ raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
+
/* Read pending and enabled interrupt with highest priority */
topei = imsic_mrif_topei(imsic->swfile, imsic->nr_eix,
imsic->nr_msis);
@@ -934,16 +936,20 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
eix->eip);
}
}
+ if (wr_mask)
+ __imsic_swfile_extirq_update(vcpu, topei);
+
+ raw_spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags);
} else {
r = imsic_mrif_rmw(imsic->swfile, imsic->nr_eix, isel,
val, new_val, wr_mask);
/* Forward unknown IMSIC register to user-space */
if (r)
rc = (r == -ENOENT) ? 0 : KVM_INSN_ILLEGAL_TRAP;
- }
- if (wr_mask)
- imsic_swfile_extirq_update(vcpu);
+ if (wr_mask)
+ imsic_swfile_extirq_update(vcpu);
+ }
return rc;
}
@@ -1034,6 +1040,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
unsigned long flags;
struct imsic_mrif_eix *eix;
struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
+ struct imsic_mrif *mrif = imsic->swfile;
/* We only emulate one IMSIC MMIO page for each Guest VCPU */
if (!imsic || !iid || guest_index ||
@@ -1050,9 +1057,17 @@ 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);
} else {
+ /*
+ * Serialize the EIP set + external-irq update against the
+ * TOPEI read-and-clear path so that an injected interrupt
+ * cannot be silently lost when the guest acknowledges a
+ * different (or the same) interrupt concurrently.
+ */
+ raw_spin_lock(&imsic->swfile_extirq_lock);
eix = &imsic->swfile->eix[iid / BITS_PER_TYPE(u64)];
set_bit(iid & (BITS_PER_TYPE(u64) - 1), eix->eip);
- imsic_swfile_extirq_update(vcpu);
+ __imsic_swfile_extirq_update(vcpu, imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis));
+ raw_spin_unlock(&imsic->swfile_extirq_lock);
}
read_unlock_irqrestore(&imsic->vsfile_lock, flags);
--
2.53.0
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v4] RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
@ 2026-11-13 10:13 ` BillXiang
0 siblings, 0 replies; 7+ messages in thread
From: BillXiang @ 2026-11-13 10:13 UTC (permalink / raw)
To: anup
Cc: xb, atish.patra, pjw, palmer, aou, alex, pbonzini, graf,
kvm-riscv, kvm, linux-riscv, linux-kernel, xiangwencheng
The TOPEI read-and-clear must be atomic with respect to
interrupt injection (set_bit in EIP). Otherwise an
interrupt injected between the topei read and the
clear_bit could be silently dropped.
Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com>
---
v4:
- pass topei value to __imsic_swfile_extirq_update()
v3:
- fix undefined eflags
v2:
- fix patch file curruption
v1: https://lore.kernel.org/kvm-riscv/20260923070332.1376132-1-xiangwencheng@lanxincomputing.com/
---
arch/riscv/kvm/aia_imsic.c | 47 +++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
index c1af23e79ae0..0a6b20f3a28d 100644
--- a/arch/riscv/kvm/aia_imsic.c
+++ b/arch/riscv/kvm/aia_imsic.c
@@ -607,26 +607,25 @@ static void imsic_vsfile_cleanup(struct imsic *imsic)
kvm_riscv_aia_free_hgei(old_vsfile_cpu, old_vsfile_hgei);
}
-static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
+static void __imsic_swfile_extirq_update(struct kvm_vcpu *vcpu, u32 topei)
{
struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
struct imsic_mrif *mrif = imsic->swfile;
- unsigned long flags;
-
- /*
- * The critical section is necessary during external interrupt
- * updates to avoid the risk of losing interrupts due to potential
- * interruptions between reading topei and updating pending status.
- */
-
- raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
- if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) &&
- imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis))
+ if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) && topei)
kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT);
else
kvm_riscv_vcpu_unset_interrupt(vcpu, IRQ_VS_EXT);
+}
+
+static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
+{
+ struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
+ struct imsic_mrif *mrif = imsic->swfile;
+ unsigned long flags;
+ raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
+ __imsic_swfile_extirq_update(vcpu, imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis));
raw_spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags);
}
@@ -912,12 +911,15 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
struct imsic_mrif_eix *eix;
int r, rc = KVM_INSN_CONTINUE_NEXT_SEPC;
struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
+ unsigned long flags;
/* If IMSIC vCPU state not initialized then forward to user space */
if (!imsic)
return KVM_INSN_EXIT_TO_USER_SPACE;
if (isel == KVM_RISCV_AIA_IMSIC_TOPEI) {
+ raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
+
/* Read pending and enabled interrupt with highest priority */
topei = imsic_mrif_topei(imsic->swfile, imsic->nr_eix,
imsic->nr_msis);
@@ -934,16 +936,20 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
eix->eip);
}
}
+ if (wr_mask)
+ __imsic_swfile_extirq_update(vcpu, topei);
+
+ raw_spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags);
} else {
r = imsic_mrif_rmw(imsic->swfile, imsic->nr_eix, isel,
val, new_val, wr_mask);
/* Forward unknown IMSIC register to user-space */
if (r)
rc = (r == -ENOENT) ? 0 : KVM_INSN_ILLEGAL_TRAP;
- }
- if (wr_mask)
- imsic_swfile_extirq_update(vcpu);
+ if (wr_mask)
+ imsic_swfile_extirq_update(vcpu);
+ }
return rc;
}
@@ -1034,6 +1040,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
unsigned long flags;
struct imsic_mrif_eix *eix;
struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
+ struct imsic_mrif *mrif = imsic->swfile;
/* We only emulate one IMSIC MMIO page for each Guest VCPU */
if (!imsic || !iid || guest_index ||
@@ -1050,9 +1057,17 @@ 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);
} else {
+ /*
+ * Serialize the EIP set + external-irq update against the
+ * TOPEI read-and-clear path so that an injected interrupt
+ * cannot be silently lost when the guest acknowledges a
+ * different (or the same) interrupt concurrently.
+ */
+ raw_spin_lock(&imsic->swfile_extirq_lock);
eix = &imsic->swfile->eix[iid / BITS_PER_TYPE(u64)];
set_bit(iid & (BITS_PER_TYPE(u64) - 1), eix->eip);
- imsic_swfile_extirq_update(vcpu);
+ __imsic_swfile_extirq_update(vcpu, imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis));
+ raw_spin_unlock(&imsic->swfile_extirq_lock);
}
read_unlock_irqrestore(&imsic->vsfile_lock, flags);
--
2.53.0
--
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v4] RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
2026-11-13 10:13 ` BillXiang
(?)
(?)
@ 2026-11-13 10:34 ` sashiko-bot
2026-11-13 11:13 ` BillXiang
-1 siblings, 1 reply; 7+ messages in thread
From: sashiko-bot @ 2026-11-13 10:34 UTC (permalink / raw)
To: BillXiang; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] RISC-V: KVM: stale topei passed to
__imsic_swfile_extirq_update()
- [High] RISC-V: KVM: imsic->swfile dereferenced before !imsic check in
inject
- [Low] RISC-V: KVM: missing Fixes trailer on IMSIC TOPEI serialization
fix
--
commit 322bd38f6016b31c1cdf5b875effdce4436f4b2f
Author: BillXiang <xiangwencheng@lanxincomputing.com>
> RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
>
> The TOPEI read-and-clear must be atomic with respect to
> interrupt injection (set_bit in EIP). Otherwise an
> interrupt injected between the topei read and the
> clear_bit could be silently dropped.
>
> Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com>
[Severity: Low]
Since this fixes a race in the IMSIC software file TOPEI handling, should
it include:
Fixes: db8b7e97d613 ("RISC-V: KVM: Add in-kernel virtualization of AIA IMSIC")
> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
> index 302938972aa0e..f4cc1691e0478 100644
> --- a/arch/riscv/kvm/aia_imsic.c
> +++ b/arch/riscv/kvm/aia_imsic.c
> @@ -526,26 +526,25 @@ static void imsic_vsfile_cleanup(struct imsic *imsic)
> kvm_riscv_aia_free_hgei(old_vsfile_cpu, old_vsfile_hgei);
> }
>
> -static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
> +static void __imsic_swfile_extirq_update(struct kvm_vcpu *vcpu, u32 topei)
> {
> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
> struct imsic_mrif *mrif = imsic->swfile;
> - unsigned long flags;
> -
> - /*
> - * The critical section is necessary during external interrupt
> - * updates to avoid the risk of losing interrupts due to potential
> - * interruptions between reading topei and updating pending status.
> - */
> -
> - raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>
> - if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) &&
> - imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis))
> + if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) && topei)
> kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT);
> else
> kvm_riscv_vcpu_unset_interrupt(vcpu, IRQ_VS_EXT);
> +}
> +
> +static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
> +{
> + struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
> + struct imsic_mrif *mrif = imsic->swfile;
> + unsigned long flags;
>
> + raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
> + __imsic_swfile_extirq_update(vcpu, imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis));
> raw_spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags);
> }
>
> @@ -830,12 +829,15 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
> struct imsic_mrif_eix *eix;
> int r, rc = KVM_INSN_CONTINUE_NEXT_SEPC;
> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
> + unsigned long flags;
>
> /* If IMSIC vCPU state not initialized then forward to user space */
> if (!imsic)
> return KVM_INSN_EXIT_TO_USER_SPACE;
>
> if (isel == KVM_RISCV_AIA_IMSIC_TOPEI) {
> + raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
> +
> /* Read pending and enabled interrupt with highest priority */
> topei = imsic_mrif_topei(imsic->swfile, imsic->nr_eix,
> imsic->nr_msis);
> @@ -852,16 +854,20 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
> eix->eip);
> }
> }
> + if (wr_mask)
> + __imsic_swfile_extirq_update(vcpu, topei);
[Severity: High]
Does this pass the pre-clear interrupt ID in topei to
__imsic_swfile_extirq_update() instead of re-reading imsic_mrif_topei()
after clear_bit()?
When a guest claims the last pending interrupt in imsic->swfile via a
TOPEI write, topei still holds the non-zero ID that was just cleared from
eix->eip. __imsic_swfile_extirq_update() then calls
kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT) instead of unsetting
IRQ_VS_EXT, leaving IRQ_VS_EXT asserted and causing spurious guest
external interrupt traps.
[ ... ]
> @@ -959,6 +965,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
> unsigned long flags;
> struct imsic_mrif_eix *eix;
> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
> + struct imsic_mrif *mrif = imsic->swfile;
>
> /* We only emulate one IMSIC MMIO page for each Guest VCPU */
> if (!imsic || !iid || guest_index ||
[Severity: High]
Does imsic->swfile dereference a NULL imsic pointer before the !imsic
check below? If a vCPU is created after aia_init(), its imsic_state is
NULL and injecting an MSI to that vCPU will crash the host kernel instead
of returning -ENODEV.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261113101358.3007601-1-xiangwencheng@lanxincomputing.com?part=1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v4] RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
2026-11-13 10:34 ` sashiko-bot
2026-11-13 11:13 ` BillXiang
@ 2026-11-13 11:13 ` BillXiang
0 siblings, 0 replies; 7+ messages in thread
From: BillXiang @ 2026-11-13 11:13 UTC (permalink / raw)
To: Anup Patel
Cc: xb, atish.patra, pjw, palmer, aou, alex, pbonzini, graf,
kvm-riscv, kvm, linux-riscv, linux-kernel
On 11/13/2026 6:34 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [High] RISC-V: KVM: stale topei passed to
> __imsic_swfile_extirq_update()
> - [High] RISC-V: KVM: imsic->swfile dereferenced before !imsic check in
> inject
> - [Low] RISC-V: KVM: missing Fixes trailer on IMSIC TOPEI serialization
> fix
> --
>
> commit 322bd38f6016b31c1cdf5b875effdce4436f4b2f
> Author: BillXiang <xiangwencheng@lanxincomputing.com>
>
>> RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
>>
>> The TOPEI read-and-clear must be atomic with respect to
>> interrupt injection (set_bit in EIP). Otherwise an
>> interrupt injected between the topei read and the
>> clear_bit could be silently dropped.
>>
>> Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com>
>
> [Severity: Low]
> Since this fixes a race in the IMSIC software file TOPEI handling, should
> it include:
>
> Fixes: db8b7e97d613 ("RISC-V: KVM: Add in-kernel virtualization of AIA IMSIC")
>
>> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
>> index 302938972aa0e..f4cc1691e0478 100644
>> --- a/arch/riscv/kvm/aia_imsic.c
>> +++ b/arch/riscv/kvm/aia_imsic.c
>> @@ -526,26 +526,25 @@ static void imsic_vsfile_cleanup(struct imsic *imsic)
>> kvm_riscv_aia_free_hgei(old_vsfile_cpu, old_vsfile_hgei);
>> }
>>
>> -static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
>> +static void __imsic_swfile_extirq_update(struct kvm_vcpu *vcpu, u32 topei)
>> {
>> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> struct imsic_mrif *mrif = imsic->swfile;
>> - unsigned long flags;
>> -
>> - /*
>> - * The critical section is necessary during external interrupt
>> - * updates to avoid the risk of losing interrupts due to potential
>> - * interruptions between reading topei and updating pending status.
>> - */
>> -
>> - raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>>
>> - if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) &&
>> - imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis))
>> + if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) && topei)
>> kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT);
>> else
>> kvm_riscv_vcpu_unset_interrupt(vcpu, IRQ_VS_EXT);
>> +}
>> +
>> +static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
>> +{
>> + struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> + struct imsic_mrif *mrif = imsic->swfile;
>> + unsigned long flags;
>>
>> + raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>> + __imsic_swfile_extirq_update(vcpu, imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis));
>> raw_spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags);
>> }
>>
>> @@ -830,12 +829,15 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
>> struct imsic_mrif_eix *eix;
>> int r, rc = KVM_INSN_CONTINUE_NEXT_SEPC;
>> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> + unsigned long flags;
>>
>> /* If IMSIC vCPU state not initialized then forward to user space */
>> if (!imsic)
>> return KVM_INSN_EXIT_TO_USER_SPACE;
>>
>> if (isel == KVM_RISCV_AIA_IMSIC_TOPEI) {
>> + raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>> +
>> /* Read pending and enabled interrupt with highest priority */
>> topei = imsic_mrif_topei(imsic->swfile, imsic->nr_eix,
>> imsic->nr_msis);
>> @@ -852,16 +854,20 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
>> eix->eip);
>> }
>> }
>> + if (wr_mask)
>> + __imsic_swfile_extirq_update(vcpu, topei);
>
> [Severity: High]
> Does this pass the pre-clear interrupt ID in topei to
> __imsic_swfile_extirq_update() instead of re-reading imsic_mrif_topei()
> after clear_bit()?
>
> When a guest claims the last pending interrupt in imsic->swfile via a
> TOPEI write, topei still holds the non-zero ID that was just cleared from
> eix->eip. __imsic_swfile_extirq_update() then calls
> kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT) instead of unsetting
> IRQ_VS_EXT, leaving IRQ_VS_EXT asserted and causing spurious guest
> external interrupt traps.
>
> [ ... ]
>
>> @@ -959,6 +965,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
>> unsigned long flags;
>> struct imsic_mrif_eix *eix;
>> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> + struct imsic_mrif *mrif = imsic->swfile;
>>
>> /* We only emulate one IMSIC MMIO page for each Guest VCPU */
>> if (!imsic || !iid || guest_index ||
>
> [Severity: High]
> Does imsic->swfile dereference a NULL imsic pointer before the !imsic
> check below? If a vCPU is created after aia_init(), its imsic_state is
> NULL and injecting an MSI to that vCPU will crash the host kernel instead
> of returning -ENODEV.
>
Ignore this version.
Regards,
Bill
--
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v4] RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
@ 2026-11-13 11:13 ` BillXiang
0 siblings, 0 replies; 7+ messages in thread
From: BillXiang @ 2026-11-13 11:13 UTC (permalink / raw)
To: Anup Patel
Cc: xb, atish.patra, pjw, palmer, aou, alex, pbonzini, graf,
kvm-riscv, kvm, linux-riscv, linux-kernel
On 11/13/2026 6:34 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [High] RISC-V: KVM: stale topei passed to
> __imsic_swfile_extirq_update()
> - [High] RISC-V: KVM: imsic->swfile dereferenced before !imsic check in
> inject
> - [Low] RISC-V: KVM: missing Fixes trailer on IMSIC TOPEI serialization
> fix
> --
>
> commit 322bd38f6016b31c1cdf5b875effdce4436f4b2f
> Author: BillXiang <xiangwencheng@lanxincomputing.com>
>
>> RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
>>
>> The TOPEI read-and-clear must be atomic with respect to
>> interrupt injection (set_bit in EIP). Otherwise an
>> interrupt injected between the topei read and the
>> clear_bit could be silently dropped.
>>
>> Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com>
>
> [Severity: Low]
> Since this fixes a race in the IMSIC software file TOPEI handling, should
> it include:
>
> Fixes: db8b7e97d613 ("RISC-V: KVM: Add in-kernel virtualization of AIA IMSIC")
>
>> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
>> index 302938972aa0e..f4cc1691e0478 100644
>> --- a/arch/riscv/kvm/aia_imsic.c
>> +++ b/arch/riscv/kvm/aia_imsic.c
>> @@ -526,26 +526,25 @@ static void imsic_vsfile_cleanup(struct imsic *imsic)
>> kvm_riscv_aia_free_hgei(old_vsfile_cpu, old_vsfile_hgei);
>> }
>>
>> -static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
>> +static void __imsic_swfile_extirq_update(struct kvm_vcpu *vcpu, u32 topei)
>> {
>> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> struct imsic_mrif *mrif = imsic->swfile;
>> - unsigned long flags;
>> -
>> - /*
>> - * The critical section is necessary during external interrupt
>> - * updates to avoid the risk of losing interrupts due to potential
>> - * interruptions between reading topei and updating pending status.
>> - */
>> -
>> - raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>>
>> - if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) &&
>> - imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis))
>> + if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) && topei)
>> kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT);
>> else
>> kvm_riscv_vcpu_unset_interrupt(vcpu, IRQ_VS_EXT);
>> +}
>> +
>> +static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
>> +{
>> + struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> + struct imsic_mrif *mrif = imsic->swfile;
>> + unsigned long flags;
>>
>> + raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>> + __imsic_swfile_extirq_update(vcpu, imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis));
>> raw_spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags);
>> }
>>
>> @@ -830,12 +829,15 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
>> struct imsic_mrif_eix *eix;
>> int r, rc = KVM_INSN_CONTINUE_NEXT_SEPC;
>> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> + unsigned long flags;
>>
>> /* If IMSIC vCPU state not initialized then forward to user space */
>> if (!imsic)
>> return KVM_INSN_EXIT_TO_USER_SPACE;
>>
>> if (isel == KVM_RISCV_AIA_IMSIC_TOPEI) {
>> + raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>> +
>> /* Read pending and enabled interrupt with highest priority */
>> topei = imsic_mrif_topei(imsic->swfile, imsic->nr_eix,
>> imsic->nr_msis);
>> @@ -852,16 +854,20 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
>> eix->eip);
>> }
>> }
>> + if (wr_mask)
>> + __imsic_swfile_extirq_update(vcpu, topei);
>
> [Severity: High]
> Does this pass the pre-clear interrupt ID in topei to
> __imsic_swfile_extirq_update() instead of re-reading imsic_mrif_topei()
> after clear_bit()?
>
> When a guest claims the last pending interrupt in imsic->swfile via a
> TOPEI write, topei still holds the non-zero ID that was just cleared from
> eix->eip. __imsic_swfile_extirq_update() then calls
> kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT) instead of unsetting
> IRQ_VS_EXT, leaving IRQ_VS_EXT asserted and causing spurious guest
> external interrupt traps.
>
> [ ... ]
>
>> @@ -959,6 +965,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
>> unsigned long flags;
>> struct imsic_mrif_eix *eix;
>> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> + struct imsic_mrif *mrif = imsic->swfile;
>>
>> /* We only emulate one IMSIC MMIO page for each Guest VCPU */
>> if (!imsic || !iid || guest_index ||
>
> [Severity: High]
> Does imsic->swfile dereference a NULL imsic pointer before the !imsic
> check below? If a vCPU is created after aia_init(), its imsic_state is
> NULL and injecting an MSI to that vCPU will crash the host kernel instead
> of returning -ENODEV.
>
Ignore this version.
Regards,
Bill
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v4] RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
@ 2026-11-13 11:13 ` BillXiang
0 siblings, 0 replies; 7+ messages in thread
From: BillXiang @ 2026-11-13 11:13 UTC (permalink / raw)
To: Anup Patel
Cc: xb, atish.patra, pjw, palmer, aou, alex, pbonzini, graf,
kvm-riscv, kvm, linux-riscv, linux-kernel
On 11/13/2026 6:34 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [High] RISC-V: KVM: stale topei passed to
> __imsic_swfile_extirq_update()
> - [High] RISC-V: KVM: imsic->swfile dereferenced before !imsic check in
> inject
> - [Low] RISC-V: KVM: missing Fixes trailer on IMSIC TOPEI serialization
> fix
> --
>
> commit 322bd38f6016b31c1cdf5b875effdce4436f4b2f
> Author: BillXiang <xiangwencheng@lanxincomputing.com>
>
>> RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
>>
>> The TOPEI read-and-clear must be atomic with respect to
>> interrupt injection (set_bit in EIP). Otherwise an
>> interrupt injected between the topei read and the
>> clear_bit could be silently dropped.
>>
>> Signed-off-by: BillXiang <xiangwencheng@lanxincomputing.com>
>
> [Severity: Low]
> Since this fixes a race in the IMSIC software file TOPEI handling, should
> it include:
>
> Fixes: db8b7e97d613 ("RISC-V: KVM: Add in-kernel virtualization of AIA IMSIC")
>
>> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
>> index 302938972aa0e..f4cc1691e0478 100644
>> --- a/arch/riscv/kvm/aia_imsic.c
>> +++ b/arch/riscv/kvm/aia_imsic.c
>> @@ -526,26 +526,25 @@ static void imsic_vsfile_cleanup(struct imsic *imsic)
>> kvm_riscv_aia_free_hgei(old_vsfile_cpu, old_vsfile_hgei);
>> }
>>
>> -static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
>> +static void __imsic_swfile_extirq_update(struct kvm_vcpu *vcpu, u32 topei)
>> {
>> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> struct imsic_mrif *mrif = imsic->swfile;
>> - unsigned long flags;
>> -
>> - /*
>> - * The critical section is necessary during external interrupt
>> - * updates to avoid the risk of losing interrupts due to potential
>> - * interruptions between reading topei and updating pending status.
>> - */
>> -
>> - raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>>
>> - if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) &&
>> - imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis))
>> + if (imsic_mrif_atomic_read(mrif, &mrif->eidelivery) && topei)
>> kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT);
>> else
>> kvm_riscv_vcpu_unset_interrupt(vcpu, IRQ_VS_EXT);
>> +}
>> +
>> +static void imsic_swfile_extirq_update(struct kvm_vcpu *vcpu)
>> +{
>> + struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> + struct imsic_mrif *mrif = imsic->swfile;
>> + unsigned long flags;
>>
>> + raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>> + __imsic_swfile_extirq_update(vcpu, imsic_mrif_topei(mrif, imsic->nr_eix, imsic->nr_msis));
>> raw_spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags);
>> }
>>
>> @@ -830,12 +829,15 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
>> struct imsic_mrif_eix *eix;
>> int r, rc = KVM_INSN_CONTINUE_NEXT_SEPC;
>> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> + unsigned long flags;
>>
>> /* If IMSIC vCPU state not initialized then forward to user space */
>> if (!imsic)
>> return KVM_INSN_EXIT_TO_USER_SPACE;
>>
>> if (isel == KVM_RISCV_AIA_IMSIC_TOPEI) {
>> + raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>> +
>> /* Read pending and enabled interrupt with highest priority */
>> topei = imsic_mrif_topei(imsic->swfile, imsic->nr_eix,
>> imsic->nr_msis);
>> @@ -852,16 +854,20 @@ int kvm_riscv_vcpu_aia_imsic_rmw(struct kvm_vcpu *vcpu, unsigned long isel,
>> eix->eip);
>> }
>> }
>> + if (wr_mask)
>> + __imsic_swfile_extirq_update(vcpu, topei);
>
> [Severity: High]
> Does this pass the pre-clear interrupt ID in topei to
> __imsic_swfile_extirq_update() instead of re-reading imsic_mrif_topei()
> after clear_bit()?
>
> When a guest claims the last pending interrupt in imsic->swfile via a
> TOPEI write, topei still holds the non-zero ID that was just cleared from
> eix->eip. __imsic_swfile_extirq_update() then calls
> kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_VS_EXT) instead of unsetting
> IRQ_VS_EXT, leaving IRQ_VS_EXT asserted and causing spurious guest
> external interrupt traps.
>
> [ ... ]
>
>> @@ -959,6 +965,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
>> unsigned long flags;
>> struct imsic_mrif_eix *eix;
>> struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
>> + struct imsic_mrif *mrif = imsic->swfile;
>>
>> /* We only emulate one IMSIC MMIO page for each Guest VCPU */
>> if (!imsic || !iid || guest_index ||
>
> [Severity: High]
> Does imsic->swfile dereference a NULL imsic pointer before the !imsic
> check below? If a vCPU is created after aia_init(), its imsic_state is
> NULL and injecting an MSI to that vCPU will crash the host kernel instead
> of returning -ENODEV.
>
Ignore this version.
Regards,
Bill
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread