[PATCH 0/4] MOVE/COPY_ENC_CONTEXT_FROM locking cleanup and tests

[PATCH 0/4] MOVE/COPY_ENC_CONTEXT_FROM locking cleanup and tests

[Paolo Bonzini]

Patches 1 and 2 are the long-awaited tests for COPY_ENC_CONTEXT_FROM,
based on the ones for intra-host migration.  The aim of patches 3
and 4 is to simplify the locking for COPY_ENC_CONTEXT_FROM, and solving
(by sidestepping the question) the problem of a VM's encryption
context being moved from and copied from at the same time.

These patches are an alternative to Sean's patch with subject "KVM:
SEV: Explicitly document that there are no TOCTOU races in copy ASID"
(https://lore.kernel.org/kvm/76c7c752-f1b0-f100-03dd-364366eff02f@redhat.com/T/).

There is another bug: a VM that is the owner of a copied context must not
be migrated, otherwise you could have 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

I'll take a look at this later, probably next week after this series has
been reviewed.

Paolo


Paolo Bonzini (4):
  selftests: sev_migrate_tests: free all VMs
  selftests: sev_migrate_tests: add tests for
    KVM_CAP_VM_COPY_ENC_CONTEXT_FROM
  KVM: SEV: cleanup locking for KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
  KVM: SEV: Do COPY_ENC_CONTEXT_FROM with both VMs locked

 arch/x86/kvm/svm/sev.c                        | 118 ++++++++----------
 .../selftests/kvm/x86_64/sev_migrate_tests.c  | 113 +++++++++++++++--
 2 files changed, 155 insertions(+), 76 deletions(-)

-- 
2.27.0

[PATCH 1/4] selftests: sev_migrate_tests: free all VMs

[Paolo Bonzini]

Ensure that the ASID are freed promptly, which becomes more important
when more tests are added to this file.

Cc: Peter Gonda <pgonda@google.com>
Cc: Sean Christopherson <seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 7 ++++++-
 1 file changed, 6 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 5ba325cd64bf..4a5d3728412b 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
@@ -162,7 +162,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,
@@ -191,6 +190,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

Re: [PATCH 1/4] selftests: sev_migrate_tests: free all VMs

[Peter Gonda]

I think we are still missing the kvm_vm_free() from
test_sev_migrate_locking(). Should we have this at the end?

for (i = 0; i < NR_LOCK_TESTING_THREADS; ++i)
    kvm_vm_free(input[i].vm);


On Wed, Nov 17, 2021 at 9:38 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Ensure that the ASID are freed promptly, which becomes more important
> when more tests are added to this file.
>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 7 ++++++-
>  1 file changed, 6 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 5ba325cd64bf..4a5d3728412b 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> @@ -162,7 +162,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,
> @@ -191,6 +190,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
>
>

Re: [PATCH 1/4] selftests: sev_migrate_tests: free all VMs

[Paolo Bonzini]

On 11/17/21 17:52, Peter Gonda wrote:
> I think we are still missing the kvm_vm_free() from
> test_sev_migrate_locking(). Should we have this at the end?
> 
> for (i = 0; i < NR_LOCK_TESTING_THREADS; ++i)
>      kvm_vm_free(input[i].vm);

Yes, we should.  Thanks!

Paolo

[PATCH 2/4] selftests: sev_migrate_tests: add tests for KVM_CAP_VM_COPY_ENC_CONTEXT_FROM

[Paolo Bonzini]

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 <pgonda@google.com>
Cc: Sean Christopherson <seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .../selftests/kvm/x86_64/sev_migrate_tests.c  | 106 ++++++++++++++++--
 1 file changed, 99 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 4a5d3728412b..986dc2ede61d 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);
@@ -157,7 +160,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);
@@ -198,11 +201,100 @@ 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, "Migration 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_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

Re: [PATCH 2/4] selftests: sev_migrate_tests: add tests for KVM_CAP_VM_COPY_ENC_CONTEXT_FROM

[Peter Gonda]

