Re: [RFC PATCH v4 01/39] KVM: arm64: Make lock_all_vcpus() available to the rest of KVM

2021-09-22 Thread Suzuki K Poulose

On 25/08/2021 17:17, Alexandru Elisei wrote:

The VGIC code uses the lock_all_vcpus() function to make sure no VCPUs are
run while it fiddles with the global VGIC state. Move the declaration of
lock_all_vcpus() and the corresponding unlock function into asm/kvm_host.h
where it can be reused by other parts of KVM/arm64 and rename the functions
to kvm_{lock,unlock}_all_vcpus() to make them more generic.

Because the scope of the code potentially using the functions has
increased, add a lockdep check that the kvm->lock is held by the caller.
Holding the lock is necessary because otherwise userspace would be able to
create new VCPUs and run them while the existing VCPUs are locked.

No functional change intended.

Signed-off-by: Alexandru Elisei 



LGTM,

Reviewed-by: Suzuki K Poulose 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[RFC PATCH v4 01/39] KVM: arm64: Make lock_all_vcpus() available to the rest of KVM

2021-08-25 Thread Alexandru Elisei
The VGIC code uses the lock_all_vcpus() function to make sure no VCPUs are
run while it fiddles with the global VGIC state. Move the declaration of
lock_all_vcpus() and the corresponding unlock function into asm/kvm_host.h
where it can be reused by other parts of KVM/arm64 and rename the functions
to kvm_{lock,unlock}_all_vcpus() to make them more generic.

Because the scope of the code potentially using the functions has
increased, add a lockdep check that the kvm->lock is held by the caller.
Holding the lock is necessary because otherwise userspace would be able to
create new VCPUs and run them while the existing VCPUs are locked.

No functional change intended.

Signed-off-by: Alexandru Elisei 
---
 arch/arm64/include/asm/kvm_host.h |  3 ++
 arch/arm64/kvm/arm.c  | 41 ++
 arch/arm64/kvm/vgic/vgic-init.c   |  4 +--
 arch/arm64/kvm/vgic/vgic-its.c|  8 ++---
 arch/arm64/kvm/vgic/vgic-kvm-device.c | 50 ---
 arch/arm64/kvm/vgic/vgic.h|  3 --
 6 files changed, 56 insertions(+), 53 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h
index 41911585ae0c..797083203603 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -601,6 +601,9 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
 void kvm_arm_halt_guest(struct kvm *kvm);
 void kvm_arm_resume_guest(struct kvm *kvm);
 
+bool kvm_lock_all_vcpus(struct kvm *kvm);
+void kvm_unlock_all_vcpus(struct kvm *kvm);
+
 #ifndef __KVM_NVHE_HYPERVISOR__
 #define kvm_call_hyp_nvhe(f, ...)  
\
({  \
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e9a2b8f27792..ddace63528f1 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -647,6 +647,47 @@ void kvm_arm_resume_guest(struct kvm *kvm)
}
 }
 
+/* unlocks vcpus from @vcpu_lock_idx and smaller */
+static void unlock_vcpus(struct kvm *kvm, int vcpu_lock_idx)
+{
+   struct kvm_vcpu *tmp_vcpu;
+
+   for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
+   tmp_vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx);
+   mutex_unlock(&tmp_vcpu->mutex);
+   }
+}
+
+void kvm_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 kvm_lock_all_vcpus(struct kvm *kvm)
+{
+   struct kvm_vcpu *tmp_vcpu;
+   int c;
+
+   lockdep_assert_held(&kvm->lock);
+
+   /*
+* Any time a vcpu is run, vcpu_load is called which tries to grab the
+* vcpu->mutex.  By grabbing the vcpu->mutex of all VCPUs we ensure that
+* no other VCPUs are run and it is safe to fiddle with KVM global
+* state.
+*/
+   kvm_for_each_vcpu(c, tmp_vcpu, kvm) {
+   if (!mutex_trylock(&tmp_vcpu->mutex)) {
+   unlock_vcpus(kvm, c - 1);
+   return false;
+   }
+   }
+
+   return true;
+}
+
 static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
 {
struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);
diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 340c51d87677..6a85aa064a6c 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -87,7 +87,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
return -ENODEV;
 
ret = -EBUSY;
-   if (!lock_all_vcpus(kvm))
+   if (!kvm_lock_all_vcpus(kvm))
return ret;
 
kvm_for_each_vcpu(i, vcpu, kvm) {
@@ -117,7 +117,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
INIT_LIST_HEAD(&kvm->arch.vgic.rd_regions);
 
 out_unlock:
-   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 61728c543eb9..3a336a678cb8 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2005,7 +2005,7 @@ static int vgic_its_attr_regs_access(struct kvm_device 
*dev,
goto out;
}
 
-   if (!lock_all_vcpus(dev->kvm)) {
+   if (!kvm_lock_all_vcpus(dev->kvm)) {
ret = -EBUSY;
goto out;
}
@@ -2023,7 +2023,7 @@ static int vgic_its_attr_regs_access(struct kvm_device 
*dev,
} else {
*reg = region->its_read(dev->kvm, its, addr, len);
}
-   unlock_all_vcpus(dev->kvm);
+   kvm_unlock_all_vcpus(dev->kvm);
 out:
mutex_unlock(&dev->kvm->lock);
return ret;
@@ -2668,7 +2668,7 @@ static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its 
*its, u64 attr)
mutex_lock(&kvm->lock);
mutex_lock(&its->its_lock);
 
-   if (!lock_all_vcpus(kvm)) {
+   if (!kvm_l