Hi Reiji,
On Mon, Feb 14, 2022 at 09:34:30PM -0800, Reiji Watanabe wrote:
> 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).
Will change.
>
> Reviewed-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
Thanks!
Alex
>
> 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
