[PATCH 0/3] Fixes for SEV mirror VM tests

[PATCH 0/3] Fixes for SEV mirror VM tests

[Peter Gonda]

Updated patch series for fixing bug in sev_ioctl() which allowed test
to look like a mirror vm could call KVM_SEV_LAUNCH_START. Adds
additional testing to validate mirror vm can only call subset of
commands.

I could not add the patch Seanjc recommended due to issues with
sev_platform_init() not correctly setting the fw error. I'll work ontop
of the INIT_EX patch series to fix this issue with the PSP driver.

Peter Gonda (3):
  selftests: sev_migrate_tests: Fix test_sev_mirror()
  selftests: sev_migrate_tests: Fix sev_ioctl()
  selftests: sev_migrate_tests: Add mirror command tests

 .../selftests/kvm/x86_64/sev_migrate_tests.c  | 59 ++++++++++++++++---
 1 file changed, 52 insertions(+), 7 deletions(-)

-- 
2.34.1.400.ga245620fadb-goog

[PATCH 1/3] selftests: sev_migrate_tests: Fix test_sev_mirror()

[Peter Gonda]

Mirrors should not be able to call LAUNCH_START. Remove the call on the
mirror to correct the test before fixing sev_ioctl() to correctly assert
on this failed ioctl.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Marc Orr <marcorr@google.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
---
 tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 5 +----
 1 file changed, 1 insertion(+), 4 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 29b18d565cf4..fbc742b42145 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
@@ -228,9 +228,6 @@ static void sev_mirror_create(int dst_fd, int src_fd)
 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);
@@ -241,7 +238,7 @@ static void test_sev_mirror(bool es)
 	/* 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);
 
-- 
2.34.1.400.ga245620fadb-goog

[PATCH 2/3] selftests: sev_migrate_tests: Fix sev_ioctl()

[Peter Gonda]

TEST_ASSERT in SEV ioctl was allowing errors because it checked return
value was good OR the FW error code was OK. This TEST_ASSERT should
require both (aka. AND) values are OK. Removes the LAUNCH_START from the
mirror VM because this call correctly fails because mirror VMs cannot
call this command. Currently issues with the PSP driver functions mean
the firmware error is not always reset to SEV_RET_SUCCESS when a call is
successful. Mainly sev_platform_init() doesn't correctly set the fw
error if the platform has already been initialized.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Marc Orr <marcorr@google.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
---
 tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 3 ++-
 1 file changed, 2 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 fbc742b42145..4bb960ca6486 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
@@ -30,8 +30,9 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data)
 	};
 	int ret;
 
+
 	ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd);
-	TEST_ASSERT((ret == 0 || cmd.error == SEV_RET_SUCCESS),
+	TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS,
 		    "%d failed: return code: %d, errno: %d, fw error: %d",
 		    cmd_id, ret, errno, cmd.error);
 }
-- 
2.34.1.400.ga245620fadb-goog

[PATCH 3/3] selftests: sev_migrate_tests: Add mirror command tests

[Peter Gonda]

Add tests to confirm mirror vms can only run correct subset of commands.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Marc Orr <marcorr@google.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
---
 .../selftests/kvm/x86_64/sev_migrate_tests.c  | 55 +++++++++++++++++--
 1 file changed, 51 insertions(+), 4 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 4bb960ca6486..80056bbbb003 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
@@ -21,7 +21,7 @@
 #define NR_LOCK_TESTING_THREADS 3
 #define NR_LOCK_TESTING_ITERATIONS 10000
 
-static void sev_ioctl(int vm_fd, int cmd_id, void *data)
+static int __sev_ioctl(int vm_fd, int cmd_id, void *data, __u32 *fw_error)
 {
 	struct kvm_sev_cmd cmd = {
 		.id = cmd_id,
@@ -30,11 +30,20 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data)
 	};
 	int ret;
 
-
 	ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd);
-	TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS,
+	*fw_error = cmd.error;
+	return ret;
+}
+
+static void sev_ioctl(int vm_fd, int cmd_id, void *data)
+{
+	int ret;
+	__u32 fw_error;
+
+	ret = __sev_ioctl(vm_fd, cmd_id, data, &fw_error);
+	TEST_ASSERT(ret == 0 && fw_error == SEV_RET_SUCCESS,
 		    "%d failed: return code: %d, errno: %d, fw error: %d",
-		    cmd_id, ret, errno, cmd.error);
+		    cmd_id, ret, errno, fw_error);
 }
 
 static struct kvm_vm *sev_vm_create(bool es)
@@ -226,6 +235,42 @@ static void sev_mirror_create(int dst_fd, int src_fd)
 	TEST_ASSERT(!ret, "Copying context failed, ret: %d, errno: %d\n", ret, errno);
 }
 
+static void verify_mirror_allowed_cmds(int vm_fd)
+{
+	struct kvm_sev_guest_status status;
+
+	for (int cmd_id = KVM_SEV_INIT; cmd_id < KVM_SEV_NR_MAX; ++cmd_id) {
+		int ret;
+		__u32 fw_error;
+
+		/*
+		 * These commands are allowed for mirror VMs, all others are
+		 * not.
+		 */
+		switch (cmd_id) {
+		case KVM_SEV_LAUNCH_UPDATE_VMSA:
+		case KVM_SEV_GUEST_STATUS:
+		case KVM_SEV_DBG_DECRYPT:
+		case KVM_SEV_DBG_ENCRYPT:
+			continue;
+		default:
+			break;
+		}
+
+		/*
+		 * These commands should be disallowed before the data
+		 * parameter is examined so NULL is OK here.
+		 */
+		ret = __sev_ioctl(vm_fd, cmd_id, NULL, &fw_error);
+		TEST_ASSERT(
+			ret == -1 && errno == EINVAL,
+			"Should not be able call command: %d. ret: %d, errno: %d\n",
+			cmd_id, ret, errno);
+	}
+
+	sev_ioctl(vm_fd, KVM_SEV_GUEST_STATUS, &status);
+}
+
 static void test_sev_mirror(bool es)
 {
 	struct kvm_vm *src_vm, *dst_vm;
@@ -243,6 +288,8 @@ static void test_sev_mirror(bool es)
 	if (es)
 		sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
 
+	verify_mirror_allowed_cmds(dst_vm->fd);
+
 	kvm_vm_free(src_vm);
 	kvm_vm_free(dst_vm);
 }
