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

[Mark Rutland <mark.rutland@arm.com>]

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>
>