[kvm-unit-tests PATCH] riscv: lib: Add sbi-exit-code to configure and environment

[kvm-unit-tests PATCH] riscv: lib: Add sbi-exit-code to configure and environment

[Jesse Taube]

Add --[enable|disable]-sbi-exit-code to configure script.
With the default value disabled.
Add a check for SBI_PASS_EXIT_CODE in the environment, so that passing
of the test status is configurable from both the
environment and the configure script

Signed-off-by: Jesse Taube <jesse@rivosinc.com>
---
 configure      | 11 +++++++++++
 lib/riscv/io.c | 12 +++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 20bf5042..7c949bdc 100755
--- a/configure
+++ b/configure
@@ -67,6 +67,7 @@ earlycon=
 console=
 efi=
 efi_direct=
+sbi_exit_code=0
 target_cpu=
 
 # Enable -Werror by default for git repositories only (i.e. developer builds)
@@ -141,6 +142,9 @@ usage() {
 	                           system and run from the UEFI shell. Ignored when efi isn't enabled
 	                           and defaults to enabled when efi is enabled for riscv64.
 	                           (arm64 and riscv64 only)
+	    --[enable|disable]-sbi-exit-code
+	                           Enable or disable sending pass/fail exit code to SBI SRST.
+	                           (disabled by default, riscv only)
 EOF
     exit 1
 }
@@ -236,6 +240,12 @@ while [[ $optno -le $argc ]]; do
 	--disable-efi-direct)
 	    efi_direct=n
 	    ;;
+	--enable-sbi-exit-code)
+	    sbi_exit_code=1
+	    ;;
+	--disable-sbi-exit-code)
+	    sbi_exit_code=0
+	    ;;
 	--enable-werror)
 	    werror=-Werror
 	    ;;
@@ -551,6 +561,7 @@ EOF
 elif [ "$arch" = "riscv32" ] || [ "$arch" = "riscv64" ]; then
     echo "#define CONFIG_UART_EARLY_BASE ${uart_early_addr}" >> lib/config.h
     [ "$console" = "sbi" ] && echo "#define CONFIG_SBI_CONSOLE" >> lib/config.h
+    echo "#define CONFIG_SBI_EXIT_CODE ${sbi_exit_code}" >> lib/config.h
     echo >> lib/config.h
 fi
 echo "#endif" >> lib/config.h
diff --git a/lib/riscv/io.c b/lib/riscv/io.c
index b1163404..0e666009 100644
--- a/lib/riscv/io.c
+++ b/lib/riscv/io.c
@@ -162,8 +162,18 @@ void halt(int code);
 
 void exit(int code)
 {
+	char *s = getenv("SBI_PASS_EXIT_CODE");
+	bool pass_exit = CONFIG_SBI_EXIT_CODE;
+
 	printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1);
-	sbi_shutdown(code == 0);
+
+	if (s)
+		pass_exit = (*s == '1' || *s == 'y' || *s == 'Y');
+
+	if (pass_exit)
+		sbi_shutdown(code == 0);
+	else
+		sbi_shutdown(true);
 	halt(code);
 	__builtin_unreachable();
 }
-- 
2.43.0

Re: [kvm-unit-tests PATCH] riscv: lib: Add sbi-exit-code to configure and environment

[Andrew Jones]

