diff mbox series

[kvm-unit-tests,v2,03/18] scripts: Refuse to run the tests if not configured for qemu

Message ID 20250120164316.31473-4-alexandru.elisei@arm.com (mailing list archive)
State New, archived
Headers show
Series arm/arm64: Add kvmtool to the runner script | expand

Commit Message

Alexandru Elisei Jan. 20, 2025, 4:43 p.m. UTC
Arm and arm64 support running the tests under kvmtool. Unsurprisingly,
kvmtool and qemu have a different command line syntax for configuring and
running a virtual machine.

On top of that, when kvm-unit-tests has been configured to run under
kvmtool (via ./configure --target=kvmtool), the early UART address changes,
and if then the tests are run with qemu, this warning is displayed:

WARNING: early print support may not work. Found uart at 0x9000000, but early base is 0x1000000.

At the moment, the only way to run a test under kvmtool is manually, as no
script has any knowledge of how to invoke kvmtool. Also, unless one looks
at the logs, it's not obvious that the test runner is using qemu to run the
tests, and not kvmtool.

To avoid any confusion for unsuspecting users, refuse to run a test via the
testing scripts when kvm-unit-tests has been configured for kvmtool.

There are four different ways to run a test using the test infrastructure:
with run_tests.sh, by invoking arm/run or arm/efi/run with the correct
parameters (only the arm directory is mentioned here because the tests can
be configured for kvmtool only on arm and arm64), and by creating
standalone tests. Add a check in each of these locations for the supported
virtual machine manager.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/efi/run             | 8 ++++++++
 arm/run                 | 9 +++++++++
 run_tests.sh            | 8 ++++++++
 scripts/mkstandalone.sh | 8 ++++++++
 4 files changed, 33 insertions(+)

Comments

Andrew Jones Jan. 21, 2025, 2:48 p.m. UTC | #1
On Mon, Jan 20, 2025 at 04:43:01PM +0000, Alexandru Elisei wrote:
> Arm and arm64 support running the tests under kvmtool. Unsurprisingly,
> kvmtool and qemu have a different command line syntax for configuring and
> running a virtual machine.
> 
> On top of that, when kvm-unit-tests has been configured to run under
> kvmtool (via ./configure --target=kvmtool), the early UART address changes,
> and if then the tests are run with qemu, this warning is displayed:
> 
> WARNING: early print support may not work. Found uart at 0x9000000, but early base is 0x1000000.
> 
> At the moment, the only way to run a test under kvmtool is manually, as no
> script has any knowledge of how to invoke kvmtool. Also, unless one looks
> at the logs, it's not obvious that the test runner is using qemu to run the
> tests, and not kvmtool.
> 
> To avoid any confusion for unsuspecting users, refuse to run a test via the
> testing scripts when kvm-unit-tests has been configured for kvmtool.
> 
> There are four different ways to run a test using the test infrastructure:
> with run_tests.sh, by invoking arm/run or arm/efi/run with the correct
> parameters (only the arm directory is mentioned here because the tests can
> be configured for kvmtool only on arm and arm64), and by creating
> standalone tests. Add a check in each of these locations for the supported
> virtual machine manager.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm/efi/run             | 8 ++++++++
>  arm/run                 | 9 +++++++++
>  run_tests.sh            | 8 ++++++++
>  scripts/mkstandalone.sh | 8 ++++++++
>  4 files changed, 33 insertions(+)
> 
> diff --git a/arm/efi/run b/arm/efi/run
> index 8f41fc02df31..916f4c4deef6 100755
> --- a/arm/efi/run
> +++ b/arm/efi/run
> @@ -12,6 +12,14 @@ fi
>  source config.mak
>  source scripts/arch-run.bash
>  
> +case "$TARGET" in
> +qemu)
> +    ;;
> +*)
> +    echo "$0 does not support '$TARGET'"
> +    exit 2
> +esac
> +
>  if [ -f /usr/share/qemu-efi-aarch64/QEMU_EFI.fd ]; then
>  	DEFAULT_UEFI=/usr/share/qemu-efi-aarch64/QEMU_EFI.fd
>  elif [ -f /usr/share/edk2/aarch64/QEMU_EFI.silent.fd ]; then
> diff --git a/arm/run b/arm/run
> index efdd44ce86a7..6db32cf09c88 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -8,6 +8,15 @@ if [ -z "$KUT_STANDALONE" ]; then
>  	source config.mak
>  	source scripts/arch-run.bash
>  fi
> +
> +case "$TARGET" in
> +qemu)
> +    ;;
> +*)
> +   echo "'$TARGET' not supported"
> +   exit 3

