[kvm-unit-tests PATCH 0/5] Minor fixes while getting rid of code duplication

[kvm-unit-tests PATCH 0/5] Minor fixes while getting rid of code duplication

[Radim Krčmář]

The series isn't well split, so patches remove the code duplication and
fix bugs at the same time.  [3/5] is s390x+powerpc specific, but has
dependency on [4/5], which affects all architectures, just like [1,5/5].
[2/5] is unrelated, but was simple enough to include.


Radim Krčmář (5):
  arch/run: simplify copy-paste of the command line
  run_tests: add summary to powerpc and s390
  s390+powerpc/run: improve output handling
  scripts/arch-run: fix qemu binary search failure path
  arch/run: unify accelerator detection

 arm/run               | 28 +++------------------
 powerpc/run           | 41 ++++--------------------------
 s390x/run             | 38 +++-------------------------
 scripts/arch-run.bash | 69 +++++++++++++++++++++++++++++++++++++++++++++++----
 scripts/runtime.bash  |  2 +-
 x86/run               |  8 +++---
 6 files changed, 81 insertions(+), 105 deletions(-)

-- 
2.13.2

[kvm-unit-tests PATCH 1/5] arch/run: simplify copy-paste of the command line

[Radim Krčmář]

The major annoyance coming from errata was "-initrd `mktemp`" that had
to be removed before running the copy-pasted command line.
Move "-initrd ..." part at the end of the line and hide it behind "#" if
the initird file is a temporary one.

This patch also brings s390 to use the same code as other architectures
and eliminates code duplication.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
Adding the arch/run command line might also be worth it
(i.e. adding echo $0 $@ to arch/run).
---
 arm/run               |  5 +----
 powerpc/run           |  5 +----
 s390x/run             |  2 --
 scripts/arch-run.bash | 18 ++++++++++++++++--
 x86/run               |  5 +----
 5 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/arm/run b/arm/run
index 1b1602c74970..8d8918a461dc 100755
--- a/arm/run
+++ b/arm/run
@@ -71,12 +71,9 @@ if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
 	pci_testdev="-device pci-testdev"
 fi
 
-initrd_create
-
 M+=",accel=$ACCEL"
 command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
-command+=" -display none -serial stdio $INITRD -kernel"
+command+=" -display none -serial stdio -kernel"
 command="$(timeout_cmd) $command"
-echo $command "$@"
 
 run_qemu $command "$@"
diff --git a/powerpc/run b/powerpc/run
index 7ccf1a36ff49..3fd5a91aeffb 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -35,14 +35,11 @@ if ! $qemu -machine '?' 2>&1 | grep 'pseries' > /dev/null; then
 	exit 2
 fi
 
-initrd_create
-
 M='-machine pseries'
 M+=",accel=$ACCEL"
 command="$qemu -nodefaults $M -bios $FIRMWARE"
-command+=" -display none -serial stdio $INITRD -kernel"
+command+=" -display none -serial stdio -kernel"
 command="$(migration_cmd) $(timeout_cmd) $command"
-echo $command "$@"
 
 # powerpc tests currently exit with rtas-poweroff, which exits with 0.
 # run_qemu treats that as a failure exit and returns 1, so we need
diff --git a/s390x/run b/s390x/run
index cf333de9ccce..89210f4fb6b6 100755
--- a/s390x/run
+++ b/s390x/run
@@ -34,10 +34,8 @@ M='-machine s390-ccw-virtio'
 M+=",accel=$ACCEL"
 command="$qemu -nodefaults -nographic $M"
 command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
-[ -f "$ENV" ] && command+=" -initrd $ENV"
 command+=" -kernel"
 command="$(timeout_cmd) $command"
-echo $command "$@"
 
 # We return the exit code via stdout, not via the QEMU return code
 lines=$(run_qemu $command "$@")
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 7c6c79ad3a9d..8ad37e4ebf97 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -28,9 +28,14 @@ run_qemu ()
 {
 	local stdout errors ret sig
 
+	echo -n $@
+	initrd_create &&
+		echo -n " #"
+	echo " $INITRD"
+
 	# stdout to {stdout}, stderr to $errors and stderr
 	exec {stdout}>&1
-	errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout)
+	errors=$("${@}" $INITRD </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout)
 	ret=$?
 	exec {stdout}>&-
 
@@ -158,14 +163,20 @@ search_qemu_binary ()
 
 initrd_create ()
 {
+	local ret
+
 	env_add_errata
+	ret=$?
+
 	unset INITRD
 	[ -f "$ENV" ] && INITRD="-initrd $ENV"
+
+	return $ret
 }
 
 env_add_errata ()
 {
-	local line errata
+	local line errata ret=1
 
 	if [ -f "$ENV" ] && grep -q '^ERRATA_' <(env); then
 		for line in $(grep '^ERRATA_' "$ENV"); do
@@ -183,7 +194,10 @@ env_add_errata ()
 		trap_exit_push 'rm -f $ENV; [ "$ENV_OLD" ] && export ENV="$ENV_OLD" || unset ENV; unset ENV_OLD'
 		[ -f "$ENV_OLD" ] && grep -v '^ERRATA_' "$ENV_OLD" > $ENV
 		grep '^ERRATA_' <(env) >> $ENV
+		ret=0
 	fi
+
+	return $ret
 }
 
 env_generate_errata ()
