diff mbox series

[v2,08/13] KVM: selftests: add SEV boot tests

Message ID 20211216171358.61140-9-michael.roth@amd.com
State New
Headers show
Series KVM: selftests: Add tests for SEV and SEV-ES guests | expand

Commit Message

Michael Roth Dec. 16, 2021, 5:13 p.m. UTC
A common aspect of booting SEV guests is checking related CPUID/MSR
bits and accessing shared/private memory. Add a basic test to cover
this.

This test will be expanded to cover basic boot of SEV-ES and SEV-SNP in
subsequent patches.

Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/x86_64/sev_all_boot_test.c  | 255 ++++++++++++++++++
 3 files changed, 257 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c

Comments

Mingwei Zhang Dec. 20, 2021, 1:49 a.m. UTC | #1
On Thu, Dec 16, 2021, Michael Roth wrote:
> A common aspect of booting SEV guests is checking related CPUID/MSR
> bits and accessing shared/private memory. Add a basic test to cover
> this.
>
> This test will be expanded to cover basic boot of SEV-ES and SEV-SNP in
> subsequent patches.
>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../selftests/kvm/x86_64/sev_all_boot_test.c  | 255 ++++++++++++++++++
>  3 files changed, 257 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
>
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 4a801cba9c62..cc73de938a2a 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -43,6 +43,7 @@
>  /x86_64/xen_vmcall_test
>  /x86_64/xss_msr_test
>  /x86_64/vmx_pmu_msrs_test
> +/x86_64/sev_all_boot_test
>  /access_tracking_perf_test
>  /demand_paging_test
>  /dirty_log_test
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index ccc382a827f1..6f250e190fde 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -81,6 +81,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/xen_shinfo_test
>  TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test
>  TEST_GEN_PROGS_x86_64 += x86_64/vmx_pi_mmio_test
>  TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests
> +TEST_GEN_PROGS_x86_64 += x86_64/sev_all_boot_test
>  TEST_GEN_PROGS_x86_64 += demand_paging_test
>  TEST_GEN_PROGS_x86_64 += dirty_log_test
>  TEST_GEN_PROGS_x86_64 += dirty_log_perf_test
> diff --git a/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> new file mode 100644
> index 000000000000..329a740a7cb2
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Basic SEV boot tests.
> + *
> + * Copyright (C) 2021 Advanced Micro Devices
> + */
> +#define _GNU_SOURCE /* for program_invocation_short_name */
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +
> +#include "test_util.h"
> +
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "svm_util.h"
> +#include "linux/psp-sev.h"
> +#include "sev.h"
> +
> +#define VCPU_ID			2
> +#define PAGE_SIZE		4096
> +#define PAGE_STRIDE		32
> +
> +#define SHARED_PAGES		8192
> +#define SHARED_VADDR_MIN	0x1000000
> +
> +#define PRIVATE_PAGES		2048
> +#define PRIVATE_VADDR_MIN	(SHARED_VADDR_MIN + SHARED_PAGES * PAGE_SIZE)
> +
> +#define TOTAL_PAGES		(512 + SHARED_PAGES + PRIVATE_PAGES)
> +
> +static void fill_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < pages; i++)
> +		for (j = 0; j < PAGE_SIZE; j += stride)
> +			buf[i * PAGE_SIZE + j] = val;
> +}
> +
> +static bool check_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < pages; i++)
> +		for (j = 0; j < PAGE_SIZE; j += stride)
> +			if (buf[i * PAGE_SIZE + j] != val)
> +				return false;
> +
> +	return true;
> +}
> +
> +static void guest_test_start(struct ucall *uc)
> +{
> +	/* Initial guest check-in. */
> +	GUEST_SHARED_SYNC(uc, 1);
> +}
> +
> +static void test_start(struct kvm_vm *vm, struct ucall *uc)
> +{
> +	vcpu_run(vm, VCPU_ID);
> +
> +	/* Initial guest check-in. */
> +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 1);
> +}
> +
> +static void
> +guest_test_common(struct ucall *uc, uint8_t *shared_buf, uint8_t *private_buf)
> +{
> +	bool success;
> +
> +	/* Initial check-in for common. */
> +	GUEST_SHARED_SYNC(uc, 100);

Probably, you want to use macros to represent those states which should
make it more clear. Otherwise, it is quite cumbersome for readers to
remember the meaning (or state) of 100/101/102.
> +
> +	/* Ensure initial shared pages are intact. */
> +	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x41);
> +	GUEST_SHARED_ASSERT(uc, success);
> +
> +	/* Ensure initial private pages are intact/encrypted. */
> +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
> +	GUEST_SHARED_ASSERT(uc, success);
> +
> +	/* Ensure host userspace can't read newly-written encrypted data. */
> +	fill_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
> +
> +	GUEST_SHARED_SYNC(uc, 101);