I think we want exit code 2 here.

> +esac
> +
>  processor="$PROCESSOR"
>  
>  if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
> diff --git a/run_tests.sh b/run_tests.sh
> index 23d81b2caaa1..61480d0c05ed 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -100,6 +100,14 @@ while [ $# -gt 0 ]; do
>      shift
>  done
>  
> +case "$TARGET" in
> +qemu)
> +    ;;
> +*)
> +    echo "$0 does not support '$TARGET'"
> +    exit 2
> +esac
> +
>  # RUNTIME_log_file will be configured later
>  if [[ $tap_output == "no" ]]; then
>      process_test_output() { cat >> $RUNTIME_log_file; }
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index 2318a85f0706..4de97056e641 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -7,6 +7,14 @@ fi
>  source config.mak
>  source scripts/common.bash
>  
> +case "$TARGET" in
> +qemu)
> +    ;;
> +*)
> +    echo "'$TARGET' not supported for standlone tests"
> +    exit 2
> +esac
> +
>  temp_file ()
>  {
>  	local var="$1"
> -- 
> 2.47.1
>

I think we could put the check in a function in scripts/arch-run.bash and
just use the same error message for all cases.

Thanks,
drew

> 
> -- 
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv
Alexandru Elisei Jan. 21, 2025, 3:54 p.m. UTC | #2
Hi Drew,

On Tue, Jan 21, 2025 at 03:48:55PM +0100, Andrew Jones wrote:
> On Mon, Jan 20, 2025 at 04:43:01PM +0000, Alexandru Elisei wrote:
> > Arm and arm64 support running the tests under kvmtool. Unsurprisingly,
> > kvmtool and qemu have a different command line syntax for configuring and
> > running a virtual machine.
> > 
> > On top of that, when kvm-unit-tests has been configured to run under
> > kvmtool (via ./configure --target=kvmtool), the early UART address changes,
> > and if then the tests are run with qemu, this warning is displayed:
> > 
> > WARNING: early print support may not work. Found uart at 0x9000000, but early base is 0x1000000.
> > 
> > At the moment, the only way to run a test under kvmtool is manually, as no
> > script has any knowledge of how to invoke kvmtool. Also, unless one looks
> > at the logs, it's not obvious that the test runner is using qemu to run the
> > tests, and not kvmtool.
> > 
> > To avoid any confusion for unsuspecting users, refuse to run a test via the
> > testing scripts when kvm-unit-tests has been configured for kvmtool.
> > 
> > There are four different ways to run a test using the test infrastructure:
> > with run_tests.sh, by invoking arm/run or arm/efi/run with the correct
> > parameters (only the arm directory is mentioned here because the tests can
> > be configured for kvmtool only on arm and arm64), and by creating
> > standalone tests. Add a check in each of these locations for the supported
> > virtual machine manager.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  arm/efi/run             | 8 ++++++++
> >  arm/run                 | 9 +++++++++
> >  run_tests.sh            | 8 ++++++++
> >  scripts/mkstandalone.sh | 8 ++++++++
> >  4 files changed, 33 insertions(+)
> > 
> > diff --git a/arm/efi/run b/arm/efi/run
> > index 8f41fc02df31..916f4c4deef6 100755
> > --- a/arm/efi/run
> > +++ b/arm/efi/run
> > @@ -12,6 +12,14 @@ fi
> >  source config.mak
> >  source scripts/arch-run.bash
> >  
> > +case "$TARGET" in
> > +qemu)
> > +    ;;
> > +*)
> > +    echo "$0 does not support '$TARGET'"
> > +    exit 2
> > +esac
> > +
> >  if [ -f /usr/share/qemu-efi-aarch64/QEMU_EFI.fd ]; then
> >  	DEFAULT_UEFI=/usr/share/qemu-efi-aarch64/QEMU_EFI.fd
> >  elif [ -f /usr/share/edk2/aarch64/QEMU_EFI.silent.fd ]; then
> > diff --git a/arm/run b/arm/run
> > index efdd44ce86a7..6db32cf09c88 100755
> > --- a/arm/run
> > +++ b/arm/run
> > @@ -8,6 +8,15 @@ if [ -z "$KUT_STANDALONE" ]; then
> >  	source config.mak
> >  	source scripts/arch-run.bash
> >  fi
> > +
> > +case "$TARGET" in
> > +qemu)
> > +    ;;
> > +*)
> > +   echo "'$TARGET' not supported"
> > +   exit 3
> 
> I think we want exit code 2 here.

