On Thu, Jan 23, 2025 at 05:54:04PM +0100, Clément Léger wrote:
> Add tests for the FWFT SBI extension. Currently, only the reserved range
> as well as the misaligned exception delegation are used.
>
> Signed-off-by: Clément Léger <cleger@xxxxxxxxxxxx>
> ---
> riscv/Makefile | 2 +-
> lib/riscv/asm/sbi.h | 34 ++++++++
> riscv/sbi-fwft.c | 189 ++++++++++++++++++++++++++++++++++++++++++++
> riscv/sbi.c | 3 +
> 4 files changed, 227 insertions(+), 1 deletion(-)
> create mode 100644 riscv/sbi-fwft.c
>
> diff --git a/riscv/Makefile b/riscv/Makefile
> index 5b5e157c..52718f3f 100644
> --- a/riscv/Makefile
> +++ b/riscv/Makefile
> @@ -17,7 +17,7 @@ tests += $(TEST_DIR)/sieve.$(exe)
>
> all: $(tests)
>
> -$(TEST_DIR)/sbi-deps = $(TEST_DIR)/sbi-asm.o
> +$(TEST_DIR)/sbi-deps = $(TEST_DIR)/sbi-asm.o $(TEST_DIR)/sbi-fwft.o
>
> # When built for EFI sieve needs extra memory, run with e.g. '-m 256' on QEMU
> $(TEST_DIR)/sieve.$(exe): AUXFLAGS = 0x1
> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> index 98a9b097..397400f2 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -11,6 +11,12 @@
> #define SBI_ERR_ALREADY_AVAILABLE -6
> #define SBI_ERR_ALREADY_STARTED -7
> #define SBI_ERR_ALREADY_STOPPED -8
> +#define SBI_ERR_NO_SHMEM -9
> +#define SBI_ERR_INVALID_STATE -10
> +#define SBI_ERR_BAD_RANGE -11
> +#define SBI_ERR_TIMEOUT -12
> +#define SBI_ERR_IO -13
> +#define SBI_ERR_LOCKED -14
>
> #ifndef __ASSEMBLY__
> #include <cpumask.h>
> @@ -23,6 +29,7 @@ enum sbi_ext_id {
> SBI_EXT_SRST = 0x53525354,
> SBI_EXT_DBCN = 0x4442434E,
> SBI_EXT_SUSP = 0x53555350,
> + SBI_EXT_FWFT = 0x46574654,
> };
>
> enum sbi_ext_base_fid {
> @@ -71,6 +78,33 @@ enum sbi_ext_dbcn_fid {
> SBI_EXT_DBCN_CONSOLE_WRITE_BYTE,
> };
>
> +
> +enum sbi_ext_fwft_fid {
> + SBI_EXT_FWFT_SET = 0,
> + SBI_EXT_FWFT_GET,
> +};
> +
> +#define SBI_FWFT_MISALIGNED_EXC_DELEG 0x0
> +#define SBI_FWFT_LANDING_PAD 0x1
> +#define SBI_FWFT_SHADOW_STACK 0x2
> +#define SBI_FWFT_DOUBLE_TRAP 0x3
> +#define SBI_FWFT_PTE_AD_HW_UPDATING 0x4
> +#define SBI_FWFT_POINTER_MASKING_PMLEN 0x5
> +#define SBI_FWFT_LOCAL_RESERVED_START 0x6
> +#define SBI_FWFT_LOCAL_RESERVED_END 0x3fffffff
> +#define SBI_FWFT_LOCAL_PLATFORM_START 0x40000000
> +#define SBI_FWFT_LOCAL_PLATFORM_END 0x7fffffff
> +
> +#define SBI_FWFT_GLOBAL_RESERVED_START 0x80000000
> +#define SBI_FWFT_GLOBAL_RESERVED_END 0xbfffffff
> +#define SBI_FWFT_GLOBAL_PLATFORM_START 0xc0000000
> +#define SBI_FWFT_GLOBAL_PLATFORM_END 0xffffffff
> +
> +#define SBI_FWFT_PLATFORM_FEATURE_BIT BIT(30)
> +#define SBI_FWFT_GLOBAL_FEATURE_BIT BIT(31)
> +
> +#define SBI_FWFT_SET_FLAG_LOCK BIT(0)
> +
> struct sbiret {
> long error;
> long value;
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> new file mode 100644
> index 00000000..7866a917
> --- /dev/null
> +++ b/riscv/sbi-fwft.c
> @@ -0,0 +1,189 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SBI verification
> + *
> + * Copyright (C) 2024, Rivos Inc., Clément Léger <cleger@xxxxxxxxxxxx>
> + */
> +#include <libcflat.h>
> +#include <stdlib.h>
> +
> +#include <asm/csr.h>
> +#include <asm/processor.h>
> +#include <asm/ptrace.h>
> +#include <asm/sbi.h>
> +
> +void check_fwft(void);
> +
> +
> +static struct sbiret fwft_set_raw(unsigned long feature_id, unsigned long value,
> + unsigned long flags)
> +{
> + return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET, feature_id, value,
> + flags, 0, 0, 0);
> +}
> +
> +static struct sbiret fwft_set(uint32_t feature_id, unsigned long value,
> + unsigned long flags)
> +{
> + return fwft_set_raw(feature_id, value, flags);
> +}
> +
> +static struct sbiret fwft_get_raw(unsigned long feature_id)
> +{
> + return sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET, feature_id, 0, 0, 0, 0,
> + 0);
> +}
> +
> +static struct sbiret fwft_get(uint32_t feature_id)
> +{
> + return fwft_get_raw(feature_id);
> +}
nit: s/feature_id/feature/ for all the above, as that would exactly match
the spec, but no biggy.
> +
> +static void fwft_check_reserved(unsigned long id)
> +{
> + struct sbiret ret;
> +
> + ret = fwft_get(id);
> + report(ret.error == SBI_ERR_DENIED,
> + "get reserved feature 0x%lx error == SBI_ERR_DENIED", id);
> +
> + ret = fwft_set(id, 1, 0);
> + report(ret.error == SBI_ERR_DENIED,
> + "set reserved feature 0x%lx error == SBI_ERR_DENIED", id);
I think we need something like gen_report(), which is in riscv/sbi.c, in
order to output the unexpected errors if we don't get what we expect. I've
pushed a few patches to my riscv/sbi branch[1] in order to provide
sbiret_report_error().
[1] https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi
> +}
> +
> +static void fwft_check_base(void)
> +{
> + struct sbiret ret;
> +
> + report_prefix_push("base");
> +
> + fwft_check_reserved(SBI_FWFT_LOCAL_RESERVED_START);
> + fwft_check_reserved(SBI_FWFT_LOCAL_RESERVED_END);
> + fwft_check_reserved(SBI_FWFT_GLOBAL_RESERVED_START);
> + fwft_check_reserved(SBI_FWFT_GLOBAL_RESERVED_END);
> +
> + /* Check id > 32 bits */
> + if (__riscv_xlen > 32) {
> + ret = fwft_get_raw(BIT(32));
> + report(ret.error == SBI_ERR_INVALID_PARAM,
> + "get feature 0x%lx error == SBI_ERR_INVALID_PARAM", BIT(32));
Need to indent the string under ret.error.
> +
> + ret = fwft_set_raw(BIT(32), 0, 0);
> + report(ret.error == SBI_ERR_INVALID_PARAM,
> + "set feature 0x%lx error == SBI_ERR_INVALID_PARAM", BIT(32));
Need to indent the string under ret.error.
> + }
> +
> + report_prefix_pop();
> +}
> +
> +static bool misaligned_handled;
> +
> +static void misaligned_handler(struct pt_regs *regs)
> +{
> + misaligned_handled = true;
> + regs->epc += 4;
> +}
> +
> +static void fwft_check_misaligned_exc_deleg(void)
> +{
> + struct sbiret ret;
> +
> + report_prefix_push("misaligned_exc_deleg");
> +
> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
> + if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> + report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
> + return;
> + }
> + report(!ret.error, "Get misaligned deleg feature no error: %lx",
> + ret.error);
> + if (ret.error)
> + return;
With the new sbiret_report_error() this can become
if (!sbiret_report_error(ret, 0, "Get misaligned deleg feature no error"))
return;
> +
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 2, 0);
> + report(ret.error == SBI_ERR_INVALID_PARAM,
> + "Set misaligned deleg feature invalid value error");
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0xFFFFFFFF, 0);
> + report(ret.error == SBI_ERR_INVALID_PARAM,
> + "Set misaligned deleg feature invalid value error");
Need sbiret_report_error() to ensure we can see what the returned error
was if it isn't what we expected.
We should have a different error message for each test so testers can
look up the one that failed by the message. Maybe just output the
invalid test value too.
> +
> + if (__riscv_xlen > 32) {
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, BIT(32), 0);
> + report(ret.error == SBI_ERR_INVALID_PARAM,
> + "Set misaligned deleg with value > 32bits invalid param error");
> +
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, BIT(32));
> + report(ret.error == SBI_ERR_INVALID_PARAM,
> + "Set misaligned deleg with flag > 32bits invalid param error");
Same two comments as above.
> + }
> +
> + /* Set to 0 and check after with get */
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
> + report(!ret.error, "Set misaligned deleg feature value no error");
sbiret_report_error()
> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
> + if (ret.error)
> + report_fail("Get misaligned deleg feature after set: %lx",
> + ret.error);
> + else
> + report(ret.value == 0, "Set misaligned deleg feature value 0");
sbiret_report(ret, 0, 0, "get/set misaligned deleg feature value 0")
> +
> + /* Set to 1 and check after with get */
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
> + report(!ret.error, "Set misaligned deleg feature value no error");
> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
> + if (ret.error)
> + report_fail("Get misaligned deleg feature after set: %lx",
> + ret.error);
> + else
> + report(ret.value == 1, "Set misaligned deleg feature value 1");
Same sbiret_report_error / sbiret_report comments as above.
We should get SBI_ERR_NOT_SUPPORTED if the platform doesn't support
delegating misaligned exceptions, right? If so, then we should tolerate
that and skip the trap test below.
> +
> + install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
> +
> + asm volatile (
> + ".option push\n"
> + /*
> + * Disable compression so the lw takes exactly 4 bytes and thus
> + * can be skipped reliably from the exception handler.
> + */
> + ".option arch,-c\n"
What's the difference between this and .option norvc ?
> + "lw %[val], 1(%[val_addr])\n"
> + ".option pop\n"
> + : [val] "+r" (ret.value)
> + : [val_addr] "r" (&ret.value)
> + : "memory");
> +
> + if (!misaligned_handled)
> + report_skip("Verify misaligned load exception trap in supervisor");
If we skip this test when we can't set SBI_FWFT_MISALIGNED_EXC_DELEG to 1,
then we should always fail this test when we don't get the exception.
> + else
> + report_pass("Verify misaligned load exception trap in supervisor");
> +
> + install_exception_handler(EXC_LOAD_MISALIGNED, NULL);
> +
> + /* Lock the feature */
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, SBI_FWFT_SET_FLAG_LOCK);
> + report(!ret.error, "Set misaligned deleg feature value 0 and lock no error");
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
> + report(ret.error == SBI_ERR_LOCKED,
> + "Set misaligned deleg feature value 0 and lock no error");
> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG);
> + report(!ret.error, "Get misaligned deleg locked value no error");
Need sbiret_report_error() for all these tests.
> +
> + report_prefix_pop();
> +}
> +
> +void check_fwft(void)
> +{
> + report_prefix_push("fwft");
> +
> + if (!sbi_probe(SBI_EXT_FWFT)) {
> + report_skip("FWFT extension not available");
> + report_prefix_pop();
> + return;
> + }
> +
> + fwft_check_base();
> + fwft_check_misaligned_exc_deleg();
> +
> + report_prefix_pop();
> +}
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 6f4ddaf1..8600e38e 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -32,6 +32,8 @@
>
> #define HIGH_ADDR_BOUNDARY ((phys_addr_t)1 << 32)
>
> +void check_fwft(void);
> +
> static long __labs(long a)
> {
> return __builtin_labs(a);
> @@ -1451,6 +1453,7 @@ int main(int argc, char **argv)
> check_hsm();
> check_dbcn();
> check_susp();
> + check_fwft();
>
> return report_summary();
> }
> --
> 2.47.1
>
Thanks,
drew