diff --git a/x86/run b/x86/run
index 88f5ff771140..a06a7c0f80c8 100755
--- a/x86/run
+++ b/x86/run
@@ -33,11 +33,8 @@ else
 	pc_testdev="-device testdev,chardev=testlog -chardev file,id=testlog,path=msr.out"
 fi
 
-initrd_create
-
 command="${qemu} -nodefaults -enable-kvm $pc_testdev -vnc none -serial stdio $pci_testdev $hyperv_testdev"
-command+=" $INITRD -kernel"
+command+=" -kernel"
 command="$(timeout_cmd) $command"
-echo ${command} "$@"
 
 run_qemu ${command} "$@"
-- 
2.13.2

[kvm-unit-tests PATCH 2/5] run_tests: add summary to powerpc and s390

[Radim Krčmář]

Looking at the last line was not enough as these architectures have
information about the exit on the last two lines.  See the last 3 lines.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
Sample output on powerpc:
  PASS selftest-setup (2 tests)
  PASS spapr_hcall (9 tests, 1 expected failures)
  PASS rtas-get-time-of-day (10 tests)
  PASS rtas-get-time-of-day-base (10 tests)
  PASS rtas-set-time-of-day (5 tests)
  PASS emulator (4 tests)
  SKIP h_cede_tm (test marked as manual run only)
  PASS sprs (46 tests)

and s390x:
  FAIL selftest-setup (7 tests)
  FAIL intercept (3 tests, 2 unexpected failures)

s390 selftest-setup is failing even though all tests passed, but there
is also:
  ABORT: selftest: Unexpected program interrupt: 5 at 0x12b7c, ilen 4
---
 scripts/runtime.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index e630279cdd37..ac09e0fd30ac 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -8,7 +8,7 @@ FAIL() { echo -ne "\e[31mFAIL\e[0m"; }
 
 extract_summary()
 {
-    tail -1 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/$/)/'
+    tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/$/)/'
 }
 
 # We assume that QEMU is going to work if it tried to load the kernel
-- 
2.13.2

Re: [kvm-unit-tests PATCH 2/5] run_tests: add summary to powerpc and s390

[Thomas Huth]

On 28.06.2017 22:08, Radim Krčmář wrote:
> Looking at the last line was not enough as these architectures have
> information about the exit on the last two lines.  See the last 3 lines.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> Sample output on powerpc:
>   PASS selftest-setup (2 tests)
>   PASS spapr_hcall (9 tests, 1 expected failures)
>   PASS rtas-get-time-of-day (10 tests)
>   PASS rtas-get-time-of-day-base (10 tests)
>   PASS rtas-set-time-of-day (5 tests)
>   PASS emulator (4 tests)
>   SKIP h_cede_tm (test marked as manual run only)
>   PASS sprs (46 tests)
> 
> and s390x:
>   FAIL selftest-setup (7 tests)
>   FAIL intercept (3 tests, 2 unexpected failures)
> 
> s390 selftest-setup is failing even though all tests passed, but there
> is also:
>   ABORT: selftest: Unexpected program interrupt: 5 at 0x12b7c, ilen 4

Please use the latest and greatest QEMU from the master branch for s390x
... there have been quite a lot of fixes since v2.9 and some of them are
required to get the kvm-unit-tests running properly.

 Thomas

Re: [kvm-unit-tests PATCH 2/5] run_tests: add summary to powerpc and s390

[Radim Krčmář]

2017-06-29 07:30+0200, Thomas Huth:
> On 28.06.2017 22:08, Radim Krčmář wrote:
> >   FAIL selftest-setup (7 tests)
> >   FAIL intercept (3 tests, 2 unexpected failures)
> > 
> > s390 selftest-setup is failing even though all tests passed, but there
> > is also:
> >   ABORT: selftest: Unexpected program interrupt: 5 at 0x12b7c, ilen 4
> 
> Please use the latest and greatest QEMU from the master branch for s390x
> ... there have been quite a lot of fixes since v2.9 and some of them are
> required to get the kvm-unit-tests running properly.

Will do, thanks.  In this case, I wanted to demonstrate that the summary
gets printed and didn't care what it was. :)

[kvm-unit-tests PATCH 3/5] s390+powerpc/run: improve output handling

[Radim Krčmář]

If the test is hanging, then using arch/run would produce no output,
because of caching in a temporary variable.
Use the `tee` trick to save the output while passing it through.

