On Wed, Jul 20, 2022 at 4:03 PM Sean Christopherson seanjc@google.com wrote:
...
- 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.
I will try to spend more time on how kvm_vm_elf_load can be modified to handle GOT fixups in different scenarios including statically/dynamically linked sefltest binaries as I currently recall limited information here.
But modifying kvm_vm_elf_load to fixup GOT entries will be insufficient as guest VM code (possibly whole selftest binary) will need to be compiled with flags that allow memset/memcpy implementations to work with specific guest VM configurations e.g. AVX extension. Same concern is outlined in https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/kvm/l....
Would it be ok to maintain selftest binary compilation flags based on guest VM configurations?
+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 :-)
Thanks Sean for all the feedback here. I will address the comments in the next series.
Regards, Vishal