ditto.
> +
> +	/* Ensure guest can read newly-written shared data from host. */
> +	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x44);
> +	GUEST_SHARED_ASSERT(uc, success);
> +
> +	/* Ensure host can read newly-written shared data from guest. */
> +	fill_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x45);
> +
> +	GUEST_SHARED_SYNC(uc, 102);

ditto.
> +}
> +
> +static void
> +test_common(struct kvm_vm *vm, struct ucall *uc,
> +		  uint8_t *shared_buf, uint8_t *private_buf)
> +{
> +	bool success;
> +
> +	/* Initial guest check-in. */
> +	vcpu_run(vm, VCPU_ID);
> +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 100);
> +
> +	/* Ensure initial private pages are intact/encrypted. */
> +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
> +	TEST_ASSERT(!success, "Initial guest memory not encrypted!");
> +
> +	vcpu_run(vm, VCPU_ID);
> +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 101);
> +
> +	/* Ensure host userspace can't read newly-written encrypted data. */
> +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);

I am not sure if it is safe here. Since the cache coherency is not there
for neither SEV or SEV-ES. Reading confidential memory from host side
will generate cache lines that is not coherent with the guest. So might
be better to add clfush here?

> +	TEST_ASSERT(!success, "Modified guest memory not encrypted!");
> +
> +	/* Ensure guest can read newly-written shared data from host. */
> +	fill_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x44);
> +
> +	vcpu_run(vm, VCPU_ID);
> +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 102);
> +
> +	/* Ensure host can read newly-written shared data from guest. */
> +	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x45);
> +	TEST_ASSERT(success, "Host can't read shared guest memory!");
> +}
> +
> +static void
> +guest_test_done(struct ucall *uc)
> +{
> +	GUEST_SHARED_DONE(uc);
> +}
> +
> +static void
> +test_done(struct kvm_vm *vm, struct ucall *uc)
> +{
> +	vcpu_run(vm, VCPU_ID);
> +	CHECK_SHARED_DONE(vm, VCPU_ID, uc);
> +}
> +
> +static void __attribute__((__flatten__))
> +guest_sev_code(struct ucall *uc, uint8_t *shared_buf, uint8_t *private_buf)
> +{
> +	uint32_t eax, ebx, ecx, edx;
> +	uint64_t sev_status;
> +
> +	guest_test_start(uc);
> +
> +	/* Check SEV CPUID bit. */
> +	eax = 0x8000001f;
> +	ecx = 0;
> +	cpuid(&eax, &ebx, &ecx, &edx);
> +	GUEST_SHARED_ASSERT(uc, eax & (1 << 1));
> +
> +	/* Check SEV MSR bit. */
> +	sev_status = rdmsr(MSR_AMD64_SEV);
> +	GUEST_SHARED_ASSERT(uc, (sev_status & 0x1) == 1);
> +
> +	guest_test_common(uc, shared_buf, private_buf);
> +
> +	guest_test_done(uc);
> +}
> +
> +static struct sev_vm *
> +setup_test_common(void *guest_code, uint64_t policy, struct ucall **uc,
> +		  uint8_t **shared_buf, uint8_t **private_buf)
> +{
> +	vm_vaddr_t uc_vaddr, shared_vaddr, private_vaddr;
> +	uint8_t measurement[512];
> +	struct sev_vm *sev;
> +	struct kvm_vm *vm;
> +	int i;
> +
> +	sev = sev_vm_create(policy, TOTAL_PAGES);
> +	if (!sev)
> +		return NULL;
> +	vm = sev_get_vm(sev);
> +
> +	/* Set up VCPU and initial guest kernel. */
> +	vm_vcpu_add_default(vm, VCPU_ID, guest_code);
> +	kvm_vm_elf_load(vm, program_invocation_name);
> +
> +	/* Set up shared ucall buffer. */
> +	uc_vaddr = ucall_shared_alloc(vm, 1);
> +
> +	/* Set up buffer for reserved shared memory. */
> +	shared_vaddr = vm_vaddr_alloc_shared(vm, SHARED_PAGES * PAGE_SIZE,
> +					     SHARED_VADDR_MIN);
> +	*shared_buf = addr_gva2hva(vm, shared_vaddr);
> +	fill_buf(*shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x41);
> +
> +	/* Set up buffer for reserved private memory. */
> +	private_vaddr = vm_vaddr_alloc(vm, PRIVATE_PAGES * PAGE_SIZE,
> +				       PRIVATE_VADDR_MIN);
> +	*private_buf = addr_gva2hva(vm, private_vaddr);
> +	fill_buf(*private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
> +
> +	/* Set up guest params. */
> +	vcpu_args_set(vm, VCPU_ID, 4, uc_vaddr, shared_vaddr, private_vaddr);
> +
> +	/*
> +	 * Hand these back to test harness, translation is needed now since page
> +	 * table will be encrypted after SEV VM launch.
> +	 */
> +	*uc = addr_gva2hva(vm, uc_vaddr);
> +	*shared_buf = addr_gva2hva(vm, shared_vaddr);
> +	*private_buf = addr_gva2hva(vm, private_vaddr);
> +
> +	/* Allocations/setup done. Encrypt initial guest payload. */
> +	sev_vm_launch(sev);
> +
> +	/* Dump the initial measurement. A test to actually verify it would be nice. */
> +	sev_vm_launch_measure(sev, measurement);
> +	pr_info("guest measurement: ");
> +	for (i = 0; i < 32; ++i)
> +		pr_info("%02x", measurement[i]);
> +	pr_info("\n");
> +
> +	sev_vm_launch_finish(sev);
> +
> +	return sev;
> +}
> +
> +static void test_sev(void *guest_code, uint64_t policy)
> +{
> +	uint8_t *shared_buf, *private_buf;
> +	struct sev_vm *sev;
> +	struct kvm_vm *vm;
> +	struct ucall *uc;
> +
> +	sev = setup_test_common(guest_code, policy, &uc, &shared_buf, &private_buf);
> +	if (!sev)
> +		return;
> +	vm = sev_get_vm(sev);
> +
> +	/* Guest is ready to run. Do the tests. */
> +	test_start(vm, uc);
> +	test_common(vm, uc, shared_buf, private_buf);
> +	test_done(vm, uc);
> +
> +	sev_vm_free(sev);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	/* SEV tests */
> +	test_sev(guest_sev_code, SEV_POLICY_NO_DBG);
> +	test_sev(guest_sev_code, 0);
> +
> +	return 0;
> +}
> --
> 2.25.1
>
Michael Roth Dec. 21, 2021, 3:40 p.m. UTC | #2
On Mon, Dec 20, 2021 at 01:49:15AM +0000, Mingwei Zhang wrote:
> On Thu, Dec 16, 2021, Michael Roth wrote:
> > A common aspect of booting SEV guests is checking related CPUID/MSR
> > bits and accessing shared/private memory. Add a basic test to cover
> > this.
> >
> > This test will be expanded to cover basic boot of SEV-ES and SEV-SNP in
> > subsequent patches.
> >
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> >  tools/testing/selftests/kvm/.gitignore        |   1 +
> >  tools/testing/selftests/kvm/Makefile          |   1 +
> >  .../selftests/kvm/x86_64/sev_all_boot_test.c  | 255 ++++++++++++++++++
> >  3 files changed, 257 insertions(+)
> >  create mode 100644 tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> >
> > diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> > index 4a801cba9c62..cc73de938a2a 100644
> > --- a/tools/testing/selftests/kvm/.gitignore
> > +++ b/tools/testing/selftests/kvm/.gitignore
> > @@ -43,6 +43,7 @@
> >  /x86_64/xen_vmcall_test
> >  /x86_64/xss_msr_test
> >  /x86_64/vmx_pmu_msrs_test
> > +/x86_64/sev_all_boot_test
> >  /access_tracking_perf_test
> >  /demand_paging_test
> >  /dirty_log_test
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index ccc382a827f1..6f250e190fde 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -81,6 +81,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/xen_shinfo_test
> >  TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test
> >  TEST_GEN_PROGS_x86_64 += x86_64/vmx_pi_mmio_test
> >  TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests
> > +TEST_GEN_PROGS_x86_64 += x86_64/sev_all_boot_test
> >  TEST_GEN_PROGS_x86_64 += demand_paging_test
> >  TEST_GEN_PROGS_x86_64 += dirty_log_test
> >  TEST_GEN_PROGS_x86_64 += dirty_log_perf_test
> > diff --git a/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> > new file mode 100644
> > index 000000000000..329a740a7cb2
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
> > @@ -0,0 +1,255 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Basic SEV boot tests.
> > + *
> > + * Copyright (C) 2021 Advanced Micro Devices
> > + */
> > +#define _GNU_SOURCE /* for program_invocation_short_name */
> > +#include <fcntl.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +
> > +#include "test_util.h"
> > +
> > +#include "kvm_util.h"
> > +#include "processor.h"
> > +#include "svm_util.h"
> > +#include "linux/psp-sev.h"
> > +#include "sev.h"
> > +
> > +#define VCPU_ID			2
> > +#define PAGE_SIZE		4096
> > +#define PAGE_STRIDE		32
> > +
> > +#define SHARED_PAGES		8192
> > +#define SHARED_VADDR_MIN	0x1000000
> > +
> > +#define PRIVATE_PAGES		2048
> > +#define PRIVATE_VADDR_MIN	(SHARED_VADDR_MIN + SHARED_PAGES * PAGE_SIZE)
> > +
> > +#define TOTAL_PAGES		(512 + SHARED_PAGES + PRIVATE_PAGES)
> > +
> > +static void fill_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val)
> > +{
> > +	int i, j;
> > +
> > +	for (i = 0; i < pages; i++)
> > +		for (j = 0; j < PAGE_SIZE; j += stride)
> > +			buf[i * PAGE_SIZE + j] = val;
> > +}
> > +
> > +static bool check_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val)
> > +{
> > +	int i, j;
> > +
> > +	for (i = 0; i < pages; i++)
> > +		for (j = 0; j < PAGE_SIZE; j += stride)
> > +			if (buf[i * PAGE_SIZE + j] != val)
> > +				return false;
> > +
> > +	return true;
> > +}
> > +
> > +static void guest_test_start(struct ucall *uc)
> > +{
> > +	/* Initial guest check-in. */
> > +	GUEST_SHARED_SYNC(uc, 1);
> > +}
> > +
> > +static void test_start(struct kvm_vm *vm, struct ucall *uc)
> > +{
> > +	vcpu_run(vm, VCPU_ID);
> > +
> > +	/* Initial guest check-in. */
> > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 1);
> > +}
> > +
> > +static void
> > +guest_test_common(struct ucall *uc, uint8_t *shared_buf, uint8_t *private_buf)
> > +{
> > +	bool success;
> > +
> > +	/* Initial check-in for common. */
> > +	GUEST_SHARED_SYNC(uc, 100);
> 
> Probably, you want to use macros to represent those states which should
> make it more clear. Otherwise, it is quite cumbersome for readers to
> remember the meaning (or state) of 100/101/102.

I agree with that approach in general, but in this case it's a bit
awkward since, these aren't necessarily similarly grouped tests where each
test falls under a certain category of TEST_GROUP_x where a macro would
provide additional information about the nature of the tests related to a
particular SYNC() call, in some cases it's an aggregate of multiple
things, for instance...


> > +
> > +	/* Ensure initial shared pages are intact. */
> > +	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x41);
> > +	GUEST_SHARED_ASSERT(uc, success);
> > +
> > +	/* Ensure initial private pages are intact/encrypted. */
> > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
> > +	GUEST_SHARED_ASSERT(uc, success);
> > +
> > +	/* Ensure host userspace can't read newly-written encrypted data. */
> > +	fill_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
> > +
> > +	GUEST_SHARED_SYNC(uc, 101);
> 
> ditto.