Exit code 2 is already in use in arm/run. Now that I'm looking more closely
at it, exit code 2 is already in use in run_tests.sh, same for
mkstandalone.sh and arm/efi/run.

How about using 3 everywhere as the exit code?

Also, your idea (below) to use a function to test for supported $TARGETs is
a very good one, I'll do it in the next iteration.

Thanks,
Alex

> 
> > +esac
> > +
> >  processor="$PROCESSOR"
> >  
> >  if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
> > diff --git a/run_tests.sh b/run_tests.sh
> > index 23d81b2caaa1..61480d0c05ed 100755
> > --- a/run_tests.sh
> > +++ b/run_tests.sh
> > @@ -100,6 +100,14 @@ while [ $# -gt 0 ]; do
> >      shift
> >  done
> >  
> > +case "$TARGET" in
> > +qemu)
> > +    ;;
> > +*)
> > +    echo "$0 does not support '$TARGET'"
> > +    exit 2
> > +esac
> > +
> >  # RUNTIME_log_file will be configured later
> >  if [[ $tap_output == "no" ]]; then
> >      process_test_output() { cat >> $RUNTIME_log_file; }
> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> > index 2318a85f0706..4de97056e641 100755
> > --- a/scripts/mkstandalone.sh
> > +++ b/scripts/mkstandalone.sh
> > @@ -7,6 +7,14 @@ fi
> >  source config.mak
> >  source scripts/common.bash
> >  
> > +case "$TARGET" in
> > +qemu)
> > +    ;;
> > +*)
> > +    echo "'$TARGET' not supported for standlone tests"
> > +    exit 2
> > +esac
> > +
> >  temp_file ()
> >  {
> >  	local var="$1"
> > -- 
> > 2.47.1
> >
> 
> I think we could put the check in a function in scripts/arch-run.bash and
> just use the same error message for all cases.
> 
> Thanks,
> drew
> 
> > 
> > -- 
> > kvm-riscv mailing list
> > kvm-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kvm-riscv
Andrew Jones Jan. 21, 2025, 4:17 p.m. UTC | #3
On Tue, Jan 21, 2025 at 03:54:17PM +0000, Alexandru Elisei wrote:
> Hi Drew,
> 
> On Tue, Jan 21, 2025 at 03:48:55PM +0100, Andrew Jones wrote:
> > On Mon, Jan 20, 2025 at 04:43:01PM +0000, Alexandru Elisei wrote:
> > > Arm and arm64 support running the tests under kvmtool. Unsurprisingly,
> > > kvmtool and qemu have a different command line syntax for configuring and
> > > running a virtual machine.
> > > 
> > > On top of that, when kvm-unit-tests has been configured to run under
> > > kvmtool (via ./configure --target=kvmtool), the early UART address changes,
> > > and if then the tests are run with qemu, this warning is displayed:
> > > 
> > > WARNING: early print support may not work. Found uart at 0x9000000, but early base is 0x1000000.
> > > 
> > > At the moment, the only way to run a test under kvmtool is manually, as no
> > > script has any knowledge of how to invoke kvmtool. Also, unless one looks
> > > at the logs, it's not obvious that the test runner is using qemu to run the
> > > tests, and not kvmtool.
> > > 
> > > To avoid any confusion for unsuspecting users, refuse to run a test via the
> > > testing scripts when kvm-unit-tests has been configured for kvmtool.
> > > 
> > > There are four different ways to run a test using the test infrastructure:
> > > with run_tests.sh, by invoking arm/run or arm/efi/run with the correct
> > > parameters (only the arm directory is mentioned here because the tests can
> > > be configured for kvmtool only on arm and arm64), and by creating
> > > standalone tests. Add a check in each of these locations for the supported
> > > virtual machine manager.
> > > 
> > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > > ---
> > >  arm/efi/run             | 8 ++++++++
> > >  arm/run                 | 9 +++++++++
> > >  run_tests.sh            | 8 ++++++++
> > >  scripts/mkstandalone.sh | 8 ++++++++
> > >  4 files changed, 33 insertions(+)
> > > 
> > > diff --git a/arm/efi/run b/arm/efi/run
> > > index 8f41fc02df31..916f4c4deef6 100755
> > > --- a/arm/efi/run
> > > +++ b/arm/efi/run
> > > @@ -12,6 +12,14 @@ fi
> > >  source config.mak
> > >  source scripts/arch-run.bash
> > >  
> > > +case "$TARGET" in
> > > +qemu)
> > > +    ;;
> > > +*)
> > > +    echo "$0 does not support '$TARGET'"
> > > +    exit 2
> > > +esac
> > > +
> > >  if [ -f /usr/share/qemu-efi-aarch64/QEMU_EFI.fd ]; then
> > >  	DEFAULT_UEFI=/usr/share/qemu-efi-aarch64/QEMU_EFI.fd
> > >  elif [ -f /usr/share/edk2/aarch64/QEMU_EFI.silent.fd ]; then
> > > diff --git a/arm/run b/arm/run
> > > index efdd44ce86a7..6db32cf09c88 100755
> > > --- a/arm/run
> > > +++ b/arm/run
> > > @@ -8,6 +8,15 @@ if [ -z "$KUT_STANDALONE" ]; then
> > >  	source config.mak
> > >  	source scripts/arch-run.bash
> > >  fi
> > > +
> > > +case "$TARGET" in
> > > +qemu)
> > > +    ;;
> > > +*)
> > > +   echo "'$TARGET' not supported"
> > > +   exit 3
> > 
> > I think we want exit code 2 here.
> 
> Exit code 2 is already in use in arm/run. Now that I'm looking more closely
> at it, exit code 2 is already in use in run_tests.sh, same for
> mkstandalone.sh and arm/efi/run.
> 
> How about using 3 everywhere as the exit code?
>