-- 
2.34.1.400.ga245620fadb-goog

Re: [PATCH 1/3] selftests: sev_migrate_tests: Fix test_sev_mirror()

[Marc Orr]

On Wed, Dec 8, 2021 at 11:16 AM Peter Gonda <pgonda@google.com> wrote:
>
> Mirrors should not be able to call LAUNCH_START. Remove the call on the
> mirror to correct the test before fixing sev_ioctl() to correctly assert
> on this failed ioctl.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Marc Orr <marcorr@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> ---
>  tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 5 +----
>  1 file changed, 1 insertion(+), 4 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 29b18d565cf4..fbc742b42145 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> @@ -228,9 +228,6 @@ static void sev_mirror_create(int dst_fd, int src_fd)
>  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);
> @@ -241,7 +238,7 @@ static void test_sev_mirror(bool es)
>         /* 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);
>
> --
> 2.34.1.400.ga245620fadb-goog
>

Reviewed-by: Marc Orr <marcorr@google.com>

Re: [PATCH 2/3] selftests: sev_migrate_tests: Fix sev_ioctl()

[Marc Orr]

On Wed, Dec 8, 2021 at 11:16 AM Peter Gonda <pgonda@google.com> wrote:
>
> TEST_ASSERT in SEV ioctl was allowing errors because it checked return
> value was good OR the FW error code was OK. This TEST_ASSERT should
> require both (aka. AND) values are OK. Removes the LAUNCH_START from the
> mirror VM because this call correctly fails because mirror VMs cannot
> call this command. Currently issues with the PSP driver functions mean

This commit description is now stale. The previous patch removes the
LAUNCH_START -- not this patch.

> the firmware error is not always reset to SEV_RET_SUCCESS when a call is
> successful. Mainly sev_platform_init() doesn't correctly set the fw
> error if the platform has already been initialized.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Marc Orr <marcorr@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> ---
>  tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 3 ++-
>  1 file changed, 2 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 fbc742b42145..4bb960ca6486 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> @@ -30,8 +30,9 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data)
>         };
>         int ret;
>
> +

nit: Looks like you picked up an extra new line. Since you need to
fixup the commit description, let's fix this up too.

>         ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd);
> -       TEST_ASSERT((ret == 0 || cmd.error == SEV_RET_SUCCESS),
> +       TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS,
>                     "%d failed: return code: %d, errno: %d, fw error: %d",
>                     cmd_id, ret, errno, cmd.error);
>  }
> --
> 2.34.1.400.ga245620fadb-goog
>

Re: [PATCH 3/3] selftests: sev_migrate_tests: Add mirror command tests

[Marc Orr]

On Wed, Dec 8, 2021 at 11:16 AM Peter Gonda <pgonda@google.com> wrote:
>
> Add tests to confirm mirror vms can only run correct subset of commands.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Marc Orr <marcorr@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> ---
>  .../selftests/kvm/x86_64/sev_migrate_tests.c  | 55 +++++++++++++++++--
>  1 file changed, 51 insertions(+), 4 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 4bb960ca6486..80056bbbb003 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> @@ -21,7 +21,7 @@
>  #define NR_LOCK_TESTING_THREADS 3
>  #define NR_LOCK_TESTING_ITERATIONS 10000
>
> -static void sev_ioctl(int vm_fd, int cmd_id, void *data)
> +static int __sev_ioctl(int vm_fd, int cmd_id, void *data, __u32 *fw_error)
>  {
>         struct kvm_sev_cmd cmd = {
>                 .id = cmd_id,
> @@ -30,11 +30,20 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data)
>         };
>         int ret;
>
> -
>         ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd);
> -       TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS,
> +       *fw_error = cmd.error;
> +       return ret;
> +}
> +
> +static void sev_ioctl(int vm_fd, int cmd_id, void *data)
> +{
> +       int ret;
> +       __u32 fw_error;
> +
> +       ret = __sev_ioctl(vm_fd, cmd_id, data, &fw_error);
> +       TEST_ASSERT(ret == 0 && fw_error == SEV_RET_SUCCESS,
>                     "%d failed: return code: %d, errno: %d, fw error: %d",
> -                   cmd_id, ret, errno, cmd.error);
> +                   cmd_id, ret, errno, fw_error);
>  }
>
>  static struct kvm_vm *sev_vm_create(bool es)
> @@ -226,6 +235,42 @@ static void sev_mirror_create(int dst_fd, int src_fd)
>         TEST_ASSERT(!ret, "Copying context failed, ret: %d, errno: %d\n", ret, errno);
>  }
>
> +static void verify_mirror_allowed_cmds(int vm_fd)
> +{
> +       struct kvm_sev_guest_status status;
> +
> +       for (int cmd_id = KVM_SEV_INIT; cmd_id < KVM_SEV_NR_MAX; ++cmd_id) {
> +               int ret;
> +               __u32 fw_error;
> +
> +               /*
> +                * These commands are allowed for mirror VMs, all others are
> +                * not.
> +                */
> +               switch (cmd_id) {
> +               case KVM_SEV_LAUNCH_UPDATE_VMSA:
> +               case KVM_SEV_GUEST_STATUS:
> +               case KVM_SEV_DBG_DECRYPT:
> +               case KVM_SEV_DBG_ENCRYPT:
> +                       continue;
> +               default:
> +                       break;
> +               }
> +
> +               /*
> +                * These commands should be disallowed before the data
> +                * parameter is examined so NULL is OK here.
> +                */
> +               ret = __sev_ioctl(vm_fd, cmd_id, NULL, &fw_error);
> +               TEST_ASSERT(
> +                       ret == -1 && errno == EINVAL,
> +                       "Should not be able call command: %d. ret: %d, errno: %d\n",
> +                       cmd_id, ret, errno);
> +       }
> +
> +       sev_ioctl(vm_fd, KVM_SEV_GUEST_STATUS, &status);

