Hi Alex,
On Wed, Nov 17, 2021 at 7:37 AM Alexandru Elisei
<alexandru.elisei@xxxxxxx> 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 <alexandru.elisei@xxxxxxx>
> ---
> 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 2a5f7f38006f..733621e41900 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -606,6 +606,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 2f03cbfefe67..e9b4ad7b5c82 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -651,6 +651,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
Nit: vcpu_load() doesn't try to grab the vcpu->mutex, but kvm_vcpu_ioctl()
does (The original comment in lock_all_vcpus() was outdated).
Reviewed-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
Thanks,
Reiji
> + * 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 0a06d0648970..cd045c7abde8 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 089fc2ffcb43..bc4197e87d95 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_lock_all_vcpus(kvm)) {
> mutex_unlock(&its->its_lock);
> mutex_unlock(&kvm->lock);
> return -EBUSY;
> @@ -2686,7 +2686,7 @@ static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr)
> break;
> }
>
> - unlock_all_vcpus(kvm);
> + kvm_unlock_all_vcpus(kvm);
> mutex_unlock(&its->its_lock);
> 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 0d000d2fe8d2..c5de904643cc 100644
> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> @@ -305,44 +305,6 @@ int vgic_v2_parse_attr(struct kvm_device *dev, struct kvm_device_attr *attr,
> return 0;
> }
>
> -/* 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 unlock_all_vcpus(struct kvm *kvm)
> -{
> - 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;
> - int c;
> -
> - /*
> - * 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 fiddle with the vgic 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;
> -}
> -
> /**
> * vgic_v2_attr_regs_access - allows user space to access VGIC v2 state
> *
> @@ -373,7 +335,7 @@ static int vgic_v2_attr_regs_access(struct kvm_device *dev,
> if (ret)
> goto out;
>
> - if (!lock_all_vcpus(dev->kvm)) {
> + if (!kvm_lock_all_vcpus(dev->kvm)) {
> ret = -EBUSY;
> goto out;
> }
> @@ -390,7 +352,7 @@ static int vgic_v2_attr_regs_access(struct kvm_device *dev,
> break;
> }
>
> - unlock_all_vcpus(dev->kvm);
> + kvm_unlock_all_vcpus(dev->kvm);
> out:
> mutex_unlock(&dev->kvm->lock);
> return ret;
> @@ -539,7 +501,7 @@ static int vgic_v3_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;
> }
> @@ -589,7 +551,7 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> break;
> }
>
> - unlock_all_vcpus(dev->kvm);
> + kvm_unlock_all_vcpus(dev->kvm);
> out:
> mutex_unlock(&dev->kvm->lock);
> return ret;
> @@ -644,12 +606,12 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
> case KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES:
> mutex_lock(&dev->kvm->lock);
>
> - if (!lock_all_vcpus(dev->kvm)) {
> + if (!kvm_lock_all_vcpus(dev->kvm)) {
> mutex_unlock(&dev->kvm->lock);
> return -EBUSY;
> }
> ret = vgic_v3_save_pending_tables(dev->kvm);
> - unlock_all_vcpus(dev->kvm);
> + kvm_unlock_all_vcpus(dev->kvm);
> mutex_unlock(&dev->kvm->lock);
> return ret;
> }
> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> index 3fd6c86a7ef3..e69c839a6941 100644
> --- a/arch/arm64/kvm/vgic/vgic.h
> +++ b/arch/arm64/kvm/vgic/vgic.h
> @@ -255,9 +255,6 @@ int vgic_init(struct kvm *kvm);
> void vgic_debug_init(struct kvm *kvm);
> void vgic_debug_destroy(struct kvm *kvm);
>
> -bool lock_all_vcpus(struct kvm *kvm);
> -void unlock_all_vcpus(struct kvm *kvm);
> -
> static inline int vgic_v3_max_apr_idx(struct kvm_vcpu *vcpu)
> {
> struct vgic_cpu *cpu_if = &vcpu->arch.vgic_cpu;
> --
> 2.33.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
