Re: [PATCH v7 05/11] kselftest: arm64: mangle_pstate_ssbs_regs

[Dave Martin <Dave.Martin@arm.com>]

On Mon, Oct 07, 2019 at 07:29:48pm +0100, Cristian Marussi wrote:
> Add a simple mangle testcase which messes with the ucontext_t from within
> the signal handler, trying to set the PSTATE SSBS bit and verify that
> SSBS bit set is preserved across sigreturn.
> Lookup PSTATE.SSBS directly using dedicated helper to grab PSTATE from a
> live sigframe.
> 
> Additionally, in order to support this test specific needs:
> - extend signal testing framework to allow the definition of a custom
>   per test initialization function to be run at the end of test setup
>   and before test run routine. This will support also test SKIP.
> - introduce also a new common utility function: get_current_context()
>   which can be used to grab a ucontext without the help of libc, and
>   detect if such ucontext has been actively used to jump back.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v6 --> v7
> - fixed missing header for memcpy
> - fixed misleading comment in get_current_context()
> - fixed retvalue checks on get_current_context() invocation
> - extend test_init()/test_result() and .init to report KSFT_SKIP
> - SKIP mangle_pstate_ssbs_regs if SSBS not supported at all
> - check SSBS support looking up ID_AA64PFR1_EL1.SSBS in test_init()
>   instead of using MRS/MSR

[...]

> diff --git a/tools/testing/selftests/arm64/signal/test_signals.c b/tools/testing/selftests/arm64/signal/test_signals.c
> index cb970346b280..416b1ff43199 100644
> --- a/tools/testing/selftests/arm64/signal/test_signals.c
> +++ b/tools/testing/selftests/arm64/signal/test_signals.c
> @@ -19,11 +19,11 @@ int main(int argc, char *argv[])
>  	current = &tde;
>  
>  	ksft_print_msg("%s :: %s\n", current->name, current->descr);
> -	if (test_setup(current)) {
> +	if (test_setup(current) && test_init(current)) {
>  		test_run(current);
> -		test_result(current);
>  		test_cleanup(current);
>  	}
> +	test_result(current);
>  
> -	return current->pass ? KSFT_PASS : KSFT_FAIL;
> +	return current->result;
>  }
> diff --git a/tools/testing/selftests/arm64/signal/test_signals.h b/tools/testing/selftests/arm64/signal/test_signals.h
> index f712b5daa10b..ad8175806034 100644
> --- a/tools/testing/selftests/arm64/signal/test_signals.h
> +++ b/tools/testing/selftests/arm64/signal/test_signals.h
> @@ -27,13 +27,25 @@
>  	: "memory");					\
>  }
>  
> +#define set_regval(regname, in)				\
> +{							\
> +	asm volatile("msr " __stringify(regname) ", %0" \
> +	:						\
> +	: "r" (in)					\
> +	: "memory");					\
> +}

Unused macro?  Now that the test doesn't try to change the SSBS state
via MSR, I don't think anything else is using this.

[...]

> diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
> new file mode 100644
> index 000000000000..780161dbd7c0
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 ARM Limited
> + *
> + * Try to mangle the ucontext from inside a signal handler, setting the
> + * SSBS bit to 1 and veryfing that such modification is preserved.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <signal.h>
> +#include <ucontext.h>
> +
> +#include <kselftest.h>
> +
> +#include "test_signals_utils.h"
> +#include "testcases.h"
> +
> +static bool mangle_invalid_pstate_ssbs_init(struct tdescr *td)
> +{
> +	bool ret;
> +
> +	ret = feats_ok(td);
> +	if (!ret) {
> +		fprintf(stderr, "%s: unsupported feature - SKIP.\n", td->name);
> +		td->result = KSFT_SKIP;
> +	}
> +
> +	return ret;
> +}
> +
> +static int mangle_invalid_pstate_ssbs_run(struct tdescr *td,
> +					  siginfo_t *si, ucontext_t *uc)
> +{
> +	ASSERT_GOOD_CONTEXT(uc);
> +
> +	/* set bit value ... should NOT be cleared by Kernel on sigreturn */
> +	uc->uc_mcontext.pstate |= PSR_SSBS_BIT;
> +	fprintf(stderr, "SSBS set to 1 -- PSTATE: 0x%016llX\n",
> +		uc->uc_mcontext.pstate);
> +	/* Save after mangling...it should be preserved */
> +	td->saved_uc = *uc;

Hmmm, now I'm wondering about how things like
prctl(PR_SET_SPECULATION_CTRL) are supposed to interact with other ways
of manipulating PSTATE.SSBS.  Before we've answered this, we don't know
what result to expect from this test in various configurations...

To avoid this series depending on answering that question immediately,
can we drop this test from the series for now?

To reduce rework effort, maybe keep this patch in its current position
in the series, with just the utility code, but drop the testcase.

Sorry for the churn -- I didn't think of this issue earlier :(

[...]

Cheers
---Dave