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

[BillXiang]

On 11/13/2026 1:36 PM, Anup Patel wrote:
> On Wed, Sep 23, 2026 at 2:47 PM BillXiang
> <xiangwencheng@xxxxxxxxxxxxxxxxxxx> wrote:
>>
>> 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.
>>
>> v3:
>>   - fix undefined eflags
>>
>> v2:
>>   - fix patch file curruption
>>
>> v1: https://lore.kernel.org/kvm-riscv/20260923070332.1376132-1-xiangwencheng@xxxxxxxxxxxxxxxxxxx/
>>
>> Signed-off-by: BillXiang <xiangwencheng@xxxxxxxxxxxxxxxxxxx>
>> ---
>>   arch/riscv/kvm/aia_imsic.c | 42 +++++++++++++++++++++++++-------------
>>   1 file changed, 28 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
>> index c1af23e79ae0..c440088651ec 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)
>>   {
>>          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))
>>                  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;
>> +       unsigned long flags;
>>
>> +       raw_spin_lock_irqsave(&imsic->swfile_extirq_lock, flags);
>> +       __imsic_swfile_extirq_update(vcpu);
>>          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);
> 
> The __imsic_swfile_extirq_update() also calls imsic_mrif_topei() so it is
> better to pass topei value to __imsic_swfile_extirq_update().

Thanks for the review. I'll incorporate it into v4.

Regards,
Bill

> 
>> +
>> +               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;
>>   }
>> @@ -1050,9 +1056,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);
>> +               raw_spin_unlock(&imsic->swfile_extirq_lock);
>>          }
>>
>>          read_unlock_irqrestore(&imsic->vsfile_lock, flags);
>> --
>> 2.53.0
> 
> Regards,
> Anup