Why is this here? I'd either delete it or maybe alternatively move it
into the `case KVM_SEV_GUEST_STATUS` with a corresponding TEST_ASSERT
to check that the command succeeded. Something like:

...
               switch (cmd_id) {
               case KVM_SEV_GUEST_STATUS:
                    sev_ioctl(vm_fd, KVM_SEV_GUEST_STATUS, &status);
                    TEST_ASSERT(ret == 0 && fw_error == SEV_RET_SUCCESS, ...);
                    continue;
               case KVM_SEV_LAUNCH_UPDATE_VMSA:
               case KVM_SEV_DBG_DECRYPT:
               case KVM_SEV_DBG_ENCRYPT:
                       continue;
               default:
                       break;
               }

> +}
> +
>  static void test_sev_mirror(bool es)
>  {
>         struct kvm_vm *src_vm, *dst_vm;
> @@ -243,6 +288,8 @@ static void test_sev_mirror(bool es)
>         if (es)
>                 sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
>
> +       verify_mirror_allowed_cmds(dst_vm->fd);
> +
>         kvm_vm_free(src_vm);
>         kvm_vm_free(dst_vm);
>  }
> --
> 2.34.1.400.ga245620fadb-goog
>

Re: [PATCH 3/3] selftests: sev_migrate_tests: Add mirror command tests