On Thu, Jul 03, 2025 at 06:36:00AM -0700, Jesse Taube wrote:
> Add --[enable|disable]-sbi-exit-code to configure script.
> With the default value disabled.
> Add a check for SBI_PASS_EXIT_CODE in the environment, so that passing
> of the test status is configurable from both the
> environment and the configure script
> 
> Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> ---
>  configure      | 11 +++++++++++
>  lib/riscv/io.c | 12 +++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 20bf5042..7c949bdc 100755
> --- a/configure
> +++ b/configure
> @@ -67,6 +67,7 @@ earlycon=
>  console=
>  efi=
>  efi_direct=
> +sbi_exit_code=0
>  target_cpu=
>  
>  # Enable -Werror by default for git repositories only (i.e. developer builds)
> @@ -141,6 +142,9 @@ usage() {
>  	                           system and run from the UEFI shell. Ignored when efi isn't enabled
>  	                           and defaults to enabled when efi is enabled for riscv64.
>  	                           (arm64 and riscv64 only)
> +	    --[enable|disable]-sbi-exit-code
> +	                           Enable or disable sending pass/fail exit code to SBI SRST.
> +	                           (disabled by default, riscv only)
>  EOF
>      exit 1
>  }
> @@ -236,6 +240,12 @@ while [[ $optno -le $argc ]]; do
>  	--disable-efi-direct)
>  	    efi_direct=n
>  	    ;;
> +	--enable-sbi-exit-code)
> +	    sbi_exit_code=1
> +	    ;;
> +	--disable-sbi-exit-code)
> +	    sbi_exit_code=0
> +	    ;;
>  	--enable-werror)
>  	    werror=-Werror
>  	    ;;
> @@ -551,6 +561,7 @@ EOF
>  elif [ "$arch" = "riscv32" ] || [ "$arch" = "riscv64" ]; then
>      echo "#define CONFIG_UART_EARLY_BASE ${uart_early_addr}" >> lib/config.h
>      [ "$console" = "sbi" ] && echo "#define CONFIG_SBI_CONSOLE" >> lib/config.h
> +    echo "#define CONFIG_SBI_EXIT_CODE ${sbi_exit_code}" >> lib/config.h
>      echo >> lib/config.h
>  fi
>  echo "#endif" >> lib/config.h
> diff --git a/lib/riscv/io.c b/lib/riscv/io.c
> index b1163404..0e666009 100644
> --- a/lib/riscv/io.c
> +++ b/lib/riscv/io.c
> @@ -162,8 +162,18 @@ void halt(int code);
>  
>  void exit(int code)
>  {
> +	char *s = getenv("SBI_PASS_EXIT_CODE");
> +	bool pass_exit = CONFIG_SBI_EXIT_CODE;

This is the first case of what may become more common - a config variable
which also has an env override. I think it may be good convention to
name them the same, i.e. the env name would also be CONFIG_SBI_EXIT_CODE,
unless you think that would be confusing for some reason?

> +
>  	printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1);
> -	sbi_shutdown(code == 0);
> +
> +	if (s)
> +		pass_exit = (*s == '1' || *s == 'y' || *s == 'Y');

We now have this logic in four places[1]. I think it's time we factor it,
and it's counterpart "!(s && (*s == '0' || *s == 'n' || *s == 'N'))"
into a couple helper macros. I'm not sure where the best place for
those macros to live is, though. I guess libcflat.h, but we really
ought to split that thing apart someday...

[1]
 - twice in lib/errata.h
 - once in riscv/sbi-tests.ha
 - and now here

> +
> +	if (pass_exit)
> +		sbi_shutdown(code == 0);
> +	else
> +		sbi_shutdown(true);

nit: can be written more concisely as

 sbi_shutdown(pass_exit ? code == 0 : true)

>  	halt(code);
>  	__builtin_unreachable();
>  }
> -- 
> 2.43.0
>

Thanks,
drew

Re: [kvm-unit-tests PATCH] riscv: lib: Add sbi-exit-code to configure and environment

[Jesse Taube]

