On Mon, Jan 06, 2025 at 04:53:20PM +0100, Clément Léger wrote:
> This commit add tests for a the FWFT SBI extension. Currently, only
s/This commit//
> the reserved range as well as the misaligned exception delegation.
>
> Signed-off-by: Clément Léger <cleger@xxxxxxxxxxxx>
> ---
> riscv/Makefile | 2 +-
> lib/riscv/asm/sbi.h | 31 +++++++++
> riscv/sbi-fwft.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
> riscv/sbi.c | 3 +
> 4 files changed, 188 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..27e6fcdb 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -11,6 +11,9 @@
> #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
Need SBI_ERR_DENIED_LOCKED (and TIMEOUT and IO) too
>
> #ifndef __ASSEMBLY__
> #include <cpumask.h>
> @@ -23,6 +26,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 +75,33 @@ enum sbi_ext_dbcn_fid {
> SBI_EXT_DBCN_CONSOLE_WRITE_BYTE,
> };
>
> +/* SBI function IDs for FW feature extension */
> +#define SBI_EXT_FWFT_SET 0x0
> +#define SBI_EXT_FWFT_GET 0x1
Use a _fid enum like the other extensions.
> +
> +enum sbi_fwft_feature_t {
Use defines for the following, like SSE does for its ranges.
> + SBI_FWFT_MISALIGNED_EXC_DELEG = 0x0,
> + SBI_FWFT_LANDING_PAD = 0x1,
> + SBI_FWFT_SHADOW_STACK = 0x2,
> + SBI_FWFT_DOUBLE_TRAP = 0x3,
> + SBI_FWFT_PTE_AD_HARDWARE_UPDATE = 0x4,
SBI_FWFT_PTE_AD_HW_UPDATING
> + SBI_FWFT_POINTER_MASKING_PMLEN = 0x5,
> + SBI_FWFT_LOCAL_RESERVED_START = 0x6,
> + SBI_FWFT_LOCAL_RESERVED_END = 0x3fffffff,
Do we need the reserved start/end? SSE doesn't define its reserved
ranges.
> + SBI_FWFT_LOCAL_PLATFORM_START = 0x40000000,
> + SBI_FWFT_LOCAL_PLATFORM_END = 0x7fffffff,
> +
> + SBI_FWFT_GLOBAL_RESERVED_START = 0x80000000,
> + SBI_FWFT_GLOBAL_RESERVED_END = 0xbfffffff,
Same reserved range question.
> + SBI_FWFT_GLOBAL_PLATFORM_START = 0xc0000000,
> + SBI_FWFT_GLOBAL_PLATFORM_END = 0xffffffff,
> +};
> +
> +#define SBI_FWFT_PLATFORM_FEATURE_BIT (1 << 30)
> +#define SBI_FWFT_GLOBAL_FEATURE_BIT (1 << 31)
> +
> +#define SBI_FWFT_SET_FLAG_LOCK (1 << 0)
BIT() for the above defines
> +
> struct sbiret {
> long error;
> long value;
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> new file mode 100644
> index 00000000..8a7f2070
> --- /dev/null
> +++ b/riscv/sbi-fwft.c
> @@ -0,0 +1,153 @@
> +// 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 int fwft_set(unsigned long feature_id, unsigned long value,
returning an int is truncating sbiret.error
s/unsigned long feature_id/uint32_t feature/
> + unsigned long flags)
> +{
> + struct sbiret ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET,
> + feature_id, value, flags, 0, 0, 0);
> +
> + return ret.error;
> +}
Probably need a fwft_set_raw() as well which takes an unsigned long for
feature in order to test feature IDs that set bits >= 32 and returns
an sbiret allowing sbiret.value to be checked.
> +
> +static int fwft_get(unsigned long feature_id, unsigned long *value)
returning an int is truncating sbiret.error
s/unsigned long feature_id/uint32_t feature/
> +{
> + struct sbiret ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_GET,
> + feature_id, 0, 0, 0, 0, 0);
> +
> + *value = ret.value;
> +
> + return ret.error;
Why not just return sbiret to return both value and error?
As a separate patch we should update struct sbiret to match the latest
spec which now has a union in it.
Same comment about needing a _raw version too.
> +}
> +
> +static void fwft_check_reserved(unsigned long id)
> +{
> + int ret;
> + bool pass = true;
> + unsigned long value;
> +
> + ret = fwft_get(id, &value);
> + if (ret != SBI_ERR_DENIED)
> + pass = false;
> +
> + ret = fwft_set(id, 1, 0);
> + if (ret != SBI_ERR_DENIED)
> + pass = false;
> +
> + report(pass, "get/set reserved feature 0x%lx error == SBI_ERR_DENIED", id);
The get and set should be split into two tests
struct sbiret ret;
ret = fwft_get(id);
report(ret.error == SBI_ERR_DENIED, ...);
ret = fwft_set(id, 1, 0);
report(ret.error == SBI_ERR_DENIED, ...);
> +}
> +
> +static void fwft_check_denied(void)
> +{
> + 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);
I see why we have the reserved ranges defined now. Shouldn't we also have
tests like these for SSE, which means we should define the reserved ranges
for it too?
> +}
> +
> +static bool misaligned_handled;
> +
> +static void misaligned_handler(struct pt_regs *regs)
> +{
> + misaligned_handled = true;
> + regs->epc += 4;
> +}
> +
> +static void fwft_check_misaligned(void)
> +{
> + int ret;
> + unsigned long value;
> +
> + report_prefix_push("misaligned_deleg");
"misaligned_exc_deleg"
> +
> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value);
> + if (ret == SBI_ERR_NOT_SUPPORTED) {
> + report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
> + return;
> + }
> + report(!ret, "Get misaligned deleg feature no error");
Should output the error too
> + if (ret)
> + return;
> +
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 2, 0);
> + report(ret == SBI_ERR_INVALID_PARAM, "Set misaligned deleg feature invalid value error");
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0xFFFFFFFF, 0);
> + report(ret == SBI_ERR_INVALID_PARAM, "Set misaligned deleg feature invalid value error");
Something like
if (__riscv_xlen > 32) {
ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, (1ul << 32), 0);
report(ret == SBI_ERR_INVALID_PARAM
}
would be a good test too (and also for the flags parameter)
> +
> + /* Set to 0 and check after with get */
> + ret = fwft_set(SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
> + report(!ret, "Set misaligned deleg feature value no error");
> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value);
> + if (ret)
> + report_fail("Get misaligned deleg feature after set");
> + else
> + report(value == 0, "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, "Set misaligned deleg feature value no error");
> + ret = fwft_get(SBI_FWFT_MISALIGNED_EXC_DELEG, &value);
> + if (ret)
> + report_fail("Get misaligned deleg feature after set");
> + else
> + report(value == 1, "Set misaligned deleg feature value 1");
> +
> + install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
> +
> + asm volatile (
> + ".option norvc\n"
We also need push/pop otherwise from here on out we stop using compression
instructions.
> + "lw %[val], 1(%[val_addr])"
> + : [val] "+r" (value)
> + : [val_addr] "r" (&value)
> + : "memory");
> +
> + if (!misaligned_handled)
> + report_skip("Verify misaligned load exception trap in supervisor");
Why is this report_skip()? Shouldn't we just do
report(misaligned_handled, ...)
> + else
> + report_pass("Verify misaligned load exception trap in supervisor");
> +
> + install_exception_handler(EXC_LOAD_MISALIGNED, NULL);
> +
> + report_prefix_pop();
> +}
> +
> +void check_fwft(void)
> +{
> + struct sbiret ret;
> +
> + report_prefix_push("fwft");
> +
> + if (!sbi_probe(SBI_EXT_FWFT)) {
> + report_skip("FWFT extension not available");
> + report_prefix_pop();
> + return;
> + }
> +
> + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_FWFT, 0, 0, 0, 0, 0);
> + report(!ret.error, "FWFT extension probing no error");
> + if (ret.error)
> + goto done;
> +
> + if (ret.value == 0) {
> + report_skip("FWFT extension is not present");
> + goto done;
> + }
The above "raw" probing looks like it should have been removed when
the sbi_probe() call was added.
> +
> + fwft_check_denied();
> + fwft_check_misaligned();
> +done:
> + 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
>
Nice start to the FWFT tests. After this is merged I'll add tests for
PTE_AD_HW_UPDATING. We also should get LOCK and local/global tests in
sooner than later.
Thanks,
drew