In kvm-unit-tests, exit code 2 is what we use for "most likely a run
script failed" (see the comment above run_qemu() in
scripts/arch-run.bash). We don't try to create a new error code for each
type of error, but we do have the error message as well. So if there's a
higher level runner, which runs this runner, it only needs to learn that
2 is likely a script failure and that an error message will hopefully
point the way to the problem.

Thanks,
drew
Alexandru Elisei Jan. 21, 2025, 4:20 p.m. UTC | #4
Hi Drew,

On Tue, Jan 21, 2025 at 05:17:22PM +0100, Andrew Jones wrote:
> On Tue, Jan 21, 2025 at 03:54:17PM +0000, Alexandru Elisei wrote:
> > Hi Drew,
> > 
> > On Tue, Jan 21, 2025 at 03:48:55PM +0100, Andrew Jones wrote:
> > > On Mon, Jan 20, 2025 at 04:43:01PM +0000, Alexandru Elisei wrote:
> > > > Arm and arm64 support running the tests under kvmtool. Unsurprisingly,
> > > > kvmtool and qemu have a different command line syntax for configuring and
> > > > running a virtual machine.
> > > > 
> > > > On top of that, when kvm-unit-tests has been configured to run under
> > > > kvmtool (via ./configure --target=kvmtool), the early UART address changes,
> > > > and if then the tests are run with qemu, this warning is displayed:
> > > > 
> > > > WARNING: early print support may not work. Found uart at 0x9000000, but early base is 0x1000000.
> > > > 
> > > > At the moment, the only way to run a test under kvmtool is manually, as no
> > > > script has any knowledge of how to invoke kvmtool. Also, unless one looks
> > > > at the logs, it's not obvious that the test runner is using qemu to run the
> > > > tests, and not kvmtool.
> > > > 
> > > > To avoid any confusion for unsuspecting users, refuse to run a test via the
> > > > testing scripts when kvm-unit-tests has been configured for kvmtool.
> > > > 
> > > > There are four different ways to run a test using the test infrastructure:
> > > > with run_tests.sh, by invoking arm/run or arm/efi/run with the correct
> > > > parameters (only the arm directory is mentioned here because the tests can
> > > > be configured for kvmtool only on arm and arm64), and by creating
> > > > standalone tests. Add a check in each of these locations for the supported
> > > > virtual machine manager.
> > > > 
> > > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > ---
> > > >  arm/efi/run             | 8 ++++++++
> > > >  arm/run                 | 9 +++++++++
> > > >  run_tests.sh            | 8 ++++++++
> > > >  scripts/mkstandalone.sh | 8 ++++++++
> > > >  4 files changed, 33 insertions(+)
> > > > 
> > > > diff --git a/arm/efi/run b/arm/efi/run
> > > > index 8f41fc02df31..916f4c4deef6 100755
> > > > --- a/arm/efi/run
> > > > +++ b/arm/efi/run
> > > > @@ -12,6 +12,14 @@ fi
> > > >  source config.mak
> > > >  source scripts/arch-run.bash
> > > >  
> > > > +case "$TARGET" in
> > > > +qemu)
> > > > +    ;;
> > > > +*)
> > > > +    echo "$0 does not support '$TARGET'"
> > > > +    exit 2
> > > > +esac
> > > > +
> > > >  if [ -f /usr/share/qemu-efi-aarch64/QEMU_EFI.fd ]; then
> > > >  	DEFAULT_UEFI=/usr/share/qemu-efi-aarch64/QEMU_EFI.fd
> > > >  elif [ -f /usr/share/edk2/aarch64/QEMU_EFI.silent.fd ]; then
> > > > diff --git a/arm/run b/arm/run
> > > > index efdd44ce86a7..6db32cf09c88 100755
> > > > --- a/arm/run
> > > > +++ b/arm/run
> > > > @@ -8,6 +8,15 @@ if [ -z "$KUT_STANDALONE" ]; then
> > > >  	source config.mak
> > > >  	source scripts/arch-run.bash
> > > >  fi
> > > > +
> > > > +case "$TARGET" in
> > > > +qemu)
> > > > +    ;;
> > > > +*)
> > > > +   echo "'$TARGET' not supported"
> > > > +   exit 3
> > > 
> > > I think we want exit code 2 here.
> > 
> > Exit code 2 is already in use in arm/run. Now that I'm looking more closely
> > at it, exit code 2 is already in use in run_tests.sh, same for
> > mkstandalone.sh and arm/efi/run.
> > 
> > How about using 3 everywhere as the exit code?
> >
> 
> In kvm-unit-tests, exit code 2 is what we use for "most likely a run
> script failed" (see the comment above run_qemu() in
> scripts/arch-run.bash). We don't try to create a new error code for each
> type of error, but we do have the error message as well. So if there's a
> higher level runner, which runs this runner, it only needs to learn that
> 2 is likely a script failure and that an error message will hopefully
> point the way to the problem.
> 