On Fri, Jul 4, 2025 at 1:17 AM Andrew Jones <andrew.jones@linux.dev> wrote:
>
> On Thu, Jul 03, 2025 at 06:36:00AM -0700, Jesse Taube wrote:
> > Add --[enable|disable]-sbi-exit-code to configure script.
> > With the default value disabled.
> > Add a check for SBI_PASS_EXIT_CODE in the environment, so that passing
> > of the test status is configurable from both the
> > environment and the configure script
> >
> > Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> > ---
> >  configure      | 11 +++++++++++
> >  lib/riscv/io.c | 12 +++++++++++-
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/configure b/configure
> > index 20bf5042..7c949bdc 100755
> > --- a/configure
> > +++ b/configure
> > @@ -67,6 +67,7 @@ earlycon=
> >  console=
> >  efi=
> >  efi_direct=
> > +sbi_exit_code=0
> >  target_cpu=
> >
> >  # Enable -Werror by default for git repositories only (i.e. developer builds)
> > @@ -141,6 +142,9 @@ usage() {
> >                                  system and run from the UEFI shell. Ignored when efi isn't enabled
> >                                  and defaults to enabled when efi is enabled for riscv64.
> >                                  (arm64 and riscv64 only)
> > +         --[enable|disable]-sbi-exit-code
> > +                                Enable or disable sending pass/fail exit code to SBI SRST.
> > +                                (disabled by default, riscv only)
> >  EOF
> >      exit 1
> >  }
> > @@ -236,6 +240,12 @@ while [[ $optno -le $argc ]]; do
> >       --disable-efi-direct)
> >           efi_direct=n
> >           ;;
> > +     --enable-sbi-exit-code)
> > +         sbi_exit_code=1
> > +         ;;
> > +     --disable-sbi-exit-code)
> > +         sbi_exit_code=0
> > +         ;;
> >       --enable-werror)
> >           werror=-Werror
> >           ;;
> > @@ -551,6 +561,7 @@ EOF
> >  elif [ "$arch" = "riscv32" ] || [ "$arch" = "riscv64" ]; then
> >      echo "#define CONFIG_UART_EARLY_BASE ${uart_early_addr}" >> lib/config.h
> >      [ "$console" = "sbi" ] && echo "#define CONFIG_SBI_CONSOLE" >> lib/config.h
> > +    echo "#define CONFIG_SBI_EXIT_CODE ${sbi_exit_code}" >> lib/config.h
> >      echo >> lib/config.h
> >  fi
> >  echo "#endif" >> lib/config.h
> > diff --git a/lib/riscv/io.c b/lib/riscv/io.c
> > index b1163404..0e666009 100644
> > --- a/lib/riscv/io.c
> > +++ b/lib/riscv/io.c
> > @@ -162,8 +162,18 @@ void halt(int code);
> >
> >  void exit(int code)
> >  {
> > +     char *s = getenv("SBI_PASS_EXIT_CODE");
> > +     bool pass_exit = CONFIG_SBI_EXIT_CODE;
>
> This is the first case of what may become more common - a config variable
> which also has an env override. I think it may be good convention to
> name them the same, i.e. the env name would also be CONFIG_SBI_EXIT_CODE,
> unless you think that would be confusing for some reason?

I changed the name because the configure option seemed very long.
I will make them both SBI_EXIT_CODE. Should I add a macro to simplify
future uses
of a config variable which also has an env override.

>
> > +
> >       printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1);
> > -     sbi_shutdown(code == 0);
> > +
> > +     if (s)
> > +             pass_exit = (*s == '1' || *s == 'y' || *s == 'Y');
>
> We now have this logic in four places[1]. I think it's time we factor it,
> and it's counterpart "!(s && (*s == '0' || *s == 'n' || *s == 'N'))"
> into a couple helper macros. I'm not sure where the best place for
> those macros to live is, though. I guess libcflat.h, but we really
> ought to split that thing apart someday...

Is lib/argv.h an ok place?
getenv is defined in lib/string.c which is interesting. I wonder if it
could be moved to lib/argv.c?

Thanks,
Jesse Taube
>
> [1]
>  - twice in lib/errata.h
>  - once in riscv/sbi-tests.ha
>  - and now here
>
> > +
> > +     if (pass_exit)
> > +             sbi_shutdown(code == 0);
> > +     else
> > +             sbi_shutdown(true);
>
> nit: can be written more concisely as
>
>  sbi_shutdown(pass_exit ? code == 0 : true)
>
> >       halt(code);
> >       __builtin_unreachable();
> >  }
> > --
> > 2.43.0
> >
>
> Thanks,
> drew

Re: [kvm-unit-tests PATCH] riscv: lib: Add sbi-exit-code to configure and environment

[Andrew Jones]

On Mon, Jul 07, 2025 at 08:57:37AM -0700, Jesse Taube wrote:
> On Fri, Jul 4, 2025 at 1:17 AM Andrew Jones <andrew.jones@linux.dev> wrote:
> >
> > On Thu, Jul 03, 2025 at 06:36:00AM -0700, Jesse Taube wrote:
> > > Add --[enable|disable]-sbi-exit-code to configure script.
> > > With the default value disabled.
> > > Add a check for SBI_PASS_EXIT_CODE in the environment, so that passing
> > > of the test status is configurable from both the
> > > environment and the configure script
> > >
> > > Signed-off-by: Jesse Taube <jesse@rivosinc.com>
> > > ---
> > >  configure      | 11 +++++++++++
> > >  lib/riscv/io.c | 12 +++++++++++-
> > >  2 files changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/configure b/configure
> > > index 20bf5042..7c949bdc 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -67,6 +67,7 @@ earlycon=
> > >  console=
> > >  efi=
> > >  efi_direct=
> > > +sbi_exit_code=0
> > >  target_cpu=
> > >
> > >  # Enable -Werror by default for git repositories only (i.e. developer builds)
> > > @@ -141,6 +142,9 @@ usage() {
> > >                                  system and run from the UEFI shell. Ignored when efi isn't enabled
> > >                                  and defaults to enabled when efi is enabled for riscv64.
> > >                                  (arm64 and riscv64 only)
> > > +         --[enable|disable]-sbi-exit-code
> > > +                                Enable or disable sending pass/fail exit code to SBI SRST.
> > > +                                (disabled by default, riscv only)
> > >  EOF
> > >      exit 1
> > >  }
> > > @@ -236,6 +240,12 @@ while [[ $optno -le $argc ]]; do
> > >       --disable-efi-direct)
> > >           efi_direct=n
> > >           ;;
> > > +     --enable-sbi-exit-code)
> > > +         sbi_exit_code=1
> > > +         ;;
> > > +     --disable-sbi-exit-code)
> > > +         sbi_exit_code=0
> > > +         ;;
> > >       --enable-werror)
> > >           werror=-Werror
> > >           ;;
> > > @@ -551,6 +561,7 @@ EOF
> > >  elif [ "$arch" = "riscv32" ] || [ "$arch" = "riscv64" ]; then
> > >      echo "#define CONFIG_UART_EARLY_BASE ${uart_early_addr}" >> lib/config.h
> > >      [ "$console" = "sbi" ] && echo "#define CONFIG_SBI_CONSOLE" >> lib/config.h
> > > +    echo "#define CONFIG_SBI_EXIT_CODE ${sbi_exit_code}" >> lib/config.h
> > >      echo >> lib/config.h
> > >  fi
> > >  echo "#endif" >> lib/config.h
> > > diff --git a/lib/riscv/io.c b/lib/riscv/io.c
> > > index b1163404..0e666009 100644
> > > --- a/lib/riscv/io.c
> > > +++ b/lib/riscv/io.c
> > > @@ -162,8 +162,18 @@ void halt(int code);
> > >
> > >  void exit(int code)
> > >  {
> > > +     char *s = getenv("SBI_PASS_EXIT_CODE");
> > > +     bool pass_exit = CONFIG_SBI_EXIT_CODE;
> >
> > This is the first case of what may become more common - a config variable
> > which also has an env override. I think it may be good convention to
> > name them the same, i.e. the env name would also be CONFIG_SBI_EXIT_CODE,
> > unless you think that would be confusing for some reason?
> 
> I changed the name because the configure option seemed very long.
> I will make them both SBI_EXIT_CODE.

We need the CONFIG_ part on the config name in order keep it consistent
with other configs, so I think we just need a long name for both.

> Should I add a macro to simplify
> future uses
> of a config variable which also has an env override.

Sounds good to me.

> 
> >
> > > +
> > >       printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1);
> > > -     sbi_shutdown(code == 0);
> > > +
> > > +     if (s)
> > > +             pass_exit = (*s == '1' || *s == 'y' || *s == 'Y');
> >
> > We now have this logic in four places[1]. I think it's time we factor it,
> > and it's counterpart "!(s && (*s == '0' || *s == 'n' || *s == 'N'))"
> > into a couple helper macros. I'm not sure where the best place for
> > those macros to live is, though. I guess libcflat.h, but we really
> > ought to split that thing apart someday...
> 
> Is lib/argv.h an ok place?

That could work.

> getenv is defined in lib/string.c which is interesting. I wonder if it
> could be moved to lib/argv.c?

getenv is libc and in lives in stdlib in libc, so we should move our
prototype to lib/stdlib.h from libcflat.h, but we don't have a stdlib.c
file so string.c and argv.c are both "wrong", but at least string.c
also has all the functions getenv depends on locally...

Thanks,
drew