[kvm-unit-tests RFC 0/2] scripts: Fix accel handling

[kvm-unit-tests RFC 0/2] scripts: Fix accel handling

[Janosch Frank]

When running on a system without KVM but with a /dev/kvm file or when
/dev/kvm has the wrong permissions we will think that we have the kvm
accelerator because we only check if /dev/kvm exists. To fix that we
instead start a qemu with the kvm accel and check the exit value we
can check if kvm is available.

Also we only compare the accel specified in unittests.conf with the
env ACCEL. That won't help if we don't have kvm but a test has KVM as
a requirement in unittests.conf.

My bash knowledge is rather limited, so maybe there's a better
solution?

Janosch Frank (2):
  scripts: Check kvm availability by asking qemu
  scripts: Set ACCEL in run_tests.sh if empty

 arm/run               |  4 +--
 powerpc/run           |  4 +--
 run_tests.sh          |  6 +++++
 s390x/run             | 10 ++++---
 scripts/accel.bash    | 63 +++++++++++++++++++++++++++++++++++++++++++
 scripts/arch-run.bash | 63 ++-----------------------------------------
 scripts/runtime.bash  |  2 +-
 x86/run               |  4 +--
 8 files changed, 85 insertions(+), 71 deletions(-)
 create mode 100644 scripts/accel.bash

-- 
2.27.0

[kvm-unit-tests RFC 1/2] scripts: Check kvm availability by asking qemu

[Janosch Frank]

The existence of the /dev/kvm character device doesn't imply that the
kvm module is part of the kernel or that it's always magically loaded
when needed.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arm/run               | 4 ++--
 powerpc/run           | 4 ++--
 s390x/run             | 4 ++--
 scripts/arch-run.bash | 7 +++++--
 x86/run               | 4 ++--
 5 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/arm/run b/arm/run
index a390ca5a..ca2d44e0 100755
--- a/arm/run
+++ b/arm/run
@@ -10,10 +10,10 @@ if [ -z "$STANDALONE" ]; then
 fi
 processor="$PROCESSOR"
 
-ACCEL=$(get_qemu_accelerator) ||
+qemu=$(search_qemu_binary) ||
 	exit $?
 
-qemu=$(search_qemu_binary) ||
+ACCEL=$(get_qemu_accelerator) ||
 	exit $?
 
 if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
diff --git a/powerpc/run b/powerpc/run
index 597ab96e..b51ac6fc 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -9,10 +9,10 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_qemu_accelerator) ||
+qemu=$(search_qemu_binary) ||
 	exit $?
 
-qemu=$(search_qemu_binary) ||
+ACCEL=$(get_qemu_accelerator) ||
 	exit $?
 
 if ! $qemu -machine '?' 2>&1 | grep 'pseries' > /dev/null; then
diff --git a/s390x/run b/s390x/run
index 09805044..2ec6da70 100755
--- a/s390x/run
+++ b/s390x/run
@@ -9,10 +9,10 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_qemu_accelerator) ||
+qemu=$(search_qemu_binary) ||
 	exit $?
 
