Skip to content

Commit 7731bdf

Browse files
xiebo-hao123gregkh
xiebo-hao123
authored and
gregkh
committed
RISC-V: KVM: Serialize virtual interrupt pending state updates
commit d024a0a upstream. KVM RISC-V tracks guest local interrupt state with two bitmaps: - irqs_pending: interrupts that should be visible to the guest - irqs_pending_mask: interrupts whose pending state changed The current code updates those bitmaps with independent atomic bitops and assumes a multiple-producer, single-consumer protocol. That model does not actually hold. kvm_riscv_vcpu_sync_interrupts() is not a pure consumer. When the guest changes guest-visible HVIP state, sync_interrupts() writes both irqs_pending and irqs_pending_mask to reflect the new guest state back into KVM state. As a result, irqs_pending and irqs_pending_mask form a single logical state transition, but they are not updated atomically as a pair. This allows a race where a newly injected interrupt is lost. For example: CPU0 CPU1 ---- ---- kvm_riscv_vcpu_set_interrupt(VS_SOFT) set_bit(VS_SOFT, irqs_pending) kvm_riscv_vcpu_sync_interrupts() sees guest-cleared HVIP.VSSIP sets irqs_pending_mask clear_bit(IRQ_VS_SOFT, irqs_pending) set_bit(VS_SOFT, irqs_pending_mask) kvm_vcpu_kick() After that interleaving, a later flush can update HVIP without VSSIP even though a new virtual interrupt was injected. In practice, the guest can remain blocked in WFI with work pending. The same pending/mask protocol is shared by VS soft interrupts, PMU overflow delivery, and AIA high interrupt synchronization, so the race is not limited to one interrupt source. Fix this by serializing all updates to irqs_pending and irqs_pending_mask with a per-vCPU raw spinlock. This keeps the pending bit and the dirty mask as one state transition across: - set/unset interrupt - guest HVIP sync - interrupt flush to guest CSR state - vCPU reset - AIA CSR writes that clear dirty state Use non-atomic bitmap operations while holding the lock. Hold the lock across the AIA sync, flush, and pending checks as well, so both bitmap words share the same serialization domain. This intentionally replaces the existing lockless protocol instead of trying to repair it with additional barriers. The problem is not memory ordering on a single field; it is that two separate bitmaps encode one shared state machine while both producers and sync paths can modify them. A per-vCPU raw spinlock keeps the fix small, local, and suitable for backporting. Fixes: cce69af ("RISC-V: KVM: Implement VCPU interrupts and requests handling") Cc: stable@vger.kernel.org Signed-off-by: Xie Bo <xb@ultrarisc.com> Reviewed-by: Anup Patel <anup@brainfault.org> Link: https://lore.kernel.org/r/20260715020359.1521354-2-xb@ultrarisc.com Signed-off-by: Anup Patel <anup@brainfault.org> [ bo: Adapt the reset and CSR helpers to their older signatures. ] Signed-off-by: Sasha Levin <sashal@kernel.org>
1 parent f0cd47a commit 7731bdf

4 files changed

Lines changed: 87 additions & 34 deletions

File tree

‎arch/riscv/include/asm/kvm_host.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -232,13 +232,13 @@ struct kvm_vcpu_arch {
232232
/*
233233
* VCPU interrupts
234234
*
235-
* We have a lockless approach for tracking pending VCPU interrupts
236-
* implemented using atomic bitops. The irqs_pending bitmap represent
237-
* pending interrupts whereas irqs_pending_mask represent bits changed
238-
* in irqs_pending. Our approach is modeled around multiple producer
239-
* and single consumer problem where the consumer is the VCPU itself.
235+
* The irqs_pending bitmap represents pending interrupts whereas
236+
* irqs_pending_mask represents bits changed in irqs_pending. Updates
237+
* to these bitmaps are serialized so vcpu interrupt sync/flush cannot
238+
* drop a newly injected interrupt while syncing guest-visible HVIP.
240239
*/
241240
#define KVM_RISCV_VCPU_NR_IRQS 64
241+
raw_spinlock_t irqs_pending_lock;
242242
DECLARE_BITMAP(irqs_pending, KVM_RISCV_VCPU_NR_IRQS);
243243
DECLARE_BITMAP(irqs_pending_mask, KVM_RISCV_VCPU_NR_IRQS);
244244

‎arch/riscv/kvm/aia.c‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,12 +71,15 @@ void kvm_riscv_vcpu_aia_flush_interrupts(struct kvm_vcpu *vcpu)
7171
struct kvm_vcpu_aia_csr *csr = &vcpu->arch.aia_context.guest_csr;
7272
unsigned long mask, val;
7373

74+
lockdep_assert_held(&vcpu->arch.irqs_pending_lock);
75+
7476
if (!kvm_riscv_aia_available())
7577
return;
7678

77-
if (READ_ONCE(vcpu->arch.irqs_pending_mask[1])) {
78-
mask = xchg_acquire(&vcpu->arch.irqs_pending_mask[1], 0);
79-
val = READ_ONCE(vcpu->arch.irqs_pending[1]) & mask;
79+
mask = vcpu->arch.irqs_pending_mask[1];
80+
if (mask) {
81+
vcpu->arch.irqs_pending_mask[1] = 0;
82+
val = vcpu->arch.irqs_pending[1] & mask;
8083

8184
csr->hviph &= ~mask;
8285
csr->hviph |= val;
@@ -87,6 +90,8 @@ void kvm_riscv_vcpu_aia_sync_interrupts(struct kvm_vcpu *vcpu)
8790
{
8891
struct kvm_vcpu_aia_csr *csr = &vcpu->arch.aia_context.guest_csr;
8992

93+
lockdep_assert_held(&vcpu->arch.irqs_pending_lock);
94+
9095
if (kvm_riscv_aia_available())
9196
csr->vsieh = csr_read(CSR_VSIEH);
9297
}
@@ -96,13 +101,21 @@ bool kvm_riscv_vcpu_aia_has_interrupts(struct kvm_vcpu *vcpu, u64 mask)
96101
{
97102
int hgei;
98103
unsigned long seip;
104+
#ifdef CONFIG_32BIT
105+
unsigned long flags;
106+
bool pending;
107+
#endif
99108

100109
if (!kvm_riscv_aia_available())
101110
return false;
102111

103112
#ifdef CONFIG_32BIT
104-
if (READ_ONCE(vcpu->arch.irqs_pending[1]) &
105-
(vcpu->arch.aia_context.guest_csr.vsieh & upper_32_bits(mask)))
113+
raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
114+
pending = vcpu->arch.irqs_pending[1] &
115+
(vcpu->arch.aia_context.guest_csr.vsieh &
116+
upper_32_bits(mask));
117+
raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
118+
if (pending)
106119
return true;
107120
#endif
108121

@@ -190,6 +203,9 @@ int kvm_riscv_vcpu_aia_set_csr(struct kvm_vcpu *vcpu,
190203
unsigned long val)
191204
{
192205
struct kvm_vcpu_aia_csr *csr = &vcpu->arch.aia_context.guest_csr;
206+
#ifdef CONFIG_32BIT
207+
unsigned long flags;
208+
#endif
193209

194210
if (reg_num >= sizeof(struct kvm_riscv_aia_csr) / sizeof(unsigned long))
195211
return -ENOENT;
@@ -198,8 +214,12 @@ int kvm_riscv_vcpu_aia_set_csr(struct kvm_vcpu *vcpu,
198214
((unsigned long *)csr)[reg_num] = val;
199215

200216
#ifdef CONFIG_32BIT
201-
if (reg_num == KVM_REG_RISCV_CSR_AIA_REG(siph))
202-
WRITE_ONCE(vcpu->arch.irqs_pending_mask[1], 0);
217+
if (reg_num == KVM_REG_RISCV_CSR_AIA_REG(siph)) {
218+
raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
219+
vcpu->arch.irqs_pending_mask[1] = 0;
220+
raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock,
221+
flags);
222+
}
203223
#endif
204224
}
205225

‎arch/riscv/kvm/vcpu.c‎

Lines changed: 49 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
5252
struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
5353
struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
5454
struct kvm_cpu_context *reset_cntx = &vcpu->arch.guest_reset_context;
55+
unsigned long flags;
5556
bool loaded;
5657

5758
/**
@@ -80,8 +81,10 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
8081

8182
kvm_riscv_vcpu_aia_reset(vcpu);
8283

84+
raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
8385
bitmap_zero(vcpu->arch.irqs_pending, KVM_RISCV_VCPU_NR_IRQS);
8486
bitmap_zero(vcpu->arch.irqs_pending_mask, KVM_RISCV_VCPU_NR_IRQS);
87+
raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
8588

8689
kvm_riscv_vcpu_pmu_reset(vcpu);
8790

@@ -126,6 +129,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
126129
/* Setup VCPU hfence queue */
127130
spin_lock_init(&vcpu->arch.hfence_lock);
128131

132+
raw_spin_lock_init(&vcpu->arch.irqs_pending_lock);
133+
129134
/* Setup reset state of shadow SSTATUS and HSTATUS CSRs */
130135
spin_lock_init(&vcpu->arch.reset_cntx_lock);
131136

@@ -341,22 +346,29 @@ void kvm_riscv_vcpu_flush_interrupts(struct kvm_vcpu *vcpu)
341346
{
342347
struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
343348
unsigned long mask, val;
349+
unsigned long flags;
350+
351+
raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
344352

345-
if (READ_ONCE(vcpu->arch.irqs_pending_mask[0])) {
346-
mask = xchg_acquire(&vcpu->arch.irqs_pending_mask[0], 0);
347-
val = READ_ONCE(vcpu->arch.irqs_pending[0]) & mask;
353+
mask = vcpu->arch.irqs_pending_mask[0];
354+
if (mask) {
355+
vcpu->arch.irqs_pending_mask[0] = 0;
356+
val = vcpu->arch.irqs_pending[0] & mask;
348357

349358
csr->hvip &= ~mask;
350359
csr->hvip |= val;
351360
}
352361

353362
/* Flush AIA high interrupts */
354363
kvm_riscv_vcpu_aia_flush_interrupts(vcpu);
364+
365+
raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
355366
}
356367

357368
void kvm_riscv_vcpu_sync_interrupts(struct kvm_vcpu *vcpu)
358369
{
359370
unsigned long hvip;
371+
unsigned long flags;
360372
struct kvm_vcpu_arch *v = &vcpu->arch;
361373
struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
362374

@@ -365,34 +377,41 @@ void kvm_riscv_vcpu_sync_interrupts(struct kvm_vcpu *vcpu)
365377

366378
/* Sync-up HVIP.VSSIP bit changes does by Guest */
367379
hvip = csr_read(CSR_HVIP);
380+
381+
raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
382+
368383
if ((csr->hvip ^ hvip) & (1UL << IRQ_VS_SOFT)) {
369384
if (hvip & (1UL << IRQ_VS_SOFT)) {
370-
if (!test_and_set_bit(IRQ_VS_SOFT,
371-
v->irqs_pending_mask))
372-
set_bit(IRQ_VS_SOFT, v->irqs_pending);
385+
if (!__test_and_set_bit(IRQ_VS_SOFT,
386+
v->irqs_pending_mask))
387+
__set_bit(IRQ_VS_SOFT, v->irqs_pending);
373388
} else {
374-
if (!test_and_set_bit(IRQ_VS_SOFT,
375-
v->irqs_pending_mask))
376-
clear_bit(IRQ_VS_SOFT, v->irqs_pending);
389+
if (!__test_and_set_bit(IRQ_VS_SOFT,
390+
v->irqs_pending_mask))
391+
__clear_bit(IRQ_VS_SOFT, v->irqs_pending);
377392
}
378393
}
379394

380395
/* Sync up the HVIP.LCOFIP bit changes (only clear) by the guest */
381396
if ((csr->hvip ^ hvip) & (1UL << IRQ_PMU_OVF)) {
382397
if (!(hvip & (1UL << IRQ_PMU_OVF)) &&
383-
!test_and_set_bit(IRQ_PMU_OVF, v->irqs_pending_mask))
384-
clear_bit(IRQ_PMU_OVF, v->irqs_pending);
398+
!__test_and_set_bit(IRQ_PMU_OVF, v->irqs_pending_mask))
399+
__clear_bit(IRQ_PMU_OVF, v->irqs_pending);
385400
}
386401

387402
/* Sync-up AIA high interrupts */
388403
kvm_riscv_vcpu_aia_sync_interrupts(vcpu);
389404

405+
raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
406+
390407
/* Sync-up timer CSRs */
391408
kvm_riscv_vcpu_timer_sync(vcpu);
392409
}
393410

394411
int kvm_riscv_vcpu_set_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
395412
{
413+
unsigned long flags;
414+
396415
/*
397416
* We only allow VS-mode software, timer, and external
398417
* interrupts when irq is one of the local interrupts
@@ -405,9 +424,10 @@ int kvm_riscv_vcpu_set_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
405424
irq != IRQ_PMU_OVF)
406425
return -EINVAL;
407426

408-
set_bit(irq, vcpu->arch.irqs_pending);
409-
smp_mb__before_atomic();
410-
set_bit(irq, vcpu->arch.irqs_pending_mask);
427+
raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
428+
__set_bit(irq, vcpu->arch.irqs_pending);
429+
__set_bit(irq, vcpu->arch.irqs_pending_mask);
430+
raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
411431

412432
kvm_vcpu_kick(vcpu);
413433

@@ -416,6 +436,8 @@ int kvm_riscv_vcpu_set_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
416436

417437
int kvm_riscv_vcpu_unset_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
418438
{
439+
unsigned long flags;
440+
419441
/*
420442
* We only allow VS-mode software, timer, counter overflow and external
421443
* interrupts when irq is one of the local interrupts
@@ -428,26 +450,33 @@ int kvm_riscv_vcpu_unset_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
428450
irq != IRQ_PMU_OVF)
429451
return -EINVAL;
430452

431-
clear_bit(irq, vcpu->arch.irqs_pending);
432-
smp_mb__before_atomic();
433-
set_bit(irq, vcpu->arch.irqs_pending_mask);
453+
raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
454+
__clear_bit(irq, vcpu->arch.irqs_pending);
455+
__set_bit(irq, vcpu->arch.irqs_pending_mask);
456+
raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
434457

435458
return 0;
436459
}
437460

438461
bool kvm_riscv_vcpu_has_interrupts(struct kvm_vcpu *vcpu, u64 mask)
439462
{
463+
unsigned long flags;
440464
unsigned long ie;
465+
bool ret;
441466

467+
raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
442468
ie = ((vcpu->arch.guest_csr.vsie & VSIP_VALID_MASK)
443469
<< VSIP_TO_HVIP_SHIFT) & (unsigned long)mask;
444470
ie |= vcpu->arch.guest_csr.vsie & ~IRQ_LOCAL_MASK &
445471
(unsigned long)mask;
446-
if (READ_ONCE(vcpu->arch.irqs_pending[0]) & ie)
447-
return true;
472+
ret = vcpu->arch.irqs_pending[0] & ie;
473+
raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
448474

449475
/* Check AIA high interrupts */
450-
return kvm_riscv_vcpu_aia_has_interrupts(vcpu, mask);
476+
if (!ret)
477+
ret = kvm_riscv_vcpu_aia_has_interrupts(vcpu, mask);
478+
479+
return ret;
451480
}
452481

453482
void __kvm_riscv_vcpu_power_off(struct kvm_vcpu *vcpu)

‎arch/riscv/kvm/vcpu_onereg.c‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,7 @@ static int kvm_riscv_vcpu_general_set_csr(struct kvm_vcpu *vcpu,
445445
unsigned long reg_val)
446446
{
447447
struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
448+
unsigned long flags;
448449

449450
if (reg_num >= sizeof(struct kvm_riscv_csr) / sizeof(unsigned long))
450451
return -ENOENT;
@@ -456,8 +457,11 @@ static int kvm_riscv_vcpu_general_set_csr(struct kvm_vcpu *vcpu,
456457

457458
((unsigned long *)csr)[reg_num] = reg_val;
458459

459-
if (reg_num == KVM_REG_RISCV_CSR_REG(sip))
460-
WRITE_ONCE(vcpu->arch.irqs_pending_mask[0], 0);
460+
if (reg_num == KVM_REG_RISCV_CSR_REG(sip)) {
461+
raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
462+
vcpu->arch.irqs_pending_mask[0] = 0;
463+
raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
464+
}
461465

462466
return 0;
463467
}

0 commit comments

Comments
 (0)