They were also using the very same code.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 powerpc/run           | 15 +--------------
 s390x/run             | 15 +--------------
 scripts/arch-run.bash | 23 +++++++++++++++++++++++
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/powerpc/run b/powerpc/run
index 3fd5a91aeffb..3f69386c642a 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -46,17 +46,4 @@ command="$(migration_cmd) $(timeout_cmd) $command"
 # to fixup the fixup below by parsing the true exit code from the output.
 # The second fixup is also a FIXME, because once we add chr-testdev
 # support for powerpc, we won't need the second fixup.
-lines=$(run_qemu $command "$@")
-ret=$?
-echo "$lines"
-if [ $ret -eq 1 ]; then
-	testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
-	if [ "$testret" ]; then
-		if [ $testret -eq 1 ]; then
-			ret=0
-		else
-			ret=$testret
-		fi
-	fi
-fi
-exit $ret
+run_qemu_status $command "$@"
diff --git a/s390x/run b/s390x/run
index 89210f4fb6b6..6df348a93783 100755
--- a/s390x/run
+++ b/s390x/run
@@ -38,17 +38,4 @@ command+=" -kernel"
 command="$(timeout_cmd) $command"
 
 # We return the exit code via stdout, not via the QEMU return code
-lines=$(run_qemu $command "$@")
-ret=$?
-echo "$lines"
-if [ $ret -eq 1 ]; then
-	testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
-	if [ "$testret" ]; then
-		if [ $testret -eq 1 ]; then
-			ret=0
-		else
-			ret=$testret
-		fi
-	fi
-fi
-exit $ret
+run_qemu_status $command "$@"
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 8ad37e4ebf97..6e429a8748f4 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -69,6 +69,29 @@ run_qemu ()
 	return $ret
 }
 
