On Thu, Sep 22, 2022, Ricardo Koller wrote:
> +/* Returns true to continue the test, and false if it should be skipped. */
> +static bool punch_hole_in_memslot(struct kvm_vm *vm,
This is a very misleading name, and IMO is flat out wrong. The helper isn't
punching a hole in the memslot, it's punching a hole in the backing store, and
those are two very different things. Encountering a hole in a _memslot_ yields
emualted MMIO semantics, not CoW zero page semantics.
Ideally, if we can come up with a not awful name, I'd also prefer to avoid "punch
hole" in the function name. I can't think of a better alternative, so it's not
the end of the world if we're stuck with e.g punch_hole_in_backing_store(), but I
think the "punch_hole" name will be confusing for readers that are unfamiliar with
PUNCH_HOLE, especially for anonymous memory as "punching a hole" in anonymous
memory is more likely to be interpreted as "munmap()".
> + struct userspace_mem_region *region)
> +{
> + void *hva = (void *)region->region.userspace_addr;
> + uint64_t paging_size = region->region.memory_size;
> + int ret, fd = region->fd;
> +
> + if (fd != -1) {
> + ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> + 0, paging_size);
> + TEST_ASSERT(ret == 0, "fallocate failed, errno: %d\n", errno);
> + } else {
> + if (is_backing_src_hugetlb(region->backing_src_type))
> + return false;
Why is hugetlb disallowed? I thought anon hugetlb supports MADV_DONTNEED?
> +
> + ret = madvise(hva, paging_size, MADV_DONTNEED);
> + TEST_ASSERT(ret == 0, "madvise failed, errno: %d\n", errno);
> + }
> +
> + return true;
> +}
...
> + /*
> + * Accessing a hole in the data memslot (punched with fallocate or
s/memslot/backing store
> + * madvise) shouldn't fault (more sanity checks).
Naming aside, please provide more detail as to why this is the correct KVM
behavior. This is quite subtle and relies on gory implementation details that a
lot of KVM developers will be unaware of.
Specifically, from an accessibility perspective, PUNCH_HOLE doesn't actually create
a hole in the file. The "hole" can still be read and written; the "expect '0'"
checks are correct specifically because those are the semantics of PUNCH_HOLE.
In other words, it's not just that the accesses shouldn't fault, reads _must_
return zeros and writes _must_ re-populate the page.
Compare that with e.g. ftruncate() that makes the size of the file smaller, in
which case an access should result in KVM exiting to userspace with -EFAULT.
> + */
> + TEST_ACCESS(guest_read64, no_af, CMD_HOLE_DATA),
> + TEST_ACCESS(guest_cas, no_af, CMD_HOLE_DATA),
> + TEST_ACCESS(guest_ld_preidx, no_af, CMD_HOLE_DATA),
> + TEST_ACCESS(guest_write64, no_af, CMD_HOLE_DATA),
> + TEST_ACCESS(guest_st_preidx, no_af, CMD_HOLE_DATA),
> + TEST_ACCESS(guest_at, no_af, CMD_HOLE_DATA),
> + TEST_ACCESS(guest_dc_zva, no_af, CMD_HOLE_DATA),
> +
> + { 0 }
> +};
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