...here 101 exit is both a checkpoint that the 2 previous tests have passed,
and that a 3rd test is ready for the host-side to complete on its end.

> > +
> > +	/* Ensure guest can read newly-written shared data from host. */
> > +	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x44);
> > +	GUEST_SHARED_ASSERT(uc, success);
> > +
> > +	/* Ensure host can read newly-written shared data from guest. */
> > +	fill_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x45);
> > +
> > +	GUEST_SHARED_SYNC(uc, 102);
> 
> ditto.

...here 102 exit is for the "Ensure host can read newly-written shared
data" test, since the host is needed to complete the test. But prior to
that there was another test checking things in the other direction:
"Ensure guest can read newly-written shared data from host."

I could try adding additional SYNC() calls to get things closer to a
1:1 mapping between test/SYNC() so we can give each particular
test/SYNC() point a unique/meaningful identifier, but that's potentially
a lot of additional exits/SYNC()s depending on how tests are added over
time, and it's compounded further by the following situation, where a
conversion of this file would leave us with something like:

  guest:
    //set up some tests relating to x for host to complete
    SYNC(..., TEST_GROUP_x_READY)
  
    //complete tests relating to y
    SYNC(..., TEST_GROUP_y_COMPLETE)
  
  host:
    vmrun()
    CHECK(..., TEST_GROUP_x_READY)
    //complete tests relating to x
  
    //set up some tests relating to y for guest to complete
    vmrun()

    CHECK(..., TEST_GROUP_y_COMPLETE)