I see, I missed the comment. Will change so it returns 2 everywhere.

Thanks,
Alex
Alexandru Elisei Feb. 10, 2025, 10:41 a.m. UTC | #5
Hi Drew,

On Tue, Jan 21, 2025 at 03:48:55PM +0100, Andrew Jones wrote:
> On Mon, Jan 20, 2025 at 04:43:01PM +0000, Alexandru Elisei wrote:
<snip>
> > ---
> >  arm/efi/run             | 8 ++++++++
> >  arm/run                 | 9 +++++++++
> >  run_tests.sh            | 8 ++++++++
> >  scripts/mkstandalone.sh | 8 ++++++++
> >  4 files changed, 33 insertions(+)
<snip>
> > +case "$TARGET" in
> > +qemu)
> > +    ;;
> > +*)
> > +    echo "'$TARGET' not supported for standlone tests"
> > +    exit 2
> > +esac
> 
> I think we could put the check in a function in scripts/arch-run.bash and
> just use the same error message for all cases.

Coming back to the series.

arm/efi/run and arm/run source scripts/arch-run.bash; run_tests.sh and
scripts/mkstandalone.sh don't source scripts/arch-run.bash. There doesn't
seem to be a common file that is sourced by all of them.

How about creating a new file in scripts (vmm.bash?) with only this
function?

