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@xxxxxxxxxx>
> ---
> 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:
> + * 1) Upon entry, guest signals VMM that it has started.
> + * 2) VMM populates the shared memory with known pattern and continues guest
> + * execution.
> + * 3) Guest writes a different pattern on the private memory and signals VMM
> + * that it has updated private memory.
> + * 4) VMM verifies its shared memory contents to be same as the data populated
> + * in step 2 and continues guest execution.
> + * 5) 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);
> +}