On Wed, Nov 17, 2021 at 9:38 AM Paolo Bonzini <pbonzini@redhat.com> 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 <pgonda@google.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  .../selftests/kvm/x86_64/sev_migrate_tests.c  | 106 ++++++++++++++++--
>  1 file changed, 99 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 4a5d3728412b..986dc2ede61d 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);
> @@ -157,7 +160,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);
> @@ -198,11 +201,100 @@ 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, "Migration failed, ret: %d, errno: %d\n", ret, errno);

Should this read "Mirroring failed..." or something?

> +}
> +
> +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);

Style question. I realized I didn't do this myself but should there
always be blank line after these conditionals/loops without {}s? Tom
had me add them to work in ccp driver, unsure if that should be
maintained everywhere.

> +       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_es_vm->fd);
> +       TEST_ASSERT(
> +               ret == -1 && errno == EINVAL,
> +               "Should not be able copy context to SEV enabled VM. ret: %d, errno: %d\n",

"Should not be able *to* copy ..." (Yea I think the exiting error is
missing the 'to')

> +               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",

"Should not be able *to* copy ..."

> +               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
>
>

Re: [PATCH 2/4] selftests: sev_migrate_tests: add tests for KVM_CAP_VM_COPY_ENC_CONTEXT_FROM

[Sean Christopherson]

On Wed, Nov 17, 2021, Peter Gonda wrote:
> On Wed, Nov 17, 2021 at 9:38 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > +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);
> 
> Style question. I realized I didn't do this myself but should there
> always be blank line after these conditionals/loops without {}s? Tom
> had me add them to work in ccp driver, unsure if that should be
> maintained everywhere.

Generally speaking, yes.  There will inevitably be exceptions where it's ok to
omit a blank line, e.g. kvm_ioapic_clear_all() is a decent example, as is
kvm_update_dr0123().  But for something like this where the for-loop is a different
"block" than the following code, a blank line is preferred.

[PATCH 3/4] KVM: SEV: cleanup locking for KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM

[Paolo Bonzini]

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(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 21ac0a5de4e0..f9256ba269e6 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);
 }
 
 
@@ -1666,15 +1675,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;
@@ -1682,13 +1682,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;
@@ -1728,13 +1728,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

Re: [PATCH 3/4] KVM: SEV: cleanup locking for KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM

[Peter Gonda]

On Wed, Nov 17, 2021 at 9:38 AM 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>

Acked-by: Peter Gonda <pgonda@google.com>

I like the cleanup thanks Paolo.

[PATCH 4/4] KVM: SEV: Do COPY_ENC_CONTEXT_FROM with both VMs locked

[Paolo Bonzini]

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 <pgonda@google.com>
Cc: Sean Christopherson <seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/sev.c | 68 ++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f9256ba269e6..47d54df7675c 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.
@@ -1951,76 +1954,57 @@ 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;
+	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

Re: [PATCH 4/4] KVM: SEV: Do COPY_ENC_CONTEXT_FROM with both VMs locked

[Peter Gonda]

On Wed, Nov 17, 2021 at 9:38 AM Paolo Bonzini <pbonzini@redhat.com> 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 <pgonda@google.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Peter Gonda <pgonda@google.com>

> ---
>  arch/x86/kvm/svm/sev.c | 68 ++++++++++++++++--------------------------
>  1 file changed, 26 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f9256ba269e6..47d54df7675c 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;
> +

Worth adding a migrate/mirror from self fails tests in
test_sev_(migrate|mirror)_parameters()? I guess it's already covered
by "the source cannot be SEV enabled" test cases.

>         /*
>          * Bail if these VMs are already involved in a migration to avoid
>          * deadlock between two VMs trying to migrate to/from each other.
> @@ -1951,76 +1954,57 @@ 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;
> +       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
>

Re: [PATCH 4/4] KVM: SEV: Do COPY_ENC_CONTEXT_FROM with both VMs locked

[Paolo Bonzini]

On 11/17/21 18:46, Peter Gonda wrote:
>> +       if (dst_kvm == src_kvm)
>> +               return -EINVAL;
>> +
> Worth adding a migrate/mirror from self fails tests in
> test_sev_(migrate|mirror)_parameters()? I guess it's already covered
> by "the source cannot be SEV enabled" test cases.
> 

It was covered by the locking test (which does not check i != j). 
There's no equivalent for the operation of creating a mirror VM, I can 
add it in v2.

Paolo