Thanks,
Alex

> 
> Thanks,
> drew
> 
> > 
> > -- 
> > kvm-riscv mailing list
> > kvm-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kvm-riscv
Andrew Jones Feb. 10, 2025, 1:56 p.m. UTC | #6
On Mon, Feb 10, 2025 at 10:41:53AM +0000, Alexandru Elisei wrote:
> Hi Drew,
> 
> On Tue, Jan 21, 2025 at 03:48:55PM +0100, Andrew Jones wrote:
> > On Mon, Jan 20, 2025 at 04:43:01PM +0000, Alexandru Elisei wrote:
> <snip>
> > > ---
> > >  arm/efi/run             | 8 ++++++++
> > >  arm/run                 | 9 +++++++++
> > >  run_tests.sh            | 8 ++++++++
> > >  scripts/mkstandalone.sh | 8 ++++++++
> > >  4 files changed, 33 insertions(+)
> <snip>
> > > +case "$TARGET" in
> > > +qemu)
> > > +    ;;
> > > +*)
> > > +    echo "'$TARGET' not supported for standlone tests"
> > > +    exit 2
> > > +esac
> > 
> > I think we could put the check in a function in scripts/arch-run.bash and
> > just use the same error message for all cases.
> 
> Coming back to the series.
> 
> arm/efi/run and arm/run source scripts/arch-run.bash; run_tests.sh and
> scripts/mkstandalone.sh don't source scripts/arch-run.bash. There doesn't
> seem to be a common file that is sourced by all of them.

scripts/mkstandalone.sh uses arch-run.bash, see generate_test().
run_tests.sh doesn't, but I'm not sure it needs to validate TARGET
since it can leave that to the lower-level scripts.

> 
> How about creating a new file in scripts (vmm.bash?) with only this
> function?

If we need a new file, then we can add one, but I'd try using
arch-run.bash or common.bash first.

Thanks,
drew
Alexandru Elisei Feb. 10, 2025, 6:04 p.m. UTC | #7
Hi Drew,

