[PATCH v2 0/2] Fix error handling for async_pf

[PATCH v2 0/2] Fix error handling for async_pf

[Christian Borntraeger]

Return mapping errors also for async_pf cases. Otherwise guests
will loop forever. Nothing critical just missing error handling.

v1->v2:
- !npages instead of npages == 0 (Sean)
- new block comment (Sean)
- fix testcase to build with 7.3 and 7.4-rc

Christian Borntraeger (2):
  KVM: Set up async page faults only for faults that need to wait
  KVM: selftests: Test that KVM_RUN fails for unresolvable host faults

 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/faultin_sigbus_test.c       | 169 ++++++++++++++++++
 virt/kvm/kvm_main.c                           |   3 +-
 3 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/kvm/faultin_sigbus_test.c

-- 
2.53.0

[PATCH v2 1/2] KVM: Set up async page faults only for faults that need to wait

[Christian Borntraeger]

hva_to_pfn() checks a FOLL_NOWAIT gup() for EINTR, EAGAIN and EHWPOISON.
There might be others like -EFAULT for VM_FAULT_SIGBUS.  For those other
errors, reporting KVM_PFN_ERR_NEEDS_IO would be wrong as it triggers an
async_pf resulting in endless retries.

Return KVM_PFN_ERR_NEEDS_IO only if gup() returned 0 pages and report
the error otherwise.

