On 15/01/2025 13:58, Andrew Jones wrote:
> 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
Indeed, i'll add that.
>
>>
>> #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.
As seen below, it is used for reserved range testing. You are right
about SSE, that would be nice to test that as well.
>
>> + 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.
Acked, I actually modified bith get/set to return an sbiret directly,
that's easier to handle a unique return type in tests.
>
>> +}
>> +
>> +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?
Yes, that should be tested in SSE as well.
>
>> +}
>> +
>> +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)
>
Acked I'll add that.
>> +
>> + /* 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.
Yeah, nice catch, I'll add push/pop.
>
>> + "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, ...)
Some platforms might actually allow you to delegate the misaligned
access trap but handle scalar misaligned accesses in hardware but trap
on vector misaligned. To be "more" complete, I should add vector testing
but I haven't had time to check vector instructions.
>
>> + 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.
Oups yes, that should have been removed.
>
>> +
>> + 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.
Acked, I'll add LOCK testing for misaligned as well.
Thanks for the review,
Clément
>
> Thanks,
> drew