[Paolo Bonzini]

On 12/8/21 20:16, Peter Gonda wrote:
> Add tests to confirm mirror vms can only run correct subset of commands.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Marc Orr <marcorr@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> ---
>   .../selftests/kvm/x86_64/sev_migrate_tests.c  | 55 +++++++++++++++++--
>   1 file changed, 51 insertions(+), 4 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 4bb960ca6486..80056bbbb003 100644
> --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> @@ -21,7 +21,7 @@
>   #define NR_LOCK_TESTING_THREADS 3
>   #define NR_LOCK_TESTING_ITERATIONS 10000
>   
> -static void sev_ioctl(int vm_fd, int cmd_id, void *data)
> +static int __sev_ioctl(int vm_fd, int cmd_id, void *data, __u32 *fw_error)
>   {
>   	struct kvm_sev_cmd cmd = {
>   		.id = cmd_id,
> @@ -30,11 +30,20 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data)
>   	};
>   	int ret;
>   
> -
>   	ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd);
> -	TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS,
> +	*fw_error = cmd.error;
> +	return ret;
> +}
> +
> +static void sev_ioctl(int vm_fd, int cmd_id, void *data)
> +{
> +	int ret;
> +	__u32 fw_error;
> +
> +	ret = __sev_ioctl(vm_fd, cmd_id, data, &fw_error);
> +	TEST_ASSERT(ret == 0 && fw_error == SEV_RET_SUCCESS,
>   		    "%d failed: return code: %d, errno: %d, fw error: %d",
> -		    cmd_id, ret, errno, cmd.error);
> +		    cmd_id, ret, errno, fw_error);
>   }
>   
>   static struct kvm_vm *sev_vm_create(bool es)
> @@ -226,6 +235,42 @@ static void sev_mirror_create(int dst_fd, int src_fd)
>   	TEST_ASSERT(!ret, "Copying context failed, ret: %d, errno: %d\n", ret, errno);
>   }
>   
> +static void verify_mirror_allowed_cmds(int vm_fd)
> +{
> +	struct kvm_sev_guest_status status;
> +
> +	for (int cmd_id = KVM_SEV_INIT; cmd_id < KVM_SEV_NR_MAX; ++cmd_id) {
> +		int ret;
> +		__u32 fw_error;
> +
> +		/*
> +		 * These commands are allowed for mirror VMs, all others are
> +		 * not.
> +		 */
> +		switch (cmd_id) {
> +		case KVM_SEV_LAUNCH_UPDATE_VMSA:
> +		case KVM_SEV_GUEST_STATUS:
> +		case KVM_SEV_DBG_DECRYPT:
> +		case KVM_SEV_DBG_ENCRYPT:
> +			continue;
> +		default:
> +			break;
> +		}
> +
> +		/*
> +		 * These commands should be disallowed before the data
> +		 * parameter is examined so NULL is OK here.
> +		 */
> +		ret = __sev_ioctl(vm_fd, cmd_id, NULL, &fw_error);
> +		TEST_ASSERT(
> +			ret == -1 && errno == EINVAL,
> +			"Should not be able call command: %d. ret: %d, errno: %d\n",
> +			cmd_id, ret, errno);
> +	}
> +
> +	sev_ioctl(vm_fd, KVM_SEV_GUEST_STATUS, &status);
> +}
> +
>   static void test_sev_mirror(bool es)
>   {
>   	struct kvm_vm *src_vm, *dst_vm;
> @@ -243,6 +288,8 @@ static void test_sev_mirror(bool es)
>   	if (es)
>   		sev_ioctl(dst_vm->fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
>   
> +	verify_mirror_allowed_cmds(dst_vm->fd);
> +
>   	kvm_vm_free(src_vm);
>   	kvm_vm_free(dst_vm);
>   }
> 

Queued, thanks.

Paolo

Re: [PATCH 2/3] selftests: sev_migrate_tests: Fix sev_ioctl()

[Peter Gonda]

On Wed, Dec 8, 2021 at 10:45 PM Marc Orr <marcorr@google.com> wrote:
>
> On Wed, Dec 8, 2021 at 11:16 AM Peter Gonda <pgonda@google.com> wrote:
> >
> > TEST_ASSERT in SEV ioctl was allowing errors because it checked return
> > value was good OR the FW error code was OK. This TEST_ASSERT should
> > require both (aka. AND) values are OK. Removes the LAUNCH_START from the
> > mirror VM because this call correctly fails because mirror VMs cannot
> > call this command. Currently issues with the PSP driver functions mean
>
> This commit description is now stale. The previous patch removes the
> LAUNCH_START -- not this patch.
>
> > the firmware error is not always reset to SEV_RET_SUCCESS when a call is
> > successful. Mainly sev_platform_init() doesn't correctly set the fw
> > error if the platform has already been initialized.
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Marc Orr <marcorr@google.com>
> > Signed-off-by: Peter Gonda <pgonda@google.com>
> > ---
> >  tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c | 3 ++-
> >  1 file changed, 2 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 fbc742b42145..4bb960ca6486 100644
> > --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> > +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> > @@ -30,8 +30,9 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data)
> >         };
> >         int ret;
> >
> > +
>
> nit: Looks like you picked up an extra new line. Since you need to
> fixup the commit description, let's fix this up too.

