Ackerley Tng <[email protected]> writes:
> Tarun Sahu <[email protected]> writes:
>
>> Add a new KVM selftest `guest_memfd_preservation_test` to verify that
>> guest memory backed by guest_memfd is preserved properly.
>>
>
> Don't think using backticks in commit messages is a common practice but
> I might be wrong here.
>
>> The test leverages the Live Update Orchestrator (LUO) infrastructure
>> to validate that memory folios and configuration layouts are
>> successfully saved and then restored during kernel live updates,
>> preventing any memory loss for the guest.
>>
>> Here, I have used the kvm selftests framework by creating a new
>> vm and mapping two memory slots to it. One is the code that is executed
>> inside the vm and other is the guest_memfd whose memory is being
>> written by the guest code.
>>
>
> Don't think commit messages with "I" are common either
Will take care of commit message everywhere as per the guidelines,
Sorry about that.
>
>> In Phase 1: Once data is written the vm exits and wait for the user
>> to trigger the kexec.
>>
>> In Phase 2: A new vm is created with retrieved kvm and again two
>> memory slots are assigned. Once for guest code, and another is for
>> retrieved guest_memfd where guest_memfd memory is verified by the
>> executed guest code. If verification succeeds, The test passes.
>>
>>
>> [...snip...]
>>
>> +#define SESSION_NAME "gmem_vm_preservation_session"
>> +#define VM_TOKEN 0x1001
>> +#define GMEM_TOKEN 0x1002
>> +
>> +#define GMEM_SIZE (16ULL * 1024 * 1024)
>> +#define DATA_SIZE (5ULL * 1024 * 1024)
>> +
>> +static size_t page_size;
>> +
>> +/* Deterministic byte pattern generation based on offset */
>> +static inline uint8_t get_pattern_byte(size_t offset)
>> +{
>> + return (uint8_t)(offset ^ 0x5A);
>> +}
>> +
>> +static void guest_code_phase1(uint64_t gpa, uint64_t size, uint64_t data_size)
>> +{
>> + uint8_t *mem = (uint8_t *)gpa;
>> + size_t i;
>> +
>> + for (i = 0; i < data_size; i++)
>> + mem[i] = get_pattern_byte(i);
>> +
>> + GUEST_DONE();
>> +}
>> +
>> +static void guest_code_phase2(uint64_t gpa, uint64_t size, uint64_t data_size)
>> +{
>> + uint8_t *mem = (uint8_t *)gpa;
>> + size_t i;
>> +
>> + for (i = 0; i < data_size; i++) {
>> + uint8_t val = get_pattern_byte(i);
>> +
>> + __GUEST_ASSERT(mem[i] == val,
>> + "Data mismatch at offset %lu! Expected 0x%x, got 0x%x",
>> + i, val, mem[i]);
>> + }
>> +
>> + GUEST_DONE();
>> +}
>> +
>> +static void do_phase1(void)
>> +{
>> + uint64_t flags = GUEST_MEMFD_FLAG_MMAP | GUEST_MEMFD_FLAG_INIT_SHARED;
>
> Is there a reason to set GUEST_MEMFD_FLAG_MMAP? We're not really
> accessing that memory from the host in this test.
Right, We can skip it.
>
>> + int gmem_fd, dev_luo_fd, session_fd, ret;
>> + const uint64_t gpa = SZ_4G;
>> + struct kvm_vcpu *vcpu;
>> + const int slot = 1;
>> + struct kvm_vm *vm;
>> +
>> + vm = __vm_create_shape_with_one_vcpu(VM_SHAPE_DEFAULT, &vcpu, 1,
>> + guest_code_phase1);
>> + gmem_fd = vm_create_guest_memfd(vm, GMEM_SIZE, flags);
>> + vm_set_user_memory_region2(vm, slot, KVM_MEM_GUEST_MEMFD, gpa, GMEM_SIZE, NULL,
>> + gmem_fd, 0);
>> +
>> + for (size_t i = 0; i < GMEM_SIZE; i += page_size)
>> + virt_pg_map(vm, gpa + i, gpa + i);
>> +
>> + vcpu_args_set(vcpu, 3, gpa, GMEM_SIZE, DATA_SIZE);
>
> If GMEM_SIZE and DATA_SIZE are static I think we don't have to set those
> as vcpu_args_set(), they can be used as macros from within the guest.
Yes, There are multiple places we can skip it, Like passing them as the
argument in the guest_code_phase1/2. Will update it.
>
>> +
>> + vcpu_run(vcpu);
>> + TEST_ASSERT_EQ(get_ucall(vcpu, NULL), UCALL_DONE);
>> +
>> + dev_luo_fd = luo_open_device();
>> + TEST_ASSERT(dev_luo_fd >= 0, "Failed to open /dev/liveupdate");
>> +
>> + session_fd = luo_create_session(dev_luo_fd, SESSION_NAME);
>> + TEST_ASSERT(session_fd >= 0, "Failed to create LUO session");
>> +
>> + ret = luo_session_preserve_fd(session_fd, vm->fd, VM_TOKEN);
>> + TEST_ASSERT(ret == 0, "Failed to preserve VM file descriptor");
>> +
>> + ret = luo_session_preserve_fd(session_fd, gmem_fd, GMEM_TOKEN);
>> + TEST_ASSERT(ret == 0, "Failed to preserve guest_memfd file descriptor");
>> +
>
> Thanks for showing how this works :)
Glad to know. it helped.
. .
v
>
>> + printf("\n============================================================\n");
>> + printf("Phase 1 Complete Successfully!\n");
>> + printf("VM file and guest_memfd file have been preserved via LUO.\n");
>> + printf("Tokens: VM_TOKEN=0x%x, GMEM_TOKEN=0x%x\n", VM_TOKEN, GMEM_TOKEN);
>> + printf("Machine Size: %llu MB, Data Size: %llu MB\n", GMEM_SIZE / SZ_1M,
>> + DATA_SIZE / SZ_1M);
>> + printf("------------------------------------------------------------\n");
>> +
>> + daemonize_and_wait();
>> +}
>> +
>> +static struct kvm_vm *vm_create_from_fd(int resurrected_vm_fd,
>> + struct vm_shape shape)
>> +{
>> + struct kvm_vm *vm;
>> +
>> + vm = calloc(1, sizeof(*vm));
>> + TEST_ASSERT(vm != NULL, "Insufficient Memory");
>> +
>> + vm_init_fields(vm, shape);
>
> What would happen if the shape was changed between preserving and
> restoring?
Shape must be consistent across kexec. It may break. struct vm_shape
includes two fields
1. mode: 4k, 16k or 64k mapping, how many page table bit etc.
which are used to setup mapping with guest code, and memory.
guest_memfd does not support mapping other than PAGE_SIZE.
2. type: This is userspace side of the vm type. And it will not have
information about the preserved vm_type via vm_file (kernel).
If userspace changes this on stage2, some action might not
work. for example, if userspace is expecting vm_type to be
COCO VM and preserved vm_type is shared, this will have
conflict when userspace will try to perform some operation that
only works with COCO VM.
My Question is: Why should someone change the shape, unless they are
planning to make the vm fail?
>
>> +
>> + vm->kvm_fd = open_path_or_exit(KVM_DEV_PATH, O_RDWR);
>> + vm->fd = resurrected_vm_fd;
>> +
>> + if (kvm_has_cap(KVM_CAP_BINARY_STATS_FD))
>> + vm->stats.fd = vm_get_stats_fd(vm);
>> + else
>> + vm->stats.fd = -1;
>> +
>> + vm_init_memory_properties(vm);
>> +
>> + return vm;
>> +}
>> +
>
> I think vm_create_from_fd() could be introduced in an earlier patch to
> reduce the amount of new code in this patch. Also, I think it could
> perhaps be moved to kvm_util.c assuming that other test will use it too.
>
Makes sense, Will take of it next revision v4.
>> +static void do_phase2(void)
>> +{
>> + int retrieved_vm_fd, retrieved_gmem_fd, dev_luo_fd, session_fd;
>> + struct vm_shape shape = VM_SHAPE_DEFAULT;
>> + const uint64_t gpa = SZ_4G;
>> + struct kvm_vcpu *vcpu;
>> + const int slot = 1;
>> + struct kvm_vm *vm;
>> +
>> + dev_luo_fd = luo_open_device();
>> + TEST_ASSERT(dev_luo_fd >= 0, "Failed to open /dev/liveupdate");
>> +
>> + session_fd = luo_retrieve_session(dev_luo_fd, SESSION_NAME);
>> + TEST_ASSERT(session_fd >= 0, "Failed to retrieve LUO session");
>> +
>> + retrieved_vm_fd = luo_session_retrieve_fd(session_fd, VM_TOKEN);
>> + TEST_ASSERT(retrieved_vm_fd >= 0, "Failed to retrieve VM file descriptor");
>> +
>> + retrieved_gmem_fd = luo_session_retrieve_fd(session_fd, GMEM_TOKEN);
>> + TEST_ASSERT(retrieved_gmem_fd >= 0, "Failed to retrieve guest_memfd file descriptor");
>> +
>> + vm = vm_create_from_fd(retrieved_vm_fd, shape);
>> +
>> + u64 nr_pages = 2048; /* 8MB is plenty for slot0 pages */
>> +
>
> I don't think declarations are usually mixed with regular code.
Okay, will udpate that.
>
>> + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, nr_pages, 0);
>> + kvm_vm_elf_load(vm, program_invocation_name);
>> +
>> + for (int i = 0; i < NR_MEM_REGIONS; i++)
>> + vm->memslots[i] = 0;
>> +
>> + struct userspace_mem_region *slot0 = memslot2region(vm, 0);
>> +
>> + ucall_init(vm, slot0->region.guest_phys_addr + slot0->region.memory_size);
>> +
>> + vm_set_user_memory_region2(vm, slot, KVM_MEM_GUEST_MEMFD, gpa, GMEM_SIZE, NULL,
>> + retrieved_gmem_fd, 0);
>> +
>> + for (size_t i = 0; i < GMEM_SIZE; i += page_size)
>> + virt_pg_map(vm, gpa + i, gpa + i);
>> +
>> + vcpu = vm_vcpu_add(vm, 0, guest_code_phase2);
>> + kvm_arch_vm_finalize_vcpus(vm);
>> +
>> + vcpu_args_set(vcpu, 3, gpa, GMEM_SIZE, DATA_SIZE);
>> +
>> + printf("Resuming / Running VM in Phase 2...\n");
>> + vcpu_run(vcpu);
>> + TEST_ASSERT_EQ(get_ucall(vcpu, NULL), UCALL_DONE);
>> +
>> + printf("\nSUCCESS: Phase 2 Complete! All 5MB complex data verified intact!\n");
>> +
>> + luo_session_finish(session_fd);
>> + close(session_fd);
>> + close(dev_luo_fd);
>> + /* This will also close the vm_fd */
>> + kvm_vm_free(vm);
>> + close(retrieved_gmem_fd);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + bool phase2 = false;
>> +
>> + TEST_REQUIRE(kvm_has_cap(KVM_CAP_GUEST_MEMFD));
>> + page_size = getpagesize();
>> +
>> + for (int i = 1; i < argc; i++) {
>> + if (strcmp(argv[i], "--phase2") == 0)
>> + phase2 = true;
>> + }
>> +
>
> Maybe use getopt() here?
In V3, it is update to use liveupdate library.
>
>> + if (phase2)
>> + do_phase2();
>> + else
>> + do_phase1();
>> +
>> + return 0;
>> +}
>> --
>> 2.54.0.1032.g2f8565e1d1-goog
>
> I think we also need tests for trying to allocate while frozen, and
> conversion while frozen, and trying to preserve while preservation is
> not allowed.
Yes, We need those tests. For this series, I wanted to focus on design.
Now that we are aligned, next revision, I will send with more tests.
~Tarun
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.