+run_qemu_status ()
+{
+	local stdout ret
+
+	exec {stdout}>&1
+	lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
+	ret=$?
+	exec {stdout}>&-
+
+	if [ $ret -eq 1 ]; then
+		testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
+		if [ "$testret" ]; then
+			if [ $testret -eq 1 ]; then
+				ret=0
+			else
+				ret=$testret
+			fi
+		fi
+	fi
+
+	return $ret
+}
+
 timeout_cmd ()
 {
 	if [ "$TIMEOUT" ] && [ "$TIMEOUT" != "0" ]; then
-- 
2.13.2

[kvm-unit-tests PATCH 4/5] scripts/arch-run: fix qemu binary search failure path

[Radim Krčmář]

Doing exit from a subshell won't exit the main shell, so arch-run will
try to continue without a working QEMU if search_qemu_binary() failed.

Also fix error message from search_qemu_binary().

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
I was thinking about using `set -e` instead, so the "return 2" from
search_qemu_binary() would stop the script without the explicit exit.
It required just four changes to make the script work (resulting in
better diffstat), but would likely complicate future development.

The second alternative would be to call just "search_qemu_binary" and
set the qemu variable from it.  This would allow "exit 2" from
search_qemu_binary() to also exit arch/run, but side-effects on
variables seem worse than the first alternative ...
---
 arm/run               | 3 ++-
 powerpc/run           | 3 ++-
 s390x/run             | 3 ++-
 scripts/arch-run.bash | 6 +++---
 x86/run               | 3 ++-
 5 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/arm/run b/arm/run
index 8d8918a461dc..d7d778fe1645 100755
--- a/arm/run
+++ b/arm/run
@@ -31,7 +31,8 @@ if [ -z "$ACCEL" ]; then
 	fi
 fi
 
-qemu=$(search_qemu_binary)
+qemu=$(search_qemu_binary) ||
+	exit 2
 
 if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
 	echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
diff --git a/powerpc/run b/powerpc/run
index 3f69386c642a..e43c9fd8063c 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -28,7 +28,8 @@ if [ -z "$ACCEL" ]; then
 	fi
 fi
 
-qemu=$(search_qemu_binary)
+qemu=$(search_qemu_binary) ||
+	exit 2
 
 if ! $qemu -machine '?' 2>&1 | grep 'pseries' > /dev/null; then
 	echo "$qemu doesn't support pSeries ('-machine pseries'). Exiting."
diff --git a/s390x/run b/s390x/run
index 6df348a93783..ce8d45c2c8fd 100755
--- a/s390x/run
+++ b/s390x/run
@@ -28,7 +28,8 @@ if [ -z "$ACCEL" ]; then
 	fi
 fi
 
-qemu=$(search_qemu_binary)
+qemu=$(search_qemu_binary) ||
+	exit 2
 
 M='-machine s390-ccw-virtio'
 M+=",accel=$ACCEL"
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 6e429a8748f4..994c1aa9c0cd 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -176,9 +176,9 @@ search_qemu_binary ()
 	done
 
 	if [ -z "$qemu" ]; then
-		echo "A QEMU binary was not found."
-		echo "You can set a custom location by using the QEMU=<path> environment variable."
-		exit 2
+		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
diff --git a/x86/run b/x86/run
index a06a7c0f80c8..1099c5ccd609 100755
--- a/x86/run
+++ b/x86/run
@@ -9,7 +9,8 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-qemu=$(search_qemu_binary)
+qemu=$(search_qemu_binary) ||
+	exit 2
 
 if ! ${qemu} -device '?' 2>&1 | grep -F -e \"testdev\" -e \"pc-testdev\" > /dev/null;
 then
-- 
2.13.2

Re: [kvm-unit-tests PATCH 4/5] scripts/arch-run: fix qemu binary search failure path

[Paolo Bonzini]

On 28/06/2017 22:08, Radim Krčmář wrote:
> Doing exit from a subshell won't exit the main shell, so arch-run will
> try to continue without a working QEMU if search_qemu_binary() failed.

Should it be "exit $?"?

Apart from this looks good.  I can do the change.

Paolo

> Also fix error message from search_qemu_binary().
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> I was thinking about using `set -e` instead, so the "return 2" from
> search_qemu_binary() would stop the script without the explicit exit.
> It required just four changes to make the script work (resulting in
> better diffstat), but would likely complicate future development.
> 
> The second alternative would be to call just "search_qemu_binary" and
> set the qemu variable from it.  This would allow "exit 2" from
> search_qemu_binary() to also exit arch/run, but side-effects on
> variables seem worse than the first alternative ...
> ---

Re: [kvm-unit-tests PATCH 4/5] scripts/arch-run: fix qemu binary search failure path

[Radim Krčmář]

2017-06-28 22:46+0200, Paolo Bonzini:
> On 28/06/2017 22:08, Radim Krčmář wrote:
> > Doing exit from a subshell won't exit the main shell, so arch-run will
> > try to continue without a working QEMU if search_qemu_binary() failed.
> 
> Should it be "exit $?"?

Makes sense.  I was thinking about 'return 1' from search_qemu_binary(),
but never actually went from it.

> Apart from this looks good.  I can do the change.

Please do, thanks.

[kvm-unit-tests PATCH 5/5] arch/run: unify accelerator detection

[Radim Krčmář]

Use the same mechanism as search_qemu_binary().

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arm/run               | 20 +-------------------
 powerpc/run           | 18 +-----------------
 s390x/run             | 18 +-----------------
 scripts/arch-run.bash | 22 ++++++++++++++++++++++
 4 files changed, 25 insertions(+), 53 deletions(-)

diff --git a/arm/run b/arm/run
index d7d778fe1645..f2e71eca022d 100755
--- a/arm/run
+++ b/arm/run
@@ -10,26 +10,8 @@ if [ -z "$STANDALONE" ]; then
 fi
 processor="$PROCESSOR"
 
-if [ -c /dev/kvm ]; then
-	if [ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]; then
-		kvm_available=yes
-	elif [ "$HOST" = "aarch64" ]; then
-		kvm_available=yes
-	fi
-fi
-
-if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
-	echo "KVM is needed, but not available on this host"
+ACCEL=$(get_accel '([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) || [ "$HOST" = "aarch64" ]') ||
 	exit 2
-fi
-
-if [ -z "$ACCEL" ]; then
-	if [ "$kvm_available" = "yes" ]; then
-		ACCEL="kvm"
-	else
-		ACCEL="tcg"
-	fi
-fi
 
 qemu=$(search_qemu_binary) ||
 	exit 2
diff --git a/powerpc/run b/powerpc/run
index e43c9fd8063c..55b987c5efc5 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -9,24 +9,8 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-if [ -c /dev/kvm ]; then
-	if [ "$HOST" = "ppc64" ] && [ "$ARCH" = "ppc64" ]; then
-		kvm_available=yes
-	fi
-fi
-
-if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
-	echo "KVM is needed, but not available on this host"
+ACCEL=$(get_accel '[ "$HOST" = "ppc64" ] && [ "$ARCH" = "ppc64" ]') ||
 	exit 2
-fi
-
-if [ -z "$ACCEL" ]; then
-	if [ "$kvm_available" = "yes" ]; then
-		ACCEL="kvm"
-	else
-		ACCEL="tcg"
-	fi
-fi
 
 qemu=$(search_qemu_binary) ||
 	exit 2
diff --git a/s390x/run b/s390x/run
index ce8d45c2c8fd..67d6e88ea7e4 100755
--- a/s390x/run
+++ b/s390x/run
@@ -9,24 +9,8 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-if [ -c /dev/kvm ]; then
-	if [ "$HOST" = "s390x" ] && [ "$ARCH" = "s390x" ]; then
-		kvm_available=yes
-	fi
-fi
-
-if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
-	echo "KVM is needed, but not available on this host"
+ACCEL=$(get_accel '[ "$HOST" = "s390x" ] && [ "$ARCH" = "s390x" ]') ||
 	exit 2
-fi
-
-if [ -z "$ACCEL" ]; then
-	if [ "$kvm_available" = "yes" ]; then
-		ACCEL="kvm"
-	else
-		ACCEL="tcg"
-	fi
-fi
 
 qemu=$(search_qemu_binary) ||
 	exit 2
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 994c1aa9c0cd..a696652c61b1 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -272,3 +272,25 @@ trap_exit_push ()
 	local old_exit=$(trap -p EXIT | sed "s/^[^']*'//;s/'[^']*$//")
 	trap -- "$1; $old_exit" EXIT
 }