On Mon, Feb 10, 2025 at 02:56:25PM +0100, Andrew Jones wrote:
> On Mon, Feb 10, 2025 at 10:41:53AM +0000, Alexandru Elisei wrote:
> > Hi Drew,
> > 
> > On Tue, Jan 21, 2025 at 03:48:55PM +0100, Andrew Jones wrote:
> > > On Mon, Jan 20, 2025 at 04:43:01PM +0000, Alexandru Elisei wrote:
> > <snip>
> > > > ---
> > > >  arm/efi/run             | 8 ++++++++
> > > >  arm/run                 | 9 +++++++++
> > > >  run_tests.sh            | 8 ++++++++
> > > >  scripts/mkstandalone.sh | 8 ++++++++
> > > >  4 files changed, 33 insertions(+)
> > <snip>
> > > > +case "$TARGET" in
> > > > +qemu)
> > > > +    ;;
> > > > +*)
> > > > +    echo "'$TARGET' not supported for standlone tests"
> > > > +    exit 2
> > > > +esac
> > > 
> > > I think we could put the check in a function in scripts/arch-run.bash and
> > > just use the same error message for all cases.
> > 
> > Coming back to the series.
> > 
> > arm/efi/run and arm/run source scripts/arch-run.bash; run_tests.sh and
> > scripts/mkstandalone.sh don't source scripts/arch-run.bash. There doesn't
> > seem to be a common file that is sourced by all of them.
> 
> scripts/mkstandalone.sh uses arch-run.bash, see generate_test().

Are you referring to this bit:

generate_test ()
{
	<snip>
        (echo "#!/usr/bin/env bash"
         cat scripts/arch-run.bash "$TEST_DIR/run")

I think scripts/arch-run.bash would need to be sourced for any functions defined
there to be usable in mkstandalone.sh.

What I was thinking is something like this:

if ! vmm_supported $TARGET; then
	echo "$0 does not support '$TARGET'"
	exit 2
fi

Were you thinking of something else?

I think mkstandalone should error at the top level (when you do make
standalone), and not rely on the individual scripts to error if the VMM is
not supported. That's because I think creating the test files, booting a
machine and copying the files only to find out that kvm-unit-tests was
misconfigured is a pretty suboptimal experience.

> run_tests.sh doesn't, but I'm not sure it needs to validate TARGET
> since it can leave that to the lower-level scripts.

I put the check in arm/run, and removed it from run_tests.sh, and this is
what I got:

$ ./run_tests.sh selftest-setup
SKIP selftest-setup (./arm/run does not supported 'kvmtool')

which looks good to me.

> 
> > 
> > How about creating a new file in scripts (vmm.bash?) with only this
> > function?
> 
> If we need a new file, then we can add one, but I'd try using
> arch-run.bash or common.bash first.

common.bash seems to work (and the name fits), so I'll give that a go.

Thanks,
Alex
Al Dunsmuir Feb. 17, 2025, 4:02 p.m. UTC | #8
Hello Alexandru,

On Monday, February 10, 2025, 1:04:29 PM, you wrote:

> Hi Drew,

> On Mon, Feb 10, 2025 at 02:56:25PM +0100, Andrew Jones wrote:
>> On Mon, Feb 10, 2025 at 10:41:53AM +0000, Alexandru Elisei wrote:
>> > Hi Drew,
>> > 
>> > On Tue, Jan 21, 2025 at 03:48:55PM +0100, Andrew Jones wrote:
>> > > On Mon, Jan 20, 2025 at 04:43:01PM +0000, Alexandru Elisei wrote:
>> > <snip>
>> > > > ---
>> > > >  arm/efi/run             | 8 ++++++++
>> > > >  arm/run                 | 9 +++++++++
>> > > >  run_tests.sh            | 8 ++++++++
>> > > >  scripts/mkstandalone.sh | 8 ++++++++
>> > > >  4 files changed, 33 insertions(+)
>> > <snip>
>> > > > +case "$TARGET" in
>> > > > +qemu)
>> > > > +    ;;
>> > > > +*)
>> > > > +    echo "'$TARGET' not supported for standlone tests"
>> > > > +    exit 2
>> > > > +esac
>> > > 
>> > > I think we could put the check in a function in scripts/arch-run.bash and
>> > > just use the same error message for all cases.
>> > 
>> > Coming back to the series.
>> > 
>> > arm/efi/run and arm/run source scripts/arch-run.bash; run_tests.sh and
>> > scripts/mkstandalone.sh don't source scripts/arch-run.bash. There doesn't
>> > seem to be a common file that is sourced by all of them.
>> 
>> scripts/mkstandalone.sh uses arch-run.bash, see generate_test().

