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

[Ricardo Koller <ricarkol@google.com>]

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 }
> > +};
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm