| Message ID | 20211123005036.2954379-6-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for KVM_CAP_VM_MOVE/COPY_ENC_CONTEXT_FROM | expand |
On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > Encapsulate the handling of the migration_in_progress flag for both VMs in > two functions sev_lock_two_vms and sev_unlock_two_vms. It does not matter > if KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM locks the source struct kvm a bit > earlier, and this change 1) keeps the cleanup chain of labels smaller 2) > makes it possible for KVM_CAP_VM_COPY_ENC_CONTEXT_FROM to reuse the logic. > > Cc: Peter Gonda <pgonda@google.com> > Cc: Sean Christopherson <seanjc@google.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Peter Gonda <pgonda@google.com> > --- > arch/x86/kvm/svm/sev.c | 50 ++++++++++++++++++++---------------------- > 1 file changed, 24 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 75955beb3770..23a4877d7bdf 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -1543,28 +1543,37 @@ static bool is_cmd_allowed_from_mirror(u32 cmd_id) > return false; > } > > -static int sev_lock_for_migration(struct kvm *kvm) > +static int sev_lock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm) > { > - struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > + struct kvm_sev_info *dst_sev = &to_kvm_svm(dst_kvm)->sev_info; > + struct kvm_sev_info *src_sev = &to_kvm_svm(src_kvm)->sev_info; > > /* > - * Bail if this VM is already involved in a migration to avoid deadlock > - * between two VMs trying to migrate to/from each other. > + * Bail if these VMs are already involved in a migration to avoid > + * deadlock between two VMs trying to migrate to/from each other. Nit: This comment will be a little stale once we use this function for clone too. Is there a generic term we could use for migration and clone? > */ > - if (atomic_cmpxchg_acquire(&sev->migration_in_progress, 0, 1)) > + if (atomic_cmpxchg_acquire(&dst_sev->migration_in_progress, 0, 1)) Nit: I guess the same comment can be made for |migration_in_progress|. > return -EBUSY; > > - mutex_lock(&kvm->lock); > + if (atomic_cmpxchg_acquire(&src_sev->migration_in_progress, 0, 1)) { > + atomic_set_release(&dst_sev->migration_in_progress, 0); > + return -EBUSY; > + } > > + mutex_lock(&dst_kvm->lock); > + mutex_lock(&src_kvm->lock); > return 0; > } > > -static void sev_unlock_after_migration(struct kvm *kvm) > +static void sev_unlock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm) > { > - struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > + struct kvm_sev_info *dst_sev = &to_kvm_svm(dst_kvm)->sev_info; > + struct kvm_sev_info *src_sev = &to_kvm_svm(src_kvm)->sev_info; > > - mutex_unlock(&kvm->lock); > - atomic_set_release(&sev->migration_in_progress, 0); > + mutex_unlock(&dst_kvm->lock); > + mutex_unlock(&src_kvm->lock); > + atomic_set_release(&dst_sev->migration_in_progress, 0); > + atomic_set_release(&src_sev->migration_in_progress, 0); > } > > > @@ -1665,15 +1674,6 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd) > bool charged = false; > int ret; > > - ret = sev_lock_for_migration(kvm); > - if (ret) > - return ret; > - > - if (sev_guest(kvm)) { > - ret = -EINVAL; > - goto out_unlock; > - } > - > source_kvm_file = fget(source_fd); > if (!file_is_kvm(source_kvm_file)) { > ret = -EBADF; > @@ -1681,13 +1681,13 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd) > } > > source_kvm = source_kvm_file->private_data; > - ret = sev_lock_for_migration(source_kvm); > + ret = sev_lock_two_vms(kvm, source_kvm); > if (ret) > goto out_fput; > > - if (!sev_guest(source_kvm)) { > + if (sev_guest(kvm) || !sev_guest(source_kvm)) { > ret = -EINVAL; > - goto out_source; > + goto out_unlock; > } > > src_sev = &to_kvm_svm(source_kvm)->sev_info; > @@ -1727,13 +1727,11 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd) > sev_misc_cg_uncharge(cg_cleanup_sev); > put_misc_cg(cg_cleanup_sev->misc_cg); > cg_cleanup_sev->misc_cg = NULL; > -out_source: > - sev_unlock_after_migration(source_kvm); > +out_unlock: > + sev_unlock_two_vms(kvm, source_kvm); > out_fput: > if (source_kvm_file) > fput(source_kvm_file); > -out_unlock: > - sev_unlock_after_migration(kvm); > return ret; > } > > -- > 2.27.0 > >
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 75955beb3770..23a4877d7bdf 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -1543,28 +1543,37 @@ static bool is_cmd_allowed_from_mirror(u32 cmd_id) return false; } -static int sev_lock_for_migration(struct kvm *kvm) +static int sev_lock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm) { - struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; + struct kvm_sev_info *dst_sev = &to_kvm_svm(dst_kvm)->sev_info; + struct kvm_sev_info *src_sev = &to_kvm_svm(src_kvm)->sev_info; /* - * Bail if this VM is already involved in a migration to avoid deadlock - * between two VMs trying to migrate to/from each other. + * Bail if these VMs are already involved in a migration to avoid + * deadlock between two VMs trying to migrate to/from each other. */ - if (atomic_cmpxchg_acquire(&sev->migration_in_progress, 0, 1)) + if (atomic_cmpxchg_acquire(&dst_sev->migration_in_progress, 0, 1)) return -EBUSY; - mutex_lock(&kvm->lock); + if (atomic_cmpxchg_acquire(&src_sev->migration_in_progress, 0, 1)) { + atomic_set_release(&dst_sev->migration_in_progress, 0); + return -EBUSY; + } + mutex_lock(&dst_kvm->lock); + mutex_lock(&src_kvm->lock); return 0; } -static void sev_unlock_after_migration(struct kvm *kvm) +static void sev_unlock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm) { - struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; + struct kvm_sev_info *dst_sev = &to_kvm_svm(dst_kvm)->sev_info; + struct kvm_sev_info *src_sev = &to_kvm_svm(src_kvm)->sev_info; - mutex_unlock(&kvm->lock); - atomic_set_release(&sev->migration_in_progress, 0); + mutex_unlock(&dst_kvm->lock); + mutex_unlock(&src_kvm->lock); + atomic_set_release(&dst_sev->migration_in_progress, 0); + atomic_set_release(&src_sev->migration_in_progress, 0); } @@ -1665,15 +1674,6 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd) bool charged = false; int ret; - ret = sev_lock_for_migration(kvm); - if (ret) - return ret; - - if (sev_guest(kvm)) { - ret = -EINVAL; - goto out_unlock; - } - source_kvm_file = fget(source_fd); if (!file_is_kvm(source_kvm_file)) { ret = -EBADF; @@ -1681,13 +1681,13 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd) } source_kvm = source_kvm_file->private_data; - ret = sev_lock_for_migration(source_kvm); + ret = sev_lock_two_vms(kvm, source_kvm); if (ret) goto out_fput; - if (!sev_guest(source_kvm)) { + if (sev_guest(kvm) || !sev_guest(source_kvm)) { ret = -EINVAL; - goto out_source; + goto out_unlock; } src_sev = &to_kvm_svm(source_kvm)->sev_info; @@ -1727,13 +1727,11 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd) sev_misc_cg_uncharge(cg_cleanup_sev); put_misc_cg(cg_cleanup_sev->misc_cg); cg_cleanup_sev->misc_cg = NULL; -out_source: - sev_unlock_after_migration(source_kvm); +out_unlock: + sev_unlock_two_vms(kvm, source_kvm); out_fput: if (source_kvm_file) fput(source_kvm_file); -out_unlock: - sev_unlock_after_migration(kvm); return ret; }
Encapsulate the handling of the migration_in_progress flag for both VMs in two functions sev_lock_two_vms and sev_unlock_two_vms. It does not matter if KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM locks the source struct kvm a bit earlier, and this change 1) keeps the cleanup chain of labels smaller 2) makes it possible for KVM_CAP_VM_COPY_ENC_CONTEXT_FROM to reuse the logic. Cc: Peter Gonda <pgonda@google.com> Cc: Sean Christopherson <seanjc@google.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/svm/sev.c | 50 ++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 26 deletions(-)