| Message ID | 20250313075845.411130-1-akshaybehl231@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,kvm-unit-tests] riscv: Refactoring sbi fwft tests | expand |
On Thu, Mar 13, 2025 at 01:28:45PM +0530, Akshay Behl wrote: > This patch refactors the current sbi fwft tests > (pte_ad_hw_updating, misaligned_exc_deleg) > > Signed-off-by: Akshay Behl <akshaybehl231@gmail.com> > --- > riscv/sbi-fwft.c | 58 +++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 45 insertions(+), 13 deletions(-) > > diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c > index ac2e3486..bf735f62 100644 > --- a/riscv/sbi-fwft.c > +++ b/riscv/sbi-fwft.c > @@ -19,6 +19,15 @@ > > void check_fwft(void); > > +static bool env_or_skip(const char *env) > +{ > + if (!getenv(env)) { > + report_skip("missing %s environment variable", env); > + return false; > + } > + > + return true; > +} We already have this in riscv/sbi.c so we should share it. We can just make it a static inline in sbi-tests.h > > static struct sbiret fwft_set_raw(unsigned long feature, unsigned long value, unsigned long flags) > { > @@ -66,6 +75,13 @@ static void fwft_check_reserved(unsigned long id) > sbiret_report_error(&ret, SBI_ERR_DENIED, "set reserved feature 0x%lx", id); > } > > +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); > +} Yes, this is the function I suggested we add, but you neglected to add the comment above it that I also suggested. /* Must be called before any fwft_set() call is made for @feature */ > + > static void fwft_check_base(void) > { > report_prefix_push("base"); > @@ -99,18 +115,32 @@ 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_or_skip("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG")) { We don't want env_or_skip() here since we don't want to output SKIP logs for _HAVE_ environment variables. env_or_skip() is for getting expected values for tests. In those cases, without the env, there's no expected value to compare with, so we must skip the test. > + expected = (unsigned long)strtoul(getenv("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG"), NULL, 0); > + if (expected == 1) We have env_enabled() in riscv/sbi.c which we can move to sbi-tests.h as a static inline. That's what we should use here. (It doesn't use strtoul, since there's no need to in order to compare with the string "1".) > + { This { should be on the same line as the if. > + report_fail("not supported by platform"); This should be sbiret_report_error(&ret, SBI_SUCCESS, "supported") since SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG says it's supported and we want to see what error code we get when it's not success. > + 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 = (unsigned long)strtoul(getenv("MISALIGNED_EXC_DELEG_RESET"), NULL, 0); No need for the (unsigned long) cast. strtoul() returns an unsigned long (it even says it will in its name). > + 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 +159,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); > > @@ -257,11 +281,20 @@ static void fwft_check_pte_ad_hw_updating(void) > { > struct sbiret ret; > bool enabled; > + unsigned long expected; > > 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_or_skip("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING")) { > + expected = (unsigned long)strtoul(getenv("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING"), NULL, 0); > + if (expected == 1) > + { > + report_fail("not supported by platform"); > + return; > + } > + } same comments as above > report_skip("not supported by platform"); > return; > } else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) { > @@ -269,10 +302,9 @@ static void fwft_check_pte_ad_hw_updating(void) > return; > } > > - report(ret.value == 0 || ret.value == 1, "first get value is 0/1"); > + fwft_check_reset(SBI_FWFT_PTE_AD_HW_UPDATING, 1); The reset value of PTE_AD_HW_UPDATING isn't 1. It's 0. See the spec. > > enabled = ret.value; > - report_kfail(true, !enabled, "resets to 0"); This removes a test case that proved opensbi is broken. Always refer to the spec when writing tests. We don't want to write tests (and especially not change tests) just to output PASS. Until we've fixed opensbi this test needs to stay as it is, i.e. with the check for 0 or 1 for the get and then the kfail report for when it's 1. > > install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler); > install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler); > -- > 2.34.1 > Thanks, drew
diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c index ac2e3486..bf735f62 100644 --- a/riscv/sbi-fwft.c +++ b/riscv/sbi-fwft.c @@ -19,6 +19,15 @@ void check_fwft(void); +static bool env_or_skip(const char *env) +{ + if (!getenv(env)) { + report_skip("missing %s environment variable", env); + return false; + } + + return true; +} static struct sbiret fwft_set_raw(unsigned long feature, unsigned long value, unsigned long flags) { @@ -66,6 +75,13 @@ static void fwft_check_reserved(unsigned long id) sbiret_report_error(&ret, SBI_ERR_DENIED, "set reserved feature 0x%lx", id); } +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 +115,32 @@ 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_or_skip("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG")) { + expected = (unsigned long)strtoul(getenv("SBI_HAVE_FWFT_MISALIGNED_EXC_DELEG"), NULL, 0); + if (expected == 1) + { + report_fail("not supported by platform"); + 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 = (unsigned long)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 +159,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); @@ -257,11 +281,20 @@ static void fwft_check_pte_ad_hw_updating(void) { struct sbiret ret; bool enabled; + unsigned long expected; 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_or_skip("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING")) { + expected = (unsigned long)strtoul(getenv("SBI_HAVE_FWFT_PTE_AD_HW_UPDATING"), NULL, 0); + if (expected == 1) + { + report_fail("not supported by platform"); + return; + } + } report_skip("not supported by platform"); return; } else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) { @@ -269,10 +302,9 @@ static void fwft_check_pte_ad_hw_updating(void) return; } - report(ret.value == 0 || ret.value == 1, "first get value is 0/1"); + fwft_check_reset(SBI_FWFT_PTE_AD_HW_UPDATING, 1); enabled = ret.value; - report_kfail(true, !enabled, "resets to 0"); install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler); install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
This patch refactors the current sbi fwft tests (pte_ad_hw_updating, misaligned_exc_deleg) Signed-off-by: Akshay Behl <akshaybehl231@gmail.com> --- riscv/sbi-fwft.c | 58 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 13 deletions(-)