> Are you referring to this bit:

> generate_test ()
> {
>         <snip>
>         (echo "#!/usr/bin/env bash"
>          cat scripts/arch-run.bash "$TEST_DIR/run")

> I think scripts/arch-run.bash would need to be sourced for any functions defined
> there to be usable in mkstandalone.sh.

> What I was thinking is something like this:

> if ! vmm_supported $TARGET; then
>         echo "$0 does not support '$TARGET'"
>         exit 2
> fi

> Were you thinking of something else?

> I think mkstandalone should error at the top level (when you do make
> standalone), and not rely on the individual scripts to error if the VMM is
> not supported. That's because I think creating the test files, booting a
> machine and copying the files only to find out that kvm-unit-tests was
> misconfigured is a pretty suboptimal experience.

>> run_tests.sh doesn't, but I'm not sure it needs to validate TARGET
>> since it can leave that to the lower-level scripts.

> I put the check in arm/run, and removed it from run_tests.sh, and this is
> what I got:

> $ ./run_tests.sh selftest-setup
> SKIP selftest-setup (./arm/run does not supported 'kvmtool')

> which looks good to me.

Grammar nit:  This should be
SKIP selftest-setup (./arm/run does not support 'kvmtool')

Al

>> 
>> > 
>> > How about creating a new file in scripts (vmm.bash?) with only this
>> > function?
>> 
>> If we need a new file, then we can add one, but I'd try using
>> arch-run.bash or common.bash first.

> common.bash seems to work (and the name fits), so I'll give that a go.

> Thanks,
> Alex
diff mbox series

Patch

diff --git a/arm/efi/run b/arm/efi/run
index 8f41fc02df31..916f4c4deef6 100755
--- a/arm/efi/run
+++ b/arm/efi/run
@@ -12,6 +12,14 @@  fi
 source config.mak
 source scripts/arch-run.bash
 
+case "$TARGET" in
+qemu)
+    ;;
+*)
+    echo "$0 does not support '$TARGET'"
+    exit 2
+esac
+
 if [ -f /usr/share/qemu-efi-aarch64/QEMU_EFI.fd ]; then
 	DEFAULT_UEFI=/usr/share/qemu-efi-aarch64/QEMU_EFI.fd
 elif [ -f /usr/share/edk2/aarch64/QEMU_EFI.silent.fd ]; then
diff --git a/arm/run b/arm/run
index efdd44ce86a7..6db32cf09c88 100755
--- a/arm/run
+++ b/arm/run
@@ -8,6 +8,15 @@  if [ -z "$KUT_STANDALONE" ]; then
 	source config.mak
 	source scripts/arch-run.bash
 fi
+
+case "$TARGET" in
+qemu)
+    ;;
+*)
+   echo "'$TARGET' not supported"
+   exit 3
+esac
+
 processor="$PROCESSOR"
 
 if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
diff --git a/run_tests.sh b/run_tests.sh
index 23d81b2caaa1..61480d0c05ed 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -100,6 +100,14 @@  while [ $# -gt 0 ]; do
     shift
 done
 
+case "$TARGET" in
+qemu)
+    ;;
+*)
+    echo "$0 does not support '$TARGET'"
+    exit 2
+esac
+
 # RUNTIME_log_file will be configured later
 if [[ $tap_output == "no" ]]; then
     process_test_output() { cat >> $RUNTIME_log_file; }
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index 2318a85f0706..4de97056e641 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -7,6 +7,14 @@  fi
 source config.mak
 source scripts/common.bash
 
+case "$TARGET" in
+qemu)
+    ;;
+*)
+    echo "'$TARGET' not supported for standlone tests"
+    exit 2
+esac
+
 temp_file ()
 {
 	local var="$1"