where it's easy for a reader to get confused as to where
TEST_GROUP_x_COMPLETE is, whether it's related to the subsequent
*_y_COMPLETE, etc, because they might be expecting the same sort of pairing
we get for guest->host with SYNC()/CHECK(), but for host->guest the pairing
is more like vmrun()/SYNC()/CHECK(), so attempting to write things to
alleviate that possible confusion, we need to introduce exits that purely
serve the purpose of helping the reader along, and end up with something
like:

  guest:
    //set up some tests relating to x for host to complete
    SYNC(..., TEST_GROUP_x_READY)

    SYNC(..., TEST_GROUP_x_COMPLETE)
  
    //complete tests relating to y
    SYNC(..., TEST_GROUP_y_COMPLETE)
  
  host:
    vmrun()
    CHECK(..., TEST_GROUP_x_READY)
    //complete tests relating to x

    //resume guest to it lets us know it is done with that test group
    vmrun()
    CHECK(..., TEST_GROUP_x_COMPLETE)

    //set up some tests relating to y for guest to complete
    vmrun()
    
    CHECK(..., TEST_GROUP_y_COMPLETE)

But what about TEST_GROUP_y_READY? Where is that set up? Take 2:

  guest:
    //set up some tests relating to x for host to complete
    SYNC(..., TEST_GROUP_x_READY)

    SYNC(..., TEST_GROUP_x_COMPLETE)

    //let host know we're ready to run TEST_GROUP_y
    SYNC(..., TEST_GROUP_y_READY)
  
    //complete tests relating to y
    SYNC(..., TEST_GROUP_y_COMPLETE)
  
  host:
    vmrun()
    CHECK(..., TEST_GROUP_x_READY)
    //complete tests relating to x

    //resume guest to it lets us know it is done with that test group
    vmrun()
    CHECK(..., TEST_GROUP_x_COMPLETE)

    //set up some tests relating to y for guest to complete
    vmrun()
    CHECK(..., TEST_GROUP_y_READY)
    vmrun()
    CHECK(..., TEST_GROUP_y_COMPLETE)

