[PATCH] KVM: selftests: add test for SVE host corruption

Mark Brown posted 1 patch 8 months ago
tools/testing/selftests/kvm/Makefile.kvm     |   1 +
tools/testing/selftests/kvm/arm64/host_sve.c | 127 +++++++++++++++++++++++++++
2 files changed, 128 insertions(+)
[PATCH] KVM: selftests: add test for SVE host corruption
Posted by Mark Brown 8 months ago
This test program, originally written by Mark Rutland and lightly modified
by me for upstream, verifies that we do not have the issues with host SVE
state being discarded which were fixed in

   fbc7e61195e2 ("KVM: arm64: Unconditionally save+flush host FPSIMD/SVE/SME state")

by running a simple VM while checking the SVE register state for
corruption.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/kvm/Makefile.kvm     |   1 +
 tools/testing/selftests/kvm/arm64/host_sve.c | 127 +++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index f62b0a5aba35..d37072054a3d 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -147,6 +147,7 @@ TEST_GEN_PROGS_arm64 = $(TEST_GEN_PROGS_COMMON)
 TEST_GEN_PROGS_arm64 += arm64/aarch32_id_regs
 TEST_GEN_PROGS_arm64 += arm64/arch_timer_edge_cases
 TEST_GEN_PROGS_arm64 += arm64/debug-exceptions
+TEST_GEN_PROGS_arm64 += arm64/host_sve
 TEST_GEN_PROGS_arm64 += arm64/hypercalls
 TEST_GEN_PROGS_arm64 += arm64/mmio_abort
 TEST_GEN_PROGS_arm64 += arm64/page_fault_test
diff --git a/tools/testing/selftests/kvm/arm64/host_sve.c b/tools/testing/selftests/kvm/arm64/host_sve.c
new file mode 100644
index 000000000000..3826772fd470
--- /dev/null
+++ b/tools/testing/selftests/kvm/arm64/host_sve.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Host SVE: Check FPSIMD/SVE/SME save/restore over KVM_RUN ioctls.
+ *
+ * Copyright 2025 Arm, Ltd
+ */
+
+#include <errno.h>
+#include <signal.h>
+#include <sys/auxv.h>
+#include <asm/kvm.h>
+#include <kvm_util.h>
+
+#include "ucall_common.h"
+
+static void guest_code(void)
+{
+	for (int i = 0; i < 10; i++) {
+		GUEST_UCALL_NONE();
+	}
+
+	GUEST_DONE();
+}
+
+void handle_sigill(int sig, siginfo_t *info, void *ctx)
+{
+	ucontext_t *uctx = ctx;
+
+	printf("  < host signal %d >\n", sig);
+
+	/*
+	 * Skip the UDF
+	 */
+	uctx->uc_mcontext.pc += 4;
+}
+
+void register_sigill_handler(void)
+{
+	struct sigaction sa = {
+		.sa_sigaction = handle_sigill,
+		.sa_flags = SA_SIGINFO,
+	};
+	sigaction(SIGILL, &sa, NULL);
+}
+
+static void do_sve_roundtrip(void)
+{
+	unsigned long before, after;
+
+	/*
+	 * Set all bits in a predicate register, force a save/restore via a
+	 * SIGILL (which handle_sigill() will recover from), then report
+	 * whether the value has changed.
+	 */
+	asm volatile(
+	"	.arch_extension sve\n"
+	"	ptrue	p0.B\n"
+	"	cntp	%[before], p0, p0.B\n"
+	"	udf #0\n"
+	"	cntp	%[after], p0, p0.B\n"
+	: [before] "=r" (before),
+	  [after] "=r" (after)
+	:
+	: "p0"
+	);
+
+	if (before != after) {
+		TEST_FAIL("Signal roundtrip discarded predicate bits (%ld => %ld)\n",
+			  before, after);
+	} else {
+		printf("Signal roundtrip preserved predicate bits (%ld => %ld)\n",
+		       before, after);
+	}
+}
+
+static void test_run(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	struct ucall uc;
+	bool guest_done = false;
+
+	register_sigill_handler();
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+
+	do_sve_roundtrip();
+
+	while (!guest_done) {
+
+		printf("Running VCPU...\n");
+		vcpu_run(vcpu);
+
+		switch (get_ucall(vcpu, &uc)) {
+		case UCALL_NONE:
+			do_sve_roundtrip();
+			do_sve_roundtrip();
+			break;
+		case UCALL_DONE:
+			guest_done = true;
+			break;
+		case UCALL_ABORT:
+			REPORT_GUEST_ASSERT(uc);
+			break;
+		default:
+			TEST_FAIL("Unexpected guest exit");
+		}
+	}
+
+	kvm_vm_free(vm);
+}
+
+int main(void)
+{
+	/*
+	 * This is testing the host environment, we don't care about
+	 * guest SVE support.
+	 */
+	if (!(getauxval(AT_HWCAP) & HWCAP_SVE)) {
+		printf("SVE not supported\n");
+		return KSFT_SKIP;
+	}
+
+	test_run();
+	return 0;
+}

