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

[Michael Roth]

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@xxxxxxx>
> > ---
> >  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