Re: [kvm-unit-tests PATCH 2/2] riscv: Add tests for SBI FWFT extension

[Andrew Jones <andrew.jones@linux.dev>]

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@rivosinc.com>
> ---
>  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@rivosinc.com>
> + */
> +#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

-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv