Re: [PATCH v8 10/14] KVM: selftests: aarch64: Add aarch64/page_fault_test

[Sean Christopherson <seanjc@google.com>]

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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm