This turned out to be a bit of a trainwreck, mostly due to
the patches being merged hastily at the end of the merge window.
For this reason, there are a few bugs for intra-host migration
as well.
Compared to the v1 I posted last week, there's many more bugfixes,
and the code I promised to avoid dangling ASIDs when a VM has
mirrors and is migrated.
Paolo
Paolo Bonzini (12):
selftests: fix check for circular KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
selftests: sev_migrate_tests: free all VMs
KVM: SEV: expose KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM capability
KVM: SEV: do not use list_replace_init on an empty list
KVM: SEV: cleanup locking for KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
KVM: SEV: initialize regions_list of a mirror VM
KVM: SEV: move mirror status to destination of
KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
selftests: sev_migrate_tests: add tests for
KVM_CAP_VM_COPY_ENC_CONTEXT_FROM
KVM: SEV: Do COPY_ENC_CONTEXT_FROM with both VMs locked
KVM: SEV: Prohibit migration of a VM that has mirrors
KVM: SEV: do not take kvm->lock when destroying
KVM: SEV: accept signals in sev_lock_two_vms
arch/x86/kvm/svm/sev.c | 161 +++++++++--------
arch/x86/kvm/svm/svm.h | 1 +
arch/x86/kvm/x86.c | 1 +
.../selftests/kvm/x86_64/sev_migrate_tests.c | 165 ++++++++++++++++--
4 files changed, 243 insertions(+), 85 deletions(-)
--
2.27.0
KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM leaves the source VM in a dead state,
so migrating back to the original source VM fails the ioctl. Adjust
the test.
Signed-off-by: Paolo Bonzini <[email protected]>
---
tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
index 5ba325cd64bf..a66b9be30239 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
@@ -89,7 +89,7 @@ static void test_sev_migrate_from(bool es)
{
struct kvm_vm *src_vm;
struct kvm_vm *dst_vms[NR_MIGRATE_TEST_VMS];
- int i;
+ int i, ret;
src_vm = sev_vm_create(es);
for (i = 0; i < NR_MIGRATE_TEST_VMS; ++i)
@@ -102,7 +102,10 @@ static void test_sev_migrate_from(bool es)
sev_migrate_from(dst_vms[i]->fd, dst_vms[i - 1]->fd);
/* Migrate the guest back to the original VM. */
- sev_migrate_from(src_vm->fd, dst_vms[NR_MIGRATE_TEST_VMS - 1]->fd);
+ ret = __sev_migrate_from(src_vm->fd, dst_vms[NR_MIGRATE_TEST_VMS - 1]->fd);
+ TEST_ASSERT(ret == -1 && errno == EIO,
+ "VM that was migrated from should be dead. ret %d, errno: %d\n", ret,
+ errno);
kvm_vm_free(src_vm);
for (i = 0; i < NR_MIGRATE_TEST_VMS; ++i)
--
2.27.0
This was broken before the introduction of KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM,
but technically harmless because the region list was unused for a mirror
VM. However, it is untidy and it now causes a NULL pointer access when
attempting to move the encryption context of a mirror VM.
Fixes: 54526d1fd593 ("KVM: x86: Support KVM VMs sharing SEV context")
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm/sev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 23a4877d7bdf..dc974c1728b6 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2004,6 +2004,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
mirror_sev->fd = source_sev.fd;
mirror_sev->es_active = source_sev.es_active;
mirror_sev->handle = source_sev.handle;
+ INIT_LIST_HEAD(&mirror_sev->regions_list);
/*
* Do not copy ap_jump_table. Since the mirror does not share the same
* KVM contexts as the original, and they may have different
--
2.27.0
list_replace_init cannot be used if the source is an empty list,
because "new->next->prev = new" will overwrite "old->next":
new old
prev = new, next = new prev = old, next = old
new->next = old->next prev = new, next = old prev = old, next = old
new->next->prev = new prev = new, next = old prev = old, next = new
new->prev = old->prev prev = old, next = old prev = old, next = old
new->next->prev = new prev = old, next = old prev = new, next = new
The desired outcome instead would be to leave both old and new the same
as they were (two empty circular lists). Use list_cut_before, which
already has the necessary check and is documented to discard the
previous contents of the list that will hold the result.
Fixes: b56639318bb2 ("KVM: SEV: Add support for SEV intra host migration")
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm/sev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 21ac0a5de4e0..75955beb3770 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1613,8 +1613,7 @@ static void sev_migrate_from(struct kvm_sev_info *dst,
src->handle = 0;
src->pages_locked = 0;
- INIT_LIST_HEAD(&dst->regions_list);
- list_replace_init(&src->regions_list, &dst->regions_list);
+ list_cut_before(&dst->regions_list, &src->regions_list, &src->regions_list);
}
static int sev_es_migrate_from(struct kvm *dst, struct kvm *src)
--
2.27.0
The capability, albeit present, was never exposed via KVM_CHECK_EXTENSION.
Fixes: b56639318bb2 ("KVM: SEV: Add support for SEV intra host migration")
Cc: Peter Gonda <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/x86.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a8f12c83db4b..7b7b56c89bd8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4133,6 +4133,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_SGX_ATTRIBUTE:
#endif
case KVM_CAP_VM_COPY_ENC_CONTEXT_FROM:
+ case KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM:
case KVM_CAP_SREGS2:
case KVM_CAP_EXIT_ON_EMULATION_FAILURE:
case KVM_CAP_VCPU_ATTRIBUTES:
--
2.27.0
Ensure that the ASID are freed promptly, which becomes more important
when more tests are added to this file.
Cc: Peter Gonda <[email protected]>
Cc: Sean Christopherson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
index a66b9be30239..0cd7e2eaa895 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
@@ -149,6 +149,8 @@ static void test_sev_migrate_locking(void)
for (i = 0; i < NR_LOCK_TESTING_THREADS; ++i)
pthread_join(pt[i], NULL);
+ for (i = 0; i < NR_LOCK_TESTING_THREADS; ++i)
+ kvm_vm_free(input[i].vm);
}
static void test_sev_migrate_parameters(void)
@@ -165,7 +167,6 @@ static void test_sev_migrate_parameters(void)
sev_ioctl(sev_es_vm_no_vmsa->fd, KVM_SEV_ES_INIT, NULL);
vm_vcpu_add(sev_es_vm_no_vmsa, 1);
-
ret = __sev_migrate_from(sev_vm->fd, sev_es_vm->fd);
TEST_ASSERT(
ret == -1 && errno == EINVAL,
@@ -194,6 +195,12 @@ static void test_sev_migrate_parameters(void)
TEST_ASSERT(ret == -1 && errno == EINVAL,
"Migrations require SEV enabled. ret %d, errno: %d\n", ret,
errno);
+
+ kvm_vm_free(sev_vm);
+ kvm_vm_free(sev_es_vm);
+ kvm_vm_free(sev_es_vm_no_vmsa);
+ kvm_vm_free(vm_no_vcpu);
+ kvm_vm_free(vm_no_sev);
}
int main(int argc, char *argv[])
--
2.27.0
Allow intra-host migration of a mirror VM; the destination VM will be
a mirror of the same ASID as the source.
Fixes: b56639318bb2 ("KVM: SEV: Add support for SEV intra host migration")
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm/sev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index dc974c1728b6..c1eb1c83401d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1616,11 +1616,13 @@ static void sev_migrate_from(struct kvm_sev_info *dst,
dst->asid = src->asid;
dst->handle = src->handle;
dst->pages_locked = src->pages_locked;
+ dst->enc_context_owner = src->enc_context_owner;
src->asid = 0;
src->active = false;
src->handle = 0;
src->pages_locked = 0;
+ src->enc_context_owner = NULL;
list_cut_before(&dst->regions_list, &src->regions_list, &src->regions_list);
}
--
2.27.0
Taking the lock is useless since there are no other references,
and there are already accesses (e.g. to sev->enc_context_owner)
that do not take it. So get rid of it.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm/sev.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 89a716290fac..bbbf980c7e40 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2048,8 +2048,6 @@ void sev_vm_destroy(struct kvm *kvm)
return;
}
- mutex_lock(&kvm->lock);
-
/*
* Ensure that all guest tagged cache entries are flushed before
* releasing the pages back to the system for use. CLFLUSH will
@@ -2069,8 +2067,6 @@ void sev_vm_destroy(struct kvm *kvm)
}
}
- mutex_unlock(&kvm->lock);
-
sev_unbind_asid(kvm, sev->handle);
sev_asid_free(sev);
}
--
2.27.0
I am putting the tests in sev_migrate_tests because the failure conditions are
very similar and some of the setup code can be reused, too.
The tests cover both successful creation of a mirror VM, and error
conditions.
Cc: Peter Gonda <[email protected]>
Cc: Sean Christopherson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
.../selftests/kvm/x86_64/sev_migrate_tests.c | 112 ++++++++++++++++--
1 file changed, 105 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
index 0cd7e2eaa895..d265cea5de85 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
@@ -54,12 +54,15 @@ static struct kvm_vm *sev_vm_create(bool es)
return vm;
}
-static struct kvm_vm *__vm_create(void)
+static struct kvm_vm *aux_vm_create(bool with_vcpus)
{
struct kvm_vm *vm;
int i;
vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
+ if (!with_vcpus)
+ return vm;
+
for (i = 0; i < NR_MIGRATE_TEST_VCPUS; ++i)
vm_vcpu_add(vm, i);
@@ -93,7 +96,7 @@ static void test_sev_migrate_from(bool es)
src_vm = sev_vm_create(es);
for (i = 0; i < NR_MIGRATE_TEST_VMS; ++i)
- dst_vms[i] = __vm_create();
+ dst_vms[i] = aux_vm_create(true);
/* Initial migration from the src to the first dst. */
sev_migrate_from(dst_vms[0]->fd, src_vm->fd);
@@ -162,7 +165,7 @@ static void test_sev_migrate_parameters(void)
sev_vm = sev_vm_create(/* es= */ false);
sev_es_vm = sev_vm_create(/* es= */ true);
vm_no_vcpu = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
- vm_no_sev = __vm_create();
+ vm_no_sev = aux_vm_create(true);
sev_es_vm_no_vmsa = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
sev_ioctl(sev_es_vm_no_vmsa->fd, KVM_SEV_ES_INIT, NULL);
vm_vcpu_add(sev_es_vm_no_vmsa, 1);
@@ -203,11 +206,106 @@ static void test_sev_migrate_parameters(void)
kvm_vm_free(vm_no_sev);
}
+static int __sev_mirror_create(int dst_fd, int src_fd)
+{
+ struct kvm_enable_cap cap = {
+ .cap = KVM_CAP_VM_COPY_ENC_CONTEXT_FROM,
+ .args = { src_fd }
+ };
+
+ return ioctl(dst_fd, KVM_ENABLE_CAP, &cap);
+}
+
+
+static void sev_mirror_create(int dst_fd, int src_fd)
+{
+ int ret;
+
+ ret = __sev_mirror_create(dst_fd, src_fd);
+ TEST_ASSERT(!ret, "Copying context failed, ret: %d, errno: %d\n", ret, errno);
+}
+
+static void test_sev_mirror(bool es)
+{
+ struct kvm_vm *src_vm, *dst_vm;
+ struct kvm_sev_launch_start start = {
+ .policy = es ? SEV_POLICY_ES : 0
+ };
+ int i;
+
+ src_vm = sev_vm_create(es);
+ dst_vm = aux_vm_create(false);
+
+ sev_mirror_create(dst_vm->fd, src_vm->fd);
+
+ /* Check that we can complete creation of the mirror VM. */
+ for (i = 0; i < NR_MIGRATE_TEST_VCPUS; ++i)
+ vm_vcpu_add(dst_vm, i);
+ sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_START, &start);
+ if (es)
+ sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
+
+ kvm_vm_free(src_vm);
+ kvm_vm_free(dst_vm);
+}
+
+static void test_sev_mirror_parameters(void)
+{
+ struct kvm_vm *sev_vm, *sev_es_vm, *vm_no_vcpu, *vm_with_vcpu;
+ int ret;
+
+ sev_vm = sev_vm_create(/* es= */ false);
+ sev_es_vm = sev_vm_create(/* es= */ true);
+ vm_with_vcpu = aux_vm_create(true);
+ vm_no_vcpu = aux_vm_create(false);
+
+ ret = __sev_mirror_create(sev_vm->fd, sev_vm->fd);
+ TEST_ASSERT(
+ ret == -1 && errno == EINVAL,
+ "Should not be able copy context to self. ret: %d, errno: %d\n",
+ ret, errno);
+
+ ret = __sev_mirror_create(sev_vm->fd, sev_es_vm->fd);
+ TEST_ASSERT(
+ ret == -1 && errno == EINVAL,
+ "Should not be able copy context to SEV enabled VM. ret: %d, errno: %d\n",
+ ret, errno);
+
+ ret = __sev_mirror_create(sev_es_vm->fd, sev_vm->fd);
+ TEST_ASSERT(
+ ret == -1 && errno == EINVAL,
+ "Should not be able copy context to SEV-ES enabled VM. ret: %d, errno: %d\n",
+ ret, errno);
+
+ ret = __sev_mirror_create(vm_no_vcpu->fd, vm_with_vcpu->fd);
+ TEST_ASSERT(ret == -1 && errno == EINVAL,
+ "Copy context requires SEV enabled. ret %d, errno: %d\n", ret,
+ errno);
+
+ ret = __sev_mirror_create(vm_with_vcpu->fd, sev_vm->fd);
+ TEST_ASSERT(
+ ret == -1 && errno == EINVAL,
+ "SEV copy context requires no vCPUS on the destination. ret: %d, errno: %d\n",
+ ret, errno);
+
+ kvm_vm_free(sev_vm);
+ kvm_vm_free(sev_es_vm);
+ kvm_vm_free(vm_with_vcpu);
+ kvm_vm_free(vm_no_vcpu);
+}
+
int main(int argc, char *argv[])
{
- test_sev_migrate_from(/* es= */ false);
- test_sev_migrate_from(/* es= */ true);
- test_sev_migrate_locking();
- test_sev_migrate_parameters();
+ if (kvm_check_cap(KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM)) {
+ test_sev_migrate_from(/* es= */ false);
+ test_sev_migrate_from(/* es= */ true);
+ test_sev_migrate_locking();
+ test_sev_migrate_parameters();
+ }
+ if (kvm_check_cap(KVM_CAP_VM_COPY_ENC_CONTEXT_FROM)) {
+ test_sev_mirror(/* es= */ false);
+ test_sev_mirror(/* es= */ true);
+ test_sev_mirror_parameters();
+ }
return 0;
}
--
2.27.0
Generally, kvm->lock is not taken for a long time, but
sev_lock_two_vms is different: it takes vCPU locks
inside, so userspace can hold it back just by calling
a vCPU ioctl. Play it safe and use mutex_lock_killable.
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm/sev.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index bbbf980c7e40..59727a966f90 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1547,6 +1547,7 @@ static int sev_lock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm)
{
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;
+ int r = -EBUSY;
if (dst_kvm == src_kvm)
return -EINVAL;
@@ -1558,14 +1559,23 @@ static int sev_lock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm)
if (atomic_cmpxchg_acquire(&dst_sev->migration_in_progress, 0, 1))
return -EBUSY;
- if (atomic_cmpxchg_acquire(&src_sev->migration_in_progress, 0, 1)) {
- atomic_set_release(&dst_sev->migration_in_progress, 0);
- return -EBUSY;
- }
+ if (atomic_cmpxchg_acquire(&src_sev->migration_in_progress, 0, 1))
+ goto release_dst;
- mutex_lock(&dst_kvm->lock);
- mutex_lock(&src_kvm->lock);
+ r = -EINTR;
+ if (mutex_lock_killable(&dst_kvm->lock))
+ goto release_src;
+ if (mutex_lock_killable(&src_kvm->lock))
+ goto unlock_dst;
return 0;
+
+unlock_dst:
+ mutex_unlock(&dst_kvm->lock);
+release_src:
+ atomic_set_release(&src_sev->migration_in_progress, 0);
+release_dst:
+ atomic_set_release(&dst_sev->migration_in_progress, 0);
+ return r;
}
static void sev_unlock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm)
--
2.27.0
Now that we have a facility to lock two VMs with deadlock
protection, use it for the creation of mirror VMs as well. One of
COPY_ENC_CONTEXT_FROM(dst, src) and COPY_ENC_CONTEXT_FROM(src, dst)
would always fail, so the combination is nonsensical and it is okay to
return -EBUSY if it is attempted.
This sidesteps the question of what happens if a VM is
MOVE_ENC_CONTEXT_FROM'd at the same time as it is
COPY_ENC_CONTEXT_FROM'd: the locking prevents that from
happening.
Cc: Peter Gonda <[email protected]>
Cc: Sean Christopherson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm/sev.c | 69 +++++++++++++++++-------------------------
1 file changed, 27 insertions(+), 42 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c1eb1c83401d..025d9731b66c 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1548,6 +1548,9 @@ static int sev_lock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm)
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;
+ if (dst_kvm == src_kvm)
+ return -EINVAL;
+
/*
* Bail if these VMs are already involved in a migration to avoid
* deadlock between two VMs trying to migrate to/from each other.
@@ -1952,77 +1955,59 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
{
struct file *source_kvm_file;
struct kvm *source_kvm;
- struct kvm_sev_info source_sev, *mirror_sev;
+ struct kvm_sev_info *source_sev, *mirror_sev;
int ret;
source_kvm_file = fget(source_fd);
if (!file_is_kvm(source_kvm_file)) {
ret = -EBADF;
- goto e_source_put;
+ goto e_source_fput;
}
source_kvm = source_kvm_file->private_data;
- mutex_lock(&source_kvm->lock);
-
- if (!sev_guest(source_kvm)) {
- ret = -EINVAL;
- goto e_source_unlock;
- }
+ ret = sev_lock_two_vms(kvm, source_kvm);
+ if (ret)
+ goto e_source_fput;
- /* Mirrors of mirrors should work, but let's not get silly */
- if (is_mirroring_enc_context(source_kvm) || source_kvm == kvm) {
+ /*
+ * Mirrors of mirrors should work, but let's not get silly. Also
+ * disallow out-of-band SEV/SEV-ES init if the target is already an
+ * SEV guest, or if vCPUs have been created. KVM relies on vCPUs being
+ * created after SEV/SEV-ES initialization, e.g. to init intercepts.
+ */
+ if (sev_guest(kvm) || !sev_guest(source_kvm) ||
+ is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {
ret = -EINVAL;
- goto e_source_unlock;
+ goto e_unlock;
}
- memcpy(&source_sev, &to_kvm_svm(source_kvm)->sev_info,
- sizeof(source_sev));
-
/*
* The mirror kvm holds an enc_context_owner ref so its asid can't
* disappear until we're done with it
*/
+ source_sev = &to_kvm_svm(source_kvm)->sev_info;
kvm_get_kvm(source_kvm);
- fput(source_kvm_file);
- mutex_unlock(&source_kvm->lock);
- mutex_lock(&kvm->lock);
-
- /*
- * Disallow out-of-band SEV/SEV-ES init if the target is already an
- * SEV guest, or if vCPUs have been created. KVM relies on vCPUs being
- * created after SEV/SEV-ES initialization, e.g. to init intercepts.
- */
- if (sev_guest(kvm) || kvm->created_vcpus) {
- ret = -EINVAL;
- goto e_mirror_unlock;
- }
-
/* Set enc_context_owner and copy its encryption context over */
mirror_sev = &to_kvm_svm(kvm)->sev_info;
mirror_sev->enc_context_owner = source_kvm;
mirror_sev->active = true;
- mirror_sev->asid = source_sev.asid;
- mirror_sev->fd = source_sev.fd;
- mirror_sev->es_active = source_sev.es_active;
- mirror_sev->handle = source_sev.handle;
+ mirror_sev->asid = source_sev->asid;
+ mirror_sev->fd = source_sev->fd;
+ mirror_sev->es_active = source_sev->es_active;
+ mirror_sev->handle = source_sev->handle;
INIT_LIST_HEAD(&mirror_sev->regions_list);
+ ret = 0;
+
/*
* Do not copy ap_jump_table. Since the mirror does not share the same
* KVM contexts as the original, and they may have different
* memory-views.
*/
- mutex_unlock(&kvm->lock);
- return 0;
-
-e_mirror_unlock:
- mutex_unlock(&kvm->lock);
- kvm_put_kvm(source_kvm);
- return ret;
-e_source_unlock:
- mutex_unlock(&source_kvm->lock);
-e_source_put:
+e_unlock:
+ sev_unlock_two_vms(kvm, source_kvm);
+e_source_fput:
if (source_kvm_file)
fput(source_kvm_file);
return ret;
--
2.27.0
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 <[email protected]>
Cc: Sean Christopherson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
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.
*/
- 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;
}
--
2.27.0
VMs that mirror an encryption context rely on the owner to keep the
ASID allocated. Performing a KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
would cause a dangling ASID:
1. copy context from A to B (gets ref to A)
2. move context from A to L (moves ASID from A to L)
3. close L (releases ASID from L, B still references it)
The right way to do the handoff instead is to create a fresh mirror VM
on the destination first:
1. copy context from A to B (gets ref to A)
[later] 2. close B (releases ref to A)
3. move context from A to L (moves ASID from A to L)
4. copy context from L to M
So, catch the situation by adding a count of how many VMs are
mirroring this one's encryption context.
Fixes: 0b020f5af092 ("KVM: SEV: Add support for SEV-ES intra host migration")
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/svm/sev.c | 22 ++++++++++-
arch/x86/kvm/svm/svm.h | 1 +
.../selftests/kvm/x86_64/sev_migrate_tests.c | 37 +++++++++++++++++++
3 files changed, 59 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 025d9731b66c..89a716290fac 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1696,6 +1696,16 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd)
}
src_sev = &to_kvm_svm(source_kvm)->sev_info;
+
+ /*
+ * VMs mirroring src's encryption context rely on it to keep the
+ * ASID allocated, but below we are clearing src_sev->asid.
+ */
+ if (src_sev->num_mirrored_vms) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+
dst_sev->misc_cg = get_current_misc_cg();
cg_cleanup_sev = dst_sev;
if (dst_sev->misc_cg != src_sev->misc_cg) {
@@ -1987,6 +1997,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
*/
source_sev = &to_kvm_svm(source_kvm)->sev_info;
kvm_get_kvm(source_kvm);
+ source_sev->num_mirrored_vms++;
/* Set enc_context_owner and copy its encryption context over */
mirror_sev = &to_kvm_svm(kvm)->sev_info;
@@ -2019,12 +2030,21 @@ void sev_vm_destroy(struct kvm *kvm)
struct list_head *head = &sev->regions_list;
struct list_head *pos, *q;
+ WARN_ON(sev->num_mirrored_vms);
+
if (!sev_guest(kvm))
return;
/* If this is a mirror_kvm release the enc_context_owner and skip sev cleanup */
if (is_mirroring_enc_context(kvm)) {
- kvm_put_kvm(sev->enc_context_owner);
+ struct kvm *owner_kvm = sev->enc_context_owner;
+ struct kvm_sev_info *owner_sev = &to_kvm_svm(owner_kvm)->sev_info;
+
+ mutex_lock(&owner_kvm->lock);
+ if (!WARN_ON(!owner_sev->num_mirrored_vms))
+ owner_sev->num_mirrored_vms--;
+ mutex_unlock(&owner_kvm->lock);
+ kvm_put_kvm(owner_kvm);
return;
}
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 5faad3dc10e2..1c7306c370fa 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -79,6 +79,7 @@ struct kvm_sev_info {
struct list_head regions_list; /* List of registered regions */
u64 ap_jump_table; /* SEV-ES AP Jump Table address */
struct kvm *enc_context_owner; /* Owner of copied encryption context */
+ unsigned long num_mirrored_vms; /* Number of VMs sharing this ASID */
struct misc_cg *misc_cg; /* For misc cgroup accounting */
atomic_t migration_in_progress;
};
diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
index d265cea5de85..29b18d565cf4 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
@@ -294,6 +294,41 @@ static void test_sev_mirror_parameters(void)
kvm_vm_free(vm_no_vcpu);
}
+static void test_sev_move_copy(void)
+{
+ struct kvm_vm *dst_vm, *sev_vm, *mirror_vm, *dst_mirror_vm;
+ int ret;
+
+ sev_vm = sev_vm_create(/* es= */ false);
+ dst_vm = aux_vm_create(true);
+ mirror_vm = aux_vm_create(false);
+ dst_mirror_vm = aux_vm_create(false);
+
+ sev_mirror_create(mirror_vm->fd, sev_vm->fd);
+ ret = __sev_migrate_from(dst_vm->fd, sev_vm->fd);
+ TEST_ASSERT(ret == -1 && errno == EBUSY,
+ "Cannot migrate VM that has mirrors. ret %d, errno: %d\n", ret,
+ errno);
+
+ /* The mirror itself can be migrated. */
+ sev_migrate_from(dst_mirror_vm->fd, mirror_vm->fd);
+ ret = __sev_migrate_from(dst_vm->fd, sev_vm->fd);
+ TEST_ASSERT(ret == -1 && errno == EBUSY,
+ "Cannot migrate VM that has mirrors. ret %d, errno: %d\n", ret,
+ errno);
+
+ /*
+ * mirror_vm is not a mirror anymore, dst_mirror_vm is. Thus,
+ * the owner can be copied as soon as dst_mirror_vm is gone.
+ */
+ kvm_vm_free(dst_mirror_vm);
+ sev_migrate_from(dst_vm->fd, sev_vm->fd);
+
+ kvm_vm_free(mirror_vm);
+ kvm_vm_free(dst_vm);
+ kvm_vm_free(sev_vm);
+}
+
int main(int argc, char *argv[])
{
if (kvm_check_cap(KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM)) {
@@ -301,6 +336,8 @@ int main(int argc, char *argv[])
test_sev_migrate_from(/* es= */ true);
test_sev_migrate_locking();
test_sev_migrate_parameters();
+ if (kvm_check_cap(KVM_CAP_VM_COPY_ENC_CONTEXT_FROM))
+ test_sev_move_copy();
}
if (kvm_check_cap(KVM_CAP_VM_COPY_ENC_CONTEXT_FROM)) {
test_sev_mirror(/* es= */ false);
--
2.27.0
On Mon, Nov 22, 2021, Paolo Bonzini wrote:
> list_replace_init cannot be used if the source is an empty list,
> because "new->next->prev = new" will overwrite "old->next":
>
> new old
> prev = new, next = new prev = old, next = old
> new->next = old->next prev = new, next = old prev = old, next = old
> new->next->prev = new prev = new, next = old prev = old, next = new
> new->prev = old->prev prev = old, next = old prev = old, next = old
> new->next->prev = new prev = old, next = old prev = new, next = new
>
> The desired outcome instead would be to leave both old and new the same
> as they were (two empty circular lists). Use list_cut_before, which
> already has the necessary check and is documented to discard the
> previous contents of the list that will hold the result.
>
> Fixes: b56639318bb2 ("KVM: SEV: Add support for SEV intra host migration")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 21ac0a5de4e0..75955beb3770 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1613,8 +1613,7 @@ static void sev_migrate_from(struct kvm_sev_info *dst,
> src->handle = 0;
> src->pages_locked = 0;
>
> - INIT_LIST_HEAD(&dst->regions_list);
> - list_replace_init(&src->regions_list, &dst->regions_list);
> + list_cut_before(&dst->regions_list, &src->regions_list, &src->regions_list);
Yeesh, that is tricky. A list_move_all() helper in list.h to do
list_cut_before(dst, src, src);
would be nice.
Reviewed-by: Sean Christopherson <[email protected]>
> }
>
> static int sev_es_migrate_from(struct kvm *dst, struct kvm *src)
> --
> 2.27.0
>
>
On Mon, Nov 22, 2021, Paolo Bonzini wrote:
> The capability, albeit present, was never exposed via KVM_CHECK_EXTENSION.
>
> Fixes: b56639318bb2 ("KVM: SEV: Add support for SEV intra host migration")
> Cc: Peter Gonda <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
Reviewed-by: Sean Christopherson <[email protected]>
On Mon, Nov 22, 2021, Paolo Bonzini wrote:
> Taking the lock is useless since there are no other references,
> and there are already accesses (e.g. to sev->enc_context_owner)
> that do not take it. So get rid of it.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
Reviewed-by: Sean Christopherson <[email protected]>
On Mon, Nov 22, 2021, Paolo Bonzini wrote:
> VMs that mirror an encryption context rely on the owner to keep the
> ASID allocated. Performing a KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
> would cause a dangling ASID:
>
> 1. copy context from A to B (gets ref to A)
> 2. move context from A to L (moves ASID from A to L)
> 3. close L (releases ASID from L, B still references it)
>
> The right way to do the handoff instead is to create a fresh mirror VM
> on the destination first:
>
> 1. copy context from A to B (gets ref to A)
> [later] 2. close B (releases ref to A)
> 3. move context from A to L (moves ASID from A to L)
> 4. copy context from L to M
>
> So, catch the situation by adding a count of how many VMs are
> mirroring this one's encryption context.
>
> Fixes: 0b020f5af092 ("KVM: SEV: Add support for SEV-ES intra host migration")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 22 ++++++++++-
> arch/x86/kvm/svm/svm.h | 1 +
> .../selftests/kvm/x86_64/sev_migrate_tests.c | 37 +++++++++++++++++++
> 3 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 025d9731b66c..89a716290fac 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1696,6 +1696,16 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd)
> }
>
> src_sev = &to_kvm_svm(source_kvm)->sev_info;
> +
> + /*
> + * VMs mirroring src's encryption context rely on it to keep the
> + * ASID allocated, but below we are clearing src_sev->asid.
Prefer something to explan why this is disallowed instead of simply saying the
src's ASID is cleared/released, e.g.
/*
* Disallow migrating a VM with active mirrors, as the mirrors rely on
* the VM to keep the ASID allocated. Transferring all mirrors to dst
* would require locking all mirrors, and there's no known use case for
* intra-host migration of a VM with mirrors.
*/
> + */
> + if (src_sev->num_mirrored_vms) {
> + ret = -EBUSY;
> + goto out_unlock;
> + }
> +
> dst_sev->misc_cg = get_current_misc_cg();
> cg_cleanup_sev = dst_sev;
> if (dst_sev->misc_cg != src_sev->misc_cg) {
> @@ -1987,6 +1997,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
> */
> source_sev = &to_kvm_svm(source_kvm)->sev_info;
> kvm_get_kvm(source_kvm);
> + source_sev->num_mirrored_vms++;
>
> /* Set enc_context_owner and copy its encryption context over */
> mirror_sev = &to_kvm_svm(kvm)->sev_info;
> @@ -2019,12 +2030,21 @@ void sev_vm_destroy(struct kvm *kvm)
> struct list_head *head = &sev->regions_list;
> struct list_head *pos, *q;
>
> + WARN_ON(sev->num_mirrored_vms);
> +
> if (!sev_guest(kvm))
> return;
>
> /* If this is a mirror_kvm release the enc_context_owner and skip sev cleanup */
> if (is_mirroring_enc_context(kvm)) {
> - kvm_put_kvm(sev->enc_context_owner);
> + struct kvm *owner_kvm = sev->enc_context_owner;
> + struct kvm_sev_info *owner_sev = &to_kvm_svm(owner_kvm)->sev_info;
> +
> + mutex_lock(&owner_kvm->lock);
> + if (!WARN_ON(!owner_sev->num_mirrored_vms))
Why not make num_mirrored_vms an atomic_t so that the destruction path doesn't
need to take owner_kvm->lock? The asymmetry is a bit odd, but this feels worse
in its own way.
And since this is effectively a refcount, I almost wonder if it would make sense
to do:
if (!refcount_inc_not_zero(&source_sev->num_mirrored_vms)) {
refcount_set(&source_sev->num_mirrored_vms, 1);
kvm_get_kvm(source_kvm);
}
and then
if (refcount_dec_and_test(&source_sev->num_mirrored_vms))
kvm_put_kvm(owner_kvm);
to "officially" refcount something.
> + owner_sev->num_mirrored_vms--;
> + mutex_unlock(&owner_kvm->lock);
> + kvm_put_kvm(owner_kvm);
> return;
> }
>
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5faad3dc10e2..1c7306c370fa 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -79,6 +79,7 @@ struct kvm_sev_info {
> struct list_head regions_list; /* List of registered regions */
> u64 ap_jump_table; /* SEV-ES AP Jump Table address */
> struct kvm *enc_context_owner; /* Owner of copied encryption context */
> + unsigned long num_mirrored_vms; /* Number of VMs sharing this ASID */
Unsigned long is odd. It's guaranteed to be u64 since SEV is 64-bit only. If
it really is possible to overflow a refcount_t/int, than u64 or atomic64_t seems
more appropriate.
> struct misc_cg *misc_cg; /* For misc cgroup accounting */
> atomic_t migration_in_progress;
> };
On Mon, Nov 22, 2021, Paolo Bonzini wrote:
> This was broken before the introduction of KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM,
> but technically harmless because the region list was unused for a mirror
> VM. However, it is untidy and it now causes a NULL pointer access when
> attempting to move the encryption context of a mirror VM.
>
> Fixes: 54526d1fd593 ("KVM: x86: Support KVM VMs sharing SEV context")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 23a4877d7bdf..dc974c1728b6 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2004,6 +2004,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
> mirror_sev->fd = source_sev.fd;
> mirror_sev->es_active = source_sev.es_active;
> mirror_sev->handle = source_sev.handle;
> + INIT_LIST_HEAD(&mirror_sev->regions_list);
Heh, I still think the list should be initialized when the VM is created.
On Wed, Feb 24, 2021 at 9:37 AM Sean Christopherson <[email protected]> wrote:
> > + ? ? mutex_unlock(&kvm->lock);
> > + ? ? mutex_lock(&mirror_kvm->lock);
> > +
> > + ? ? /* Set enc_context_owner and copy its encryption context over */
> > + ? ? mirror_kvm_sev = &to_kvm_svm(mirror_kvm)->sev_info;
> > + ? ? mirror_kvm_sev->enc_context_owner = kvm;
> > + ? ? mirror_kvm_sev->asid = asid;
> > + ? ? mirror_kvm_sev->active = true;
>
> I would prefer a prep patch to move "INIT_LIST_HEAD(&sev->regions_list);" from
> sev_guest_init() to when the VM is instantiated. ?Shaving a few cycles in that
> flow is meaningless, and not initializing the list of regions is odd, and will
> cause problems if mirrors are allowed to pin memory (or do PSP commands).
On Mon, Nov 22, 2021, Paolo Bonzini wrote:
> Allow intra-host migration of a mirror VM; the destination VM will be
> a mirror of the same ASID as the source.
>
> Fixes: b56639318bb2 ("KVM: SEV: Add support for SEV intra host migration")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index dc974c1728b6..c1eb1c83401d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1616,11 +1616,13 @@ static void sev_migrate_from(struct kvm_sev_info *dst,
> dst->asid = src->asid;
> dst->handle = src->handle;
> dst->pages_locked = src->pages_locked;
> + dst->enc_context_owner = src->enc_context_owner;
>
> src->asid = 0;
> src->active = false;
> src->handle = 0;
> src->pages_locked = 0;
> + src->enc_context_owner = NULL;
Ah, since this is effectively transferring the kvm_put_kvm() "owner", I definitely
think a refcount on the number of mirrors is the way to go.
Reviewed-by: Sean Christopherson <[email protected]>
On Mon, Nov 22, 2021, Paolo Bonzini wrote:
> Now that we have a facility to lock two VMs with deadlock
> protection, use it for the creation of mirror VMs as well. One of
> COPY_ENC_CONTEXT_FROM(dst, src) and COPY_ENC_CONTEXT_FROM(src, dst)
> would always fail, so the combination is nonsensical and it is okay to
> return -EBUSY if it is attempted.
>
> This sidesteps the question of what happens if a VM is
> MOVE_ENC_CONTEXT_FROM'd at the same time as it is
> COPY_ENC_CONTEXT_FROM'd: the locking prevents that from
> happening.
>
> Cc: Peter Gonda <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 69 +++++++++++++++++-------------------------
> 1 file changed, 27 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c1eb1c83401d..025d9731b66c 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1548,6 +1548,9 @@ static int sev_lock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm)
> 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;
>
> + if (dst_kvm == src_kvm)
> + return -EINVAL;
This should go into
KVM: SEV: cleanup locking for KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
because before that, KVM would never attempt mutex_lock() on the second VM, one
of the SEV || !SEV was guaranteed to fail.
With that change,
Reviewed-by: Sean Christopherson <[email protected]>
On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <[email protected]> wrote:
>
> KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM leaves the source VM in a dead state,
> so migrating back to the original source VM fails the ioctl. Adjust
> the test.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
Reviewed-by: Peter Gonda <[email protected]>
On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <[email protected]> wrote:
>
> Ensure that the ASID are freed promptly, which becomes more important
> when more tests are added to this file.
>
> Cc: Peter Gonda <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
Reviewed-by: Peter Gonda <[email protected]>
On Mon, Nov 29, 2021 at 3:28 PM Sean Christopherson <[email protected]> wrote:
>
> On Mon, Nov 22, 2021, Paolo Bonzini wrote:
> > The capability, albeit present, was never exposed via KVM_CHECK_EXTENSION.
> >
> > Fixes: b56639318bb2 ("KVM: SEV: Add support for SEV intra host migration")
> > Cc: Peter Gonda <[email protected]>
> > Signed-off-by: Paolo Bonzini <[email protected]>
> > ---
>
> Reviewed-by: Sean Christopherson <[email protected]>
Reviewed-by: Peter Gonda <[email protected]>
On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <[email protected]> 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 <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
Reviewed-by: Peter Gonda <[email protected]>
> ---
> 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
>
>
On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <[email protected]> wrote:
>
> I am putting the tests in sev_migrate_tests because the failure conditions are
> very similar and some of the setup code can be reused, too.
>
> The tests cover both successful creation of a mirror VM, and error
> conditions.
>
> Cc: Peter Gonda <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> .../selftests/kvm/x86_64/sev_migrate_tests.c | 112 ++++++++++++++++--
> 1 file changed, 105 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> index 0cd7e2eaa895..d265cea5de85 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> @@ -54,12 +54,15 @@ static struct kvm_vm *sev_vm_create(bool es)
> return vm;
> }
>
> -static struct kvm_vm *__vm_create(void)
> +static struct kvm_vm *aux_vm_create(bool with_vcpus)
> {
> struct kvm_vm *vm;
> int i;
>
> vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
> + if (!with_vcpus)
> + return vm;
> +
> for (i = 0; i < NR_MIGRATE_TEST_VCPUS; ++i)
> vm_vcpu_add(vm, i);
>
> @@ -93,7 +96,7 @@ static void test_sev_migrate_from(bool es)
>
> src_vm = sev_vm_create(es);
> for (i = 0; i < NR_MIGRATE_TEST_VMS; ++i)
> - dst_vms[i] = __vm_create();
> + dst_vms[i] = aux_vm_create(true);
>
> /* Initial migration from the src to the first dst. */
> sev_migrate_from(dst_vms[0]->fd, src_vm->fd);
> @@ -162,7 +165,7 @@ static void test_sev_migrate_parameters(void)
> sev_vm = sev_vm_create(/* es= */ false);
> sev_es_vm = sev_vm_create(/* es= */ true);
> vm_no_vcpu = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
> - vm_no_sev = __vm_create();
> + vm_no_sev = aux_vm_create(true);
> sev_es_vm_no_vmsa = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
> sev_ioctl(sev_es_vm_no_vmsa->fd, KVM_SEV_ES_INIT, NULL);
> vm_vcpu_add(sev_es_vm_no_vmsa, 1);
> @@ -203,11 +206,106 @@ static void test_sev_migrate_parameters(void)
> kvm_vm_free(vm_no_sev);
> }
>
> +static int __sev_mirror_create(int dst_fd, int src_fd)
> +{
> + struct kvm_enable_cap cap = {
> + .cap = KVM_CAP_VM_COPY_ENC_CONTEXT_FROM,
> + .args = { src_fd }
> + };
> +
> + return ioctl(dst_fd, KVM_ENABLE_CAP, &cap);
> +}
> +
> +
> +static void sev_mirror_create(int dst_fd, int src_fd)
> +{
> + int ret;
> +
> + ret = __sev_mirror_create(dst_fd, src_fd);
> + TEST_ASSERT(!ret, "Copying context failed, ret: %d, errno: %d\n", ret, errno);
> +}
> +
> +static void test_sev_mirror(bool es)
> +{
> + struct kvm_vm *src_vm, *dst_vm;
> + struct kvm_sev_launch_start start = {
> + .policy = es ? SEV_POLICY_ES : 0
> + };
> + int i;
> +
> + src_vm = sev_vm_create(es);
> + dst_vm = aux_vm_create(false);
> +
> + sev_mirror_create(dst_vm->fd, src_vm->fd);
> +
> + /* Check that we can complete creation of the mirror VM. */
> + for (i = 0; i < NR_MIGRATE_TEST_VCPUS; ++i)
> + vm_vcpu_add(dst_vm, i);
> + sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_START, &start);
I don't think this should be called on the mirror and I think it
should be an error.
In is_cmd_allowed_from_mirror() KVM_SEV_LAUNCH_START should not be allowed:
if (cmd_id == KVM_SEV_LAUNCH_UPDATE_VMSA ||
cmd_id == KVM_SEV_GUEST_STATUS || cmd_id == KVM_SEV_DBG_DECRYPT ||
cmd_id == KVM_SEV_DBG_ENCRYPT)
return true;
This overrides the mirrored values and sets up the VM as a new SEV
context. I would have thought the sev_bind_asid() in
sev_launch_start() would fail because the asid is already used by the
source.
> + if (es)
> + sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
> +
> + kvm_vm_free(src_vm);
> + kvm_vm_free(dst_vm);
> +}
> +
> +static void test_sev_mirror_parameters(void)
> +{
> + struct kvm_vm *sev_vm, *sev_es_vm, *vm_no_vcpu, *vm_with_vcpu;
> + int ret;
> +
> + sev_vm = sev_vm_create(/* es= */ false);
> + sev_es_vm = sev_vm_create(/* es= */ true);
> + vm_with_vcpu = aux_vm_create(true);
> + vm_no_vcpu = aux_vm_create(false);
> +
> + ret = __sev_mirror_create(sev_vm->fd, sev_vm->fd);
> + TEST_ASSERT(
> + ret == -1 && errno == EINVAL,
> + "Should not be able copy context to self. ret: %d, errno: %d\n",
> + ret, errno);
> +
> + ret = __sev_mirror_create(sev_vm->fd, sev_es_vm->fd);
> + TEST_ASSERT(
> + ret == -1 && errno == EINVAL,
> + "Should not be able copy context to SEV enabled VM. ret: %d, errno: %d\n",
> + ret, errno);
> +
> + ret = __sev_mirror_create(sev_es_vm->fd, sev_vm->fd);
> + TEST_ASSERT(
> + ret == -1 && errno == EINVAL,
> + "Should not be able copy context to SEV-ES enabled VM. ret: %d, errno: %d\n",
> + ret, errno);
> +
> + ret = __sev_mirror_create(vm_no_vcpu->fd, vm_with_vcpu->fd);
> + TEST_ASSERT(ret == -1 && errno == EINVAL,
> + "Copy context requires SEV enabled. ret %d, errno: %d\n", ret,
> + errno);
> +
> + ret = __sev_mirror_create(vm_with_vcpu->fd, sev_vm->fd);
> + TEST_ASSERT(
> + ret == -1 && errno == EINVAL,
> + "SEV copy context requires no vCPUS on the destination. ret: %d, errno: %d\n",
> + ret, errno);
> +
> + kvm_vm_free(sev_vm);
> + kvm_vm_free(sev_es_vm);
> + kvm_vm_free(vm_with_vcpu);
> + kvm_vm_free(vm_no_vcpu);
> +}
> +
> int main(int argc, char *argv[])
> {
> - test_sev_migrate_from(/* es= */ false);
> - test_sev_migrate_from(/* es= */ true);
> - test_sev_migrate_locking();
> - test_sev_migrate_parameters();
> + if (kvm_check_cap(KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM)) {
> + test_sev_migrate_from(/* es= */ false);
> + test_sev_migrate_from(/* es= */ true);
> + test_sev_migrate_locking();
> + test_sev_migrate_parameters();
> + }
> + if (kvm_check_cap(KVM_CAP_VM_COPY_ENC_CONTEXT_FROM)) {
> + test_sev_mirror(/* es= */ false);
> + test_sev_mirror(/* es= */ true);
> + test_sev_mirror_parameters();
> + }
> return 0;
> }
> --
> 2.27.0
>
>
On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <[email protected]> wrote:
>
> VMs that mirror an encryption context rely on the owner to keep the
> ASID allocated. Performing a KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
> would cause a dangling ASID:
>
> 1. copy context from A to B (gets ref to A)
> 2. move context from A to L (moves ASID from A to L)
> 3. close L (releases ASID from L, B still references it)
>
> The right way to do the handoff instead is to create a fresh mirror VM
> on the destination first:
>
> 1. copy context from A to B (gets ref to A)
> [later] 2. close B (releases ref to A)
> 3. move context from A to L (moves ASID from A to L)
> 4. copy context from L to M
>
> So, catch the situation by adding a count of how many VMs are
> mirroring this one's encryption context.
>
> Fixes: 0b020f5af092 ("KVM: SEV: Add support for SEV-ES intra host migration")
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 22 ++++++++++-
> arch/x86/kvm/svm/svm.h | 1 +
> .../selftests/kvm/x86_64/sev_migrate_tests.c | 37 +++++++++++++++++++
> 3 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 025d9731b66c..89a716290fac 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1696,6 +1696,16 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd)
> }
>
> src_sev = &to_kvm_svm(source_kvm)->sev_info;
> +
> + /*
> + * VMs mirroring src's encryption context rely on it to keep the
> + * ASID allocated, but below we are clearing src_sev->asid.
> + */
> + if (src_sev->num_mirrored_vms) {
> + ret = -EBUSY;
> + goto out_unlock;
> + }
> +
> dst_sev->misc_cg = get_current_misc_cg();
> cg_cleanup_sev = dst_sev;
> if (dst_sev->misc_cg != src_sev->misc_cg) {
> @@ -1987,6 +1997,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
> */
> source_sev = &to_kvm_svm(source_kvm)->sev_info;
> kvm_get_kvm(source_kvm);
> + source_sev->num_mirrored_vms++;
>
> /* Set enc_context_owner and copy its encryption context over */
> mirror_sev = &to_kvm_svm(kvm)->sev_info;
> @@ -2019,12 +2030,21 @@ void sev_vm_destroy(struct kvm *kvm)
> struct list_head *head = &sev->regions_list;
> struct list_head *pos, *q;
>
> + WARN_ON(sev->num_mirrored_vms);
> +
If we don't change to atomic doesn't this need to happen when we have
the kvm->lock?
> if (!sev_guest(kvm))
> return;
>
> /* If this is a mirror_kvm release the enc_context_owner and skip sev cleanup */
> if (is_mirroring_enc_context(kvm)) {
> - kvm_put_kvm(sev->enc_context_owner);
> + struct kvm *owner_kvm = sev->enc_context_owner;
> + struct kvm_sev_info *owner_sev = &to_kvm_svm(owner_kvm)->sev_info;
> +
> + mutex_lock(&owner_kvm->lock);
> + if (!WARN_ON(!owner_sev->num_mirrored_vms))
> + owner_sev->num_mirrored_vms--;
> + mutex_unlock(&owner_kvm->lock);
> + kvm_put_kvm(owner_kvm);
> return;
> }
>
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5faad3dc10e2..1c7306c370fa 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -79,6 +79,7 @@ struct kvm_sev_info {
> struct list_head regions_list; /* List of registered regions */
> u64 ap_jump_table; /* SEV-ES AP Jump Table address */
> struct kvm *enc_context_owner; /* Owner of copied encryption context */
> + unsigned long num_mirrored_vms; /* Number of VMs sharing this ASID */
> struct misc_cg *misc_cg; /* For misc cgroup accounting */
> atomic_t migration_in_progress;
> };
> diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> index d265cea5de85..29b18d565cf4 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> @@ -294,6 +294,41 @@ static void test_sev_mirror_parameters(void)
> kvm_vm_free(vm_no_vcpu);
> }
>
> +static void test_sev_move_copy(void)
> +{
> + struct kvm_vm *dst_vm, *sev_vm, *mirror_vm, *dst_mirror_vm;
> + int ret;
> +
> + sev_vm = sev_vm_create(/* es= */ false);
> + dst_vm = aux_vm_create(true);
> + mirror_vm = aux_vm_create(false);
> + dst_mirror_vm = aux_vm_create(false);
> +
> + sev_mirror_create(mirror_vm->fd, sev_vm->fd);
> + ret = __sev_migrate_from(dst_vm->fd, sev_vm->fd);
> + TEST_ASSERT(ret == -1 && errno == EBUSY,
> + "Cannot migrate VM that has mirrors. ret %d, errno: %d\n", ret,
> + errno);
> +
> + /* The mirror itself can be migrated. */
> + sev_migrate_from(dst_mirror_vm->fd, mirror_vm->fd);
> + ret = __sev_migrate_from(dst_vm->fd, sev_vm->fd);
> + TEST_ASSERT(ret == -1 && errno == EBUSY,
> + "Cannot migrate VM that has mirrors. ret %d, errno: %d\n", ret,
> + errno);
> +
> + /*
> + * mirror_vm is not a mirror anymore, dst_mirror_vm is. Thus,
> + * the owner can be copied as soon as dst_mirror_vm is gone.
> + */
> + kvm_vm_free(dst_mirror_vm);
> + sev_migrate_from(dst_vm->fd, sev_vm->fd);
> +
> + kvm_vm_free(mirror_vm);
> + kvm_vm_free(dst_vm);
> + kvm_vm_free(sev_vm);
> +}
> +
> int main(int argc, char *argv[])
> {
> if (kvm_check_cap(KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM)) {
> @@ -301,6 +336,8 @@ int main(int argc, char *argv[])
> test_sev_migrate_from(/* es= */ true);
> test_sev_migrate_locking();
> test_sev_migrate_parameters();
> + if (kvm_check_cap(KVM_CAP_VM_COPY_ENC_CONTEXT_FROM))
> + test_sev_move_copy();
> }
> if (kvm_check_cap(KVM_CAP_VM_COPY_ENC_CONTEXT_FROM)) {
> test_sev_mirror(/* es= */ false);
> --
> 2.27.0
>
>
On 12/1/21 19:17, Peter Gonda wrote:
> On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <[email protected]> wrote:
>>
>> VMs that mirror an encryption context rely on the owner to keep the
>> ASID allocated. Performing a KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
>> would cause a dangling ASID:
>>
>> 1. copy context from A to B (gets ref to A)
>> 2. move context from A to L (moves ASID from A to L)
>> 3. close L (releases ASID from L, B still references it)
>>
>> The right way to do the handoff instead is to create a fresh mirror VM
>> on the destination first:
>>
>> 1. copy context from A to B (gets ref to A)
>> [later] 2. close B (releases ref to A)
>> 3. move context from A to L (moves ASID from A to L)
>> 4. copy context from L to M
>>
>> So, catch the situation by adding a count of how many VMs are
>> mirroring this one's encryption context.
>>
>> Fixes: 0b020f5af092 ("KVM: SEV: Add support for SEV-ES intra host migration")
>> Signed-off-by: Paolo Bonzini <[email protected]>
>> ---
>> arch/x86/kvm/svm/sev.c | 22 ++++++++++-
>> arch/x86/kvm/svm/svm.h | 1 +
>> .../selftests/kvm/x86_64/sev_migrate_tests.c | 37 +++++++++++++++++++
>> 3 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 025d9731b66c..89a716290fac 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -1696,6 +1696,16 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd)
>> }
>>
>> src_sev = &to_kvm_svm(source_kvm)->sev_info;
>> +
>> + /*
>> + * VMs mirroring src's encryption context rely on it to keep the
>> + * ASID allocated, but below we are clearing src_sev->asid.
>> + */
>> + if (src_sev->num_mirrored_vms) {
>> + ret = -EBUSY;
>> + goto out_unlock;
>> + }
>> +
>> dst_sev->misc_cg = get_current_misc_cg();
>> cg_cleanup_sev = dst_sev;
>> if (dst_sev->misc_cg != src_sev->misc_cg) {
>> @@ -1987,6 +1997,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
>> */
>> source_sev = &to_kvm_svm(source_kvm)->sev_info;
>> kvm_get_kvm(source_kvm);
>> + source_sev->num_mirrored_vms++;
>>
>> /* Set enc_context_owner and copy its encryption context over */
>> mirror_sev = &to_kvm_svm(kvm)->sev_info;
>> @@ -2019,12 +2030,21 @@ void sev_vm_destroy(struct kvm *kvm)
>> struct list_head *head = &sev->regions_list;
>> struct list_head *pos, *q;
>>
>> + WARN_ON(sev->num_mirrored_vms);
>> +
>
> If we don't change to atomic doesn't this need to happen when we have
> the kvm->lock?
Hi,
there can be no concurrency when destroying a VM. (If you do Rust, it's
similar to using x.get_mut().get_mut() on an Arc<Mutex<T>>).
Paolo
On Wed, Dec 1, 2021 at 11:09 AM Peter Gonda <[email protected]> wrote:
>
> On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <[email protected]> wrote:
> >
> > I am putting the tests in sev_migrate_tests because the failure conditions are
> > very similar and some of the setup code can be reused, too.
> >
> > The tests cover both successful creation of a mirror VM, and error
> > conditions.
> >
> > Cc: Peter Gonda <[email protected]>
> > Cc: Sean Christopherson <[email protected]>
> > Signed-off-by: Paolo Bonzini <[email protected]>
> > ---
> > .../selftests/kvm/x86_64/sev_migrate_tests.c | 112 ++++++++++++++++--
> > 1 file changed, 105 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> > index 0cd7e2eaa895..d265cea5de85 100644
> > --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> > +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> > @@ -54,12 +54,15 @@ static struct kvm_vm *sev_vm_create(bool es)
> > return vm;
> > }
> >
> > -static struct kvm_vm *__vm_create(void)
> > +static struct kvm_vm *aux_vm_create(bool with_vcpus)
> > {
> > struct kvm_vm *vm;
> > int i;
> >
> > vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
> > + if (!with_vcpus)
> > + return vm;
> > +
> > for (i = 0; i < NR_MIGRATE_TEST_VCPUS; ++i)
> > vm_vcpu_add(vm, i);
> >
> > @@ -93,7 +96,7 @@ static void test_sev_migrate_from(bool es)
> >
> > src_vm = sev_vm_create(es);
> > for (i = 0; i < NR_MIGRATE_TEST_VMS; ++i)
> > - dst_vms[i] = __vm_create();
> > + dst_vms[i] = aux_vm_create(true);
> >
> > /* Initial migration from the src to the first dst. */
> > sev_migrate_from(dst_vms[0]->fd, src_vm->fd);
> > @@ -162,7 +165,7 @@ static void test_sev_migrate_parameters(void)
> > sev_vm = sev_vm_create(/* es= */ false);
> > sev_es_vm = sev_vm_create(/* es= */ true);
> > vm_no_vcpu = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
> > - vm_no_sev = __vm_create();
> > + vm_no_sev = aux_vm_create(true);
> > sev_es_vm_no_vmsa = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
> > sev_ioctl(sev_es_vm_no_vmsa->fd, KVM_SEV_ES_INIT, NULL);
> > vm_vcpu_add(sev_es_vm_no_vmsa, 1);
> > @@ -203,11 +206,106 @@ static void test_sev_migrate_parameters(void)
> > kvm_vm_free(vm_no_sev);
> > }
> >
> > +static int __sev_mirror_create(int dst_fd, int src_fd)
> > +{
> > + struct kvm_enable_cap cap = {
> > + .cap = KVM_CAP_VM_COPY_ENC_CONTEXT_FROM,
> > + .args = { src_fd }
> > + };
> > +
> > + return ioctl(dst_fd, KVM_ENABLE_CAP, &cap);
> > +}
> > +
> > +
> > +static void sev_mirror_create(int dst_fd, int src_fd)
> > +{
> > + int ret;
> > +
> > + ret = __sev_mirror_create(dst_fd, src_fd);
> > + TEST_ASSERT(!ret, "Copying context failed, ret: %d, errno: %d\n", ret, errno);
> > +}
> > +
> > +static void test_sev_mirror(bool es)
> > +{
> > + struct kvm_vm *src_vm, *dst_vm;
> > + struct kvm_sev_launch_start start = {
> > + .policy = es ? SEV_POLICY_ES : 0
> > + };
> > + int i;
> > +
> > + src_vm = sev_vm_create(es);
> > + dst_vm = aux_vm_create(false);
> > +
> > + sev_mirror_create(dst_vm->fd, src_vm->fd);
> > +
> > + /* Check that we can complete creation of the mirror VM. */
> > + for (i = 0; i < NR_MIGRATE_TEST_VCPUS; ++i)
> > + vm_vcpu_add(dst_vm, i);
> > + sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_START, &start);
>
> I don't think this should be called on the mirror and I think it
> should be an error.
>
> In is_cmd_allowed_from_mirror() KVM_SEV_LAUNCH_START should not be allowed:
>
> if (cmd_id == KVM_SEV_LAUNCH_UPDATE_VMSA ||
> cmd_id == KVM_SEV_GUEST_STATUS || cmd_id == KVM_SEV_DBG_DECRYPT ||
> cmd_id == KVM_SEV_DBG_ENCRYPT)
> return true;
>
> This overrides the mirrored values and sets up the VM as a new SEV
> context. I would have thought the sev_bind_asid() in
> sev_launch_start() would fail because the asid is already used by the
> source.
Since you already queue'd this I sent another patch to fix the issue
with sev_ioctl() and remove this call.
>
> > + if (es)
> > + sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
> > +
> > + kvm_vm_free(src_vm);
> > + kvm_vm_free(dst_vm);
> > +}
> > +
> > +static void test_sev_mirror_parameters(void)
> > +{
> > + struct kvm_vm *sev_vm, *sev_es_vm, *vm_no_vcpu, *vm_with_vcpu;
> > + int ret;
> > +
> > + sev_vm = sev_vm_create(/* es= */ false);
> > + sev_es_vm = sev_vm_create(/* es= */ true);
> > + vm_with_vcpu = aux_vm_create(true);
> > + vm_no_vcpu = aux_vm_create(false);
> > +
> > + ret = __sev_mirror_create(sev_vm->fd, sev_vm->fd);
> > + TEST_ASSERT(
> > + ret == -1 && errno == EINVAL,
> > + "Should not be able copy context to self. ret: %d, errno: %d\n",
> > + ret, errno);
> > +
> > + ret = __sev_mirror_create(sev_vm->fd, sev_es_vm->fd);
> > + TEST_ASSERT(
> > + ret == -1 && errno == EINVAL,
> > + "Should not be able copy context to SEV enabled VM. ret: %d, errno: %d\n",
> > + ret, errno);
> > +
> > + ret = __sev_mirror_create(sev_es_vm->fd, sev_vm->fd);
> > + TEST_ASSERT(
> > + ret == -1 && errno == EINVAL,
> > + "Should not be able copy context to SEV-ES enabled VM. ret: %d, errno: %d\n",
> > + ret, errno);
> > +
> > + ret = __sev_mirror_create(vm_no_vcpu->fd, vm_with_vcpu->fd);
> > + TEST_ASSERT(ret == -1 && errno == EINVAL,
> > + "Copy context requires SEV enabled. ret %d, errno: %d\n", ret,
> > + errno);
> > +
> > + ret = __sev_mirror_create(vm_with_vcpu->fd, sev_vm->fd);
> > + TEST_ASSERT(
> > + ret == -1 && errno == EINVAL,
> > + "SEV copy context requires no vCPUS on the destination. ret: %d, errno: %d\n",
> > + ret, errno);
> > +
> > + kvm_vm_free(sev_vm);
> > + kvm_vm_free(sev_es_vm);
> > + kvm_vm_free(vm_with_vcpu);
> > + kvm_vm_free(vm_no_vcpu);
> > +}
> > +
> > int main(int argc, char *argv[])
> > {
> > - test_sev_migrate_from(/* es= */ false);
> > - test_sev_migrate_from(/* es= */ true);
> > - test_sev_migrate_locking();
> > - test_sev_migrate_parameters();
> > + if (kvm_check_cap(KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM)) {
> > + test_sev_migrate_from(/* es= */ false);
> > + test_sev_migrate_from(/* es= */ true);
> > + test_sev_migrate_locking();
> > + test_sev_migrate_parameters();
> > + }
> > + if (kvm_check_cap(KVM_CAP_VM_COPY_ENC_CONTEXT_FROM)) {
> > + test_sev_mirror(/* es= */ false);
> > + test_sev_mirror(/* es= */ true);
> > + test_sev_mirror_parameters();
> > + }
> > return 0;
> > }
> > --
> > 2.27.0
> >
> >