Fixes: 0857b9e95c1a ("KVM: Enable async page fault processing")
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 virt/kvm/kvm_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 5a782849521c..7b6172810f34 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3047,7 +3047,8 @@ kvm_pfn_t hva_to_pfn(struct kvm_follow_pfn *kfp)
 		if (r < 0)
 			pfn = KVM_PFN_ERR_FAULT;
 	} else {
-		if ((kfp->flags & FOLL_NOWAIT) &&
+		/* Note, GUP returns '0' if retry is needed. */
+		if (!npages && (kfp->flags & FOLL_NOWAIT) &&
 		    vma_is_valid(vma, kfp->flags & FOLL_WRITE))
 			pfn = KVM_PFN_ERR_NEEDS_IO;
 		else
-- 
2.53.0

[PATCH v2 2/2] KVM: selftests: Test that KVM_RUN fails for unresolvable host faults

[Christian Borntraeger]

Add a test in which the guest writes to a shmem-backed memslot beyond
the end of the file.  KVM_RUN must fail with -EFAULT.

On x86 the guest enables interrupts, and on s390 the test sets up
pfault, so that KVM could handle the fault with an asynchronous page
fault.  If KVM mistakes the error for a fault that needs I/O, the vCPU
retries the access forever, and the test fails after a timeout.

The test is not built for LoongArch, which handles a failed mapping as
an MMIO access.

Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/faultin_sigbus_test.c       | 169 ++++++++++++++++++
 2 files changed, 170 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/faultin_sigbus_test.c

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index 68bcec3d0070..24fdfc3e0eba 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -59,6 +59,7 @@ TEST_PROGS_x86 += x86/nx_huge_pages_test.sh
 # Compiled test targets valid on all architectures with libkvm support
 TEST_GEN_PROGS_COMMON = demand_paging_test
 TEST_GEN_PROGS_COMMON += dirty_log_test
+TEST_GEN_PROGS_COMMON += faultin_sigbus_test
 TEST_GEN_PROGS_COMMON += guest_print_test
 TEST_GEN_PROGS_COMMON += irqfd_test
 TEST_GEN_PROGS_COMMON += kvm_binary_stats_test
diff --git a/tools/testing/selftests/kvm/faultin_sigbus_test.c b/tools/testing/selftests/kvm/faultin_sigbus_test.c
new file mode 100644
index 000000000000..9394b04efcd6
--- /dev/null
+++ b/tools/testing/selftests/kvm/faultin_sigbus_test.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * A guest access to memory whose host mapping cannot be faulted in (here: a
+ * shmem page beyond EOF, i.e. SIGBUS) must make KVM_RUN fail with -EFAULT.
+ *
+ * On architectures with asynchronous page faults, KVM first tries to fault
+ * the page in without waiting (FOLL_NOWAIT) and hands the fault to a worker
+ * if that would need I/O.  The error must not be mistaken for "needs I/O",
+ * otherwise the vCPU retries the access forever and KVM_RUN never returns.
+ * The architecture hooks below make sure that the async path is actually
+ * possible, i.e. that the error would be misclassified if KVM got it wrong.
+ */
+#include <signal.h>
+#include <unistd.h>
+#include <linux/sizes.h>
+#include <linux/stringify.h>
+
+#include "kvm_util.h"
+#include "processor.h"
+#include "test_util.h"
+#include "ucall_common.h"
+
+#define TEST_SLOT	10
+#define TEST_SIZE	SZ_1M
+#define TIMEOUT_SECS	10
+
+#ifdef __s390x__
+#define __LC_EXT_NEW_PSW		0x1b0
+#define __LC_SAVED_PSW			0x200	/* free for the OS */
+#define __LC_WAIT_PSW			0x210	/* free for the OS */
+#define PSW_MASK_EXT			0x0100000000000000ULL
+#define PSW_MASK_DAT			0x0400000000000000ULL
+#define PSW_MASK_WAIT			0x0002000000000000ULL
+#define CR0_SERVICE_SIGNAL_SUBMASK	BIT_ULL(63 - 54)
+
+/*
+ * Handle pfault like Linux does: on the init interrupt (ext cpu address
+ * 0x0600) remember the interrupted PSW and wait until the done interrupt
+ * (0x0680), then retry the access.  A guest that retried at once would
+ * fill up KVM's queue of async page faults and get the synchronous fault
+ * path.  Runs with DAT off, so lowcore is addressed by real addresses:
+ * 0x84 is the ext cpu address, 0x130 the ext old PSW.  Done differs from
+ * init only in bit 0x80 of 0x85, test it like Linux tests PF_COMPLETE.
+ */
+extern char ext_int_handler[];
+asm(".pushsection .text\n"
+	"ext_int_handler:\n"
+	"	tm	0x85,0x80\n"
+	"	jo	1f\n"
+	"	mvc	" __stringify(__LC_SAVED_PSW) "(16),0x130\n"
+	"	lpswe	" __stringify(__LC_WAIT_PSW) "\n"
+	"1:	lpswe	" __stringify(__LC_SAVED_PSW) "\n"
+	".popsection\n");
+
+static void arch_enable_async_pf(struct kvm_vm *vm, struct kvm_vcpu *vcpu)
+{
+	u64 *lowcore = addr_gpa2hva(vm, 0);
+	struct kvm_sregs sregs;
+	gva_t token_gva;
+	gpa_t handler;
+	int flic;
+
+	flic = kvm_create_device(vm, KVM_DEV_TYPE_FLIC);
+	kvm_device_attr_set(flic, KVM_DEV_FLIC_APF_ENABLE, 0, NULL);
+
+	/* The DIAG 0x258 handshake, with masks that match any guest PSW. */
+	token_gva = vm_alloc_page(vm);
+	vcpu_set_reg(vcpu, KVM_REG_S390_PFTOKEN, addr_gva2gpa(vm, token_gva));
+	vcpu_set_reg(vcpu, KVM_REG_S390_PFSELECT, 0);
+	vcpu_set_reg(vcpu, KVM_REG_S390_PFCOMPARE, 0);
+
+	vcpu_sregs_get(vcpu, &sregs);
+	sregs.crs[0] |= CR0_SERVICE_SIGNAL_SUBMASK;
+	vcpu_sregs_set(vcpu, &sregs);
+	vcpu->run->psw_mask |= PSW_MASK_EXT;
+
+	handler = addr_gva2gpa(vm, (gva_t)ext_int_handler);
+	lowcore[__LC_EXT_NEW_PSW / 8] = vcpu->run->psw_mask &
+					~(PSW_MASK_EXT | PSW_MASK_DAT);
+	lowcore[__LC_EXT_NEW_PSW / 8 + 1] = handler;
+	lowcore[__LC_WAIT_PSW / 8] = (vcpu->run->psw_mask & ~PSW_MASK_DAT) |
+				     PSW_MASK_WAIT;
+	lowcore[__LC_WAIT_PSW / 8 + 1] = handler;
+}
+#else
+/* x86 needs guest interrupts on, see guest_code(); nothing to do elsewhere. */
+static void arch_enable_async_pf(struct kvm_vm *vm, struct kvm_vcpu *vcpu)
+{
+}
+#endif
+
+static void guest_code(u64 gva, u64 size)
+{
+#ifdef __x86_64__
+	/* KVM does async #PF, or its halt fallback, only with IRQs enabled. */
+	sti_nop();
+#endif
+	/* Within EOF: must work, proves that the mapping is set up right. */
+	WRITE_ONCE(*(u64 *)gva, 1);
+	GUEST_SYNC(0);
+
+	/* Beyond EOF: KVM_RUN must fail with -EFAULT. */
+	WRITE_ONCE(*(u64 *)(gva + size - sizeof(u64)), 1);
+	GUEST_FAIL("Store beyond EOF succeeded");
+}
+
+static void sig_alarm(int sig)
+{
+}
+
+int main(void)
+{
+	struct sigaction sa = { .sa_handler = sig_alarm };	/* no SA_RESTART */
+	struct userspace_mem_region *region;
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	struct ucall uc;
+	gpa_t gpa;
+	gva_t gva;
+	u64 npages;
+	int r;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+
+	/* After slot 0 and the page that some architectures use for ucalls. */
+	region = memslot2region(vm, 0);
+	gpa = align_up(region->region.guest_phys_addr +
+		       region->region.memory_size + vm->page_size, TEST_SIZE);
+	npages = TEST_SIZE / vm->page_size;
+
+	vm_userspace_mem_region_add(vm, VM_MEM_SRC_SHMEM, gpa, TEST_SLOT,
+				    npages, 0);
+	region = memslot2region(vm, TEST_SLOT);
+	/* Only the first host page is backed, everything behind it is SIGBUS. */
+	TEST_ASSERT(!ftruncate(region->fd, getpagesize()), "ftruncate() failed");
+
+	gva = vm_unused_gva_gap(vm, TEST_SIZE, KVM_UTIL_MIN_VADDR);
+	virt_map(vm, gva, gpa, npages);
+	vcpu_args_set(vcpu, 2, gva, TEST_SIZE);
+
+	arch_enable_async_pf(vm, vcpu);
+
+	vcpu_run(vcpu);
+	switch (get_ucall(vcpu, &uc)) {
+	case UCALL_SYNC:
+		break;
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT(uc);
+		break;
+	default:
+		TEST_FAIL("Unexpected ucall %lu", uc.cmd);
+	}
+	TEST_ASSERT_EQ(*(u64 *)region->host_mem, 1);
+
+	TEST_ASSERT(!sigaction(SIGALRM, &sa, NULL), "sigaction() failed");
+	alarm(TIMEOUT_SECS);
+	r = __vcpu_run(vcpu);
+	alarm(0);
+
+	TEST_ASSERT(!(r == -1 && errno == EINTR),
+		    "KVM_RUN did not return within %d seconds: the fault on the SIGBUS page is retried forever",
+		    TIMEOUT_SECS);
+	TEST_ASSERT(r == -1 && errno == EFAULT,
+		    "Expected KVM_RUN to fail with EFAULT, got r = %d, errno = %d (%s), exit reason %s",
+		    r, errno, strerror(errno), exit_reason_str(vcpu->run->exit_reason));
+
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.53.0

Re: [PATCH v2 1/2] KVM: Set up async page faults only for faults that need to wait

[sashiko-bot]

> hva_to_pfn() checks a FOLL_NOWAIT gup() for EINTR, EAGAIN and EHWPOISON.
> There might be others like -EFAULT for VM_FAULT_SIGBUS.  For those other
> errors, reporting KVM_PFN_ERR_NEEDS_IO would be wrong as it triggers an
> async_pf resulting in endless retries.
> 
> Return KVM_PFN_ERR_NEEDS_IO only if gup() returned 0 pages and report
> the error otherwise.
> 
> Fixes: 0857b9e95c1a ("KVM: Enable async page fault processing")
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020175818.375700-1-borntraeger@linux.ibm.com?part=1

Re: [PATCH v2 2/2] KVM: selftests: Test that KVM_RUN fails for unresolvable host faults

[sashiko-bot]

> Add a test in which the guest writes to a shmem-backed memslot beyond
> the end of the file.  KVM_RUN must fail with -EFAULT.
> 
> On x86 the guest enables interrupts, and on s390 the test sets up
> pfault, so that KVM could handle the fault with an asynchronous page
> fault.  If KVM mistakes the error for a fault that needs I/O, the vCPU
> retries the access forever, and the test fails after a timeout.
> 
> The test is not built for LoongArch, which handles a failed mapping as
> an MMIO access.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020175818.375700-1-borntraeger@linux.ibm.com?part=2