| Message ID | 20191016160649.24622-21-anup.patel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM RISC-V Support | expand |
On 16/10/19 18:12, Anup Patel wrote: > + /* Read current VSIP and VSIE CSRs */ > + vsip = csr_read(CSR_VSIP); > + csr->vsie = csr_read(CSR_VSIE); > + > + /* Sync-up VSIP.SSIP bit changes does by Guest */ > + if ((csr->vsip ^ vsip) & (1UL << IRQ_S_SOFT)) { > + if (!test_and_set_bit(IRQ_S_SOFT, &v->irqs_pending_mask)) { > + if (vsip & (1UL << IRQ_S_SOFT)) > + set_bit(IRQ_S_SOFT, &v->irqs_pending); > + else > + clear_bit(IRQ_S_SOFT, &v->irqs_pending); > + } Looks good, but I wonder if this could just be "csr->vsip = csr_read(CSR_VSIP)", which will be fixed up by flush_interrupts on the next entry. Paolo
On Mon, Oct 21, 2019 at 10:57 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 16/10/19 18:12, Anup Patel wrote: > > + /* Read current VSIP and VSIE CSRs */ > > + vsip = csr_read(CSR_VSIP); > > + csr->vsie = csr_read(CSR_VSIE); > > + > > + /* Sync-up VSIP.SSIP bit changes does by Guest */ > > + if ((csr->vsip ^ vsip) & (1UL << IRQ_S_SOFT)) { > > + if (!test_and_set_bit(IRQ_S_SOFT, &v->irqs_pending_mask)) { > > + if (vsip & (1UL << IRQ_S_SOFT)) > > + set_bit(IRQ_S_SOFT, &v->irqs_pending); > > + else > > + clear_bit(IRQ_S_SOFT, &v->irqs_pending); > > + } > > Looks good, but I wonder if this could just be "csr->vsip = > csr_read(CSR_VSIP)", which will be fixed up by flush_interrupts on the > next entry. It's not just "csr->vsip = csr_read(CSR_VSIP" because "irqs_pending" bitmap has to be in-sync with Guest updates to VSIP because WFI trap-n-emulate will check for pending interrupts which in-turn checks "irqs_pending" bitmap. Regards, Anup
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c index f1a218d3a8cf..844542dd13e4 100644 --- a/arch/riscv/kvm/vcpu.c +++ b/arch/riscv/kvm/vcpu.c @@ -662,15 +662,22 @@ void kvm_riscv_vcpu_flush_interrupts(struct kvm_vcpu *vcpu) void kvm_riscv_vcpu_sync_interrupts(struct kvm_vcpu *vcpu) { - vcpu->arch.guest_csr.vsip = csr_read(CSR_VSIP); - vcpu->arch.guest_csr.vsie = csr_read(CSR_VSIE); + unsigned long vsip; + struct kvm_vcpu_arch *v = &vcpu->arch; + struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr; - /* Guest can directly update VSIP software interrupt bits */ - if (vcpu->arch.guest_csr.vsip ^ READ_ONCE(vcpu->arch.irqs_pending)) { - if (vcpu->arch.guest_csr.vsip & (1UL << IRQ_S_SOFT)) - kvm_riscv_vcpu_set_interrupt(vcpu, IRQ_S_SOFT); - else - kvm_riscv_vcpu_unset_interrupt(vcpu, IRQ_S_SOFT); + /* Read current VSIP and VSIE CSRs */ + vsip = csr_read(CSR_VSIP); + csr->vsie = csr_read(CSR_VSIE); + + /* Sync-up VSIP.SSIP bit changes does by Guest */ + if ((csr->vsip ^ vsip) & (1UL << IRQ_S_SOFT)) { + if (!test_and_set_bit(IRQ_S_SOFT, &v->irqs_pending_mask)) { + if (vsip & (1UL << IRQ_S_SOFT)) + set_bit(IRQ_S_SOFT, &v->irqs_pending); + else + clear_bit(IRQ_S_SOFT, &v->irqs_pending); + } } }
Currently, we sync-up Guest VSIP and VSIE CSRs with HW state upon VM-exit. This helps us track enable/disable state of interrupts and VSIP.SSIP bit updates by Guest. Unfortunately, the implementation of kvm_riscv_vcpu_sync_interrupts() is racey when running SMP Guest on SMP Host because it can happen that IRQ_S_SOFT is already queued from other Host CPU and we might accidentally clear a valid pending IRQ_S_SOFT. To fix this, we use test_and_set_bit() to update irqs_pending_mask in kvm_riscv_vcpu_sync_interrupts() instead of directly calling kvm_riscv_vcpu_set/unset_interrupt() functions. Signed-off-by: Anup Patel <anup.patel@wdc.com> --- arch/riscv/kvm/vcpu.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)