On Thu, Sep 22, 2022 at 07:32:42PM +0000, Sean Christopherson wrote:
> 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.
Interestingly, we used to refer those as "gaps", as in "gaps between memslots".
But I get the point.
>
> 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
Ack.
> 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?
>
It fails with EINVAL (only tried on arm) for both the PAGE_SIZE and the huge
page size. And note that the address is aligned as well.
madvise(0xffffb7c00000, 2097152, MADV_DONTNEED) = -1 EINVAL (Invalid argument)
^^^^^^^^^^^^^^ ^^^^^^^
2M aligned 2M (hugepage size)
madvise(0xffff9e800000, 4096, MADV_DONTNEED) = -1 EINVAL (Invalid argument)
^^^^
PAGE_SIZE
> > +
> > + 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.
Ack.
>
> 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.
Moreover, the behavior from the guest POV should be the same as userspace
reading/writing on a hole (with PUNCH_HOLE). Will describe this as well.
>
> 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 }
> > +};
