Re: [PATCH v4] RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation

From: "BillXiang" <xiangwencheng@lanxincomputing.com>
To: "Anup Patel" <anup@brainfault.org>
Cc: <xb@ultrarisc.com>, <atish.patra@linux.dev>, <pjw@kernel.org>,
	 <palmer@dabbelt.com>, <aou@eecs.berkeley.edu>, <alex@ghiti.fr>,
	 <pbonzini@redhat.com>, <graf@amazon.com>,
	 <kvm-riscv@lists.infradead.org>, <kvm@vger.kernel.org>,
	 <linux-riscv@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4] RISC-V: KVM: Serialize and atomize the IMSIC's TOPEI read-and-clear operation
Date: Fri, 13 Nov 2026 19:13:29 +0800	[thread overview]
Message-ID: <44886809-744d-4e4d-9a55-45e2fcc6dd08@lanxincomputing.com> (raw)
In-Reply-To: <sashiko-outbox-212624@kernel.org>

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