| Message ID | 20231213061610.11100-1-yongxuan.wang@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] RISCV: KVM: update external interrupt atomically for IMSIC swfile | expand |
On Wed, Dec 13, 2023 at 11:46 AM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote: > > The emulated IMSIC update the external interrupt pending depending on the > value of eidelivery and topei. It might lose an interrupt when it is > interrupted before setting the new value to the pending status. > > For example, when VCPU0 sends an IPI to VCPU1 via IMSIC: > > VCPU0 VCPU1 > > CSRSWAP topei = 0 > The VCPU1 has claimed all the external > interrupt in its interrupt handler. > > topei of VCPU1's IMSIC = 0 > > set pending in VCPU1's IMSIC > > topei of VCPU1' IMSIC = 1 > > set the external interrupt > pending of VCPU1 > > clear the external interrupt pending > of VCPU1 > > When the VCPU1 switches back to VS mode, it exits the interrupt handler > because the result of CSRSWAP topei is 0. If there are no other external > interrupts injected into the VCPU1's IMSIC, VCPU1 will never know this > pending interrupt unless it initiative read the topei. > > If the interruption occurs between updating interrupt pending in IMSIC > and updating external interrupt pending of VCPU, it will not cause a > problem. Suppose that the VCPU1 clears the IPI pending in IMSIC right > after VCPU0 sets the pending, the external interrupt pending of VCPU1 > will not be set because the topei is 0. But when the VCPU1 goes back to > VS mode, the pending IPI will be reported by the CSRSWAP topei, it will > not lose this interrupt. > > So we only need to make the external interrupt updating procedure as a > critical section to avoid the problem. > > Fixes: db8b7e97d613 ("RISC-V: KVM: Add in-kernel virtualization of AIA IMSIC") > Tested-by: Roy Lin <roy.lin@sifive.com> > Tested-by: Wayling Chen <wayling.chen@sifive.com> > Co-developed-by: Vincent Chen <vincent.chen@sifive.com> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com> Queued as fixes for Linux-6.7 Thanks, Anup > --- > Changelog > v2: > - rename the variable and add a short comment in the code > --- > arch/riscv/kvm/aia_imsic.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c > index 6cf23b8adb71..e808723a85f1 100644 > --- a/arch/riscv/kvm/aia_imsic.c > +++ b/arch/riscv/kvm/aia_imsic.c > @@ -55,6 +55,7 @@ struct imsic { > /* IMSIC SW-file */ > struct imsic_mrif *swfile; > phys_addr_t swfile_pa; > + spinlock_t swfile_extirq_lock; > }; > > #define imsic_vs_csr_read(__c) \ > @@ -613,12 +614,23 @@ 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. > + */ > + > + 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); > + > + spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags); > } > > static void imsic_swfile_read(struct kvm_vcpu *vcpu, bool clear, > @@ -1039,6 +1051,7 @@ int kvm_riscv_vcpu_aia_imsic_init(struct kvm_vcpu *vcpu) > } > imsic->swfile = page_to_virt(swfile_page); > imsic->swfile_pa = page_to_phys(swfile_page); > + spin_lock_init(&imsic->swfile_extirq_lock); > > /* Setup IO device */ > kvm_iodevice_init(&imsic->iodev, &imsic_iodoev_ops); > -- > 2.17.1 >
diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c index 6cf23b8adb71..e808723a85f1 100644 --- a/arch/riscv/kvm/aia_imsic.c +++ b/arch/riscv/kvm/aia_imsic.c @@ -55,6 +55,7 @@ struct imsic { /* IMSIC SW-file */ struct imsic_mrif *swfile; phys_addr_t swfile_pa; + spinlock_t swfile_extirq_lock; }; #define imsic_vs_csr_read(__c) \ @@ -613,12 +614,23 @@ 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. + */ + + 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); + + spin_unlock_irqrestore(&imsic->swfile_extirq_lock, flags); } static void imsic_swfile_read(struct kvm_vcpu *vcpu, bool clear, @@ -1039,6 +1051,7 @@ int kvm_riscv_vcpu_aia_imsic_init(struct kvm_vcpu *vcpu) } imsic->swfile = page_to_virt(swfile_page); imsic->swfile_pa = page_to_phys(swfile_page); + spin_lock_init(&imsic->swfile_extirq_lock); /* Setup IO device */ kvm_iodevice_init(&imsic->iodev, &imsic_iodoev_ops);
The emulated IMSIC update the external interrupt pending depending on the value of eidelivery and topei. It might lose an interrupt when it is interrupted before setting the new value to the pending status. For example, when VCPU0 sends an IPI to VCPU1 via IMSIC: VCPU0 VCPU1 CSRSWAP topei = 0 The VCPU1 has claimed all the external interrupt in its interrupt handler. topei of VCPU1's IMSIC = 0 set pending in VCPU1's IMSIC topei of VCPU1' IMSIC = 1 set the external interrupt pending of VCPU1 clear the external interrupt pending of VCPU1 When the VCPU1 switches back to VS mode, it exits the interrupt handler because the result of CSRSWAP topei is 0. If there are no other external interrupts injected into the VCPU1's IMSIC, VCPU1 will never know this pending interrupt unless it initiative read the topei. If the interruption occurs between updating interrupt pending in IMSIC and updating external interrupt pending of VCPU, it will not cause a problem. Suppose that the VCPU1 clears the IPI pending in IMSIC right after VCPU0 sets the pending, the external interrupt pending of VCPU1 will not be set because the topei is 0. But when the VCPU1 goes back to VS mode, the pending IPI will be reported by the CSRSWAP topei, it will not lose this interrupt. So we only need to make the external interrupt updating procedure as a critical section to avoid the problem. Fixes: db8b7e97d613 ("RISC-V: KVM: Add in-kernel virtualization of AIA IMSIC") Tested-by: Roy Lin <roy.lin@sifive.com> Tested-by: Wayling Chen <wayling.chen@sifive.com> Co-developed-by: Vincent Chen <vincent.chen@sifive.com> Signed-off-by: Vincent Chen <vincent.chen@sifive.com> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com> --- Changelog v2: - rename the variable and add a short comment in the code --- arch/riscv/kvm/aia_imsic.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)