---
base-commit: 8ffd015db85fea3e15a77027fda6c02ced4d2444
change-id: 20250226-kvm-selftest-sve-signal-1add0d9d716c

Best regards,
-- 
Mark Brown <broonie@kernel.org>
Re: [PATCH] KVM: selftests: add test for SVE host corruption
Posted by Marc Zyngier 7 months, 1 week ago
On Thu, 17 Apr 2025 00:32:49 +0100, Mark Brown wrote:
> This test program, originally written by Mark Rutland and lightly modified
> by me for upstream, verifies that we do not have the issues with host SVE
> state being discarded which were fixed in
> 
>    fbc7e61195e2 ("KVM: arm64: Unconditionally save+flush host FPSIMD/SVE/SME state")
> 
> by running a simple VM while checking the SVE register state for
> corruption.
> 
> [...]

Applied to kvm-arm64/misc-6.16, thanks!

[1/1] KVM: selftests: add test for SVE host corruption
      commit: e0ccc45b056d626d4b271820faeedf3837337ceb

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.
Re: [PATCH] KVM: selftests: add test for SVE host corruption
Posted by Mark Rutland 7 months, 2 weeks ago
On Thu, Apr 17, 2025 at 12:32:49AM +0100, Mark Brown wrote:
> This test program, originally written by Mark Rutland and lightly modified
> by me for upstream,

For context, I had originally pushed this as a WIP to:

  https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/kvm/fpsimd-tests&id=a2f7319f5b13f5f5354e6186925b3bb8f2d2966e

> verifies that we do not have the issues with host SVE
> state being discarded which were fixed in
> 
>    fbc7e61195e2 ("KVM: arm64: Unconditionally save+flush host FPSIMD/SVE/SME state")
> 
> by running a simple VM while checking the SVE register state for
> corruption.

Minor nit, but this doesn't verify the absence of the issue, as that can
be masked by preemption. I would suggest:

| Until recently, the kernel could unexpectedly discard SVE state for a
| period after a KVM_RUN ioctl, when the guest did not execute any
| FPSIMD/SVE/SME instructions. We fixed that issue in commit:
|
|   fbc7e61195e2 ("KVM: arm64: Unconditionally save+flush host FPSIMD/SVE/SME state")
|
| Add a test which tries to provoke that issue by manipulating SVE state
| before/after running a guest which does not execute any FPSIMD/SVE/SME
| instructions. The test executes a handful of iterations to miminize
| the risk that the issue is masked by preemption.

> Signed-off-by: Mark Brown <broonie@kernel.org>

Looks like my Signed-off-by got dropped by accident; it should be above
yours here.

Aside from that, and the initial feature test, this looks pretty much
identical to my original WIP.

I'm not sure if it's worth keeping all the printf() calls, which were
only there to help me check the UDF trap was being handled correctly.

