Re: [kvm-unit-tests PATCH v2 10/18] scripts/arch-run: Add support for kvmtool

[Andrew Jones <andrew.jones@linux.dev>]

On Mon, Jan 20, 2025 at 04:43:08PM +0000, Alexandru Elisei wrote:
> Add two new functions, search_kvmtool_binary(), which, like the name
> suggests, searches for the location of the kvmtool binary, and
> run_kvmtool(), which runs a test with kvmtool as the VMM.
> 
> initrd_create() has also been modified to use the kvmtool syntax for
> supplying an initrd, which is --initrd (two dashes instead of the single
> dash that qemu uses).
> 
> arm/run does not know how to use these functions yet, but this will be
> added in a subsequent patch.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  scripts/arch-run.bash | 94 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 81 insertions(+), 13 deletions(-)
> 
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index d6eaf0ee5f09..34f633cade01 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -75,16 +75,47 @@ run_qemu ()
>  	return $ret
>  }
>  
> +run_kvmtool ()
> +{
> +	local stdout errors ret sig
> +
> +	initrd_create || return $?
> +
> +	echo -n "$@"
> +	[ "$ENVIRON_DEFAULT" = "yes" ] && echo -n " #"
> +	echo " $INITRD"
> +
> +	# stdout to {stdout}, stderr to $errors and stderr
> +	exec {stdout}>&1
> +	"${@}" $INITRD </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout
> +	ret=$?
> +	exec {stdout}>&-
> +
> +	return $ret
> +}

It seems like run_qemu should mostly apply to kvmtool since kvmtool could
also terminate on a signal, like the timeout SIGALRM. Maybe we should try
to get them to use the same thing and name it run_test()? Or at least we
can factor out the common parts to avoid duplication.

> +
>  run_test_status ()
>  {
> -	local stdout ret
> +	local stdout ret ret_success
>  
>  	exec {stdout}>&1
> -	lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
> +
> +	# For qemu, an exit status of 1 means that the test completed. For kvmtool,
> +	# 0 means the same thing.
> +	case "$TARGET" in
> +	qemu)
> +		ret_success=1
> +		lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
> +		;;
> +	kvmtool)
> +		ret_success=0
> +		lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout))
> +		;;
> +	esac
>  	ret=$?
>  	exec {stdout}>&-
>  
> -	if [ $ret -eq 1 ]; then
> +	if [ $ret -eq $ret_success ]; then
>  		testret=$(grep '^EXIT: ' <<<"$lines" | head -n1 | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
>  		if [ "$testret" ]; then
>  			if [ $testret -eq 1 ]; then
> @@ -422,6 +453,25 @@ search_qemu_binary ()
>  	export PATH=$save_path
>  }
>  
> +search_kvmtool_binary ()
> +{
> +	local kvmtoolcmd kvmtool
> +
> +	for kvmtoolcmd in lkvm vm lkvm-static; do
> +		if $kvmtoolcmd --help 2>/dev/null| grep -q 'The most commonly used'; then
> +			kvmtool="$kvmtoolcmd"
> +			break
> +		fi
> +	done
> +
> +	if [ -z "$kvmtool" ]; then
> +		echo "A kvmtool binary was not found." >&2

Do we want to support a KVMTOOL environment variable analogous to $QEMU?
If so we can also add the help text that search_qemu_binary() has here.

> +		return 2
> +	fi
> +
> +	command -v $kvmtool
> +}
> +
>  initrd_cleanup ()
>  {
>  	rm -f $KVM_UNIT_TESTS_ENV
> @@ -447,7 +497,18 @@ initrd_create ()
>  	fi
>  
>  	unset INITRD
> -	[ -f "$KVM_UNIT_TESTS_ENV" ] && INITRD="-initrd $KVM_UNIT_TESTS_ENV"
> +	if [ ! -f "$KVM_UNIT_TESTS_ENV" ]; then
> +		return 0
> +	fi
> +
> +	case "$TARGET" in
> +		qemu)
> +			INITRD="-initrd $KVM_UNIT_TESTS_ENV"
> +			;;
> +		kvmtool)
> +			INITRD="--initrd $KVM_UNIT_TESTS_ENV"
> +			;;
> +	esac

vmm_opts[qemu,initrd]='-initrd'
vmm_opts[kvmtool,initrd]='--initrd'

>  
>  	return 0
>  }
> @@ -471,18 +532,25 @@ env_params ()
>  	local qemu have_qemu
>  	local _ rest
>  
> -	qemu=$(search_qemu_binary) && have_qemu=1
> +	env_add_params TARGET
> +
> +	# kvmtool's versioning has been broken since it was split from the kernel
> +	# source.
> +	if [ "$TARGET" = "qemu" ]; then
> +		qemu=$(search_qemu_binary) && have_qemu=1
>  
> -	if [ "$have_qemu" ]; then
> -		if [ -n "$ACCEL" ] || [ -n "$QEMU_ACCEL" ]; then
> -			[ -n "$ACCEL" ] && QEMU_ACCEL=$ACCEL
> +		if [ "$have_qemu" ]; then
> +			if [ -n "$ACCEL" ] || [ -n "$QEMU_ACCEL" ]; then
> +				[ -n "$ACCEL" ] && QEMU_ACCEL=$ACCEL
> +			fi
> +			QEMU_VERSION_STRING="$($qemu -h | head -1)"
> +			# Shellcheck does not see QEMU_MAJOR|MINOR|MICRO are used
> +			# shellcheck disable=SC2034
> +			IFS='[ .]' read -r _ _ _ QEMU_MAJOR QEMU_MINOR QEMU_MICRO rest <<<"$QEMU_VERSION_STRING"
>  		fi
> -		QEMU_VERSION_STRING="$($qemu -h | head -1)"
> -		# Shellcheck does not see QEMU_MAJOR|MINOR|MICRO are used
> -		# shellcheck disable=SC2034
> -		IFS='[ .]' read -r _ _ _ QEMU_MAJOR QEMU_MINOR QEMU_MICRO rest <<<"$QEMU_VERSION_STRING"
> +
> +		env_add_params QEMU_ACCEL QEMU_VERSION_STRING QEMU_MAJOR QEMU_MINOR QEMU_MICRO
>  	fi
> -	env_add_params QEMU_ACCEL QEMU_VERSION_STRING QEMU_MAJOR QEMU_MINOR QEMU_MICRO
>  
>  	KERNEL_VERSION_STRING=$(uname -r)
>  	IFS=. read -r KERNEL_VERSION KERNEL_PATCHLEVEL rest <<<"$KERNEL_VERSION_STRING"
> -- 
> 2.47.1
>

Thanks,
drew