...which to me seems less readable that just treating each SYNC() as a
unique checkpoint and relying on comments to explain the individual
tests: 

  guest:
    //set up some tests relating to x for host to complete

    SYNC(..., CHECKPOINT1)
  
    //complete tests relating to y

    SYNC(..., CHECKPOINT2)
  
  host:
    vmrun()
    CHECK(..., CHECKPOINT1)

    //complete tests relating to x
  
    //set up some tests relating to y for guest to complete
    vmrun()

    CHECK(..., CHECKPOINT2)

Really don't mind changing things, but just doesn't seem like it would
make things more readable than the current approach. I could add
actual CHECKPOINT1/2/3 macros to make it clearer how SYNC() is being
used here, but it seems like a waste adding a bunch of '#define
CHECKPOINT1 1' etc.

> > +}
> > +
> > +static void
> > +test_common(struct kvm_vm *vm, struct ucall *uc,
> > +		  uint8_t *shared_buf, uint8_t *private_buf)
> > +{
> > +	bool success;
> > +
> > +	/* Initial guest check-in. */
> > +	vcpu_run(vm, VCPU_ID);
> > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 100);
> > +
> > +	/* Ensure initial private pages are intact/encrypted. */
> > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
> > +	TEST_ASSERT(!success, "Initial guest memory not encrypted!");
> > +
> > +	vcpu_run(vm, VCPU_ID);
> > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 101);
> > +
> > +	/* Ensure host userspace can't read newly-written encrypted data. */
> > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
> 
> I am not sure if it is safe here. Since the cache coherency is not there
> for neither SEV or SEV-ES. Reading confidential memory from host side
> will generate cache lines that is not coherent with the guest. So might
> be better to add clfush here?

On the guest side, the cachelines are tagged based on ASID, so in this case
the guest would populate it's own cachelines when it writes new data to
private buf.