Either way, with the commit message updated and SoB restored:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  tools/testing/selftests/kvm/Makefile.kvm     |   1 +
>  tools/testing/selftests/kvm/arm64/host_sve.c | 127 +++++++++++++++++++++++++++
>  2 files changed, 128 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index f62b0a5aba35..d37072054a3d 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -147,6 +147,7 @@ TEST_GEN_PROGS_arm64 = $(TEST_GEN_PROGS_COMMON)
>  TEST_GEN_PROGS_arm64 += arm64/aarch32_id_regs
>  TEST_GEN_PROGS_arm64 += arm64/arch_timer_edge_cases
>  TEST_GEN_PROGS_arm64 += arm64/debug-exceptions
> +TEST_GEN_PROGS_arm64 += arm64/host_sve
>  TEST_GEN_PROGS_arm64 += arm64/hypercalls
>  TEST_GEN_PROGS_arm64 += arm64/mmio_abort
>  TEST_GEN_PROGS_arm64 += arm64/page_fault_test
> diff --git a/tools/testing/selftests/kvm/arm64/host_sve.c b/tools/testing/selftests/kvm/arm64/host_sve.c
> new file mode 100644
> index 000000000000..3826772fd470
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/arm64/host_sve.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Host SVE: Check FPSIMD/SVE/SME save/restore over KVM_RUN ioctls.
> + *
> + * Copyright 2025 Arm, Ltd
> + */
> +
> +#include <errno.h>
> +#include <signal.h>
> +#include <sys/auxv.h>
> +#include <asm/kvm.h>
> +#include <kvm_util.h>
> +
> +#include "ucall_common.h"
> +
> +static void guest_code(void)
> +{
> +	for (int i = 0; i < 10; i++) {
> +		GUEST_UCALL_NONE();
> +	}
> +
> +	GUEST_DONE();
> +}
> +
> +void handle_sigill(int sig, siginfo_t *info, void *ctx)
> +{
> +	ucontext_t *uctx = ctx;
> +
> +	printf("  < host signal %d >\n", sig);
> +
> +	/*
> +	 * Skip the UDF
> +	 */
> +	uctx->uc_mcontext.pc += 4;
> +}
> +
> +void register_sigill_handler(void)
> +{
> +	struct sigaction sa = {
> +		.sa_sigaction = handle_sigill,
> +		.sa_flags = SA_SIGINFO,
> +	};
> +	sigaction(SIGILL, &sa, NULL);
> +}
> +
> +static void do_sve_roundtrip(void)
> +{
> +	unsigned long before, after;
> +
> +	/*
> +	 * Set all bits in a predicate register, force a save/restore via a
> +	 * SIGILL (which handle_sigill() will recover from), then report
> +	 * whether the value has changed.
> +	 */
> +	asm volatile(
> +	"	.arch_extension sve\n"
> +	"	ptrue	p0.B\n"
> +	"	cntp	%[before], p0, p0.B\n"
> +	"	udf #0\n"
> +	"	cntp	%[after], p0, p0.B\n"
> +	: [before] "=r" (before),
> +	  [after] "=r" (after)
> +	:
> +	: "p0"
> +	);
> +
> +	if (before != after) {
> +		TEST_FAIL("Signal roundtrip discarded predicate bits (%ld => %ld)\n",
> +			  before, after);
> +	} else {
> +		printf("Signal roundtrip preserved predicate bits (%ld => %ld)\n",
> +		       before, after);
> +	}
> +}
> +
> +static void test_run(void)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	struct ucall uc;
> +	bool guest_done = false;
> +
> +	register_sigill_handler();
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +
> +	do_sve_roundtrip();
> +
> +	while (!guest_done) {
> +
> +		printf("Running VCPU...\n");
> +		vcpu_run(vcpu);
> +
> +		switch (get_ucall(vcpu, &uc)) {
> +		case UCALL_NONE:
> +			do_sve_roundtrip();
> +			do_sve_roundtrip();
> +			break;
> +		case UCALL_DONE:
> +			guest_done = true;
> +			break;
> +		case UCALL_ABORT:
> +			REPORT_GUEST_ASSERT(uc);
> +			break;
> +		default:
> +			TEST_FAIL("Unexpected guest exit");
> +		}
> +	}
> +
> +	kvm_vm_free(vm);
> +}
> +
> +int main(void)
> +{
> +	/*
> +	 * This is testing the host environment, we don't care about
> +	 * guest SVE support.
> +	 */
> +	if (!(getauxval(AT_HWCAP) & HWCAP_SVE)) {
> +		printf("SVE not supported\n");
> +		return KSFT_SKIP;
> +	}
> +
> +	test_run();
> +	return 0;
> +}
> 
> ---
> base-commit: 8ffd015db85fea3e15a77027fda6c02ced4d2444
> change-id: 20250226-kvm-selftest-sve-signal-1add0d9d716c
> 
> Best regards,
> -- 
> Mark Brown <broonie@kernel.org>
>
Re: [PATCH] KVM: selftests: add test for SVE host corruption
Posted by Mark Brown 7 months, 2 weeks ago
On Tue, Apr 29, 2025 at 04:27:27PM +0100, Mark Rutland wrote:
> On Thu, Apr 17, 2025 at 12:32:49AM +0100, Mark Brown wrote:

> > Signed-off-by: Mark Brown <broonie@kernel.org>

> Looks like my Signed-off-by got dropped by accident; it should be above
> yours here.

> Aside from that, and the initial feature test, this looks pretty much
> identical to my original WIP.

Sorry, for a while I had a version of the program that was a lot more
modified (I was trying to make it work with a wider range of supervised
programs) so it didn't feel right to carry your SoB forward, but those
didn't actually go all the way to working enough to enable any of the
additional programs I was looking at so I ended up unwinding them and
didn't also unwind that change.

> I'm not sure if it's worth keeping all the printf() calls, which were
> only there to help me check the UDF trap was being handled correctly.

I've found them handy when using it.