On Wed, May 11, 2022, Vishal Annapurve wrote:
Add KVM selftest to access private memory privately from the guest to test that memory updates from guest and userspace vmm don't affect each other.
Signed-off-by: Vishal Annapurve vannapurve@google.com
tools/testing/selftests/kvm/Makefile | 1 + tools/testing/selftests/kvm/priv_memfd_test.c | 283 ++++++++++++++++++
If this name stays around in any form, just spell out "private". The file system can handle three more characters.
2 files changed, 284 insertions(+) create mode 100644 tools/testing/selftests/kvm/priv_memfd_test.c +/* Guest code in selftests is loaded to guest memory using kvm_vm_elf_load
Kernel style (except for net/ apparently?) for multi-line comments is to have a "blank" first line:
/* * blahal;sdkfjas;flkjasd;flkj;aslkfjdsa;lkfjsa;lkfjsa;dlkfjas;dlkfj * as;dflkjasdf;lkasjdf;lkasdjf;lkasdjf;lkjsad;flkjasd;flkjas;dflkj */
And if you haven't already read through Documentation/process/coding-style.rst, though I thikn this and indentation are the only glaring issues.
- which doesn't handle global offset table updates. Calling standard libc
- functions would normally result in referring to the global offset table.
- Adding O1 here seems to prohibit compiler from replacing the memory
- operations with standard libc functions such as memset.
- */
Eww. We should either fix kvm_vm_elf_load() or override the problematic libc variants. Playing games with per-function attributes is not maintainable.
+static bool __attribute__((optimize("O1"))) do_mem_op(enum mem_op op,
void *mem, uint64_t pat, uint32_t size)
Oof. Don't be so agressive in shortening names, _especially_ when there's no established/universal abbreviation. It took me forever to figure out that "pat" is "pattern". And for x86, "pat" is especially confusing because it already a very well-established name that just so happens to be relevant to memory types, just a different kind of a memory type...
+{
- uint64_t *buf = (uint64_t *)mem;
- uint32_t chunk_size = sizeof(pat);
- uint64_t mem_addr = (uint64_t)mem;
- if (((mem_addr % chunk_size) != 0) || ((size % chunk_size) != 0))
All the patterns are a repeating byte, why restrict this to 8-byte chunks? Then this confusing assert-but-not-an-assert goes away.
return false;- for (uint32_t i = 0; i < (size / chunk_size); i++) {
if (op == SET_PAT)buf[i] = pat;if (op == VERIFY_PAT) {if (buf[i] != pat)return false;
If overriding memset() and memcmp() doesn't work for whatever reason, add proper helpers instead of a do_stuff() wrapper.
}- }
- return true;
+}
+/* Test to verify guest private accesses on private memory with following steps:
- Upon entry, guest signals VMM that it has started.
- VMM populates the shared memory with known pattern and continues guest
- execution.
- Guest writes a different pattern on the private memory and signals VMM
that it has updated private memory.
- VMM verifies its shared memory contents to be same as the data populated
in step 2 and continues guest execution.
- Guest verifies its private memory contents to be same as the data
populated in step 3 and marks the end of the guest execution.- */
+#define PMPAT_ID 0 +#define PMPAT_DESC "PrivateMemoryPrivateAccessTest"
+/* Guest code execution stages for private mem access test */ +#define PMPAT_GUEST_STARTED 0ULL +#define PMPAT_GUEST_PRIV_MEM_UPDATED 1ULL
+static bool pmpat_handle_vm_stage(struct kvm_vm *vm,
void *test_info,uint64_t stage)
Align parameters, both in prototypes and in invocations. And don't wrap unnecessarily.
static bool pmpat_handle_vm_stage(struct kvm_vm *vm, void *test_info, uint64_t stage)
Or even let that poke out (probably not in this case, but do keep in mind that the 80 char "limit" is a soft limit that can be broken if doing so yields more readable code).
static bool pmpat_handle_vm_stage(struct kvm_vm *vm, void *test_info, uint64_t stage)
+{
- void *shared_mem = ((struct test_run_helper *)test_info)->shared_mem;
- switch (stage) {
- case PMPAT_GUEST_STARTED: {
/* Initialize the contents of shared memory */TEST_ASSERT(do_mem_op(SET_PAT, shared_mem,TEST_MEM_DATA_PAT1, TEST_MEM_SIZE),"Shared memory update failure");
Align indentation (here and many other places).
VM_STAGE_PROCESSED(PMPAT_GUEST_STARTED);break;- }
- case PMPAT_GUEST_PRIV_MEM_UPDATED: {
/* verify host updated data is still intact */TEST_ASSERT(do_mem_op(VERIFY_PAT, shared_mem,TEST_MEM_DATA_PAT1, TEST_MEM_SIZE),"Shared memory view mismatch");VM_STAGE_PROCESSED(PMPAT_GUEST_PRIV_MEM_UPDATED);break;- }
- default:
printf("Unhandled VM stage %ld\n", stage);return false;- }
- return true;
+}
+static void pmpat_guest_code(void) +{
- void *priv_mem = (void *)TEST_MEM_GPA;
- int ret;
- GUEST_SYNC(PMPAT_GUEST_STARTED);
- /* Mark the GPA range to be treated as always accessed privately */
- ret = kvm_hypercall(KVM_HC_MAP_GPA_RANGE, TEST_MEM_GPA,
TEST_MEM_SIZE >> MIN_PAGE_SHIFT,KVM_MARK_GPA_RANGE_ENC_ACCESS, 0);- GUEST_ASSERT_1(ret == 0, ret);
"!ret" instead of "ret == 0"
- GUEST_ASSERT(do_mem_op(SET_PAT, priv_mem, TEST_MEM_DATA_PAT2,
TEST_MEM_SIZE));- GUEST_SYNC(PMPAT_GUEST_PRIV_MEM_UPDATED);
- GUEST_ASSERT(do_mem_op(VERIFY_PAT, priv_mem,
TEST_MEM_DATA_PAT2, TEST_MEM_SIZE));- GUEST_DONE();
+}
+static struct test_run_helper priv_memfd_testsuite[] = {
- [PMPAT_ID] = {
.test_desc = PMPAT_DESC,.vmst_handler = pmpat_handle_vm_stage,.guest_fn = pmpat_guest_code,- },
+};
...
+/* Do private access to the guest's private memory */ +static void setup_and_execute_test(uint32_t test_id)
This helper appears to be the bulk of the shared code between tests. This can and should be a helper to create a VM with private memory. Not sure what to call such a helper, maybe vm_create_with_private_memory()? A little verbose, but literal isn't always bad.
+{
- struct kvm_vm *vm;
- int priv_memfd;
- int ret;
- void *shared_mem;
- struct kvm_enable_cap cap;
- vm = vm_create_default(VCPU_ID, 0,
priv_memfd_testsuite[test_id].guest_fn);- /* Allocate shared memory */
- shared_mem = mmap(NULL, TEST_MEM_SIZE,
PROT_READ | PROT_WRITE,MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);- TEST_ASSERT(shared_mem != MAP_FAILED, "Failed to mmap() host");
- /* Allocate private memory */
- priv_memfd = memfd_create("vm_private_mem", MFD_INACCESSIBLE);
- TEST_ASSERT(priv_memfd != -1, "Failed to create priv_memfd");
- ret = fallocate(priv_memfd, 0, 0, TEST_MEM_SIZE);
- TEST_ASSERT(ret != -1, "fallocate failed");
- priv_memory_region_add(vm, shared_mem,
TEST_MEM_SLOT, TEST_MEM_SIZE,TEST_MEM_GPA, priv_memfd, 0);- pr_info("Mapping test memory pages 0x%x page_size 0x%x\n",
TEST_MEM_SIZE/vm_get_page_size(vm),vm_get_page_size(vm));- virt_map(vm, TEST_MEM_GPA, TEST_MEM_GPA,
(TEST_MEM_SIZE/vm_get_page_size(vm)));- /* Enable exit on KVM_HC_MAP_GPA_RANGE */
- pr_info("Enabling exit on map_gpa_range hypercall\n");
- ret = ioctl(vm_get_fd(vm), KVM_CHECK_EXTENSION, KVM_CAP_EXIT_HYPERCALL);
- TEST_ASSERT(ret & (1 << KVM_HC_MAP_GPA_RANGE),
"VM exit on MAP_GPA_RANGE HC not supported");
Impressively bizarre indentation :-)
- cap.cap = KVM_CAP_EXIT_HYPERCALL;
- cap.flags = 0;
- cap.args[0] = (1 << KVM_HC_MAP_GPA_RANGE);
- ret = ioctl(vm_get_fd(vm), KVM_ENABLE_CAP, &cap);
- TEST_ASSERT(ret == 0,
"Failed to enable exit on MAP_GPA_RANGE hypercall\n");- priv_memfd_testsuite[test_id].shared_mem = shared_mem;
- priv_memfd_testsuite[test_id].priv_memfd = priv_memfd;
- vcpu_work(vm, test_id);
- munmap(shared_mem, TEST_MEM_SIZE);
- priv_memfd_testsuite[test_id].shared_mem = NULL;
- close(priv_memfd);
- priv_memfd_testsuite[test_id].priv_memfd = -1;
- kvm_vm_free(vm);
+}