On Fri, Mar 14, 2025 at 01:57:45PM +0100, Andrew Jones wrote:
> On Thu, Mar 13, 2025 at 10:42:23PM +0530, Akshay Behl wrote:
> > This patch refactors the current sbi fwft tests
> > (pte_ad_hw_updating, misaligned_exc_deleg)
> >
> > v2:
> > - Made env_or_skip and env_enabled methods shared by adding
> > them to sbi-tests.h
> > - Used env_enabled check instead of env_or_skip for
> > platform support
> > - Added the reset to 0/1 test back for pte_ad_hw_updating
> > - Made other suggested changes
>
> The v2 changelog should go under the '---' below to keep it out of the
> file commit message.
>
> >
> > Signed-off-by: Akshay Behl <akshaybehl231@xxxxxxxxx>
> > ---
> > riscv/sbi-tests.h | 22 ++++++++++++++++++++++
> > riscv/sbi-fwft.c | 38 +++++++++++++++++++++++++++-----------
> > riscv/sbi.c | 17 -----------------
> > 3 files changed, 49 insertions(+), 28 deletions(-)
> >
> > diff --git a/riscv/sbi-tests.h b/riscv/sbi-tests.h
> > index b081464d..91eba7b7 100644
> > --- a/riscv/sbi-tests.h
> > +++ b/riscv/sbi-tests.h
> > @@ -70,6 +70,28 @@
> > #define sbiret_check(ret, expected_error, expected_value) \
> > sbiret_report(ret, expected_error, expected_value, "check sbi.error and sbi.value")
> >
> > +/**
> > + * Check if environment variable exists, skip test if missing
> > + *
> > + * @param env The environment variable name to check
> > + * @return true if environment variable exists, false otherwise
> > + */
I also dropped this comment block. The '/**' made check-kerneldoc complain
and the comment didn't tell us anything that the four lines of code that
the function has was already easily telling us.
Thanks,
drew
> > +static inline bool env_or_skip(const char *env)
> > +{
> > + if (!getenv(env)) {
> > + report_skip("missing %s environment variable", env);
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > +static inline bool env_enabled(const char *env)
> > +{
> > + char *s = getenv(env);
> > +
> > + return s && (*s == '1' || *s == 'y' || *s == 'Y');
> > +}
>
> We should include libcflat.h now that we've added these functions. Make
> sure the include is under the '#ifndef __ASSEMBLER__' (and above the
> '#include <asm/sbi.h>')
>
> > +
> > void sbi_bad_fid(int ext);
> >
> > #endif /* __ASSEMBLER__ */
> > diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> > index ac2e3486..581cbf6b 100644
> > --- a/riscv/sbi-fwft.c
> > +++ b/riscv/sbi-fwft.c
> > @@ -66,6 +66,14 @@ static void fwft_check_reserved(unsigned long id)
> > sbiret_report_error(&ret, SBI_ERR_DENIED, "set reserved feature 0x%lx", id);
> > }
> >
> > +/* Must be called before any fwft_set() call is made for @feature */
> > +static void fwft_check_reset(uint32_t feature, unsigned long reset)
> > +{
> > + struct sbiret ret = fwft_get(feature);
> > +
> > + sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
> > +}
> > +
> > static void fwft_check_base(void)
> > {
> > report_prefix_push("base");
> > @@ -99,18 +107,28 @@ static struct sbiret fwft_misaligned_exc_get(void)
> > static void fwft_check_misaligned_exc_deleg(void)
> > {
> > struct sbiret ret;
> > + unsigned long expected;
> >
> > report_prefix_push("misaligned_exc_deleg");
> >
> > ret = fwft_misaligned_exc_get();
> > - if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> > - report_skip("SBI_FWFT_MISALIGNED_EXC_DELEG is not supported");
> > + if (ret.error != SBI_SUCCESS) {
> > + if (env_enabled("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG")) {
> > + sbiret_report_error(&ret, SBI_SUCCESS, "supported");
> > + return;
> > + }
> > + report_skip("not supported by platform");
> > return;
> > }
> >
> > if (!sbiret_report_error(&ret, SBI_SUCCESS, "Get misaligned deleg feature"))
> > return;
> >
> > + if (env_or_skip("MISALIGNED_EXC_DELEG_RESET")) {
> > + expected = strtoul(getenv("MISALIGNED_EXC_DELEG_RESET"), NULL, 0);
> > + fwft_check_reset(SBI_FWFT_MISALIGNED_EXC_DELEG, expected);
> > + }
> > +
> > ret = fwft_misaligned_exc_set(2, 0);
> > sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM,
> > "Set misaligned deleg feature invalid value 2");
> > @@ -129,16 +147,10 @@ static void fwft_check_misaligned_exc_deleg(void)
> > #endif
> >
> > /* Set to 0 and check after with get */
> > - ret = fwft_misaligned_exc_set(0, 0);
> > - sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0");
> > - ret = fwft_misaligned_exc_get();
> > - sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg feature expected value 0");
> > + fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 0, 0);
> >
> > /* Set to 1 and check after with get */
> > - ret = fwft_misaligned_exc_set(1, 0);
> > - sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 1");
> > - ret = fwft_misaligned_exc_get();
> > - sbiret_report(&ret, SBI_SUCCESS, 1, "Get misaligned deleg feature expected value 1");
> > + fwft_set_and_check_raw("", SBI_FWFT_MISALIGNED_EXC_DELEG, 1, 0);
> >
> > install_exception_handler(EXC_LOAD_MISALIGNED, misaligned_handler);
> >
> > @@ -261,7 +273,11 @@ static void fwft_check_pte_ad_hw_updating(void)
> > report_prefix_push("pte_ad_hw_updating");
> >
> > ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> > - if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> > + if (ret.error != SBI_SUCCESS) {
> > + if (env_enabled("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING")) {
> > + sbiret_report_error(&ret, SBI_SUCCESS, "supported");
> > + return;
> > + }
> > report_skip("not supported by platform");
> > return;
> > } else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
> > diff --git a/riscv/sbi.c b/riscv/sbi.c
> > index 0404bb81..219f7187 100644
> > --- a/riscv/sbi.c
> > +++ b/riscv/sbi.c
> > @@ -131,23 +131,6 @@ static phys_addr_t get_highest_addr(void)
> > return highest_end - 1;
> > }
> >
> > -static bool env_enabled(const char *env)
> > -{
> > - char *s = getenv(env);
> > -
> > - return s && (*s == '1' || *s == 'y' || *s == 'Y');
> > -}
> > -
> > -static bool env_or_skip(const char *env)
> > -{
> > - if (!getenv(env)) {
> > - report_skip("missing %s environment variable", env);
> > - return false;
> > - }
> > -
> > - return true;
> > -}
> > -
> > static bool get_invalid_addr(phys_addr_t *paddr, bool allow_default)
> > {
> > if (env_enabled("INVALID_ADDR_AUTO")) {
> > --
> > 2.34.1
> >
>
> Other than the two comments above, it looks good. I've made the changes
> myself while applying to riscv/sbi
>
> https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi
>
> Thanks,
> drew
