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