* [PATCH V2 0/6] Add riscv tests to cover the base extension specs
@ 2024-03-13 8:46 cem
2024-03-13 8:46 ` [PATCH V2 1/6] riscv: Add test to probe SBI Extension cem
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: cem @ 2024-03-13 8:46 UTC (permalink / raw)
To: kvm-riscv
From: Carlos Maiolino <cem@kernel.org>
Hi,
This is a new version of this series to create tests to cover functions
of the riscv's SBI base implementation spec. The series also includes a
a few helpers to reduce code duplication.
Detailed updates are specified on a patch-basis
Carlos Maiolino (6):
riscv: Add test to probe SBI Extension
riscv: Factor out environment variable check and report generation
riscv: Implement test for architecture ID register
riscv: Enable gen_report() to print the wrong value in case of failure
riscv: Test for specific SBI implementation ID
riscv: Add a wrapper to call sbi_ecall for base extension
riscv/sbi.c | 79 +++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 67 insertions(+), 12 deletions(-)
--
2.44.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 1/6] riscv: Add test to probe SBI Extension
2024-03-13 8:46 [PATCH V2 0/6] Add riscv tests to cover the base extension specs cem
@ 2024-03-13 8:46 ` cem
2024-03-13 11:06 ` Andrew Jones
2024-03-13 8:46 ` [PATCH V2 2/6] riscv: Factor out environment variable check and report generation cem
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: cem @ 2024-03-13 8:46 UTC (permalink / raw)
To: kvm-riscv
From: Carlos Maiolino <cem@kernel.org>
Factor out vendor id test to a new helper, and add a new test for
probing the SBI extension.
Compare the retrieved value against an environment variable, as the
implementation can return any non-zero value.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V2:
- change sbi_ecall()'s format, to fit a single line.
- Make PROBE_EXT envvar optional, as suggested by Drew. This prevents
the test to bail if the envvar is not set.
riscv/sbi.c | 43 ++++++++++++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 11 deletions(-)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index ffb07a25..1d73d8f1 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -14,28 +14,49 @@ static void help(void)
puts("An environ must be provided where expected values are given.\n");
}
-int main(int argc, char **argv)
+static void check_base(void)
{
struct sbiret ret;
long expected;
- if (argc > 1 && !strcmp(argv[1], "-h")) {
- help();
- exit(0);
- }
-
- report_prefix_push("sbi");
+ report_prefix_push("base");
if (!getenv("MVENDORID")) {
report_skip("mvendorid: missing MVENDORID environment variable");
- goto done;
+ return;
}
+
+ report_prefix_push("mvendorid");
expected = strtol(getenv("MVENDORID"), NULL, 0);
ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
- report(!ret.error, "mvendorid: no error");
- report(ret.value == expected, "mvendorid");
-done:
+ report(!ret.error, "no sbi.error");
+ report(ret.value == expected, "expected sbi.value");
+ report_prefix_pop();
+
+ report_prefix_push("probe_ext");
+ expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
+
+ ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
+
+ report(!ret.error, "no sbi.error");
+ report(ret.value == expected, "expected sbi.value");
+ report_prefix_pop();
+
+ report_prefix_pop();
+}
+
+int main(int argc, char **argv)
+{
+
+ if (argc > 1 && !strcmp(argv[1], "-h")) {
+ help();
+ exit(0);
+ }
+
+ report_prefix_push("sbi");
+ check_base();
+
return report_summary();
}
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 2/6] riscv: Factor out environment variable check and report generation
2024-03-13 8:46 [PATCH V2 0/6] Add riscv tests to cover the base extension specs cem
2024-03-13 8:46 ` [PATCH V2 1/6] riscv: Add test to probe SBI Extension cem
@ 2024-03-13 8:46 ` cem
2024-03-13 11:11 ` Andrew Jones
2024-03-13 8:46 ` [PATCH V2 3/6] riscv: Implement test for architecture ID register cem
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: cem @ 2024-03-13 8:46 UTC (permalink / raw)
To: kvm-riscv
From: Carlos Maiolino <cem@kernel.org>
We do check Environment variables and generate reports all the time,
so use a couple of helpers for that
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V2:
- Rename env_is_defined() to env_or_skip()
- Add information about gen_report() in the commit message
- PROBE_EXT is not mandatory, so we don't need to pass it to
env_or_skip().
riscv/sbi.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 1d73d8f1..519fcbe3 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -14,6 +14,23 @@ static void help(void)
puts("An environ must be provided where expected values are given.\n");
}
+static bool env_or_skip(const char *env)
+{
+
+ if (!getenv(env)) {
+ report_skip("missing %s environment variable", env);
+ return false;
+ }
+
+ return true;
+}
+
+static void gen_report(struct sbiret *ret, long expected)
+{
+ report(!ret->error, "no sbi.error");
+ report(ret->value == expected, "expected sbi.value");
+}
+
static void check_base(void)
{
struct sbiret ret;
@@ -21,27 +38,18 @@ static void check_base(void)
report_prefix_push("base");
- if (!getenv("MVENDORID")) {
- report_skip("mvendorid: missing MVENDORID environment variable");
- return;
- }
-
report_prefix_push("mvendorid");
- expected = strtol(getenv("MVENDORID"), NULL, 0);
-
- ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
-
- report(!ret.error, "no sbi.error");
- report(ret.value == expected, "expected sbi.value");
+ if (env_or_skip("MVENDORID")) {
+ expected = strtol(getenv("MVENDORID"), NULL, 0);
+ ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
+ gen_report(&ret, expected);
+ }
report_prefix_pop();
report_prefix_push("probe_ext");
expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
-
ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
-
- report(!ret.error, "no sbi.error");
- report(ret.value == expected, "expected sbi.value");
+ gen_report(&ret, expected);
report_prefix_pop();
report_prefix_pop();
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 3/6] riscv: Implement test for architecture ID register
2024-03-13 8:46 [PATCH V2 0/6] Add riscv tests to cover the base extension specs cem
2024-03-13 8:46 ` [PATCH V2 1/6] riscv: Add test to probe SBI Extension cem
2024-03-13 8:46 ` [PATCH V2 2/6] riscv: Factor out environment variable check and report generation cem
@ 2024-03-13 8:46 ` cem
2024-03-13 11:21 ` Andrew Jones
2024-03-13 8:46 ` [PATCH V2 4/6] riscv: Enable gen_report() to print the wrong value in case of failure cem
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: cem @ 2024-03-13 8:46 UTC (permalink / raw)
To: kvm-riscv
From: Carlos Maiolino <cem@kernel.org>
Probe the MARCHID register and compare it to the specified MARCHID
environment variable.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
V2:
- Wrap commit message around 70 chars
- Remove unneeded blank lines
riscv/sbi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 519fcbe3..76f6111d 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -52,6 +52,15 @@ static void check_base(void)
gen_report(&ret, expected);
report_prefix_pop();
+ report_prefix_push("marchid");
+ if (env_or_skip("MARCHID")) {
+ expected = strtol(getenv("MARCHID"), NULL, 0);
+ ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT,
+ SBI_EXT_BASE_GET_MARCHID, 0, 0, 0, 0, 0);
+ gen_report(&ret, expected);
+ }
+ report_prefix_pop();
+
report_prefix_pop();
}
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 4/6] riscv: Enable gen_report() to print the wrong value in case of failure
2024-03-13 8:46 [PATCH V2 0/6] Add riscv tests to cover the base extension specs cem
` (2 preceding siblings ...)
2024-03-13 8:46 ` [PATCH V2 3/6] riscv: Implement test for architecture ID register cem
@ 2024-03-13 8:46 ` cem
2024-03-13 11:19 ` Andrew Jones
2024-03-13 8:46 ` [PATCH V2 5/6] riscv: Test for specific SBI implementation ID cem
2024-03-13 8:46 ` [PATCH 6/6] riscv: Add a wrapper to call sbi_ecall for base extension cem
5 siblings, 1 reply; 14+ messages in thread
From: cem @ 2024-03-13 8:46 UTC (permalink / raw)
To: kvm-riscv
From: Carlos Maiolino <cem@kernel.org>
If the test fails because the expected value doesn't match, it's
useful to know what value was actually printed.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V2:
- Reduce subject to fit 70 chars
- Use report_info() to output expected vs received values,
leaving only necessary information for parsers on report()
riscv/sbi.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 76f6111d..bc0f5c68 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -25,10 +25,15 @@ static bool env_or_skip(const char *env)
return true;
}
-static void gen_report(struct sbiret *ret, long expected)
+static void gen_report(struct sbiret *ret,
+ long expected_error, long expected_value)
{
- report(!ret->error, "no sbi.error");
- report(ret->value == expected, "expected sbi.value");
+ if (ret->value != expected_value)
+ report_info("expected (error: %ld, value: %ld), received: (error: %ld, value %ld)\n",
+ expected_error, expected_value, ret->error, ret->value);
+
+ report(ret->error == expected_error, "expected sbi.error");
+ report(ret->value == expected_value, "expected sbi.value");
}
static void check_base(void)
@@ -42,14 +47,14 @@ static void check_base(void)
if (env_or_skip("MVENDORID")) {
expected = strtol(getenv("MVENDORID"), NULL, 0);
ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
- gen_report(&ret, expected);
+ gen_report(&ret, 0, expected);
}
report_prefix_pop();
report_prefix_push("probe_ext");
expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
- gen_report(&ret, expected);
+ gen_report(&ret, 0, expected);
report_prefix_pop();
report_prefix_push("marchid");
@@ -57,7 +62,7 @@ static void check_base(void)
expected = strtol(getenv("MARCHID"), NULL, 0);
ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT,
SBI_EXT_BASE_GET_MARCHID, 0, 0, 0, 0, 0);
- gen_report(&ret, expected);
+ gen_report(&ret, 0, expected);
}
report_prefix_pop();
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 5/6] riscv: Test for specific SBI implementation ID
2024-03-13 8:46 [PATCH V2 0/6] Add riscv tests to cover the base extension specs cem
` (3 preceding siblings ...)
2024-03-13 8:46 ` [PATCH V2 4/6] riscv: Enable gen_report() to print the wrong value in case of failure cem
@ 2024-03-13 8:46 ` cem
2024-03-13 11:23 ` Andrew Jones
2024-03-13 8:46 ` [PATCH 6/6] riscv: Add a wrapper to call sbi_ecall for base extension cem
5 siblings, 1 reply; 14+ messages in thread
From: cem @ 2024-03-13 8:46 UTC (permalink / raw)
To: kvm-riscv
From: Carlos Maiolino <cem@kernel.org>
Retrieve the ID from the SBI, and test it against the SBI_IMPLID
enviroment variable.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
V2:
- Update commit description to fit 70 chars
- Move sbi_ecall() after expected assignment to make consistent with
other tests
riscv/sbi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index bc0f5c68..65492fd6 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -51,6 +51,15 @@ static void check_base(void)
}
report_prefix_pop();
+ report_prefix_push("sbi_impl_id");
+ if (env_or_skip("SBI_IMPLID")) {
+ expected = strtol(getenv("SBI_IMPLID"), NULL, 0);
+ ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID,
+ SBI_EXT_BASE, 0, 0, 0, 0, 0);
+ gen_report(&ret, 0, expected);
+ }
+ report_prefix_pop();
+
report_prefix_push("probe_ext");
expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/6] riscv: Add a wrapper to call sbi_ecall for base extension
2024-03-13 8:46 [PATCH V2 0/6] Add riscv tests to cover the base extension specs cem
` (4 preceding siblings ...)
2024-03-13 8:46 ` [PATCH V2 5/6] riscv: Test for specific SBI implementation ID cem
@ 2024-03-13 8:46 ` cem
2024-03-13 11:33 ` Andrew Jones
5 siblings, 1 reply; 14+ messages in thread
From: cem @ 2024-03-13 8:46 UTC (permalink / raw)
To: kvm-riscv
From: Carlos Maiolino <cem@kernel.org>
All SBI extension functions accepts at most one argument, so create a
wrapper around sbi_ecall() to avoid needing to pass in arguments 1 to 5
all the time, also, the wrapper can specify SBI_EXT_BASE directly.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
riscv/sbi.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 65492fd6..82513d61 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -36,6 +36,11 @@ static void gen_report(struct sbiret *ret,
report(ret->value == expected_value, "expected sbi.value");
}
+static inline struct sbiret __base_sbi_ecall(int fid, unsigned long arg0)
+{
+ return sbi_ecall(SBI_EXT_BASE, fid, arg0, 0, 0, 0, 0, 0);
+}
+
static void check_base(void)
{
struct sbiret ret;
@@ -46,7 +51,7 @@ static void check_base(void)
report_prefix_push("mvendorid");
if (env_or_skip("MVENDORID")) {
expected = strtol(getenv("MVENDORID"), NULL, 0);
- ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
+ ret = __base_sbi_ecall(SBI_EXT_BASE_GET_MVENDORID, 0);
gen_report(&ret, 0, expected);
}
report_prefix_pop();
@@ -54,23 +59,21 @@ static void check_base(void)
report_prefix_push("sbi_impl_id");
if (env_or_skip("SBI_IMPLID")) {
expected = strtol(getenv("SBI_IMPLID"), NULL, 0);
- ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID,
- SBI_EXT_BASE, 0, 0, 0, 0, 0);
+ ret = __base_sbi_ecall(SBI_EXT_BASE_GET_IMP_ID, SBI_EXT_BASE);
gen_report(&ret, 0, expected);
}
report_prefix_pop();
report_prefix_push("probe_ext");
expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
- ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
+ ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE);
gen_report(&ret, 0, expected);
report_prefix_pop();
report_prefix_push("marchid");
if (env_or_skip("MARCHID")) {
expected = strtol(getenv("MARCHID"), NULL, 0);
- ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT,
- SBI_EXT_BASE_GET_MARCHID, 0, 0, 0, 0, 0);
+ ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE_GET_MARCHID);
gen_report(&ret, 0, expected);
}
report_prefix_pop();
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 1/6] riscv: Add test to probe SBI Extension
2024-03-13 8:46 ` [PATCH V2 1/6] riscv: Add test to probe SBI Extension cem
@ 2024-03-13 11:06 ` Andrew Jones
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2024-03-13 11:06 UTC (permalink / raw)
To: kvm-riscv
On Wed, Mar 13, 2024 at 09:46:19AM +0100, cem at kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
>
> Factor out vendor id test to a new helper, and add a new test for
> probing the SBI extension.
> Compare the retrieved value against an environment variable, as the
> implementation can return any non-zero value.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>
> V2:
> - change sbi_ecall()'s format, to fit a single line.
> - Make PROBE_EXT envvar optional, as suggested by Drew. This prevents
> the test to bail if the envvar is not set.
>
> riscv/sbi.c | 43 ++++++++++++++++++++++++++++++++-----------
> 1 file changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index ffb07a25..1d73d8f1 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -14,28 +14,49 @@ static void help(void)
> puts("An environ must be provided where expected values are given.\n");
> }
>
> -int main(int argc, char **argv)
> +static void check_base(void)
> {
> struct sbiret ret;
> long expected;
>
> - if (argc > 1 && !strcmp(argv[1], "-h")) {
> - help();
> - exit(0);
> - }
> -
> - report_prefix_push("sbi");
> + report_prefix_push("base");
>
> if (!getenv("MVENDORID")) {
> report_skip("mvendorid: missing MVENDORID environment variable");
> - goto done;
> + return;
> }
> +
> + report_prefix_push("mvendorid");
> expected = strtol(getenv("MVENDORID"), NULL, 0);
>
> ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
> - report(!ret.error, "mvendorid: no error");
> - report(ret.value == expected, "mvendorid");
>
> -done:
> + report(!ret.error, "no sbi.error");
> + report(ret.value == expected, "expected sbi.value");
> + report_prefix_pop();
> +
> + report_prefix_push("probe_ext");
> + expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
> +
> + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
> +
> + report(!ret.error, "no sbi.error");
> + report(ret.value == expected, "expected sbi.value");
> + report_prefix_pop();
> +
> + report_prefix_pop();
> +}
> +
> +int main(int argc, char **argv)
> +{
> +
> + if (argc > 1 && !strcmp(argv[1], "-h")) {
> + help();
> + exit(0);
> + }
> +
> + report_prefix_push("sbi");
> + check_base();
> +
> return report_summary();
> }
> --
> 2.44.0
>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 2/6] riscv: Factor out environment variable check and report generation
2024-03-13 8:46 ` [PATCH V2 2/6] riscv: Factor out environment variable check and report generation cem
@ 2024-03-13 11:11 ` Andrew Jones
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2024-03-13 11:11 UTC (permalink / raw)
To: kvm-riscv
On Wed, Mar 13, 2024 at 09:46:20AM +0100, cem at kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
>
> We do check Environment variables and generate reports all the time,
> so use a couple of helpers for that
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>
> V2:
> - Rename env_is_defined() to env_or_skip()
> - Add information about gen_report() in the commit message
> - PROBE_EXT is not mandatory, so we don't need to pass it to
> env_or_skip().
>
> riscv/sbi.c | 38 +++++++++++++++++++++++---------------
> 1 file changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 1d73d8f1..519fcbe3 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -14,6 +14,23 @@ static void help(void)
> puts("An environ must be provided where expected values are given.\n");
> }
>
> +static bool env_or_skip(const char *env)
> +{
> +
> + if (!getenv(env)) {
> + report_skip("missing %s environment variable", env);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static void gen_report(struct sbiret *ret, long expected)
I see you add expected_error in a later patch. I probably would have done
that here at function introduction time, but OK.
> +{
> + report(!ret->error, "no sbi.error");
> + report(ret->value == expected, "expected sbi.value");
> +}
> +
> static void check_base(void)
> {
> struct sbiret ret;
> @@ -21,27 +38,18 @@ static void check_base(void)
>
> report_prefix_push("base");
>
> - if (!getenv("MVENDORID")) {
> - report_skip("mvendorid: missing MVENDORID environment variable");
> - return;
> - }
> -
> report_prefix_push("mvendorid");
> - expected = strtol(getenv("MVENDORID"), NULL, 0);
> -
> - ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
> -
> - report(!ret.error, "no sbi.error");
> - report(ret.value == expected, "expected sbi.value");
> + if (env_or_skip("MVENDORID")) {
> + expected = strtol(getenv("MVENDORID"), NULL, 0);
> + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
> + gen_report(&ret, expected);
> + }
> report_prefix_pop();
>
> report_prefix_push("probe_ext");
> expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
> -
> ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
> -
> - report(!ret.error, "no sbi.error");
> - report(ret.value == expected, "expected sbi.value");
> + gen_report(&ret, expected);
> report_prefix_pop();
>
> report_prefix_pop();
> --
> 2.44.0
>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 4/6] riscv: Enable gen_report() to print the wrong value in case of failure
2024-03-13 8:46 ` [PATCH V2 4/6] riscv: Enable gen_report() to print the wrong value in case of failure cem
@ 2024-03-13 11:19 ` Andrew Jones
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2024-03-13 11:19 UTC (permalink / raw)
To: kvm-riscv
On Wed, Mar 13, 2024 at 09:46:22AM +0100, cem at kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
>
> If the test fails because the expected value doesn't match, it's
> useful to know what value was actually printed.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> V2:
> - Reduce subject to fit 70 chars
> - Use report_info() to output expected vs received values,
> leaving only necessary information for parsers on report()
>
> riscv/sbi.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 76f6111d..bc0f5c68 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -25,10 +25,15 @@ static bool env_or_skip(const char *env)
> return true;
> }
>
> -static void gen_report(struct sbiret *ret, long expected)
> +static void gen_report(struct sbiret *ret,
> + long expected_error, long expected_value)
> {
> - report(!ret->error, "no sbi.error");
> - report(ret->value == expected, "expected sbi.value");
> + if (ret->value != expected_value)
This should be
if (ret->error != expected_error || ret->value != expected_value)
> + report_info("expected (error: %ld, value: %ld), received: (error: %ld, value %ld)\n",
> + expected_error, expected_value, ret->error, ret->value);
> +
> + report(ret->error == expected_error, "expected sbi.error");
> + report(ret->value == expected_value, "expected sbi.value");
nit: We can reduce some code clutter with
bool check_error = ret->error == expected_error;
bool check_value = ret->value == expected_value;
if (!check_error || !check_value)
report_info(...);
report(check_error, ...);
report(check_value, ...);
> }
>
> static void check_base(void)
> @@ -42,14 +47,14 @@ static void check_base(void)
> if (env_or_skip("MVENDORID")) {
> expected = strtol(getenv("MVENDORID"), NULL, 0);
> ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
> - gen_report(&ret, expected);
> + gen_report(&ret, 0, expected);
> }
> report_prefix_pop();
>
> report_prefix_push("probe_ext");
> expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
> ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
> - gen_report(&ret, expected);
> + gen_report(&ret, 0, expected);
> report_prefix_pop();
>
> report_prefix_push("marchid");
> @@ -57,7 +62,7 @@ static void check_base(void)
> expected = strtol(getenv("MARCHID"), NULL, 0);
> ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT,
> SBI_EXT_BASE_GET_MARCHID, 0, 0, 0, 0, 0);
> - gen_report(&ret, expected);
> + gen_report(&ret, 0, expected);
> }
> report_prefix_pop();
>
> --
> 2.44.0
>
Thanks,
drew
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 3/6] riscv: Implement test for architecture ID register
2024-03-13 8:46 ` [PATCH V2 3/6] riscv: Implement test for architecture ID register cem
@ 2024-03-13 11:21 ` Andrew Jones
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2024-03-13 11:21 UTC (permalink / raw)
To: kvm-riscv
On Wed, Mar 13, 2024 at 09:46:21AM +0100, cem at kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
>
> Probe the MARCHID register and compare it to the specified MARCHID
> environment variable.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
> V2:
> - Wrap commit message around 70 chars
> - Remove unneeded blank lines
>
> riscv/sbi.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 519fcbe3..76f6111d 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -52,6 +52,15 @@ static void check_base(void)
> gen_report(&ret, expected);
> report_prefix_pop();
>
> + report_prefix_push("marchid");
> + if (env_or_skip("MARCHID")) {
> + expected = strtol(getenv("MARCHID"), NULL, 0);
> + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT,
> + SBI_EXT_BASE_GET_MARCHID, 0, 0, 0, 0, 0);
This should be
sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MARCHID, 0, ...)
> + gen_report(&ret, expected);
> + }
> + report_prefix_pop();
> +
> report_prefix_pop();
> }
>
> --
> 2.44.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 5/6] riscv: Test for specific SBI implementation ID
2024-03-13 8:46 ` [PATCH V2 5/6] riscv: Test for specific SBI implementation ID cem
@ 2024-03-13 11:23 ` Andrew Jones
2024-03-13 11:28 ` Andrew Jones
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Jones @ 2024-03-13 11:23 UTC (permalink / raw)
To: kvm-riscv
On Wed, Mar 13, 2024 at 09:46:23AM +0100, cem at kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
>
> Retrieve the ID from the SBI, and test it against the SBI_IMPLID
> enviroment variable.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
> V2:
> - Update commit description to fit 70 chars
> - Move sbi_ecall() after expected assignment to make consistent with
> other tests
>
> riscv/sbi.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index bc0f5c68..65492fd6 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -51,6 +51,15 @@ static void check_base(void)
> }
> report_prefix_pop();
>
> + report_prefix_push("sbi_impl_id");
> + if (env_or_skip("SBI_IMPLID")) {
Let's spell this SBI_IMPL_ID (just like its report prefix)
> + expected = strtol(getenv("SBI_IMPLID"), NULL, 0);
> + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID,
> + SBI_EXT_BASE, 0, 0, 0, 0, 0);
This should be
sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID, 0, ...)
> + gen_report(&ret, 0, expected);
> + }
> + report_prefix_pop();
> +
> report_prefix_push("probe_ext");
> expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
> ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
> --
> 2.44.0
>
Thanks,
drew
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 5/6] riscv: Test for specific SBI implementation ID
2024-03-13 11:23 ` Andrew Jones
@ 2024-03-13 11:28 ` Andrew Jones
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2024-03-13 11:28 UTC (permalink / raw)
To: kvm-riscv
On Wed, Mar 13, 2024 at 12:23:38PM +0100, Andrew Jones wrote:
> On Wed, Mar 13, 2024 at 09:46:23AM +0100, cem at kernel.org wrote:
> > From: Carlos Maiolino <cem@kernel.org>
> >
> > Retrieve the ID from the SBI, and test it against the SBI_IMPLID
> > enviroment variable.
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> > V2:
> > - Update commit description to fit 70 chars
> > - Move sbi_ecall() after expected assignment to make consistent with
> > other tests
> >
> > riscv/sbi.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/riscv/sbi.c b/riscv/sbi.c
> > index bc0f5c68..65492fd6 100644
> > --- a/riscv/sbi.c
> > +++ b/riscv/sbi.c
> > @@ -51,6 +51,15 @@ static void check_base(void)
> > }
> > report_prefix_pop();
> >
> > + report_prefix_push("sbi_impl_id");
> > + if (env_or_skip("SBI_IMPLID")) {
>
> Let's spell this SBI_IMPL_ID (just like its report prefix)
Actually, let's drop the SBI_ from the name (both report prefix and
environment variable) to be consistent with the other tests. Or, only
drop sbi_ from the report prefix (since we have the 'sbi' prefix already)
and add SBI_ to all the other environment variables.
>
> > + expected = strtol(getenv("SBI_IMPLID"), NULL, 0);
> > + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID,
> > + SBI_EXT_BASE, 0, 0, 0, 0, 0);
>
> This should be
>
> sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID, 0, ...)
>
> > + gen_report(&ret, 0, expected);
> > + }
> > + report_prefix_pop();
> > +
> > report_prefix_push("probe_ext");
> > expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
> > ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
> > --
> > 2.44.0
> >
>
> Thanks,
> drew
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 6/6] riscv: Add a wrapper to call sbi_ecall for base extension
2024-03-13 8:46 ` [PATCH 6/6] riscv: Add a wrapper to call sbi_ecall for base extension cem
@ 2024-03-13 11:33 ` Andrew Jones
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2024-03-13 11:33 UTC (permalink / raw)
To: kvm-riscv
On Wed, Mar 13, 2024 at 09:46:24AM +0100, cem at kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
>
> All SBI extension functions accepts at most one argument, so create a
> wrapper around sbi_ecall() to avoid needing to pass in arguments 1 to 5
> all the time, also, the wrapper can specify SBI_EXT_BASE directly.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> riscv/sbi.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 65492fd6..82513d61 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -36,6 +36,11 @@ static void gen_report(struct sbiret *ret,
> report(ret->value == expected_value, "expected sbi.value");
> }
>
> +static inline struct sbiret __base_sbi_ecall(int fid, unsigned long arg0)
Drop 'inline'. It's useless for static functions in .c files.
> +{
> + return sbi_ecall(SBI_EXT_BASE, fid, arg0, 0, 0, 0, 0, 0);
> +}
> +
> static void check_base(void)
> {
> struct sbiret ret;
> @@ -46,7 +51,7 @@ static void check_base(void)
> report_prefix_push("mvendorid");
> if (env_or_skip("MVENDORID")) {
> expected = strtol(getenv("MVENDORID"), NULL, 0);
> - ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID, 0, 0, 0, 0, 0, 0);
> + ret = __base_sbi_ecall(SBI_EXT_BASE_GET_MVENDORID, 0);
> gen_report(&ret, 0, expected);
> }
> report_prefix_pop();
> @@ -54,23 +59,21 @@ static void check_base(void)
> report_prefix_push("sbi_impl_id");
> if (env_or_skip("SBI_IMPLID")) {
> expected = strtol(getenv("SBI_IMPLID"), NULL, 0);
> - ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID,
> - SBI_EXT_BASE, 0, 0, 0, 0, 0);
> + ret = __base_sbi_ecall(SBI_EXT_BASE_GET_IMP_ID, SBI_EXT_BASE);
As pointed out in a previous patch the arg should be zero.
> gen_report(&ret, 0, expected);
> }
> report_prefix_pop();
>
> report_prefix_push("probe_ext");
> expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
> - ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE, 0, 0, 0, 0, 0);
> + ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE);
> gen_report(&ret, 0, expected);
> report_prefix_pop();
>
> report_prefix_push("marchid");
> if (env_or_skip("MARCHID")) {
> expected = strtol(getenv("MARCHID"), NULL, 0);
> - ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT,
> - SBI_EXT_BASE_GET_MARCHID, 0, 0, 0, 0, 0);
> + ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE_GET_MARCHID);
As pointed out in a previous patch the fid should get-marchid and the arg
should be zero.
> gen_report(&ret, 0, expected);
> }
> report_prefix_pop();
> --
> 2.44.0
>
The wrapper is good, but this patch should come near the beginning of the
series so we're not adding a bunch of code in earlier patches just to
modify it later.
Otherwise,
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-03-13 11:33 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-13 8:46 [PATCH V2 0/6] Add riscv tests to cover the base extension specs cem
2024-03-13 8:46 ` [PATCH V2 1/6] riscv: Add test to probe SBI Extension cem
2024-03-13 11:06 ` Andrew Jones
2024-03-13 8:46 ` [PATCH V2 2/6] riscv: Factor out environment variable check and report generation cem
2024-03-13 11:11 ` Andrew Jones
2024-03-13 8:46 ` [PATCH V2 3/6] riscv: Implement test for architecture ID register cem
2024-03-13 11:21 ` Andrew Jones
2024-03-13 8:46 ` [PATCH V2 4/6] riscv: Enable gen_report() to print the wrong value in case of failure cem
2024-03-13 11:19 ` Andrew Jones
2024-03-13 8:46 ` [PATCH V2 5/6] riscv: Test for specific SBI implementation ID cem
2024-03-13 11:23 ` Andrew Jones
2024-03-13 11:28 ` Andrew Jones
2024-03-13 8:46 ` [PATCH 6/6] riscv: Add a wrapper to call sbi_ecall for base extension cem
2024-03-13 11:33 ` Andrew Jones
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.