+
+get_accel ()
+{
+	local kvm_available
+
+	if [ -c /dev/kvm ] && eval "$@"; then
+		kvm_available=yes
+	fi
+
+	if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
+		echo "KVM is needed, but not available on this host" >&2
+		return 2
+	fi
+
+	if [ "$ACCEL" ]; then
+		echo $ACCEL
+	elif [ "$kvm_available" = "yes" ]; then
+		echo kvm
+	else
+		echo tcg
+	fi
+}
-- 
2.13.2

Re: [kvm-unit-tests PATCH 5/5] arch/run: unify accelerator detection

[Paolo Bonzini]

On 28/06/2017 22:08, Radim Krčmář wrote:
> -if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
> -	echo "KVM is needed, but not available on this host"
> +ACCEL=$(get_accel '([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) || [ "$HOST" = "aarch64" ]') ||
>  	exit 2

Should x86 do the same as ARM (follow-up patch)?

And maybe the test could move from an argument of get_accel to a
function "arch_kvm_available" that returns 0 if available and 1 if not?

Paolo

Re: [kvm-unit-tests PATCH 5/5] arch/run: unify accelerator detection

[Radim Krčmář]

2017-06-28 22:48+0200, Paolo Bonzini:
> On 28/06/2017 22:08, Radim Krčmář wrote:
> > -if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
> > -	echo "KVM is needed, but not available on this host"
> > +ACCEL=$(get_accel '([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) || [ "$HOST" = "aarch64" ]') ||
> >  	exit 2
> 
> Should x86 do the same as ARM (follow-up patch)?

Yeah, I'll prepare it when we've agreed on the format.

> And maybe the test could move from an argument of get_accel to a
> function "arch_kvm_available" that returns 0 if available and 1 if not?

Right, the arm condition needs to be fixed as

  ([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) ||
  ([ "$HOST" = "aarch64" ] && [ "$ARCH" = arm -o "$ARCH" = arm64 ])

and it would look even worse in the argument ...

Would it be ok like this?

diff --git a/arm/run b/arm/run
index f2e71eca022d..82d36d0e83f8 100755
--- a/arm/run
+++ b/arm/run
@@ -10,7 +10,12 @@ if [ -z "$STANDALONE" ]; then
 fi
 processor="$PROCESSOR"
 
-ACCEL=$(get_accel '([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) || [ "$HOST" = "aarch64" ]') ||
+arch_kvm_available ()
+{
+	([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) || [ "$HOST" = "aarch64" ]
+}
+
+ACCEL=$(get_accel) ||
 	exit 2
 
 qemu=$(search_qemu_binary) ||
diff --git a/powerpc/run b/powerpc/run
index 55b987c5efc5..9ae21f333205 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -9,7 +9,12 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_accel '[ "$HOST" = "ppc64" ] && [ "$ARCH" = "ppc64" ]') ||
+arch_kvm_available ()
+{
+	[ "$HOST" = "ppc64" ] && [ "$ARCH" = "ppc64" ]
+}
+
+ACCEL=$(get_accel) ||
 	exit 2
 
 qemu=$(search_qemu_binary) ||
diff --git a/s390x/run b/s390x/run
index 67d6e88ea7e4..361693ee4b84 100755
--- a/s390x/run
+++ b/s390x/run
@@ -9,7 +9,12 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_accel '[ "$HOST" = "s390x" ] && [ "$ARCH" = "s390x" ]') ||
+arch_kvm_available ()
+{
+	[ "$HOST" = "s390x" ] && [ "$ARCH" = "s390x" ]
+}
+
+ACCEL=$(get_accel) ||
 	exit 2
 
 qemu=$(search_qemu_binary) ||
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index a696652c61b1..6c6abc6b36bc 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -277,7 +277,7 @@ get_accel ()
 {
 	local kvm_available
 
-	if [ -c /dev/kvm ] && eval "$@"; then
+	if [ -c /dev/kvm ] && arch_kvm_available; then
 		kvm_available=yes
 	fi
 

Re: [kvm-unit-tests PATCH 5/5] arch/run: unify accelerator detection

[Radim Krčmář]

2017-06-29 14:45+0200, Radim Krčmář:
> 2017-06-28 22:48+0200, Paolo Bonzini:
> > On 28/06/2017 22:08, Radim Krčmář wrote:
> > > -if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
> > > -	echo "KVM is needed, but not available on this host"
> > > +ACCEL=$(get_accel '([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) || [ "$HOST" = "aarch64" ]') ||
> > >  	exit 2
> > 
> > Should x86 do the same as ARM (follow-up patch)?
> 
> Yeah, I'll prepare it when we've agreed on the format.
> 
> > And maybe the test could move from an argument of get_accel to a
> > function "arch_kvm_available" that returns 0 if available and 1 if not?
> 
> Right, the arm condition needs to be fixed as
> 
>   ([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) ||
>   ([ "$HOST" = "aarch64" ] && [ "$ARCH" = arm -o "$ARCH" = arm64 ])
> 
> and it would look even worse in the argument ...
> 
> Would it be ok like this?

No, we can make a better use of the $HOST variable:

diff --git a/arm/run b/arm/run
index f2e71eca022d..1245d42edae7 100755
--- a/arm/run
+++ b/arm/run
@@ -10,7 +10,7 @@ if [ -z "$STANDALONE" ]; then
 fi
 processor="$PROCESSOR"
 
-ACCEL=$(get_accel '([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) || [ "$HOST" = "aarch64" ]') ||
+ACCEL=$(get_accel) ||
 	exit 2
 
 qemu=$(search_qemu_binary) ||
diff --git a/powerpc/run b/powerpc/run
index 55b987c5efc5..58921e81eb8f 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -9,7 +9,7 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_accel '[ "$HOST" = "ppc64" ] && [ "$ARCH" = "ppc64" ]') ||
+ACCEL=$(get_accel) ||
 	exit 2
 
 qemu=$(search_qemu_binary) ||
diff --git a/s390x/run b/s390x/run
index 67d6e88ea7e4..2242ebb79fbc 100755
--- a/s390x/run
+++ b/s390x/run
@@ -9,7 +9,7 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-ACCEL=$(get_accel '[ "$HOST" = "s390x" ] && [ "$ARCH" = "s390x" ]') ||
+ACCEL=$(get_accel) ||
 	exit 2
 
 qemu=$(search_qemu_binary) ||
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index a696652c61b1..77071216ff99 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -273,22 +273,26 @@ trap_exit_push ()
 	trap -- "$1; $old_exit" EXIT
 }
 