On a host without SME coherency bit, there is a possibility that whatever
data the host had previously written to private_buf with C=0/ASID=0, prior
to the guest writing to it, might still be present in the cache, but for
this test that's okay since the guest has purposely written new data to
confirm that the host does not see the new data. What data the host
*actually* sees, stale cache data vs. new reads of guest private memory
with C=0 (e.g. ciphertext) are both okay as far as the test is concerned.
clflush() would probably make sense here, but if failure to do so
somehow results in the above assumptions not holding, and the test ends
up seeing the newly-written data, we definitely want this test to fail
loudly, so leaving out the clflush() to cover that corner case seems
like a good idea.

On a host with SME coherency bit, I believe (based on a recent writeup from
David Kaplan) each new read/write with a different C-bit/ASID than what was
previously written would force an eviction, so we don't need to worry about
the stale cases above, and the test should work in that scenario as
well.

Thanks!

-Mike
Michael Roth Dec. 21, 2021, 5:26 p.m. UTC | #3
On Tue, Dec 21, 2021 at 09:40:36AM -0600, Michael Roth wrote:
> On Mon, Dec 20, 2021 at 01:49:15AM +0000, Mingwei Zhang wrote:
> > On Thu, Dec 16, 2021, Michael Roth wrote:
> > > +}
> > > +
> > > +static void
> > > +test_common(struct kvm_vm *vm, struct ucall *uc,
> > > +		  uint8_t *shared_buf, uint8_t *private_buf)
> > > +{
> > > +	bool success;
> > > +
> > > +	/* Initial guest check-in. */
> > > +	vcpu_run(vm, VCPU_ID);
> > > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 100);
> > > +
> > > +	/* Ensure initial private pages are intact/encrypted. */
> > > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
> > > +	TEST_ASSERT(!success, "Initial guest memory not encrypted!");
> > > +
> > > +	vcpu_run(vm, VCPU_ID);
> > > +	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 101);
> > > +
> > > +	/* Ensure host userspace can't read newly-written encrypted data. */
> > > +	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
> > 
> > I am not sure if it is safe here. Since the cache coherency is not there
> > for neither SEV or SEV-ES. Reading confidential memory from host side
> > will generate cache lines that is not coherent with the guest. So might
> > be better to add clfush here?
> 
> On the guest side, the cachelines are tagged based on ASID, so in this case
> the guest would populate it's own cachelines when it writes new data to
> private buf.
> 
> On a host without SME coherency bit, there is a possibility that whatever
> data the host had previously written to private_buf with C=0/ASID=0, prior
> to the guest writing to it, might still be present in the cache, but for
> this test that's okay since the guest has purposely written new data to
> confirm that the host does not see the new data. What data the host
> *actually* sees, stale cache data vs. new reads of guest private memory
> with C=0 (e.g. ciphertext) are both okay as far as the test is concerned.
> clflush() would probably make sense here, but if failure to do so
> somehow results in the above assumptions not holding, and the test ends
> up seeing the newly-written data, we definitely want this test to fail
> loudly, so leaving out the clflush() to cover that corner case seems
> like a good idea.

Actually it might be good to check both of those cases, e.g.:

  //check private buf (possibly with stale cache for sme_coherency=0)
  clflush()
  //check private buf again (with fresh read of guest memory)

I'll take a look at that.

-Mike
Paolo Bonzini Dec. 22, 2021, 2:55 p.m. UTC | #4
On 12/21/21 16:40, Michael Roth wrote:
>> Probably, you want to use macros to represent those states which should
>> make it more clear. Otherwise, it is quite cumbersome for readers to
>> remember the meaning (or state) of 100/101/102.
> I agree with that approach in general, but in this case it's a bit
> awkward since, these aren't necessarily similarly grouped tests where each
> test falls under a certain category of TEST_GROUP_x where a macro would
> provide additional information about the nature of the tests related to a
> particular SYNC() call, in some cases it's an aggregate of multiple
> things, for instance...

Indeed, these are just numbers that match between guest_test_common and 
test_common.  It's not too hard to follow if you look at the two 
functions side-by-side.

Paolo
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 4a801cba9c62..cc73de938a2a 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -43,6 +43,7 @@ 
 /x86_64/xen_vmcall_test
 /x86_64/xss_msr_test
 /x86_64/vmx_pmu_msrs_test
+/x86_64/sev_all_boot_test
 /access_tracking_perf_test
 /demand_paging_test
 /dirty_log_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index ccc382a827f1..6f250e190fde 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -81,6 +81,7 @@  TEST_GEN_PROGS_x86_64 += x86_64/xen_shinfo_test
 TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_pi_mmio_test
 TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests
