* [PATCH 1/6] riscv: Add test to probe SBI Extension
2024-02-29 12:42 [PATCH 0/6] Add riscv tests to cover the base extension specs cem
@ 2024-02-29 12:42 ` cem
2024-03-01 8:25 ` Andrew Jones
2024-02-29 12:42 ` [PATCH 2/6] riscv: Factor out environment variable check cem
` (5 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: cem @ 2024-02-29 12:42 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>
---
riscv/sbi.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 40 insertions(+), 12 deletions(-)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index ffb07a25..9dc82ecb 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -14,28 +14,56 @@ 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");
+ 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");
+ report_prefix_pop();
+
+ if (!getenv("PROBE_EXT")) {
+ report_skip("probe_ext: missing PROBE_EXT environment variable");
+ return;
+ }
+
+ report_prefix_push("probe_ext");
+ expected = strtol(getenv("PROBE_EXT"), NULL, 0);
+
+ 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();
-done:
return report_summary();
}
--
2.43.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 1/6] riscv: Add test to probe SBI Extension
2024-02-29 12:42 ` [PATCH 1/6] riscv: Add test to probe SBI Extension cem
@ 2024-03-01 8:25 ` Andrew Jones
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2024-03-01 8:25 UTC (permalink / raw)
To: kvm-riscv
On Thu, Feb 29, 2024 at 01:42:07PM +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>
> ---
> riscv/sbi.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index ffb07a25..9dc82ecb 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -14,28 +14,56 @@ 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;
We don't want to bail on all SBI base tests when a single expected value
environment variable is missing, but, looking ahead, I see the pattern
changes with the next patch to only skip the one test, so this patch is
fine.
> }
> +
> + 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");
> + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID,
> + 0, 0, 0, 0, 0, 0);
We use the Linux kernel's coding style (we encourage running the kernel's
checkpatch on patches). The kernel allows 100 chars and we're even more
tolerant of long lines in kvm-unit-tests. The zeros in sbi_ecall can be
annoying, though, so feel free to create a wrapper for a short-hand.
> +
> + report(!ret.error, "no sbi.error");
> + report(ret.value == expected, "expected sbi.value");
> + report_prefix_pop();
> +
> + if (!getenv("PROBE_EXT")) {
> + report_skip("probe_ext: missing PROBE_EXT environment variable");
> + return;
> + }
> +
> + report_prefix_push("probe_ext");
> + expected = strtol(getenv("PROBE_EXT"), NULL, 0);
> +
> + 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");
I think the expected value environment variable (PROBE_EXT) can be
optional in this case. Most implementations will return 0 or 1,
particularly for probing the Base extension. So, here I'd do
expected = getenv("PROBE_EXT") ? strtol(getenv("PROBE_EXT"), NULL, 0) : 1;
> + 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();
>
> -done:
> return report_summary();
> }
> --
> 2.43.2
>
Thanks,
drew
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/6] riscv: Factor out environment variable check
2024-02-29 12:42 [PATCH 0/6] Add riscv tests to cover the base extension specs cem
2024-02-29 12:42 ` [PATCH 1/6] riscv: Add test to probe SBI Extension cem
@ 2024-02-29 12:42 ` cem
2024-03-01 8:28 ` Andrew Jones
2024-02-29 12:42 ` [PATCH 3/6] riscv: Implement test for architecture ID register cem
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: cem @ 2024-02-29 12:42 UTC (permalink / raw)
To: kvm-riscv
From: Carlos Maiolino <cem@kernel.org>
Environment variables are checked all the time, so use a helper for that
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
riscv/sbi.c | 49 +++++++++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 9dc82ecb..636f907a 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_is_defined(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,34 +38,26 @@ 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);
+ if (env_is_defined("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 = 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");
- report_prefix_pop();
-
- if (!getenv("PROBE_EXT")) {
- report_skip("probe_ext: missing PROBE_EXT environment variable");
- return;
+ gen_report(&ret, expected);
}
+ report_prefix_pop();
report_prefix_push("probe_ext");
- expected = strtol(getenv("PROBE_EXT"), NULL, 0);
+ if (env_is_defined("PROBE_EXT")) {
+ expected = strtol(getenv("PROBE_EXT"), NULL, 0);
- ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT,
- SBI_EXT_BASE, 0, 0, 0, 0, 0);
+ 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.43.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 2/6] riscv: Factor out environment variable check
2024-02-29 12:42 ` [PATCH 2/6] riscv: Factor out environment variable check cem
@ 2024-03-01 8:28 ` Andrew Jones
2024-03-04 10:52 ` Carlos Maiolino
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Jones @ 2024-03-01 8:28 UTC (permalink / raw)
To: kvm-riscv
On Thu, Feb 29, 2024 at 01:42:08PM +0100, cem at kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
>
> Environment variables are checked all the time, so use a helper for that
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> riscv/sbi.c | 49 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 9dc82ecb..636f907a 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_is_defined(const char *env)
The function name doesn't imply anything about the report_skip side
effect, which is actually the important part of the function. Maybe
name it something like env_or_skip()?
> +{
> +
> + 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");
> +}
This refactoring isn't called out in the commit message.
> +
> static void check_base(void)
> {
> struct sbiret ret;
> @@ -21,34 +38,26 @@ 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);
> + if (env_is_defined("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 = 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");
> - report_prefix_pop();
> -
> - if (!getenv("PROBE_EXT")) {
> - report_skip("probe_ext: missing PROBE_EXT environment variable");
> - return;
> + gen_report(&ret, expected);
> }
> + report_prefix_pop();
>
> report_prefix_push("probe_ext");
> - expected = strtol(getenv("PROBE_EXT"), NULL, 0);
> + if (env_is_defined("PROBE_EXT")) {
> + expected = strtol(getenv("PROBE_EXT"), NULL, 0);
>
> - ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT,
> - SBI_EXT_BASE, 0, 0, 0, 0, 0);
> + 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.43.2
>
Thanks,
drew
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 2/6] riscv: Factor out environment variable check
2024-03-01 8:28 ` Andrew Jones
@ 2024-03-04 10:52 ` Carlos Maiolino
0 siblings, 0 replies; 21+ messages in thread
From: Carlos Maiolino @ 2024-03-04 10:52 UTC (permalink / raw)
To: kvm-riscv
On Fri, Mar 01, 2024 at 09:28:58AM +0100, Andrew Jones wrote:
> On Thu, Feb 29, 2024 at 01:42:08PM +0100, cem at kernel.org wrote:
> > From: Carlos Maiolino <cem@kernel.org>
> >
> > Environment variables are checked all the time, so use a helper for that
> >
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> > riscv/sbi.c | 49 +++++++++++++++++++++++++++++--------------------
> > 1 file changed, 29 insertions(+), 20 deletions(-)
> >
> > diff --git a/riscv/sbi.c b/riscv/sbi.c
> > index 9dc82ecb..636f907a 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_is_defined(const char *env)
>
> The function name doesn't imply anything about the report_skip side
> effect, which is actually the important part of the function. Maybe
> name it something like env_or_skip()?
>
> > +{
> > +
> > + 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");
> > +}
>
> This refactoring isn't called out in the commit message.
Sorry, I did this in a separated patch, and through a few rebases I accidentally
squashed the patch together on this one. I don't think this needs a new patch,
so I'll just update the commit message to also mention this refactoring.
I'll fix it and send it on the V2.
Thanks for the reviews.
Carlos
>
> > +
> > static void check_base(void)
> > {
> > struct sbiret ret;
> > @@ -21,34 +38,26 @@ 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);
> > + if (env_is_defined("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 = 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");
> > - report_prefix_pop();
> > -
> > - if (!getenv("PROBE_EXT")) {
> > - report_skip("probe_ext: missing PROBE_EXT environment variable");
> > - return;
> > + gen_report(&ret, expected);
> > }
> > + report_prefix_pop();
> >
> > report_prefix_push("probe_ext");
> > - expected = strtol(getenv("PROBE_EXT"), NULL, 0);
> > + if (env_is_defined("PROBE_EXT")) {
> > + expected = strtol(getenv("PROBE_EXT"), NULL, 0);
> >
> > - ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT,
> > - SBI_EXT_BASE, 0, 0, 0, 0, 0);
> > + 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.43.2
> >
>
> Thanks,
> drew
>
> --
> kvm-riscv mailing list
> kvm-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/6] riscv: Implement test for architecture ID register
2024-02-29 12:42 [PATCH 0/6] Add riscv tests to cover the base extension specs cem
2024-02-29 12:42 ` [PATCH 1/6] riscv: Add test to probe SBI Extension cem
2024-02-29 12:42 ` [PATCH 2/6] riscv: Factor out environment variable check cem
@ 2024-02-29 12:42 ` cem
2024-03-01 8:31 ` Andrew Jones
2024-02-29 12:42 ` [PATCH 4/6] riscv: Enable gen_report() to print the wrong value in case of test failure cem
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: cem @ 2024-02-29 12:42 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>
---
riscv/sbi.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 636f907a..fa28d7c8 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -60,6 +60,17 @@ static void check_base(void)
}
report_prefix_pop();
+ report_prefix_push("marchid");
+ if (env_is_defined("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.43.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 3/6] riscv: Implement test for architecture ID register
2024-02-29 12:42 ` [PATCH 3/6] riscv: Implement test for architecture ID register cem
@ 2024-03-01 8:31 ` Andrew Jones
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2024-03-01 8:31 UTC (permalink / raw)
To: kvm-riscv
On Thu, Feb 29, 2024 at 01:42:09PM +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
Please keep commit message lines to around 70 chars long (same comment
applies to other patches as well)
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> riscv/sbi.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 636f907a..fa28d7c8 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -60,6 +60,17 @@ static void check_base(void)
> }
> report_prefix_pop();
>
> + report_prefix_push("marchid");
> + if (env_is_defined("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);
nit: no need for the two blank lines
> + }
> + report_prefix_pop();
> +
> report_prefix_pop();
> }
With the rename of env_is_defined to include 'skip' somehow, then I like
the looks of this pattern.
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Thanks,
drew
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/6] riscv: Enable gen_report() to print the wrong value in case of test failure
2024-02-29 12:42 [PATCH 0/6] Add riscv tests to cover the base extension specs cem
` (2 preceding siblings ...)
2024-02-29 12:42 ` [PATCH 3/6] riscv: Implement test for architecture ID register cem
@ 2024-02-29 12:42 ` cem
2024-03-01 8:43 ` Andrew Jones
2024-03-01 8:46 ` Andrew Jones
2024-02-29 12:42 ` [PATCH 5/6] riscv: Test for specific SBI implementation ID cem
` (2 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: cem @ 2024-02-29 12:42 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>
---
riscv/sbi.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index fa28d7c8..8ad8f375 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -28,7 +28,12 @@ static bool env_is_defined(const char *env)
static void gen_report(struct sbiret *ret, long expected)
{
report(!ret->error, "no sbi.error");
- report(ret->value == expected, "expected sbi.value");
+
+ if (ret->value == expected)
+ report(true, "expected sbi.value");
+ else
+ report(false, "expected sbi.value: %ld - Got: %ld",
+ expected, ret->value);
}
static void check_base(void)
--
2.43.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 4/6] riscv: Enable gen_report() to print the wrong value in case of test failure
2024-02-29 12:42 ` [PATCH 4/6] riscv: Enable gen_report() to print the wrong value in case of test failure cem
@ 2024-03-01 8:43 ` Andrew Jones
2024-03-05 8:05 ` Carlos Maiolino
2024-03-01 8:46 ` Andrew Jones
1 sibling, 1 reply; 21+ messages in thread
From: Andrew Jones @ 2024-03-01 8:43 UTC (permalink / raw)
To: kvm-riscv
On Thu, Feb 29, 2024 at 01:42:10PM +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>
> ---
> riscv/sbi.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index fa28d7c8..8ad8f375 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -28,7 +28,12 @@ static bool env_is_defined(const char *env)
> static void gen_report(struct sbiret *ret, long expected)
> {
> report(!ret->error, "no sbi.error");
> - report(ret->value == expected, "expected sbi.value");
> +
> + if (ret->value == expected)
> + report(true, "expected sbi.value");
> + else
> + report(false, "expected sbi.value: %ld - Got: %ld",
> + expected, ret->value);
We want to keep the output line consistent for both pass and fail
in order to simplify parsing. But, since the expected value is
variable, depending on the SBI implementation under test, it's
probably best not to put the numbers in the pass/fail line, but as
info. So how about
static void gen_report(struct sbiret *ret,
long expected_error, long expected_value)
{
report_info("expected (error: %ld, value: %ld), received: (error: %ld, value: %ld)",
expected_error, expected_value, ret->error, ret->value);
report(ret->error == expected_error, "expected sbi.error");
report(ret->value == expected_value, "expected sbi.value");
}
I've changed this function to take the error as input because negative
tests (which we should also have) will be looking for specific errors.
Thanks,
drew
> }
>
> static void check_base(void)
> --
> 2.43.2
>
>
> --
> kvm-riscv mailing list
> kvm-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 4/6] riscv: Enable gen_report() to print the wrong value in case of test failure
2024-03-01 8:43 ` Andrew Jones
@ 2024-03-05 8:05 ` Carlos Maiolino
2024-03-05 12:16 ` Andrew Jones
0 siblings, 1 reply; 21+ messages in thread
From: Carlos Maiolino @ 2024-03-05 8:05 UTC (permalink / raw)
To: kvm-riscv
>
> We want to keep the output line consistent for both pass and fail
> in order to simplify parsing. But, since the expected value is
> variable, depending on the SBI implementation under test, it's
> probably best not to put the numbers in the pass/fail line, but as
> info. So how about
>
> static void gen_report(struct sbiret *ret,
> long expected_error, long expected_value)
> {
> report_info("expected (error: %ld, value: %ld), received: (error: %ld, value: %ld)",
> expected_error, expected_value, ret->error, ret->value);
> report(ret->error == expected_error, "expected sbi.error");
> report(ret->value == expected_value, "expected sbi.value");
> }
Something just came up to my mind while implementing this.
I agree with the use of report_info() here, but I still think this could be
under a conditional to only print it when we have something different from the
expectations. I particularly don't see much value in printing the report_info if
nothing unexpected, but maybe I'm wrong?
Carlos
>
> I've changed this function to take the error as input because negative
> tests (which we should also have) will be looking for specific errors.
>
> Thanks,
> drew
>
>
> > }
> >
> > static void check_base(void)
> > --
> > 2.43.2
> >
> >
> > --
> > kvm-riscv mailing list
> > kvm-riscv at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kvm-riscv
>
> --
> kvm-riscv mailing list
> kvm-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 4/6] riscv: Enable gen_report() to print the wrong value in case of test failure
2024-03-05 8:05 ` Carlos Maiolino
@ 2024-03-05 12:16 ` Andrew Jones
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2024-03-05 12:16 UTC (permalink / raw)
To: kvm-riscv
On Tue, Mar 05, 2024 at 09:05:31AM +0100, Carlos Maiolino wrote:
> >
> > We want to keep the output line consistent for both pass and fail
> > in order to simplify parsing. But, since the expected value is
> > variable, depending on the SBI implementation under test, it's
> > probably best not to put the numbers in the pass/fail line, but as
> > info. So how about
> >
> > static void gen_report(struct sbiret *ret,
> > long expected_error, long expected_value)
> > {
> > report_info("expected (error: %ld, value: %ld), received: (error: %ld, value: %ld)",
> > expected_error, expected_value, ret->error, ret->value);
> > report(ret->error == expected_error, "expected sbi.error");
> > report(ret->value == expected_value, "expected sbi.value");
> > }
>
> Something just came up to my mind while implementing this.
>
> I agree with the use of report_info() here, but I still think this could be
> under a conditional to only print it when we have something different from the
> expectations. I particularly don't see much value in printing the report_info if
> nothing unexpected, but maybe I'm wrong?
I don't mind either way. report_info() is for human readers, so parsers
should ignore them and not freak out when they appear or disappear. Not
cluttering the output by removing useless info sounds good to me, but
also not cluttering the code, by not adding unnecessary logic, sounds
good to me. So, it's up to you :-)
Thanks,
drew
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/6] riscv: Enable gen_report() to print the wrong value in case of test failure
2024-02-29 12:42 ` [PATCH 4/6] riscv: Enable gen_report() to print the wrong value in case of test failure cem
2024-03-01 8:43 ` Andrew Jones
@ 2024-03-01 8:46 ` Andrew Jones
1 sibling, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2024-03-01 8:46 UTC (permalink / raw)
To: kvm-riscv
This patch's summary ($SUBJECT) is too long.
Thanks,
drew
On Thu, Feb 29, 2024 at 01:42:10PM +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>
> ---
> riscv/sbi.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index fa28d7c8..8ad8f375 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -28,7 +28,12 @@ static bool env_is_defined(const char *env)
> static void gen_report(struct sbiret *ret, long expected)
> {
> report(!ret->error, "no sbi.error");
> - report(ret->value == expected, "expected sbi.value");
> +
> + if (ret->value == expected)
> + report(true, "expected sbi.value");
> + else
> + report(false, "expected sbi.value: %ld - Got: %ld",
> + expected, ret->value);
> }
>
> static void check_base(void)
> --
> 2.43.2
>
>
> --
> kvm-riscv mailing list
> kvm-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/6] riscv: Test for specific SBI implementation ID
2024-02-29 12:42 [PATCH 0/6] Add riscv tests to cover the base extension specs cem
` (3 preceding siblings ...)
2024-02-29 12:42 ` [PATCH 4/6] riscv: Enable gen_report() to print the wrong value in case of test failure cem
@ 2024-02-29 12:42 ` cem
2024-03-01 8:45 ` Andrew Jones
2024-02-29 12:42 ` [PATCH 6/6] riscv: Test for a SBI implementation ID range cem
2024-02-29 14:05 ` [PATCH 0/6] Add riscv tests to cover the base extension specs Andrew Jones
6 siblings, 1 reply; 21+ messages in thread
From: cem @ 2024-02-29 12:42 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>
---
riscv/sbi.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 8ad8f375..9daab9dc 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -54,6 +54,16 @@ static void check_base(void)
}
report_prefix_pop();
+ report_prefix_push("sbi_impl_id");
+ if (env_is_defined("SBI_IMPLID")) {
+ ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID,
+ SBI_EXT_BASE, 0, 0, 0, 0, 0);
+ expected = strtol(getenv("SBI_IMPLID"), NULL, 0);
+
+ gen_report(&ret, expected);
+ }
+ report_prefix_pop();
+
report_prefix_push("probe_ext");
if (env_is_defined("PROBE_EXT")) {
expected = strtol(getenv("PROBE_EXT"), NULL, 0);
--
2.43.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 5/6] riscv: Test for specific SBI implementation ID
2024-02-29 12:42 ` [PATCH 5/6] riscv: Test for specific SBI implementation ID cem
@ 2024-03-01 8:45 ` Andrew Jones
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2024-03-01 8:45 UTC (permalink / raw)
To: kvm-riscv
On Thu, Feb 29, 2024 at 01:42:11PM +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>
> ---
> riscv/sbi.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 8ad8f375..9daab9dc 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -54,6 +54,16 @@ static void check_base(void)
> }
> report_prefix_pop();
>
> + report_prefix_push("sbi_impl_id");
> + if (env_is_defined("SBI_IMPLID")) {
> + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_ID,
> + SBI_EXT_BASE, 0, 0, 0, 0, 0);
> + expected = strtol(getenv("SBI_IMPLID"), NULL, 0);
> +
> + gen_report(&ret, expected);
nit: keep this sequence of lines consistent to make it easier for
readers, i.e. always use
expected = ...
ret = sbi_ecal(...
gen_report(...
> + }
> + report_prefix_pop();
> +
> report_prefix_push("probe_ext");
> if (env_is_defined("PROBE_EXT")) {
> expected = strtol(getenv("PROBE_EXT"), NULL, 0);
> --
> 2.43.2
>
Otherwise,
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 6/6] riscv: Test for a SBI implementation ID range
2024-02-29 12:42 [PATCH 0/6] Add riscv tests to cover the base extension specs cem
` (4 preceding siblings ...)
2024-02-29 12:42 ` [PATCH 5/6] riscv: Test for specific SBI implementation ID cem
@ 2024-02-29 12:42 ` cem
2024-03-01 8:52 ` Andrew Jones
2024-02-29 14:05 ` [PATCH 0/6] Add riscv tests to cover the base extension specs Andrew Jones
6 siblings, 1 reply; 21+ messages in thread
From: cem @ 2024-02-29 12:42 UTC (permalink / raw)
To: kvm-riscv
From: Carlos Maiolino <cem@kernel.org>
This tests the SBI ID against SBI_IMPLID_MAX, so it can be used to test a SBI
against the available IDs from a specification version.
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
riscv/sbi.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 9daab9dc..37f5680c 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -64,6 +64,14 @@ static void check_base(void)
}
report_prefix_pop();
+ report_prefix_push("sbi_impl_id_max");
+ if (env_is_defined("SBI_IMPLID_MAX")) {
+ expected = strtol(getenv("SBI_IMPLID_MAX"), NULL, 0);
+
+ gen_report(&ret, (ret.value <= expected));
+ }
+ report_prefix_pop();
+
report_prefix_push("probe_ext");
if (env_is_defined("PROBE_EXT")) {
expected = strtol(getenv("PROBE_EXT"), NULL, 0);
--
2.43.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 6/6] riscv: Test for a SBI implementation ID range
2024-02-29 12:42 ` [PATCH 6/6] riscv: Test for a SBI implementation ID range cem
@ 2024-03-01 8:52 ` Andrew Jones
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Jones @ 2024-03-01 8:52 UTC (permalink / raw)
To: kvm-riscv
On Thu, Feb 29, 2024 at 01:42:12PM +0100, cem at kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
>
> This tests the SBI ID against SBI_IMPLID_MAX, so it can be used to test a SBI
> against the available IDs from a specification version.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> riscv/sbi.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 9daab9dc..37f5680c 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -64,6 +64,14 @@ static void check_base(void)
> }
> report_prefix_pop();
>
> + report_prefix_push("sbi_impl_id_max");
> + if (env_is_defined("SBI_IMPLID_MAX")) {
> + expected = strtol(getenv("SBI_IMPLID_MAX"), NULL, 0);
> +
> + gen_report(&ret, (ret.value <= expected));
> + }
> + report_prefix_pop();
tl;dr: let's drop this patch
There's nothing in the spec about a maximum and the previous test already
would have failed if the value wasn't exactly what the tester expected
it to be. I can see a case for having a maximum test instead of the exact
value test, though, if test configurations were meant to work on more than
one SBI implementation, but it's probably best that each configuration be
tailored to specific implementations, so a maximum instead of an explicit
set probably isn't what we want.
Thanks,
drew
> +
> report_prefix_push("probe_ext");
> if (env_is_defined("PROBE_EXT")) {
> expected = strtol(getenv("PROBE_EXT"), NULL, 0);
> --
> 2.43.2
>
>
> --
> kvm-riscv mailing list
> kvm-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/6] Add riscv tests to cover the base extension specs
2024-02-29 12:42 [PATCH 0/6] Add riscv tests to cover the base extension specs cem
` (5 preceding siblings ...)
2024-02-29 12:42 ` [PATCH 6/6] riscv: Test for a SBI implementation ID range cem
@ 2024-02-29 14:05 ` Andrew Jones
2024-02-29 14:12 ` Carlos Maiolino
6 siblings, 1 reply; 21+ messages in thread
From: Andrew Jones @ 2024-02-29 14:05 UTC (permalink / raw)
To: kvm-riscv
On Thu, Feb 29, 2024 at 01:42:06PM +0100, cem at kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
>
> Hi,
>
> this is my first attempt to create tests to cover some functions of the riscv's
> SBI base implementation spec. The series also includes a couple helpers to
> reduce code duplication.
>
> Carlos Maiolino (6):
> riscv: Add test to probe SBI Extension
> riscv: Factor out environment variable check
> riscv: Implement test for architecture ID register
> riscv: Enable gen_report() to print the wrong value in case of test
> failure
> riscv: Test for specific SBI implementation ID
> riscv: Test for a SBI implementation ID range
>
> riscv/sbi.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 83 insertions(+), 12 deletions(-)
>
> --
> 2.43.2
Hi Carlos,
If you can, please CC tech-prs at lists.riscv.org when posting SBI tests.
Thanks,
drew
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 0/6] Add riscv tests to cover the base extension specs
2024-02-29 14:05 ` [PATCH 0/6] Add riscv tests to cover the base extension specs Andrew Jones
@ 2024-02-29 14:12 ` Carlos Maiolino
2024-02-29 15:03 ` Andrew Jones
0 siblings, 1 reply; 21+ messages in thread
From: Carlos Maiolino @ 2024-02-29 14:12 UTC (permalink / raw)
To: kvm-riscv
On Thu, Feb 29, 2024 at 03:05:48PM +0100, Andrew Jones wrote:
> On Thu, Feb 29, 2024 at 01:42:06PM +0100, cem at kernel.org wrote:
> > From: Carlos Maiolino <cem@kernel.org>
> >
> > Hi,
> >
> > this is my first attempt to create tests to cover some functions of the riscv's
> > SBI base implementation spec. The series also includes a couple helpers to
> > reduce code duplication.
> >
> > Carlos Maiolino (6):
> > riscv: Add test to probe SBI Extension
> > riscv: Factor out environment variable check
> > riscv: Implement test for architecture ID register
> > riscv: Enable gen_report() to print the wrong value in case of test
> > failure
> > riscv: Test for specific SBI implementation ID
> > riscv: Test for a SBI implementation ID range
> >
> > riscv/sbi.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 83 insertions(+), 12 deletions(-)
> >
> > --
> > 2.43.2
>
> Hi Carlos,
>
> If you can, please CC tech-prs at lists.riscv.org when posting SBI tests.
Ok, will do, should I resend this series?
Carlos
>
> Thanks,
> drew
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/6] Add riscv tests to cover the base extension specs
2024-02-29 14:12 ` Carlos Maiolino
@ 2024-02-29 15:03 ` Andrew Jones
2024-02-29 15:31 ` Carlos Maiolino
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Jones @ 2024-02-29 15:03 UTC (permalink / raw)
To: kvm-riscv
On Thu, Feb 29, 2024 at 03:12:43PM +0100, Carlos Maiolino wrote:
> On Thu, Feb 29, 2024 at 03:05:48PM +0100, Andrew Jones wrote:
> > On Thu, Feb 29, 2024 at 01:42:06PM +0100, cem at kernel.org wrote:
> > > From: Carlos Maiolino <cem@kernel.org>
> > >
> > > Hi,
> > >
> > > this is my first attempt to create tests to cover some functions of the riscv's
> > > SBI base implementation spec. The series also includes a couple helpers to
> > > reduce code duplication.
> > >
> > > Carlos Maiolino (6):
> > > riscv: Add test to probe SBI Extension
> > > riscv: Factor out environment variable check
> > > riscv: Implement test for architecture ID register
> > > riscv: Enable gen_report() to print the wrong value in case of test
> > > failure
> > > riscv: Test for specific SBI implementation ID
> > > riscv: Test for a SBI implementation ID range
> > >
> > > riscv/sbi.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++-------
> > > 1 file changed, 83 insertions(+), 12 deletions(-)
> > >
> > > --
> > > 2.43.2
> >
> > Hi Carlos,
> >
> > If you can, please CC tech-prs at lists.riscv.org when posting SBI tests.
>
> Ok, will do, should I resend this series?
Might as well, or at least pick up the CC when posting v2.
Thanks,
drew
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/6] Add riscv tests to cover the base extension specs
2024-02-29 15:03 ` Andrew Jones
@ 2024-02-29 15:31 ` Carlos Maiolino
0 siblings, 0 replies; 21+ messages in thread
From: Carlos Maiolino @ 2024-02-29 15:31 UTC (permalink / raw)
To: kvm-riscv
> > > > riscv: Factor out environment variable check
> > > > riscv: Implement test for architecture ID register
> > > > riscv: Enable gen_report() to print the wrong value in case of test
> > > > failure
> > > > riscv: Test for specific SBI implementation ID
> > > > riscv: Test for a SBI implementation ID range
> > > >
> > > > riscv/sbi.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++-------
> > > > 1 file changed, 83 insertions(+), 12 deletions(-)
> > > >
> > > > --
> > > > 2.43.2
> > >
> > > Hi Carlos,
> > >
> > > If you can, please CC tech-prs at lists.riscv.org when posting SBI tests.
> >
> > Ok, will do, should I resend this series?
>
> Might as well, or at least pick up the CC when posting v2.
Sounds good, I'll wait for reviews, and post a V2 CC'ing tech-prs.
Carlos
>
> Thanks,
> drew
^ permalink raw reply [flat|nested] 21+ messages in thread