On Tue, Dec 3, 2024 at 4:51 PM bibo mao <maobibo@xxxxxxxxxxx> wrote: > > > > On 2024/12/3 下午2:50, Huacai Chen wrote: > > When we enable lockdep we get such a warning: > > > > ============================= > > WARNING: suspicious RCU usage > > 6.12.0-rc7+ #1891 Tainted: G W > > ----------------------------- > > arch/loongarch/kvm/../../../virt/kvm/kvm_main.c:5945 suspicious rcu_dereference_check() usage! > > other info that might help us debug this: > > rcu_scheduler_active = 2, debug_locks = 1 > > 1 lock held by qemu-system-loo/948: > > #0: 90000001184a00a8 (&vcpu->mutex){+.+.}-{4:4}, at: kvm_vcpu_ioctl+0xf4/0xe20 [kvm] > > stack backtrace: > > CPU: 2 UID: 0 PID: 948 Comm: qemu-system-loo Tainted: G W 6.12.0-rc7+ #1891 > > Tainted: [W]=WARN > > Hardware name: Loongson Loongson-3A5000-7A1000-1w-CRB/Loongson-LS3A5000-7A1000-1w-CRB, BIOS vUDK2018-LoongArch-V2.0.0-prebeta9 10/21/2022 > > Stack : 0000000000000089 9000000005a0db9c 90000000071519c8 900000012c578000 > > 900000012c57b940 0000000000000000 900000012c57b948 9000000007e53788 > > 900000000815bcc8 900000000815bcc0 900000012c57b7b0 0000000000000001 > > 0000000000000001 4b031894b9d6b725 0000000005dec000 9000000100427b00 > > 00000000000003d2 0000000000000001 000000000000002d 0000000000000003 > > 0000000000000030 00000000000003b4 0000000005dec000 0000000000000000 > > 900000000806d000 9000000007e53788 00000000000000b4 0000000000000004 > > 0000000000000004 0000000000000000 0000000000000000 9000000107baf600 > > 9000000008916000 9000000007e53788 9000000005924778 000000001fe001e5 > > 00000000000000b0 0000000000000007 0000000000000000 0000000000071c1d > > ... > > Call Trace: > > [<9000000005924778>] show_stack+0x38/0x180 > > [<90000000071519c4>] dump_stack_lvl+0x94/0xe4 > > [<90000000059eb754>] lockdep_rcu_suspicious+0x194/0x240 > > [<ffff80000221f47c>] kvm_io_bus_read+0x19c/0x1e0 [kvm] > > [<ffff800002225118>] kvm_emu_mmio_read+0xd8/0x440 [kvm] > > [<ffff8000022254bc>] kvm_handle_read_fault+0x3c/0xe0 [kvm] > > [<ffff80000222b3c8>] kvm_handle_exit+0x228/0x480 [kvm] > > > > Fix it by protecting kvm_io_bus_{read,write}() with SRCU. > > > > Cc: stable@xxxxxxxxxxxxxxx > > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx> > > --- > > arch/loongarch/kvm/exit.c | 31 +++++++++++++++++++++---------- > > arch/loongarch/kvm/intc/ipi.c | 6 +++++- > > 2 files changed, 26 insertions(+), 11 deletions(-) > > > > diff --git a/arch/loongarch/kvm/exit.c b/arch/loongarch/kvm/exit.c > > index 69f3e3782cc9..a7893bd01e73 100644 > > --- a/arch/loongarch/kvm/exit.c > > +++ b/arch/loongarch/kvm/exit.c > > @@ -156,7 +156,7 @@ static int kvm_handle_csr(struct kvm_vcpu *vcpu, larch_inst inst) > > > > int kvm_emu_iocsr(larch_inst inst, struct kvm_run *run, struct kvm_vcpu *vcpu) > > { > > - int ret; > > + int idx, ret; > > unsigned long *val; > > u32 addr, rd, rj, opcode; > > > > @@ -167,7 +167,6 @@ int kvm_emu_iocsr(larch_inst inst, struct kvm_run *run, struct kvm_vcpu *vcpu) > > rj = inst.reg2_format.rj; > > opcode = inst.reg2_format.opcode; > > addr = vcpu->arch.gprs[rj]; > > - ret = EMULATE_DO_IOCSR; > > run->iocsr_io.phys_addr = addr; > > run->iocsr_io.is_write = 0; > > val = &vcpu->arch.gprs[rd]; > > @@ -207,20 +206,28 @@ int kvm_emu_iocsr(larch_inst inst, struct kvm_run *run, struct kvm_vcpu *vcpu) > > } > > > > if (run->iocsr_io.is_write) { > > - if (!kvm_io_bus_write(vcpu, KVM_IOCSR_BUS, addr, run->iocsr_io.len, val)) > > + idx = srcu_read_lock(&vcpu->kvm->srcu); > > + ret = kvm_io_bus_write(vcpu, KVM_IOCSR_BUS, addr, run->iocsr_io.len, val); > > + srcu_read_unlock(&vcpu->kvm->srcu, idx); > > + if (ret == 0) > > ret = EMULATE_DONE; > > - else > > + else { > > + ret = EMULATE_DO_IOCSR; > > /* Save data and let user space to write it */ > > memcpy(run->iocsr_io.data, val, run->iocsr_io.len); > > - > > + } > > trace_kvm_iocsr(KVM_TRACE_IOCSR_WRITE, run->iocsr_io.len, addr, val); > > } else { > > - if (!kvm_io_bus_read(vcpu, KVM_IOCSR_BUS, addr, run->iocsr_io.len, val)) > > + idx = srcu_read_lock(&vcpu->kvm->srcu); > > + ret = kvm_io_bus_read(vcpu, KVM_IOCSR_BUS, addr, run->iocsr_io.len, val); > > + srcu_read_unlock(&vcpu->kvm->srcu, idx); > > + if (ret == 0) > > ret = EMULATE_DONE; > > - else > > + else { > > + ret = EMULATE_DO_IOCSR; > > /* Save register id for iocsr read completion */ > > vcpu->arch.io_gpr = rd; > > - > > + } > > trace_kvm_iocsr(KVM_TRACE_IOCSR_READ, run->iocsr_io.len, addr, NULL); > > } > > > > @@ -359,7 +366,7 @@ static int kvm_handle_gspr(struct kvm_vcpu *vcpu) > > > > int kvm_emu_mmio_read(struct kvm_vcpu *vcpu, larch_inst inst) > > { > > - int ret; > > + int idx, ret; > > unsigned int op8, opcode, rd; > > struct kvm_run *run = vcpu->run; > > > > @@ -464,8 +471,10 @@ int kvm_emu_mmio_read(struct kvm_vcpu *vcpu, larch_inst inst) > > * it need not return to user space to handle the mmio > > * exception. > > */ > > + idx = srcu_read_lock(&vcpu->kvm->srcu); > > ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, vcpu->arch.badv, > > run->mmio.len, &vcpu->arch.gprs[rd]); > > + srcu_read_unlock(&vcpu->kvm->srcu, idx); > > if (!ret) { > > update_pc(&vcpu->arch); > > vcpu->mmio_needed = 0; > > @@ -531,7 +540,7 @@ int kvm_complete_mmio_read(struct kvm_vcpu *vcpu, struct kvm_run *run) > > > > int kvm_emu_mmio_write(struct kvm_vcpu *vcpu, larch_inst inst) > > { > > - int ret; > > + int idx, ret; > > unsigned int rd, op8, opcode; > > unsigned long curr_pc, rd_val = 0; > > struct kvm_run *run = vcpu->run; > > @@ -631,7 +640,9 @@ int kvm_emu_mmio_write(struct kvm_vcpu *vcpu, larch_inst inst) > > * it need not return to user space to handle the mmio > > * exception. > > */ > > + idx = srcu_read_lock(&vcpu->kvm->srcu); > > ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, vcpu->arch.badv, run->mmio.len, data); > > + srcu_read_unlock(&vcpu->kvm->srcu, idx); > > if (!ret) > > return EMULATE_DONE; > > > > diff --git a/arch/loongarch/kvm/intc/ipi.c b/arch/loongarch/kvm/intc/ipi.c > > index a233a323e295..4b7ff20ed438 100644 > > --- a/arch/loongarch/kvm/intc/ipi.c > > +++ b/arch/loongarch/kvm/intc/ipi.c > > @@ -98,7 +98,7 @@ static void write_mailbox(struct kvm_vcpu *vcpu, int offset, uint64_t data, int > > > > static int send_ipi_data(struct kvm_vcpu *vcpu, gpa_t addr, uint64_t data) > > { > > - int i, ret; > > + int i, idx, ret; > > uint32_t val = 0, mask = 0; > > > > /* > > @@ -107,7 +107,9 @@ static int send_ipi_data(struct kvm_vcpu *vcpu, gpa_t addr, uint64_t data) > > */ > > if ((data >> 27) & 0xf) { > > /* Read the old val */ > > + srcu_read_unlock(&vcpu->kvm->srcu, idx); > here should be idx = srcu_read_lock(&vcpu->kvm->srcu) ? > > > ret = kvm_io_bus_read(vcpu, KVM_IOCSR_BUS, addr, sizeof(val), &val); > > + srcu_read_unlock(&vcpu->kvm->srcu, idx); > > if (unlikely(ret)) { > > kvm_err("%s: : read date from addr %llx failed\n", __func__, addr); > > return ret; > > @@ -121,7 +123,9 @@ static int send_ipi_data(struct kvm_vcpu *vcpu, gpa_t addr, uint64_t data) > > val &= mask; > > } > > val |= ((uint32_t)(data >> 32) & ~mask); > > + srcu_read_unlock(&vcpu->kvm->srcu, idx); > here should be idx = srcu_read_lock(&vcpu->kvm->srcu) Yes, this is my mistake, thank you. Huacai > > > ret = kvm_io_bus_write(vcpu, KVM_IOCSR_BUS, addr, sizeof(val), &val); > > + srcu_read_unlock(&vcpu->kvm->srcu, idx); > > if (unlikely(ret)) > > kvm_err("%s: : write date to addr %llx failed\n", __func__, addr); > > > > > otherwise looks good to me. > > Reviewed-by: Bibo Mao <maobibo@xxxxxxxxxxx> > >