+TEST_GEN_PROGS_x86_64 += x86_64/sev_all_boot_test
 TEST_GEN_PROGS_x86_64 += demand_paging_test
 TEST_GEN_PROGS_x86_64 += dirty_log_test
 TEST_GEN_PROGS_x86_64 += dirty_log_perf_test
diff --git a/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
new file mode 100644
index 000000000000..329a740a7cb2
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c
@@ -0,0 +1,255 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Basic SEV boot tests.
+ *
+ * Copyright (C) 2021 Advanced Micro Devices
+ */
+#define _GNU_SOURCE /* for program_invocation_short_name */
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+
+#include "kvm_util.h"
+#include "processor.h"
+#include "svm_util.h"
+#include "linux/psp-sev.h"
+#include "sev.h"
+
+#define VCPU_ID			2
+#define PAGE_SIZE		4096
+#define PAGE_STRIDE		32
+
+#define SHARED_PAGES		8192
+#define SHARED_VADDR_MIN	0x1000000
+
+#define PRIVATE_PAGES		2048
+#define PRIVATE_VADDR_MIN	(SHARED_VADDR_MIN + SHARED_PAGES * PAGE_SIZE)
+
+#define TOTAL_PAGES		(512 + SHARED_PAGES + PRIVATE_PAGES)
+
+static void fill_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val)
+{
+	int i, j;
+
+	for (i = 0; i < pages; i++)
+		for (j = 0; j < PAGE_SIZE; j += stride)
+			buf[i * PAGE_SIZE + j] = val;
+}
+
+static bool check_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val)
+{
+	int i, j;
+
+	for (i = 0; i < pages; i++)
+		for (j = 0; j < PAGE_SIZE; j += stride)
+			if (buf[i * PAGE_SIZE + j] != val)
+				return false;
+
+	return true;
+}
+
+static void guest_test_start(struct ucall *uc)
+{
+	/* Initial guest check-in. */
+	GUEST_SHARED_SYNC(uc, 1);
+}
+
+static void test_start(struct kvm_vm *vm, struct ucall *uc)
+{
+	vcpu_run(vm, VCPU_ID);
+
+	/* Initial guest check-in. */
+	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 1);
+}
+
+static void
+guest_test_common(struct ucall *uc, uint8_t *shared_buf, uint8_t *private_buf)
+{
+	bool success;
+
+	/* Initial check-in for common. */
+	GUEST_SHARED_SYNC(uc, 100);
+
+	/* Ensure initial shared pages are intact. */
+	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x41);
+	GUEST_SHARED_ASSERT(uc, success);
+
+	/* Ensure initial private pages are intact/encrypted. */
+	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
+	GUEST_SHARED_ASSERT(uc, success);
+
+	/* Ensure host userspace can't read newly-written encrypted data. */
+	fill_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
+
+	GUEST_SHARED_SYNC(uc, 101);
+
+	/* Ensure guest can read newly-written shared data from host. */
+	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x44);
+	GUEST_SHARED_ASSERT(uc, success);
+
+	/* Ensure host can read newly-written shared data from guest. */
+	fill_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x45);
+
+	GUEST_SHARED_SYNC(uc, 102);
+}
+
+static void
+test_common(struct kvm_vm *vm, struct ucall *uc,
+		  uint8_t *shared_buf, uint8_t *private_buf)
+{
+	bool success;
+
+	/* Initial guest check-in. */
+	vcpu_run(vm, VCPU_ID);
+	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 100);
+
+	/* Ensure initial private pages are intact/encrypted. */
+	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
+	TEST_ASSERT(!success, "Initial guest memory not encrypted!");
+
+	vcpu_run(vm, VCPU_ID);
+	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 101);
+
+	/* Ensure host userspace can't read newly-written encrypted data. */
+	success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
+	TEST_ASSERT(!success, "Modified guest memory not encrypted!");
+
+	/* Ensure guest can read newly-written shared data from host. */
+	fill_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x44);
+
+	vcpu_run(vm, VCPU_ID);
+	CHECK_SHARED_SYNC(vm, VCPU_ID, uc, 102);
+
+	/* Ensure host can read newly-written shared data from guest. */
+	success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x45);
+	TEST_ASSERT(success, "Host can't read shared guest memory!");
+}
+
+static void
+guest_test_done(struct ucall *uc)
+{
+	GUEST_SHARED_DONE(uc);
+}
+
+static void
+test_done(struct kvm_vm *vm, struct ucall *uc)
+{
+	vcpu_run(vm, VCPU_ID);
+	CHECK_SHARED_DONE(vm, VCPU_ID, uc);
+}
+
+static void __attribute__((__flatten__))
+guest_sev_code(struct ucall *uc, uint8_t *shared_buf, uint8_t *private_buf)
+{
+	uint32_t eax, ebx, ecx, edx;
+	uint64_t sev_status;
+
+	guest_test_start(uc);
+
+	/* Check SEV CPUID bit. */
+	eax = 0x8000001f;
+	ecx = 0;
+	cpuid(&eax, &ebx, &ecx, &edx);
+	GUEST_SHARED_ASSERT(uc, eax & (1 << 1));
+
+	/* Check SEV MSR bit. */
+	sev_status = rdmsr(MSR_AMD64_SEV);
+	GUEST_SHARED_ASSERT(uc, (sev_status & 0x1) == 1);
+
+	guest_test_common(uc, shared_buf, private_buf);
+
+	guest_test_done(uc);
+}
+
+static struct sev_vm *
+setup_test_common(void *guest_code, uint64_t policy, struct ucall **uc,
+		  uint8_t **shared_buf, uint8_t **private_buf)
+{
+	vm_vaddr_t uc_vaddr, shared_vaddr, private_vaddr;
+	uint8_t measurement[512];
+	struct sev_vm *sev;
+	struct kvm_vm *vm;
+	int i;
+
+	sev = sev_vm_create(policy, TOTAL_PAGES);
+	if (!sev)
+		return NULL;
+	vm = sev_get_vm(sev);
+
+	/* Set up VCPU and initial guest kernel. */
+	vm_vcpu_add_default(vm, VCPU_ID, guest_code);
+	kvm_vm_elf_load(vm, program_invocation_name);
+
+	/* Set up shared ucall buffer. */
+	uc_vaddr = ucall_shared_alloc(vm, 1);
+
+	/* Set up buffer for reserved shared memory. */
+	shared_vaddr = vm_vaddr_alloc_shared(vm, SHARED_PAGES * PAGE_SIZE,
+					     SHARED_VADDR_MIN);
+	*shared_buf = addr_gva2hva(vm, shared_vaddr);
+	fill_buf(*shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x41);
+
+	/* Set up buffer for reserved private memory. */
+	private_vaddr = vm_vaddr_alloc(vm, PRIVATE_PAGES * PAGE_SIZE,
+				       PRIVATE_VADDR_MIN);
+	*private_buf = addr_gva2hva(vm, private_vaddr);
+	fill_buf(*private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
+
+	/* Set up guest params. */
+	vcpu_args_set(vm, VCPU_ID, 4, uc_vaddr, shared_vaddr, private_vaddr);
+
+	/*
+	 * Hand these back to test harness, translation is needed now since page
+	 * table will be encrypted after SEV VM launch.
+	 */
+	*uc = addr_gva2hva(vm, uc_vaddr);
+	*shared_buf = addr_gva2hva(vm, shared_vaddr);
+	*private_buf = addr_gva2hva(vm, private_vaddr);
+
+	/* Allocations/setup done. Encrypt initial guest payload. */
+	sev_vm_launch(sev);
+
+	/* Dump the initial measurement. A test to actually verify it would be nice. */
+	sev_vm_launch_measure(sev, measurement);
+	pr_info("guest measurement: ");
+	for (i = 0; i < 32; ++i)
+		pr_info("%02x", measurement[i]);
+	pr_info("\n");
+
+	sev_vm_launch_finish(sev);
+
+	return sev;
+}
+
+static void test_sev(void *guest_code, uint64_t policy)
+{
+	uint8_t *shared_buf, *private_buf;
+	struct sev_vm *sev;
+	struct kvm_vm *vm;
+	struct ucall *uc;
+
+	sev = setup_test_common(guest_code, policy, &uc, &shared_buf, &private_buf);
+	if (!sev)
+		return;
+	vm = sev_get_vm(sev);
+
+	/* Guest is ready to run. Do the tests. */
+	test_start(vm, uc);
+	test_common(vm, uc, shared_buf, private_buf);
+	test_done(vm, uc);
+
+	sev_vm_free(sev);
+}
+
+int main(int argc, char *argv[])
+{
+	/* SEV tests */
+	test_sev(guest_sev_code, SEV_POLICY_NO_DBG);
+	test_sev(guest_sev_code, 0);
+
+	return 0;
+}