+kvm_available ()
+{
+	[ -c /dev/kvm ] ||
+		return 1
+
+	[ "$HOST" = "$ARCH_NAME" ] ||
+		[ "$HOST" = aarch64 -a "$ARCH" = arm ] ||
+		[ "$HOST" = x86_64 -a "$ARCH" = i386 ]
+}
+
 get_accel ()
 {
-	local kvm_available
-
-	if [ -c /dev/kvm ] && eval "$@"; then
-		kvm_available=yes
-	fi
-
-	if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
+	if [ "$ACCEL" = "kvm" ] && ! kvm_available; then
 		echo "KVM is needed, but not available on this host" >&2
 		return 2
 	fi
 
 	if [ "$ACCEL" ]; then
 		echo $ACCEL
-	elif [ "$kvm_available" = "yes" ]; then
+	elif kvm_available; then
 		echo kvm
 	else
 		echo tcg

Re: [kvm-unit-tests PATCH 5/5] arch/run: unify accelerator detection

[Paolo Bonzini]

On 29/06/2017 15:46, Radim Krčmář wrote:
> 2017-06-29 14:45+0200, Radim Krčmář:
>> 2017-06-28 22:48+0200, Paolo Bonzini:
>>> On 28/06/2017 22:08, Radim Krčmář wrote:
>>>> -if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
>>>> -	echo "KVM is needed, but not available on this host"
>>>> +ACCEL=$(get_accel '([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) || [ "$HOST" = "aarch64" ]') ||
>>>>  	exit 2
>>>
>>> Should x86 do the same as ARM (follow-up patch)?
>>
>> Yeah, I'll prepare it when we've agreed on the format.
>>
>>> And maybe the test could move from an argument of get_accel to a
>>> function "arch_kvm_available" that returns 0 if available and 1 if not?
>>
>> Right, the arm condition needs to be fixed as
>>
>>   ([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) ||
>>   ([ "$HOST" = "aarch64" ] && [ "$ARCH" = arm -o "$ARCH" = arm64 ])
>>
>> and it would look even worse in the argument ...
>>
>> Would it be ok like this?
> 
> No, we can make a better use of the $HOST variable:

Either is okay.

Paolo

> diff --git a/arm/run b/arm/run
> index f2e71eca022d..1245d42edae7 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -10,7 +10,7 @@ if [ -z "$STANDALONE" ]; then
>  fi
>  processor="$PROCESSOR"
>  
> -ACCEL=$(get_accel '([ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]) || [ "$HOST" = "aarch64" ]') ||
> +ACCEL=$(get_accel) ||
>  	exit 2
>  
>  qemu=$(search_qemu_binary) ||
> diff --git a/powerpc/run b/powerpc/run
> index 55b987c5efc5..58921e81eb8f 100755
> --- a/powerpc/run
> +++ b/powerpc/run
> @@ -9,7 +9,7 @@ if [ -z "$STANDALONE" ]; then
>  	source scripts/arch-run.bash
>  fi
>  
> -ACCEL=$(get_accel '[ "$HOST" = "ppc64" ] && [ "$ARCH" = "ppc64" ]') ||
> +ACCEL=$(get_accel) ||
>  	exit 2
>  
>  qemu=$(search_qemu_binary) ||
> diff --git a/s390x/run b/s390x/run
> index 67d6e88ea7e4..2242ebb79fbc 100755
> --- a/s390x/run
> +++ b/s390x/run
> @@ -9,7 +9,7 @@ if [ -z "$STANDALONE" ]; then
>  	source scripts/arch-run.bash
>  fi
>  
> -ACCEL=$(get_accel '[ "$HOST" = "s390x" ] && [ "$ARCH" = "s390x" ]') ||
> +ACCEL=$(get_accel) ||
>  	exit 2
>  
>  qemu=$(search_qemu_binary) ||
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index a696652c61b1..77071216ff99 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -273,22 +273,26 @@ trap_exit_push ()
>  	trap -- "$1; $old_exit" EXIT
>  }
>  
> +kvm_available ()
> +{
> +	[ -c /dev/kvm ] ||
> +		return 1
> +
> +	[ "$HOST" = "$ARCH_NAME" ] ||
> +		[ "$HOST" = aarch64 -a "$ARCH" = arm ] ||
> +		[ "$HOST" = x86_64 -a "$ARCH" = i386 ]
> +}
> +
>  get_accel ()
>  {
> -	local kvm_available
> -
> -	if [ -c /dev/kvm ] && eval "$@"; then
> -		kvm_available=yes
> -	fi
> -
> -	if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
> +	if [ "$ACCEL" = "kvm" ] && ! kvm_available; then
>  		echo "KVM is needed, but not available on this host" >&2
>  		return 2
>  	fi
>  
>  	if [ "$ACCEL" ]; then
>  		echo $ACCEL
> -	elif [ "$kvm_available" = "yes" ]; then
> +	elif kvm_available; then
>  		echo kvm
>  	else
>  		echo tcg
> 

[kvm-unit-tests PATCH v2 5/5] arch/run: unify accelerator detection

[Radim Krčmář]

Use the same mechanism as search_qemu_binary().

The simplified [ "$HOST" = "$ARCH_NAME" ] check will mishandle a case if
you compile for $arch on $arch then try to run it with $other_arch/run,
but that isn't really worth checking.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arm/run               | 22 ++--------------------
 powerpc/run           | 20 ++------------------
 s390x/run             | 20 ++------------------
 scripts/arch-run.bash | 26 ++++++++++++++++++++++++++
 4 files changed, 32 insertions(+), 56 deletions(-)

diff --git a/arm/run b/arm/run
index 6fc9bb6bb111..fd280ee19837 100755
--- a/arm/run
+++ b/arm/run
@@ -10,26 +10,8 @@ if [ -z "$STANDALONE" ]; then
 fi
 processor="$PROCESSOR"
 
-if [ -c /dev/kvm ]; then
-	if [ "$HOST" = "arm" ] && [ "$ARCH" = "arm" ]; then
-		kvm_available=yes
-	elif [ "$HOST" = "aarch64" ]; then
-		kvm_available=yes
-	fi
-fi
-
-if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
-	echo "KVM is needed, but not available on this host"
-	exit 2
-fi
-
-if [ -z "$ACCEL" ]; then
-	if [ "$kvm_available" = "yes" ]; then
-		ACCEL="kvm"
-	else
-		ACCEL="tcg"
-	fi
-fi
+ACCEL=$(get_qemu_accelerator) ||
+	exit $?
 
 qemu=$(search_qemu_binary) ||
 	exit $?
diff --git a/powerpc/run b/powerpc/run
index 288504e0c330..597ab96ed8a8 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -9,24 +9,8 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-if [ -c /dev/kvm ]; then
-	if [ "$HOST" = "ppc64" ] && [ "$ARCH" = "ppc64" ]; then
-		kvm_available=yes
-	fi
-fi
-
-if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
-	echo "KVM is needed, but not available on this host"
-	exit 2
-fi
-
-if [ -z "$ACCEL" ]; then
-	if [ "$kvm_available" = "yes" ]; then
-		ACCEL="kvm"
-	else
-		ACCEL="tcg"
-	fi
-fi
+ACCEL=$(get_qemu_accelerator) ||
+	exit $?
 
 qemu=$(search_qemu_binary) ||
 	exit $?
diff --git a/s390x/run b/s390x/run
index af22e8b269d6..0980504448ce 100755
--- a/s390x/run
+++ b/s390x/run
@@ -9,24 +9,8 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
-if [ -c /dev/kvm ]; then
-	if [ "$HOST" = "s390x" ] && [ "$ARCH" = "s390x" ]; then
-		kvm_available=yes
-	fi
-fi
-
-if [ "$ACCEL" = "kvm" ] && [ "$kvm_available" != "yes" ]; then
-	echo "KVM is needed, but not available on this host"
-	exit 2
-fi
-
-if [ -z "$ACCEL" ]; then
-	if [ "$kvm_available" = "yes" ]; then
-		ACCEL="kvm"
-	else
-		ACCEL="tcg"
-	fi
-fi
+ACCEL=$(get_qemu_accelerator) ||
+	exit $?
 
 qemu=$(search_qemu_binary) ||
 	exit $?
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 994c1aa9c0cd..afad43d2d1ad 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -272,3 +272,29 @@ trap_exit_push ()
 	local old_exit=$(trap -p EXIT | sed "s/^[^']*'//;s/'[^']*$//")
 	trap -- "$1; $old_exit" EXIT
 }
+
+kvm_available ()
+{
+	[ -c /dev/kvm ] ||
+		return 1
+
+	[ "$HOST" = "$ARCH_NAME" ] ||
+		[ "$HOST" = aarch64 -a "$ARCH" = arm ] ||
+		[ "$HOST" = x86_64 -a "$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" ]; then
+		echo $ACCEL
+	elif kvm_available; then
+		echo kvm
+	else
+		echo tcg
+	fi
+}
-- 
2.13.2

[kvm-unit-tests PATCH v2 6/5] x86/run: support accelerator controls

[Radim Krčmář]

It's easy to support and we do have the option for other architectures.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 x86/run | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/x86/run b/x86/run
index f319b68ef693..e59c6c590a20 100755
--- a/x86/run
+++ b/x86/run
@@ -9,6 +9,9 @@ if [ -z "$STANDALONE" ]; then
 	source scripts/arch-run.bash
 fi
 
+ACCEL=$(get_qemu_accelerator) ||
+	exit $?
+
 qemu=$(search_qemu_binary) ||
 	exit $?
 
@@ -34,8 +37,8 @@ else
 	pc_testdev="-device testdev,chardev=testlog -chardev file,id=testlog,path=msr.out"
 fi
 
-command="${qemu} -nodefaults -enable-kvm $pc_testdev -vnc none -serial stdio $pci_testdev $hyperv_testdev"
-command+=" -kernel"
+command="${qemu} -nodefaults $pc_testdev -vnc none -serial stdio $pci_testdev $hyperv_testdev"
+command+=" -machine accel=$ACCEL -kernel"
 command="$(timeout_cmd) $command"
 
 run_qemu ${command} "$@"
-- 
2.13.2

Re: [kvm-unit-tests PATCH v2 6/5] x86/run: support accelerator controls

[Paolo Bonzini]

Applied both, thanks.

Paolo

On 29/06/2017 16:32, Radim Krčmář wrote:
> It's easy to support and we do have the option for other architectures.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  x86/run | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/x86/run b/x86/run
> index f319b68ef693..e59c6c590a20 100755
> --- a/x86/run
> +++ b/x86/run
> @@ -9,6 +9,9 @@ if [ -z "$STANDALONE" ]; then
>  	source scripts/arch-run.bash
>  fi
>  
> +ACCEL=$(get_qemu_accelerator) ||
> +	exit $?
> +
>  qemu=$(search_qemu_binary) ||
>  	exit $?
>  
> @@ -34,8 +37,8 @@ else
>  	pc_testdev="-device testdev,chardev=testlog -chardev file,id=testlog,path=msr.out"
>  fi
>  
> -command="${qemu} -nodefaults -enable-kvm $pc_testdev -vnc none -serial stdio $pci_testdev $hyperv_testdev"
> -command+=" -kernel"
> +command="${qemu} -nodefaults $pc_testdev -vnc none -serial stdio $pci_testdev $hyperv_testdev"
> +command+=" -machine accel=$ACCEL -kernel"
>  command="$(timeout_cmd) $command"
>  
>  run_qemu ${command} "$@"
> 

Re: [kvm-unit-tests PATCH 0/5] Minor fixes while getting rid of code duplication

[Andrew Jones]

On Wed, Jun 28, 2017 at 10:08:52PM +0200, Radim Krčmář wrote:
> The series isn't well split, so patches remove the code duplication and
> fix bugs at the same time.  [3/5] is s390x+powerpc specific, but has
> dependency on [4/5], which affects all architectures, just like [1,5/5].
> [2/5] is unrelated, but was simple enough to include.
> 
> 
> Radim Krčmář (5):
>   arch/run: simplify copy-paste of the command line
>   run_tests: add summary to powerpc and s390
>   s390+powerpc/run: improve output handling
>   scripts/arch-run: fix qemu binary search failure path
>   arch/run: unify accelerator detection
> 
>  arm/run               | 28 +++------------------
>  powerpc/run           | 41 ++++--------------------------
>  s390x/run             | 38 +++-------------------------
>  scripts/arch-run.bash | 69 +++++++++++++++++++++++++++++++++++++++++++++++----
>  scripts/runtime.bash  |  2 +-
>  x86/run               |  8 +++---
>  6 files changed, 81 insertions(+), 105 deletions(-)
> 
> -- 
> 2.13.2
>

With Paolo's fixups, the series looks good to me. For the series

Reviewed-by: Andrew Jones <drjones@redhat.com>