I just sent a V0.1 with these fixes, thanks Marc.

Paolo is that an OK way to handle that, I saw you queued the series?

>
> >         ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd);
> > -       TEST_ASSERT((ret == 0 || cmd.error == SEV_RET_SUCCESS),
> > +       TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS,
> >                     "%d failed: return code: %d, errno: %d, fw error: %d",
> >                     cmd_id, ret, errno, cmd.error);
> >  }
> > --
> > 2.34.1.400.ga245620fadb-goog
> >

Re: [PATCH 3/3] selftests: sev_migrate_tests: Add mirror command tests

[Marc Orr]

On Wed, Dec 8, 2021 at 9:53 PM Marc Orr <marcorr@google.com> wrote:
>
> On Wed, Dec 8, 2021 at 11:16 AM Peter Gonda <pgonda@google.com> wrote:
> >
> > Add tests to confirm mirror vms can only run correct subset of commands.
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Marc Orr <marcorr@google.com>
> > Signed-off-by: Peter Gonda <pgonda@google.com>
> > ---
> >  .../selftests/kvm/x86_64/sev_migrate_tests.c  | 55 +++++++++++++++++--
> >  1 file changed, 51 insertions(+), 4 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 4bb960ca6486..80056bbbb003 100644
> > --- a/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> > +++ b/tools/testing/selftests/kvm/x86_64/sev_migrate_tests.c
> > @@ -21,7 +21,7 @@
> >  #define NR_LOCK_TESTING_THREADS 3
> >  #define NR_LOCK_TESTING_ITERATIONS 10000
> >
> > -static void sev_ioctl(int vm_fd, int cmd_id, void *data)
> > +static int __sev_ioctl(int vm_fd, int cmd_id, void *data, __u32 *fw_error)
> >  {
> >         struct kvm_sev_cmd cmd = {
> >                 .id = cmd_id,
> > @@ -30,11 +30,20 @@ static void sev_ioctl(int vm_fd, int cmd_id, void *data)
> >         };
> >         int ret;
> >
> > -
> >         ret = ioctl(vm_fd, KVM_MEMORY_ENCRYPT_OP, &cmd);
> > -       TEST_ASSERT(ret == 0 && cmd.error == SEV_RET_SUCCESS,
> > +       *fw_error = cmd.error;
> > +       return ret;
> > +}
> > +
> > +static void sev_ioctl(int vm_fd, int cmd_id, void *data)
> > +{
> > +       int ret;
> > +       __u32 fw_error;
> > +
> > +       ret = __sev_ioctl(vm_fd, cmd_id, data, &fw_error);
> > +       TEST_ASSERT(ret == 0 && fw_error == SEV_RET_SUCCESS,
> >                     "%d failed: return code: %d, errno: %d, fw error: %d",
> > -                   cmd_id, ret, errno, cmd.error);
> > +                   cmd_id, ret, errno, fw_error);
> >  }
> >
> >  static struct kvm_vm *sev_vm_create(bool es)
> > @@ -226,6 +235,42 @@ static void sev_mirror_create(int dst_fd, int src_fd)
> >         TEST_ASSERT(!ret, "Copying context failed, ret: %d, errno: %d\n", ret, errno);
> >  }
> >
> > +static void verify_mirror_allowed_cmds(int vm_fd)
> > +{
> > +       struct kvm_sev_guest_status status;
> > +
> > +       for (int cmd_id = KVM_SEV_INIT; cmd_id < KVM_SEV_NR_MAX; ++cmd_id) {
> > +               int ret;
> > +               __u32 fw_error;
> > +
> > +               /*
> > +                * These commands are allowed for mirror VMs, all others are
> > +                * not.
> > +                */
> > +               switch (cmd_id) {
> > +               case KVM_SEV_LAUNCH_UPDATE_VMSA:
> > +               case KVM_SEV_GUEST_STATUS:
> > +               case KVM_SEV_DBG_DECRYPT:
> > +               case KVM_SEV_DBG_ENCRYPT:
> > +                       continue;
> > +               default:
> > +                       break;
> > +               }
> > +
> > +               /*
> > +                * These commands should be disallowed before the data
> > +                * parameter is examined so NULL is OK here.
> > +                */
> > +               ret = __sev_ioctl(vm_fd, cmd_id, NULL, &fw_error);
> > +               TEST_ASSERT(
> > +                       ret == -1 && errno == EINVAL,
> > +                       "Should not be able call command: %d. ret: %d, errno: %d\n",
> > +                       cmd_id, ret, errno);
> > +       }
> > +
> > +       sev_ioctl(vm_fd, KVM_SEV_GUEST_STATUS, &status);
>
> Why is this here? I'd either delete it or maybe alternatively move it
> into the `case KVM_SEV_GUEST_STATUS` with a corresponding TEST_ASSERT
> to check that the command succeeded. Something like:
>
> ...
>                switch (cmd_id) {
>                case KVM_SEV_GUEST_STATUS:
>                     sev_ioctl(vm_fd, KVM_SEV_GUEST_STATUS, &status);
>                     TEST_ASSERT(ret == 0 && fw_error == SEV_RET_SUCCESS, ...);
>                     continue;
>                case KVM_SEV_LAUNCH_UPDATE_VMSA:
>                case KVM_SEV_DBG_DECRYPT:
>                case KVM_SEV_DBG_ENCRYPT:
>                        continue;
>                default:
>                        break;
>                }

For posterity: Peter pointed out to me offline that `sev_ioctl()` in
fact does the TEST_ASSERT internally. Doh! So this line is fine as is.