| Message ID | 20250211000917.166856-3-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | KVM: extract lock_all_vcpus/unlock_all_vcpus | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
On Tue, 11 Feb 2025 00:09:16 +0000, Maxim Levitsky <mlevitsk@redhat.com> wrote: > > Switch to kvm_lock/unlock_all_vcpus instead of arm's own > version. > > This fixes lockdep warning about reaching maximum lock depth: > > [ 328.171264] BUG: MAX_LOCK_DEPTH too low! > [ 328.175227] turning off the locking correctness validator. > [ 328.180726] Please attach the output of /proc/lock_stat to the bug report > [ 328.187531] depth: 48 max: 48! > [ 328.190678] 48 locks held by qemu-kvm/11664: > [ 328.194957] #0: ffff800086de5ba0 (&kvm->lock){+.+.}-{3:3}, at: kvm_ioctl_create_device+0x174/0x5b0 > [ 328.204048] #1: ffff0800e78800b8 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 > [ 328.212521] #2: ffff07ffeee51e98 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 > [ 328.220991] #3: ffff0800dc7d80b8 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 > [ 328.229463] #4: ffff07ffe0c980b8 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 > [ 328.237934] #5: ffff0800a3883c78 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 > [ 328.246405] #6: ffff07fffbe480b8 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 > > No functional change intended. Actually plenty of it. This sort of broad assertion is really an indication of the contrary. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > arch/arm64/include/asm/kvm_host.h | 3 --- > arch/arm64/kvm/arch_timer.c | 8 +++---- > arch/arm64/kvm/arm.c | 32 --------------------------- > arch/arm64/kvm/vgic/vgic-init.c | 11 +++++---- > arch/arm64/kvm/vgic/vgic-its.c | 18 ++++++++------- > arch/arm64/kvm/vgic/vgic-kvm-device.c | 21 ++++++++++-------- > 6 files changed, 33 insertions(+), 60 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 7cfa024de4e3..bba97ea700ca 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -1234,9 +1234,6 @@ int __init populate_sysreg_config(const struct sys_reg_desc *sr, > unsigned int idx); > int __init populate_nv_trap_config(void); > > -bool lock_all_vcpus(struct kvm *kvm); > -void unlock_all_vcpus(struct kvm *kvm); > - > void kvm_calculate_traps(struct kvm_vcpu *vcpu); > > /* MMIO helpers */ > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > index 231c0cd9c7b4..3af1da807f9c 100644 > --- a/arch/arm64/kvm/arch_timer.c > +++ b/arch/arm64/kvm/arch_timer.c > @@ -1769,7 +1769,9 @@ int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm, > > mutex_lock(&kvm->lock); > > - if (lock_all_vcpus(kvm)) { > + ret = kvm_lock_all_vcpus(kvm); > + > + if (!ret) { > set_bit(KVM_ARCH_FLAG_VM_COUNTER_OFFSET, &kvm->arch.flags); > > /* > @@ -1781,9 +1783,7 @@ int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm, > kvm->arch.timer_data.voffset = offset->counter_offset; > kvm->arch.timer_data.poffset = offset->counter_offset; > > - unlock_all_vcpus(kvm); > - } else { > - ret = -EBUSY; > + kvm_unlock_all_vcpus(kvm); > } This is a userspace ABI change. This ioctl is documented as being able to return -EINVAL or -EBUSY, and nothing else other than 0. Yet the new helper returns -EINTR, which you blindly forward to userspace. > > mutex_unlock(&kvm->lock); > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 071a7d75be68..f58849c5b4f0 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -1895,38 +1895,6 @@ static void unlock_vcpus(struct kvm *kvm, int vcpu_lock_idx) > } > } > > -void unlock_all_vcpus(struct kvm *kvm) > -{ > - lockdep_assert_held(&kvm->lock); > - > - unlock_vcpus(kvm, atomic_read(&kvm->online_vcpus) - 1); > -} > - > -/* Returns true if all vcpus were locked, false otherwise */ > -bool lock_all_vcpus(struct kvm *kvm) > -{ > - struct kvm_vcpu *tmp_vcpu; > - unsigned long c; > - > - lockdep_assert_held(&kvm->lock); > - > - /* > - * Any time a vcpu is in an ioctl (including running), the > - * core KVM code tries to grab the vcpu->mutex. > - * > - * By grabbing the vcpu->mutex of all VCPUs we ensure that no > - * other VCPUs can fiddle with the state while we access it. > - */ > - kvm_for_each_vcpu(c, tmp_vcpu, kvm) { > - if (!mutex_trylock(&tmp_vcpu->mutex)) { > - unlock_vcpus(kvm, c - 1); > - return false; > - } > - } > - > - return true; > -} The semantics are different. Other than the return values mentioned above, the new version fails on signal delivery, which isn't expected. The guarantee given to userspace is that unless a vcpu thread is currently in KVM, the locking will succeed. Not "will succeed unless something that is outside of your control happens". The arm64 version is also built around a mutex_trylock() because we don't want to wait forever until the vcpu's mutex is released. We want it now, or never. That's consistent with the above requirement on userspace. We can argue whether or not these are good guarantees (or requirements) to give to (or demand from) userspace, but that's what we have, and I'm not prepared to break any of it. At the end of the day, the x86 locking serves completely different purposes. It wants to gracefully wait for vcpus to exit and is happy to replay things, because migration (which is what x86 seems to be using this for) is a stupidly long process. Our locking is designed to either succeed or fail quickly, because some of the lock paths are on the critical path for VM startup and configuration. So for this series to be acceptable, you'd have to provide the same semantics. It is probably doable with a bit of macro magic, at the expense of readability. What I would also like to see is for this primitive to be usable with scoped_cond_guard(), which would make the code much more readable. Thanks, M.
On Tue, Feb 11, 2025 at 10:25 AM Marc Zyngier <maz@kernel.org> wrote: > > No functional change intended. > > Actually plenty of it. Yes, definitely. That's not "no functional change intended", it's "no breakage of sane userspace intended" which is pretty much the opposite. > Yet the new helper returns -EINTR, which you blindly forward to userspace. It won't quite reach userspace, since the task won't survive mutex_lock_killable(). With your current code the kill will arrive just after the ioctl returns to userspace, so there's no change in behavior there. The -EBUSY change is the one that matters. > At the end of the day, the x86 locking serves completely different > purposes. It wants to gracefully wait for vcpus to exit and is happy > to replay things, because migration (which is what x86 seems to be > using this for) is a stupidly long process. No, it's not using it for the whole length of migration. It serves the same exact purpose as ARM: do some stuff on something that spans a whole struct kvm. The only difference is the behavior on contended mutex, where x86 simply says don't do it. > Our locking is designed to > either succeed or fail quickly, because some of the lock paths are on > the critical path for VM startup and configuration. The only long-running vCPU ioctl is KVM_RUN and you don't have that during VM startup and configuration. The interesting case is snapshotting, i.e. reading attributes. Failing quickly when reading attributes makes sense, and I'd be rightly wary of changing it. I know that QEMU is doing them only when it knows CPUs are stopped, but it does seem to affect crosvm and Firecracker more, for snapshotting more than startup/configuration. The GIC snapshotting code in crosvm returns an anyhow::Result and ends up logging an error with println!, Firecracker is similar as it also propagates the error. So neither crosvm nor Firecracker have particularly sophisticated error handling but they do seem to want to fail their RPCs quickly. Failing quickly when creating the device or setting attributes makes less sense (Firecracker for one does an unwrap() there). But anyhow the change would have to be a separate patch, certainly not one applied through the generic KVM tree; and if you decide that you're stuck with it, that would be more than understandable. > So for this series to be acceptable, you'd have to provide the same > semantics. It is probably doable with a bit of macro magic, at the > expense of readability. Or it can be just an extra bool argument, plus wrappers. For RISC-V it's only used at device creation time, and the ioctl fails if the vCPU ran at least once. Removing the "try" in trylock is totally feasible there, however it must be documented in the commit message. Thanks, Paolo > What I would also like to see is for this primitive to be usable with > scoped_cond_guard(), which would make the code much more readable.
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 7cfa024de4e3..bba97ea700ca 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -1234,9 +1234,6 @@ int __init populate_sysreg_config(const struct sys_reg_desc *sr, unsigned int idx); int __init populate_nv_trap_config(void); -bool lock_all_vcpus(struct kvm *kvm); -void unlock_all_vcpus(struct kvm *kvm); - void kvm_calculate_traps(struct kvm_vcpu *vcpu); /* MMIO helpers */ diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c index 231c0cd9c7b4..3af1da807f9c 100644 --- a/arch/arm64/kvm/arch_timer.c +++ b/arch/arm64/kvm/arch_timer.c @@ -1769,7 +1769,9 @@ int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm, mutex_lock(&kvm->lock); - if (lock_all_vcpus(kvm)) { + ret = kvm_lock_all_vcpus(kvm); + + if (!ret) { set_bit(KVM_ARCH_FLAG_VM_COUNTER_OFFSET, &kvm->arch.flags); /* @@ -1781,9 +1783,7 @@ int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm, kvm->arch.timer_data.voffset = offset->counter_offset; kvm->arch.timer_data.poffset = offset->counter_offset; - unlock_all_vcpus(kvm); - } else { - ret = -EBUSY; + kvm_unlock_all_vcpus(kvm); } mutex_unlock(&kvm->lock); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 071a7d75be68..f58849c5b4f0 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1895,38 +1895,6 @@ static void unlock_vcpus(struct kvm *kvm, int vcpu_lock_idx) } } -void unlock_all_vcpus(struct kvm *kvm) -{ - lockdep_assert_held(&kvm->lock); - - unlock_vcpus(kvm, atomic_read(&kvm->online_vcpus) - 1); -} - -/* Returns true if all vcpus were locked, false otherwise */ -bool lock_all_vcpus(struct kvm *kvm) -{ - struct kvm_vcpu *tmp_vcpu; - unsigned long c; - - lockdep_assert_held(&kvm->lock); - - /* - * Any time a vcpu is in an ioctl (including running), the - * core KVM code tries to grab the vcpu->mutex. - * - * By grabbing the vcpu->mutex of all VCPUs we ensure that no - * other VCPUs can fiddle with the state while we access it. - */ - kvm_for_each_vcpu(c, tmp_vcpu, kvm) { - if (!mutex_trylock(&tmp_vcpu->mutex)) { - unlock_vcpus(kvm, c - 1); - return false; - } - } - - return true; -} - static unsigned long nvhe_percpu_size(void) { return (unsigned long)CHOOSE_NVHE_SYM(__per_cpu_end) - diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c index bc7e22ab5d81..8fbce4db5c2e 100644 --- a/arch/arm64/kvm/vgic/vgic-init.c +++ b/arch/arm64/kvm/vgic/vgic-init.c @@ -85,8 +85,8 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) /* Must be held to avoid race with vCPU creation */ lockdep_assert_held(&kvm->lock); - ret = -EBUSY; - if (!lock_all_vcpus(kvm)) + ret = kvm_lock_all_vcpus(kvm); + if (ret) return ret; mutex_lock(&kvm->arch.config_lock); @@ -97,9 +97,12 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) } kvm_for_each_vcpu(i, vcpu, kvm) { - if (vcpu_has_run_once(vcpu)) + if (vcpu_has_run_once(vcpu)) { + ret = -EBUSY; goto out_unlock; + } } + ret = 0; if (type == KVM_DEV_TYPE_ARM_VGIC_V2) @@ -124,7 +127,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) out_unlock: mutex_unlock(&kvm->arch.config_lock); - unlock_all_vcpus(kvm); + kvm_unlock_all_vcpus(kvm); return ret; } diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c index fb96802799c6..02b02b4fff5d 100644 --- a/arch/arm64/kvm/vgic/vgic-its.c +++ b/arch/arm64/kvm/vgic/vgic-its.c @@ -1977,7 +1977,7 @@ static int vgic_its_attr_regs_access(struct kvm_device *dev, struct vgic_its *its; gpa_t addr, offset; unsigned int len; - int align, ret = 0; + int align, ret; its = dev->private; offset = attr->attr; @@ -1999,9 +1999,10 @@ static int vgic_its_attr_regs_access(struct kvm_device *dev, mutex_lock(&dev->kvm->lock); - if (!lock_all_vcpus(dev->kvm)) { + ret = kvm_lock_all_vcpus(sdev->kvm); + if (ret) { mutex_unlock(&dev->kvm->lock); - return -EBUSY; + return ret; } mutex_lock(&dev->kvm->arch.config_lock); @@ -2034,7 +2035,7 @@ static int vgic_its_attr_regs_access(struct kvm_device *dev, } out: mutex_unlock(&dev->kvm->arch.config_lock); - unlock_all_vcpus(dev->kvm); + kvm_unlock_all_vcpus(dev->kvm); mutex_unlock(&dev->kvm->lock); return ret; } @@ -2697,16 +2698,17 @@ static int vgic_its_has_attr(struct kvm_device *dev, static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr) { const struct vgic_its_abi *abi = vgic_its_get_abi(its); - int ret = 0; + int ret; if (attr == KVM_DEV_ARM_VGIC_CTRL_INIT) /* Nothing to do */ return 0; mutex_lock(&kvm->lock); - if (!lock_all_vcpus(kvm)) { + ret = kvm_lock_all_vcpus(kvm); + if (ret) { mutex_unlock(&kvm->lock); - return -EBUSY; + return ret; } mutex_lock(&kvm->arch.config_lock); @@ -2726,7 +2728,7 @@ static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr) mutex_unlock(&its->its_lock); mutex_unlock(&kvm->arch.config_lock); - unlock_all_vcpus(kvm); + kvm_unlock_all_vcpus(kvm); mutex_unlock(&kvm->lock); return ret; } diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c index 5f4f57aaa23e..ee70a9d642ed 100644 --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c @@ -268,15 +268,16 @@ static int vgic_set_common_attr(struct kvm_device *dev, return -ENXIO; mutex_lock(&dev->kvm->lock); - if (!lock_all_vcpus(dev->kvm)) { + r = kvm_lock_all_vcpus(dev->kvm); + if (r) { mutex_unlock(&dev->kvm->lock); - return -EBUSY; + return r; } mutex_lock(&dev->kvm->arch.config_lock); r = vgic_v3_save_pending_tables(dev->kvm); mutex_unlock(&dev->kvm->arch.config_lock); - unlock_all_vcpus(dev->kvm); + kvm_unlock_all_vcpus(dev->kvm); mutex_unlock(&dev->kvm->lock); return r; } @@ -384,9 +385,10 @@ static int vgic_v2_attr_regs_access(struct kvm_device *dev, mutex_lock(&dev->kvm->lock); - if (!lock_all_vcpus(dev->kvm)) { + ret = kvm_lock_all_vcpus(dev->kvm); + if (ret) { mutex_unlock(&dev->kvm->lock); - return -EBUSY; + return ret; } mutex_lock(&dev->kvm->arch.config_lock); @@ -409,7 +411,7 @@ static int vgic_v2_attr_regs_access(struct kvm_device *dev, out: mutex_unlock(&dev->kvm->arch.config_lock); - unlock_all_vcpus(dev->kvm); + kvm_unlock_all_vcpus(dev->kvm); mutex_unlock(&dev->kvm->lock); if (!ret && !is_write) @@ -545,9 +547,10 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev, mutex_lock(&dev->kvm->lock); - if (!lock_all_vcpus(dev->kvm)) { + ret = kvm_lock_all_vcpus(dev->kvm); + if (ret) { mutex_unlock(&dev->kvm->lock); - return -EBUSY; + return ret; } mutex_lock(&dev->kvm->arch.config_lock); @@ -589,7 +592,7 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev, out: mutex_unlock(&dev->kvm->arch.config_lock); - unlock_all_vcpus(dev->kvm); + kvm_unlock_all_vcpus(dev->kvm); mutex_unlock(&dev->kvm->lock); if (!ret && uaccess && !is_write) {
Switch to kvm_lock/unlock_all_vcpus instead of arm's own version. This fixes lockdep warning about reaching maximum lock depth: [ 328.171264] BUG: MAX_LOCK_DEPTH too low! [ 328.175227] turning off the locking correctness validator. [ 328.180726] Please attach the output of /proc/lock_stat to the bug report [ 328.187531] depth: 48 max: 48! [ 328.190678] 48 locks held by qemu-kvm/11664: [ 328.194957] #0: ffff800086de5ba0 (&kvm->lock){+.+.}-{3:3}, at: kvm_ioctl_create_device+0x174/0x5b0 [ 328.204048] #1: ffff0800e78800b8 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 [ 328.212521] #2: ffff07ffeee51e98 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 [ 328.220991] #3: ffff0800dc7d80b8 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 [ 328.229463] #4: ffff07ffe0c980b8 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 [ 328.237934] #5: ffff0800a3883c78 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 [ 328.246405] #6: ffff07fffbe480b8 (&vcpu->mutex){+.+.}-{3:3}, at: lock_all_vcpus+0x16c/0x2a0 No functional change intended. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/arm64/include/asm/kvm_host.h | 3 --- arch/arm64/kvm/arch_timer.c | 8 +++---- arch/arm64/kvm/arm.c | 32 --------------------------- arch/arm64/kvm/vgic/vgic-init.c | 11 +++++---- arch/arm64/kvm/vgic/vgic-its.c | 18 ++++++++------- arch/arm64/kvm/vgic/vgic-kvm-device.c | 21 ++++++++++-------- 6 files changed, 33 insertions(+), 60 deletions(-)