-qemu=$(search_qemu_binary) ||
+ACCEL=$(get_qemu_accelerator) ||
 	exit $?
 
 M='-machine s390-ccw-virtio'
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 5997e384..8cc9a61e 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -342,8 +342,11 @@ trap_exit_push ()
 
 kvm_available ()
 {
-	[ -c /dev/kvm ] ||
-		return 1
+	if $($qemu -accel kvm 2> /dev/null); then
+		return 0;
+	else
+		return 1;
+	fi
 
 	[ "$HOST" = "$ARCH_NAME" ] ||
 		( [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ) ||
diff --git a/x86/run b/x86/run
index 8b2425f4..cbf49143 100755
--- a/x86/run
+++ b/x86/run
@@ -9,10 +9,10 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_qemu_accelerator) ||
+qemu=$(search_qemu_binary) ||
 	exit $?
 
-qemu=$(search_qemu_binary) ||
+ACCEL=$(get_qemu_accelerator) ||
 	exit $?
 
 if ! ${qemu} -device '?' 2>&1 | grep -F -e \"testdev\" -e \"pc-testdev\" > /dev/null;
-- 
2.27.0

[kvm-unit-tests RFC 2/2] scripts: Set ACCEL in run_tests.sh if empty

[Janosch Frank]

The current checks compare the env ACCEL to the unittests.conf
provided accel. That's all fine as long as the user always specifies
the ACCEL env variable.

If that's not the case and KVM is not available or if a test specifies
tcg and we start a qemu with the kvm acceleration as it's the default
we'll run into problems.

So let's fetch the accelerator before calling the arch/run script and
check it against the test's specified accel. Yes, we now do that
twice, once in the run_tests.sh and one in arch/run, but I don't think
there's a good way around it since you can execute arch/run
without run_tests.sh.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 run_tests.sh          |  6 ++++
 s390x/run             |  6 +++-
 scripts/accel.bash    | 63 +++++++++++++++++++++++++++++++++++++++++
 scripts/arch-run.bash | 66 ++-----------------------------------------
 scripts/runtime.bash  |  2 +-
 5 files changed, 77 insertions(+), 66 deletions(-)
 create mode 100644 scripts/accel.bash

diff --git a/run_tests.sh b/run_tests.sh
index 65108e73..9ccb97bd 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -10,6 +10,7 @@ if [ ! -f config.mak ]; then
 fi
 source config.mak
 source scripts/common.bash
+source scripts/accel.bash
 
 function usage()
 {
@@ -164,6 +165,11 @@ if [[ $tap_output == "yes" ]]; then
     echo "TAP version 13"
 fi
 
+qemu=$(search_qemu_binary)
+if [ -z "$ACCEL" ]; then
+    ACCEL=$(get_qemu_accelerator)
+fi
+
 trap "wait; exit 130" SIGINT
 
 (
diff --git a/s390x/run b/s390x/run
index 2ec6da70..df7ef5ca 100755
--- a/s390x/run
+++ b/s390x/run
@@ -12,8 +12,12 @@ fi
 qemu=$(search_qemu_binary) ||
 	exit $?
 
-ACCEL=$(get_qemu_accelerator) ||
+if [ -z "$DEF_ACCEL "]; then
+    ACCEL=$(get_qemu_accelerator) ||
 	exit $?
+else
+    ACCEL=$DEF_ACCEL
+fi
 
 M='-machine s390-ccw-virtio'
 M+=",accel=$ACCEL"
diff --git a/scripts/accel.bash b/scripts/accel.bash
new file mode 100644
index 00000000..ea12412a
--- /dev/null
+++ b/scripts/accel.bash
@@ -0,0 +1,63 @@
+search_qemu_binary ()
+{
+	local save_path=$PATH
+	local qemucmd qemu
+
+	export PATH=$PATH:/usr/libexec
+	for qemucmd in ${QEMU:-qemu-system-$ARCH_NAME qemu-kvm}; do
+		if $qemucmd --help 2>/dev/null | grep -q 'QEMU'; then
+			qemu="$qemucmd"
+			break
+		fi
+	done
+
+	if [ -z "$qemu" ]; then
+		echo "A QEMU binary was not found." >&2
+		echo "You can set a custom location by using the QEMU=<path> environment variable." >&2
+		return 2
+	fi
+	command -v $qemu
+	export PATH=$save_path
+}
+
+kvm_available ()
+{
+	if $($qemu -accel kvm 2> /dev/null); then
+		return 0;
+	else
+		return 1;
+	fi
+
+	[ "$HOST" = "$ARCH_NAME" ] ||
+		( [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ) ||
+		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
+}
+
+hvf_available ()
+{
+	[ "$(sysctl -n kern.hv_support 2>/dev/null)" = "1" ] || return 1
+	[ "$HOST" = "$ARCH_NAME" ] ||
+		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
+}
+
+get_qemu_accelerator ()
+{
+	if [ "$ACCEL" = "kvm" ] && ! kvm_available; then
+		echo "KVM is needed, but not available on this host" >&2
+		return 2
+	fi
+	if [ "$ACCEL" = "hvf" ] && ! hvf_available; then
+		echo "HVF is needed, but not available on this host" >&2
+		return 2
+	fi
+
+	if [ "$ACCEL" ]; then
+		echo $ACCEL
+	elif kvm_available; then
+		echo kvm
+	elif hvf_available; then
+		echo hvf
+	else
+		echo tcg
+	fi
+}
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 8cc9a61e..85c07792 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -24,6 +24,8 @@
 # 1..127 - FAILURE (could be QEMU, a run script, or the unittest)
 # >= 128 - Signal (signum = status - 128)
 ##############################################################################
+source scripts/accel.bash
+
 run_qemu ()
 {
 	local stdout errors ret sig
@@ -171,28 +173,6 @@ migration_cmd ()
 	fi
 }
 
-search_qemu_binary ()
-{
-	local save_path=$PATH
-	local qemucmd qemu
-
-	export PATH=$PATH:/usr/libexec
-	for qemucmd in ${QEMU:-qemu-system-$ARCH_NAME qemu-kvm}; do
-		if $qemucmd --help 2>/dev/null | grep -q 'QEMU'; then
-			qemu="$qemucmd"
-			break
-		fi
-	done
-
-	if [ -z "$qemu" ]; then
-		echo "A QEMU binary was not found." >&2
-		echo "You can set a custom location by using the QEMU=<path> environment variable." >&2
-		return 2
-	fi
-	command -v $qemu
-	export PATH=$save_path
-}
-
 initrd_create ()
 {
 	if [ "$ENVIRON_DEFAULT" = "yes" ]; then
@@ -339,45 +319,3 @@ trap_exit_push ()
 	local old_exit=$(trap -p EXIT | sed "s/^[^']*'//;s/'[^']*$//")
 	trap -- "$1; $old_exit" EXIT
 }
-
-kvm_available ()
-{
-	if $($qemu -accel kvm 2> /dev/null); then
-		return 0;
-	else
-		return 1;
-	fi
-
-	[ "$HOST" = "$ARCH_NAME" ] ||
-		( [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ) ||
-		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
-}
-
-hvf_available ()
-{
-	[ "$(sysctl -n kern.hv_support 2>/dev/null)" = "1" ] || return 1
-	[ "$HOST" = "$ARCH_NAME" ] ||
-		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
-}
-
-get_qemu_accelerator ()
-{
-	if [ "$ACCEL" = "kvm" ] && ! kvm_available; then
-		echo "KVM is needed, but not available on this host" >&2
-		return 2
-	fi
-	if [ "$ACCEL" = "hvf" ] && ! hvf_available; then
-		echo "HVF is needed, but not available on this host" >&2
-		return 2
-	fi
-
-	if [ "$ACCEL" ]; then
-		echo $ACCEL
-	elif kvm_available; then
-		echo kvm
-	elif hvf_available; then
-		echo hvf
-	else
-		echo tcg
-	fi
-}
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 132389c7..5d444db4 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -30,7 +30,7 @@ premature_failure()
 get_cmdline()
 {
     local kernel=$1
-    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
+    echo "TESTNAME=$testname TIMEOUT=$timeout DEF_ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
 }
 
 skip_nodefault()
-- 
2.27.0

Re: [kvm-unit-tests RFC 1/2] scripts: Check kvm availability by asking qemu

[Andrew Jones]

On Thu, Mar 18, 2021 at 12:44:59PM +0000, Janosch Frank wrote:
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index 5997e384..8cc9a61e 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -342,8 +342,11 @@ trap_exit_push ()
>  
>  kvm_available ()
>  {
> -	[ -c /dev/kvm ] ||
> -		return 1
> +	if $($qemu -accel kvm 2> /dev/null); then
> +		return 0;
> +	else
> +		return 1;
> +	fi
>

Hi Janosch,

Are we sure that even old QEMU supports this type of probing? If not,
then we should probably keep the /dev/kvm test as a fallback.

Thanks,
drew

Re: [kvm-unit-tests RFC 1/2] scripts: Check kvm availability by asking qemu

[Andrew Jones]

On Thu, Mar 18, 2021 at 12:44:59PM +0000, Janosch Frank wrote:
> The existence of the /dev/kvm character device doesn't imply that the
> kvm module is part of the kernel or that it's always magically loaded
> when needed.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arm/run               | 4 ++--
>  powerpc/run           | 4 ++--
>  s390x/run             | 4 ++--
>  scripts/arch-run.bash | 7 +++++--
>  x86/run               | 4 ++--
>  5 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/arm/run b/arm/run
> index a390ca5a..ca2d44e0 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -10,10 +10,10 @@ if [ -z "$STANDALONE" ]; then
>  fi
>  processor="$PROCESSOR"
>  
> -ACCEL=$(get_qemu_accelerator) ||
> +qemu=$(search_qemu_binary) ||
>  	exit $?
>  
> -qemu=$(search_qemu_binary) ||
> +ACCEL=$(get_qemu_accelerator) ||
>  	exit $?

How about renaming search_qemu_binary() to set_qemu_accelerator(), which
would also ensure QEMU is set (if it doesn't error out on failure) and
then call that from get_qemu_accelerator()? That way we don't need to
worry about this order of calls nor this lowercase 'qemu' variable being
set. Also, we can rename get_qemu_accelerator() to set_qemu_accelerator()
and ensure it sets ACCEL.

Thanks,
drew

Re: [kvm-unit-tests RFC 1/2] scripts: Check kvm availability by asking qemu

[Janosch Frank]

On 3/18/21 4:31 PM, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 12:44:59PM +0000, Janosch Frank wrote:
>> The existence of the /dev/kvm character device doesn't imply that the
>> kvm module is part of the kernel or that it's always magically loaded
>> when needed.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  arm/run               | 4 ++--
>>  powerpc/run           | 4 ++--
>>  s390x/run             | 4 ++--
>>  scripts/arch-run.bash | 7 +++++--
>>  x86/run               | 4 ++--
>>  5 files changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/arm/run b/arm/run
>> index a390ca5a..ca2d44e0 100755
>> --- a/arm/run
>> +++ b/arm/run
>> @@ -10,10 +10,10 @@ if [ -z "$STANDALONE" ]; then
>>  fi
>>  processor="$PROCESSOR"
>>  
>> -ACCEL=$(get_qemu_accelerator) ||
>> +qemu=$(search_qemu_binary) ||
>>  	exit $?
>>  
>> -qemu=$(search_qemu_binary) ||
>> +ACCEL=$(get_qemu_accelerator) ||
>>  	exit $?
> 
> How about renaming search_qemu_binary() to set_qemu_accelerator(), which
> would also ensure QEMU is set (if it doesn't error out on failure) and
> then call that from get_qemu_accelerator()? That way we don't need to
> worry about this order of calls nor this lowercase 'qemu' variable being
> set. Also, we can rename get_qemu_accelerator() to set_qemu_accelerator()
> and ensure it sets ACCEL.

Sure, I was already considering that.

The fact that qemu and ACCEL are basically global and accel is not the
same as ACCEL makes all of this harder to understand than it should be.

> 
> Thanks,
> drew
> 

Re: [kvm-unit-tests RFC 2/2] scripts: Set ACCEL in run_tests.sh if empty

[Andrew Jones]

On Thu, Mar 18, 2021 at 12:45:00PM +0000, Janosch Frank wrote:
> The current checks compare the env ACCEL to the unittests.conf
> provided accel. That's all fine as long as the user always specifies
> the ACCEL env variable.
> 
> If that's not the case and KVM is not available or if a test specifies
> tcg and we start a qemu with the kvm acceleration as it's the default
> we'll run into problems.
> 
> So let's fetch the accelerator before calling the arch/run script and
> check it against the test's specified accel. Yes, we now do that
> twice, once in the run_tests.sh and one in arch/run, but I don't think
> there's a good way around it since you can execute arch/run
> without run_tests.sh.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  run_tests.sh          |  6 ++++
>  s390x/run             |  6 +++-
>  scripts/accel.bash    | 63 +++++++++++++++++++++++++++++++++++++++++
>  scripts/arch-run.bash | 66 ++-----------------------------------------
>  scripts/runtime.bash  |  2 +-
>  5 files changed, 77 insertions(+), 66 deletions(-)
>  create mode 100644 scripts/accel.bash
> 
> diff --git a/run_tests.sh b/run_tests.sh
> index 65108e73..9ccb97bd 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -10,6 +10,7 @@ if [ ! -f config.mak ]; then
>  fi
>  source config.mak
>  source scripts/common.bash
> +source scripts/accel.bash
>  
>  function usage()
>  {
> @@ -164,6 +165,11 @@ if [[ $tap_output == "yes" ]]; then
>      echo "TAP version 13"
>  fi
>  
> +qemu=$(search_qemu_binary)
> +if [ -z "$ACCEL" ]; then
> +    ACCEL=$(get_qemu_accelerator)
> +fi
> +
>  trap "wait; exit 130" SIGINT
>  
>  (
> diff --git a/s390x/run b/s390x/run
> index 2ec6da70..df7ef5ca 100755
> --- a/s390x/run
> +++ b/s390x/run
> @@ -12,8 +12,12 @@ fi
>  qemu=$(search_qemu_binary) ||
>  	exit $?
>  
> -ACCEL=$(get_qemu_accelerator) ||
> +if [ -z "$DEF_ACCEL "]; then
> +    ACCEL=$(get_qemu_accelerator) ||
>  	exit $?
> +else
> +    ACCEL=$DEF_ACCEL
> +fi
>  
>  M='-machine s390-ccw-virtio'
>  M+=",accel=$ACCEL"
> diff --git a/scripts/accel.bash b/scripts/accel.bash
> new file mode 100644
> index 00000000..ea12412a
> --- /dev/null
> +++ b/scripts/accel.bash
> @@ -0,0 +1,63 @@
> +search_qemu_binary ()
> +{
> +	local save_path=$PATH
> +	local qemucmd qemu
> +
> +	export PATH=$PATH:/usr/libexec
> +	for qemucmd in ${QEMU:-qemu-system-$ARCH_NAME qemu-kvm}; do
> +		if $qemucmd --help 2>/dev/null | grep -q 'QEMU'; then
> +			qemu="$qemucmd"
> +			break
> +		fi
> +	done
> +
> +	if [ -z "$qemu" ]; then
> +		echo "A QEMU binary was not found." >&2
> +		echo "You can set a custom location by using the QEMU=<path> environment variable." >&2
> +		return 2
> +	fi
> +	command -v $qemu
> +	export PATH=$save_path
> +}
> +
> +kvm_available ()
> +{
> +	if $($qemu -accel kvm 2> /dev/null); then
> +		return 0;
> +	else
> +		return 1;
> +	fi
> +
> +	[ "$HOST" = "$ARCH_NAME" ] ||
> +		( [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ) ||
> +		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
> +}
> +
> +hvf_available ()
> +{
> +	[ "$(sysctl -n kern.hv_support 2>/dev/null)" = "1" ] || return 1
> +	[ "$HOST" = "$ARCH_NAME" ] ||
> +		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
> +}
> +
> +get_qemu_accelerator ()
> +{
> +	if [ "$ACCEL" = "kvm" ] && ! kvm_available; then
> +		echo "KVM is needed, but not available on this host" >&2
> +		return 2
> +	fi
> +	if [ "$ACCEL" = "hvf" ] && ! hvf_available; then
> +		echo "HVF is needed, but not available on this host" >&2
> +		return 2
> +	fi
> +
> +	if [ "$ACCEL" ]; then
> +		echo $ACCEL
> +	elif kvm_available; then
> +		echo kvm
> +	elif hvf_available; then
> +		echo hvf
> +	else
> +		echo tcg
> +	fi
> +}
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index 8cc9a61e..85c07792 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -24,6 +24,8 @@
>  # 1..127 - FAILURE (could be QEMU, a run script, or the unittest)
>  # >= 128 - Signal (signum = status - 128)
>  ##############################################################################
> +source scripts/accel.bash
> +
>  run_qemu ()
>  {
>  	local stdout errors ret sig
> @@ -171,28 +173,6 @@ migration_cmd ()
>  	fi
>  }
>  
> -search_qemu_binary ()
> -{
> -	local save_path=$PATH
> -	local qemucmd qemu
> -
> -	export PATH=$PATH:/usr/libexec
> -	for qemucmd in ${QEMU:-qemu-system-$ARCH_NAME qemu-kvm}; do
> -		if $qemucmd --help 2>/dev/null | grep -q 'QEMU'; then
> -			qemu="$qemucmd"
> -			break
> -		fi
> -	done
> -
> -	if [ -z "$qemu" ]; then
> -		echo "A QEMU binary was not found." >&2
> -		echo "You can set a custom location by using the QEMU=<path> environment variable." >&2
> -		return 2
> -	fi
> -	command -v $qemu
> -	export PATH=$save_path
> -}
> -
>  initrd_create ()
>  {
>  	if [ "$ENVIRON_DEFAULT" = "yes" ]; then
> @@ -339,45 +319,3 @@ trap_exit_push ()
>  	local old_exit=$(trap -p EXIT | sed "s/^[^']*'//;s/'[^']*$//")
>  	trap -- "$1; $old_exit" EXIT
>  }
> -
> -kvm_available ()
> -{
> -	if $($qemu -accel kvm 2> /dev/null); then
> -		return 0;
> -	else
> -		return 1;
> -	fi
> -
> -	[ "$HOST" = "$ARCH_NAME" ] ||
> -		( [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ) ||
> -		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
> -}
> -
> -hvf_available ()
> -{
> -	[ "$(sysctl -n kern.hv_support 2>/dev/null)" = "1" ] || return 1
> -	[ "$HOST" = "$ARCH_NAME" ] ||
> -		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
> -}
> -
> -get_qemu_accelerator ()
> -{
> -	if [ "$ACCEL" = "kvm" ] && ! kvm_available; then
> -		echo "KVM is needed, but not available on this host" >&2
> -		return 2
> -	fi
> -	if [ "$ACCEL" = "hvf" ] && ! hvf_available; then
> -		echo "HVF is needed, but not available on this host" >&2
> -		return 2
> -	fi
> -
> -	if [ "$ACCEL" ]; then
> -		echo $ACCEL
> -	elif kvm_available; then
> -		echo kvm
> -	elif hvf_available; then
> -		echo hvf
> -	else
> -		echo tcg
> -	fi
> -}
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index 132389c7..5d444db4 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -30,7 +30,7 @@ premature_failure()
>  get_cmdline()
>  {
>      local kernel=$1
> -    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
> +    echo "TESTNAME=$testname TIMEOUT=$timeout DEF_ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"

I think this name change can break any $TEST_DIR/run files that aren't
updated to check DEF_ACCEL. This patch only updates the runner for s390.

Also, it'd be better to do the code movement (scripts/accel.bash) of this
patch separately with a patch that does not have any functional change.

Thanks,
drew

Re: [kvm-unit-tests RFC 1/2] scripts: Check kvm availability by asking qemu

[Janosch Frank]

On 3/18/21 4:31 PM, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 12:44:59PM +0000, Janosch Frank wrote:
>> The existence of the /dev/kvm character device doesn't imply that the
>> kvm module is part of the kernel or that it's always magically loaded
>> when needed.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  arm/run               | 4 ++--
>>  powerpc/run           | 4 ++--
>>  s390x/run             | 4 ++--
>>  scripts/arch-run.bash | 7 +++++--
>>  x86/run               | 4 ++--
>>  5 files changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/arm/run b/arm/run
>> index a390ca5a..ca2d44e0 100755
>> --- a/arm/run
>> +++ b/arm/run
>> @@ -10,10 +10,10 @@ if [ -z "$STANDALONE" ]; then
>>  fi
>>  processor="$PROCESSOR"
>>  
>> -ACCEL=$(get_qemu_accelerator) ||
>> +qemu=$(search_qemu_binary) ||
>>  	exit $?
>>  
>> -qemu=$(search_qemu_binary) ||
>> +ACCEL=$(get_qemu_accelerator) ||
>>  	exit $?
> 
> How about renaming search_qemu_binary() to set_qemu_accelerator(), which
> would also ensure QEMU is set (if it doesn't error out on failure) and
> then call that from get_qemu_accelerator()? That way we don't need to
> worry about this order of calls nor this lowercase 'qemu' variable being
> set. Also, we can rename get_qemu_accelerator() to set_qemu_accelerator()
> and ensure it sets ACCEL.
> 
> Thanks,
> drew
> 

I've already broken off the accel.bash change and fixed the arch/run
problem but this here will have two wait until after my easter vacation
so don't wait for patches the next ~2 weeks.