[kvm-unit-tests,RFC,3/5] run_tests.sh: Add kvmtool support

Message ID	20210702163122.96110-4-alexandru.elisei@arm.com
State	New
Headers	show Return-Path: <kvm-ppc-owner@vger.kernel.org> From: Alexandru Elisei <alexandru.elisei@arm.com> To: drjones@redhat.com, thuth@redhat.com, pbonzini@redhat.com, lvivier@redhat.com, kvm-ppc@vger.kernel.org, david@redhat.com, frankja@linux.ibm.com, cohuck@redhat.com, imbrenda@linux.ibm.com, linux-s390@vger.kernel.org, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu Cc: andre.przywara@arm.com, maz@kernel.org, vivek.gautam@arm.com Subject: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support Date: Fri, 2 Jul 2021 17:31:20 +0100 Message-Id: <20210702163122.96110-4-alexandru.elisei@arm.com> In-Reply-To: <20210702163122.96110-1-alexandru.elisei@arm.com> References: <20210702163122.96110-1-alexandru.elisei@arm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	arm: Add kvmtool to the runner script \| expand [kvm-unit-tests,RFC,0/5] arm: Add kvmtool to the runner script [kvm-unit-tests,RFC,1/5] lib: arm: Print test exit status on exit if chr-testdev is not available [kvm-unit-tests,RFC,2/5] scripts: Rename run_qemu_status -> run_test_status [kvm-unit-tests,RFC,3/5] run_tests.sh: Add kvmtool support [kvm-unit-tests,RFC,4/5] scripts: Generate kvmtool standalone tests [kvm-unit-tests,RFC,5/5] configure: Ignore --erratatxt when --target=kvmtool

Alexandru Elisei July 2, 2021, 4:31 p.m. UTC

Modify run_tests.sh to use kvmtool instead of qemu to run tests when
kvm-unit-tests has been configured with --target=kvmtool.

Example invocation:

$ ./configure --target=kvmtool
$ make clean && make
$ ./run_scripts.sh

A custom location for the kvmtool binary can be set using the environment
variable KVMTOOL:

$ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh

Standalone test support is absent, but will be added in subsequent patches.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 scripts/arch-run.bash   |  48 ++++++++++++++++--
 scripts/runtime.bash    |  94 ++++++++++++++++++++++++++++------
 scripts/mkstandalone.sh |   5 ++
 arm/run                 | 110 ++++++++++++++++++++++++----------------
 run_tests.sh            |  11 +++-
 5 files changed, 204 insertions(+), 64 deletions(-)

Andre Przywara July 12, 2021, 4:52 p.m. UTC | #1

On Fri,  2 Jul 2021 17:31:20 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi Alex,

> Modify run_tests.sh to use kvmtool instead of qemu to run tests when
> kvm-unit-tests has been configured with --target=kvmtool.
> 
> Example invocation:
> 
> $ ./configure --target=kvmtool
> $ make clean && make
> $ ./run_scripts.sh
> 
> A custom location for the kvmtool binary can be set using the environment
> variable KVMTOOL:
> 
> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh
> 
> Standalone test support is absent, but will be added in subsequent patches.

So I skimmed over this, and it's at least better than what I had tried
a few years back ;-)
And while there might be other ways to sort out command line
differences between the two, this fixup_kvmtool_opts() looks like a
clever solution to the problem.

The only problem with this patch is that it's rather big, I wonder if
this could be split up? For instance move any QEMU specific
functionality into separate functions first (like run_test_qemu()),
also use 'if [ "$TARGET" = "qemu" ]' in this first (or second?) patch.
Then later on just add the kvmtool specifics.

Cheers,
Andre

> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  scripts/arch-run.bash   |  48 ++++++++++++++++--
>  scripts/runtime.bash    |  94 ++++++++++++++++++++++++++++------
>  scripts/mkstandalone.sh |   5 ++
>  arm/run                 | 110 ++++++++++++++++++++++++----------------
>  run_tests.sh            |  11 +++-
>  5 files changed, 204 insertions(+), 64 deletions(-)
> 
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index 8ceed53ed7f8..b916b0e79aca 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -69,16 +69,39 @@ run_qemu ()
>  	return $ret
>  }
>  
> +run_kvmtool()
> +{
> +	local stdout errors ret sig
> +
> +	# kvmtool doesn't allow an initrd argument with --firmware, but configure
> +	# sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target.
> +
> +	# stdout to {stdout}, stderr to $errors and stderr
> +	exec {stdout}>&1
> +	errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout)
> +	ret=$?
> +	exec {stdout}>&-
> +
> +	# ret=0 success, everything else is failure.
> +	return $ret
> +}
> +
>  run_test_status ()
>  {
> -	local stdout ret
> +	local stdout ret exit_status
>  
>  	exec {stdout}>&1
> -	lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
> +	if [ "$TARGET" = "kvmtool" ]; then
> +		lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout))
> +		exit_status=0
> +	else
> +		lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
> +		exit_status=1
> +	fi
>  	ret=$?
>  	exec {stdout}>&-
>  
> -	if [ $ret -eq 1 ]; then
> +	if [ $ret -eq $exit_status ]; then
>  		testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
>  		if [ "$testret" ]; then
>  			if [ $testret -eq 1 ]; then
> @@ -193,6 +216,25 @@ search_qemu_binary ()
>  	export PATH=$save_path
>  }
>  
> +search_kvmtool_binary ()
> +{
> +	local lkvm kvmtool
> +
> +	for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do
> +		if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then
> +			kvmtool="$lkvm"
> +			break
> +		fi
> +	done
> +
> +	if [ -z "$kvmtool" ]; then
> +		echo "A kvmtool binary was not found." >&2
> +		echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2
> +		return 2
> +	fi
> +	command -v $kvmtool
> +}
> +
>  initrd_create ()
>  {
>  	if [ "$ENVIRON_DEFAULT" = "yes" ]; then
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index 132389c7dd59..23b238a6ab6f 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -12,14 +12,19 @@ extract_summary()
>      tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/'
>  }
>  
> -# We assume that QEMU is going to work if it tried to load the kernel
> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel
>  premature_failure()
>  {
>      local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)"
>  
> -    echo "$log" | grep "_NO_FILE_4Uhere_" |
> -        grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
> -        return 1
> +    if [ "$TARGET" = "kvmtool" ]; then
> +        echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" &&
> +            return 1
> +    else
> +        echo "$log" | grep "_NO_FILE_4Uhere_" |
> +            grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
> +            return 1
> +    fi
>  
>      RUNTIME_log_stderr <<< "$log"
>  
> @@ -30,7 +35,14 @@ premature_failure()
>  get_cmdline()
>  {
>      local kernel=$1
> -    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
> +    local smp_param
> +
> +    if [ "$TARGET" = "kvmtool" ]; then
> +        smp_param="--cpus $smp"
> +    else
> +        smp_param="-smp $smp"
> +    fi
> +    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts"
>  }
>  
>  skip_nodefault()
> @@ -70,6 +82,35 @@ function find_word()
>      grep -Fq " $1 " <<< " $2 "
>  }
>  
> +fixup_kvmtool_opts()
> +{
> +    local opts=$1
> +    local groups=$2
> +    local gic
> +    local gic_version
> +
> +    if find_word "pmu" $groups; then
> +        opts+=" --pmu"
> +    fi
> +
> +    if find_word "its" $groups; then
> +        gic_version=3
> +        gic="gicv3-its"
> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
> +        gic_version="${BASH_REMATCH[1]}"
> +        gic="gicv$gic_version"
> +    fi
> +
> +    if [ -n "$gic" ]; then
> +        opts=${opts/-machine gic-version=$gic_version/}
> +        opts+=" --irqchip=$gic"
> +    fi
> +
> +    opts=${opts/-append/--params}
> +
> +    echo "$opts"
> +}
> +
>  function run()
>  {
>      local testname="$1"
> @@ -105,7 +146,12 @@ function run()
>          return 2
>      fi
>  
> -    if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
> +    if [ "$TARGET" = "kvmtool" ]; then
> +        if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then
> +            print_result "SKIP" $testname "" "$accel not supported by kvmtool"
> +            return 2
> +        fi
> +    elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
>          print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL"
>          return 2
>      elif [ -n "$ACCEL" ]; then
> @@ -126,6 +172,10 @@ function run()
>          done
>      fi
>  
> +    if [ "$TARGET" = "kvmtool" ]; then
> +        opts=$(fixup_kvmtool_opts "$opts" "$groups")
> +    fi
> +
>      last_line=$(premature_failure > >(tail -1)) && {
>          print_result "SKIP" $testname "" "$last_line"
>          return 77
> @@ -165,13 +215,25 @@ function run()
>  #
>  # Probe for MAX_SMP, in case it's less than the number of host cpus.
>  #
> -# This probing currently only works for ARM, as x86 bails on another
> -# error first. Also, this probing isn't necessary for any ARM hosts
> -# running kernels later than v4.3, i.e. those including ef748917b52
> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
> -# point when maintaining the while loop gets too tiresome, we can
> -# just remove it...
> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> -		|& grep -qi 'exceeds max CPUs'; do
> -	MAX_SMP=$((MAX_SMP >> 1))
> -done
> +# This probing currently only works for ARM, as x86 bails on another error
> +# first. Also, this probing isn't necessary for any ARM hosts running kernels
> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove
> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop
> +# gets too tiresome, we can just remove it...
> +#
> +# We don't need this check for kvmtool, as kvmtool will automatically limit the
> +# number of VCPUs to what the host supports instead of exiting with an error.
> +# kvmtool prints a message when that happens, but it's harmless and the chance
> +# of running a kernel so old that the number of VCPUs is smaller than the number
> +# of physical CPUs is vanishingly small.
> +#
> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0,
> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test
> +# is running on a recent x86 machine, there's a fairly good chance that more
> +# than 8 logical CPUs are available.
> +if [ "$TARGET" = "qemu" ]; then
> +    while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> +            |& grep -qi 'exceeds max CPUs'; do
> +        MAX_SMP=$((MAX_SMP >> 1))
> +    done
> +fi
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index cefdec30cb33..16f461c06842 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -95,6 +95,11 @@ function mkstandalone()
>  	echo Written $standalone.
>  }
>  
> +if [ "$TARGET" = "kvmtool" ]; then
> +	echo "Standalone tests not supported with kvmtool"
> +	exit 2
> +fi
> +
>  if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then
>  	echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2
>  	exit 2
> diff --git a/arm/run b/arm/run
> index a390ca5ae0ba..cc5890e7fec4 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then
>  	source config.mak
>  	source scripts/arch-run.bash
>  fi
> -processor="$PROCESSOR"
>  
> -ACCEL=$(get_qemu_accelerator) ||
> -	exit $?
> +run_test_qemu()
> +{
> +    processor="$PROCESSOR"
>  
> -qemu=$(search_qemu_binary) ||
> -	exit $?
> +    ACCEL=$(get_qemu_accelerator) ||
> +        exit $?
>  
> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
> -	echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
> -	exit 2
> -fi
> +    qemu=$(search_qemu_binary) ||
> +        exit $?
>  
> -M='-machine virt'
> +    if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
> +        echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
> +        exit 2
> +    fi
>  
> -if [ "$ACCEL" = "kvm" ]; then
> -	if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
> -		M+=',gic-version=host'
> -	fi
> -	if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
> -		processor="host"
> -		if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
> -			processor+=",aarch64=off"
> -		fi
> -	fi
> -fi
> +    M='-machine virt'
>  
> -if [ "$ARCH" = "arm" ]; then
> -	M+=",highmem=off"
> -fi
> +    if [ "$ACCEL" = "kvm" ]; then
> +        if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
> +            M+=',gic-version=host'
> +        fi
> +        if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
> +            processor="host"
> +            if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
> +                processor+=",aarch64=off"
> +            fi
> +        fi
> +    fi
>  
> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
> -	echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
> -	exit 2
> -fi
> +    if [ "$ARCH" = "arm" ]; then
> +        M+=",highmem=off"
> +    fi
>  
> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
> -		| grep backend > /dev/null; then
> -	echo "$qemu doesn't support chr-testdev. Exiting."
> -	exit 2
> -fi
> +    if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
> +        echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
> +        exit 2
> +    fi
>  
> -chr_testdev='-device virtio-serial-device'
> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
> +    if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
> +            | grep backend > /dev/null; then
> +        echo "$qemu doesn't support chr-testdev. Exiting."
> +        exit 2
> +    fi
>  
> -pci_testdev=
> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
> -	pci_testdev="-device pci-testdev"
> -fi
> +    chr_testdev='-device virtio-serial-device'
> +    chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
> +
> +    pci_testdev=
> +    if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
> +        pci_testdev="-device pci-testdev"
> +    fi
> +
> +    M+=",accel=$ACCEL"
> +    command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
> +    command+=" -display none -serial stdio -kernel"
> +    command="$(migration_cmd) $(timeout_cmd) $command"
> +
> +    run_qemu $command "$@"
> +}
> +
> +run_test_kvmtool()
> +{
> +    kvmtool=$(search_kvmtool_binary) ||
> +        exit $?
>  
> -M+=",accel=$ACCEL"
> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
> -command+=" -display none -serial stdio -kernel"
> -command="$(migration_cmd) $(timeout_cmd) $command"
> +    local command="$(timeout_cmd) $kvmtool run --firmware "
> +    run_test_status $command "$@"
> +}
>  
> -run_qemu $command "$@"
> +case "$TARGET" in
> +    "qemu")
> +        run_test_qemu "$@"
> +        ;;
> +    "kvmtool")
> +        run_test_kvmtool "$@"
> +        ;;
> +esac
> diff --git a/run_tests.sh b/run_tests.sh
> index 65108e73a2c0..b010ee3ab348 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t]
>      -t, --tap13     Output test results in TAP format
>  
>  Set the environment variable QEMU=/path/to/qemu-system-ARCH to
> -specify the appropriate qemu binary for ARCH-run.
> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool
> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool
> +can be used to specify a custom location for the kvmtool binary.
>  
>  EOF
>  }
> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then
>      exit 1
>  fi
>  
> +if [ "$TARGET" = "kvmtool" ]; then
> +    if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then
> +        echo "kvmtool supports only the kvm accelerator"
> +        exit 1
> +    fi
> +fi
> +
>  only_tests=""
>  args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*`
>  [ $? -ne 0 ] && exit 2;

Alexandru Elisei Sept. 6, 2021, 10:28 a.m. UTC | #2

Hi Andre,

Thanks for having a look!

On 7/12/21 5:52 PM, Andre Przywara wrote:
> On Fri,  2 Jul 2021 17:31:20 +0100
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi Alex,
>
>> Modify run_tests.sh to use kvmtool instead of qemu to run tests when
>> kvm-unit-tests has been configured with --target=kvmtool.
>>
>> Example invocation:
>>
>> $ ./configure --target=kvmtool
>> $ make clean && make
>> $ ./run_scripts.sh
>>
>> A custom location for the kvmtool binary can be set using the environment
>> variable KVMTOOL:
>>
>> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh
>>
>> Standalone test support is absent, but will be added in subsequent patches.
> So I skimmed over this, and it's at least better than what I had tried
> a few years back ;-)
> And while there might be other ways to sort out command line
> differences between the two, this fixup_kvmtool_opts() looks like a
> clever solution to the problem.
>
> The only problem with this patch is that it's rather big, I wonder if
> this could be split up? For instance move any QEMU specific
> functionality into separate functions first (like run_test_qemu()),
> also use 'if [ "$TARGET" = "qemu" ]' in this first (or second?) patch.
> Then later on just add the kvmtool specifics.

You're right, this patch looks very big and difficult to review, it needs to be
split into smaller chunks for the next iteration.

Drew, did you manage to take a quick look at this patch? Do you think this is the
right direction? I would prefer to hear your opinion about it before I start
reworking it.

Thanks,

Alex

>
> Cheers,
> Andre
>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  scripts/arch-run.bash   |  48 ++++++++++++++++--
>>  scripts/runtime.bash    |  94 ++++++++++++++++++++++++++++------
>>  scripts/mkstandalone.sh |   5 ++
>>  arm/run                 | 110 ++++++++++++++++++++++++----------------
>>  run_tests.sh            |  11 +++-
>>  5 files changed, 204 insertions(+), 64 deletions(-)
>>
>> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
>> index 8ceed53ed7f8..b916b0e79aca 100644
>> --- a/scripts/arch-run.bash
>> +++ b/scripts/arch-run.bash
>> @@ -69,16 +69,39 @@ run_qemu ()
>>  	return $ret
>>  }
>>  
>> +run_kvmtool()
>> +{
>> +	local stdout errors ret sig
>> +
>> +	# kvmtool doesn't allow an initrd argument with --firmware, but configure
>> +	# sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target.
>> +
>> +	# stdout to {stdout}, stderr to $errors and stderr
>> +	exec {stdout}>&1
>> +	errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout)
>> +	ret=$?
>> +	exec {stdout}>&-
>> +
>> +	# ret=0 success, everything else is failure.
>> +	return $ret
>> +}
>> +
>>  run_test_status ()
>>  {
>> -	local stdout ret
>> +	local stdout ret exit_status
>>  
>>  	exec {stdout}>&1
>> -	lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
>> +	if [ "$TARGET" = "kvmtool" ]; then
>> +		lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout))
>> +		exit_status=0
>> +	else
>> +		lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
>> +		exit_status=1
>> +	fi
>>  	ret=$?
>>  	exec {stdout}>&-
>>  
>> -	if [ $ret -eq 1 ]; then
>> +	if [ $ret -eq $exit_status ]; then
>>  		testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
>>  		if [ "$testret" ]; then
>>  			if [ $testret -eq 1 ]; then
>> @@ -193,6 +216,25 @@ search_qemu_binary ()
>>  	export PATH=$save_path
>>  }
>>  
>> +search_kvmtool_binary ()
>> +{
>> +	local lkvm kvmtool
>> +
>> +	for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do
>> +		if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then
>> +			kvmtool="$lkvm"
>> +			break
>> +		fi
>> +	done
>> +
>> +	if [ -z "$kvmtool" ]; then
>> +		echo "A kvmtool binary was not found." >&2
>> +		echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2
>> +		return 2
>> +	fi
>> +	command -v $kvmtool
>> +}
>> +
>>  initrd_create ()
>>  {
>>  	if [ "$ENVIRON_DEFAULT" = "yes" ]; then
>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>> index 132389c7dd59..23b238a6ab6f 100644
>> --- a/scripts/runtime.bash
>> +++ b/scripts/runtime.bash
>> @@ -12,14 +12,19 @@ extract_summary()
>>      tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/'
>>  }
>>  
>> -# We assume that QEMU is going to work if it tried to load the kernel
>> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel
>>  premature_failure()
>>  {
>>      local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)"
>>  
>> -    echo "$log" | grep "_NO_FILE_4Uhere_" |
>> -        grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
>> -        return 1
>> +    if [ "$TARGET" = "kvmtool" ]; then
>> +        echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" &&
>> +            return 1
>> +    else
>> +        echo "$log" | grep "_NO_FILE_4Uhere_" |
>> +            grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
>> +            return 1
>> +    fi
>>  
>>      RUNTIME_log_stderr <<< "$log"
>>  
>> @@ -30,7 +35,14 @@ premature_failure()
>>  get_cmdline()
>>  {
>>      local kernel=$1
>> -    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
>> +    local smp_param
>> +
>> +    if [ "$TARGET" = "kvmtool" ]; then
>> +        smp_param="--cpus $smp"
>> +    else
>> +        smp_param="-smp $smp"
>> +    fi
>> +    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts"
>>  }
>>  
>>  skip_nodefault()
>> @@ -70,6 +82,35 @@ function find_word()
>>      grep -Fq " $1 " <<< " $2 "
>>  }
>>  
>> +fixup_kvmtool_opts()
>> +{
>> +    local opts=$1
>> +    local groups=$2
>> +    local gic
>> +    local gic_version
>> +
>> +    if find_word "pmu" $groups; then
>> +        opts+=" --pmu"
>> +    fi
>> +
>> +    if find_word "its" $groups; then
>> +        gic_version=3
>> +        gic="gicv3-its"
>> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
>> +        gic_version="${BASH_REMATCH[1]}"
>> +        gic="gicv$gic_version"
>> +    fi
>> +
>> +    if [ -n "$gic" ]; then
>> +        opts=${opts/-machine gic-version=$gic_version/}
>> +        opts+=" --irqchip=$gic"
>> +    fi
>> +
>> +    opts=${opts/-append/--params}
>> +
>> +    echo "$opts"
>> +}
>> +
>>  function run()
>>  {
>>      local testname="$1"
>> @@ -105,7 +146,12 @@ function run()
>>          return 2
>>      fi
>>  
>> -    if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
>> +    if [ "$TARGET" = "kvmtool" ]; then
>> +        if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then
>> +            print_result "SKIP" $testname "" "$accel not supported by kvmtool"
>> +            return 2
>> +        fi
>> +    elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
>>          print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL"
>>          return 2
>>      elif [ -n "$ACCEL" ]; then
>> @@ -126,6 +172,10 @@ function run()
>>          done
>>      fi
>>  
>> +    if [ "$TARGET" = "kvmtool" ]; then
>> +        opts=$(fixup_kvmtool_opts "$opts" "$groups")
>> +    fi
>> +
>>      last_line=$(premature_failure > >(tail -1)) && {
>>          print_result "SKIP" $testname "" "$last_line"
>>          return 77
>> @@ -165,13 +215,25 @@ function run()
>>  #
>>  # Probe for MAX_SMP, in case it's less than the number of host cpus.
>>  #
>> -# This probing currently only works for ARM, as x86 bails on another
>> -# error first. Also, this probing isn't necessary for any ARM hosts
>> -# running kernels later than v4.3, i.e. those including ef748917b52
>> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
>> -# point when maintaining the while loop gets too tiresome, we can
>> -# just remove it...
>> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
>> -		|& grep -qi 'exceeds max CPUs'; do
>> -	MAX_SMP=$((MAX_SMP >> 1))
>> -done
>> +# This probing currently only works for ARM, as x86 bails on another error
>> +# first. Also, this probing isn't necessary for any ARM hosts running kernels
>> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove
>> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop
>> +# gets too tiresome, we can just remove it...
>> +#
>> +# We don't need this check for kvmtool, as kvmtool will automatically limit the
>> +# number of VCPUs to what the host supports instead of exiting with an error.
>> +# kvmtool prints a message when that happens, but it's harmless and the chance
>> +# of running a kernel so old that the number of VCPUs is smaller than the number
>> +# of physical CPUs is vanishingly small.
>> +#
>> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0,
>> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test
>> +# is running on a recent x86 machine, there's a fairly good chance that more
>> +# than 8 logical CPUs are available.
>> +if [ "$TARGET" = "qemu" ]; then
>> +    while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
>> +            |& grep -qi 'exceeds max CPUs'; do
>> +        MAX_SMP=$((MAX_SMP >> 1))
>> +    done
>> +fi
>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>> index cefdec30cb33..16f461c06842 100755
>> --- a/scripts/mkstandalone.sh
>> +++ b/scripts/mkstandalone.sh
>> @@ -95,6 +95,11 @@ function mkstandalone()
>>  	echo Written $standalone.
>>  }
>>  
>> +if [ "$TARGET" = "kvmtool" ]; then
>> +	echo "Standalone tests not supported with kvmtool"
>> +	exit 2
>> +fi
>> +
>>  if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then
>>  	echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2
>>  	exit 2
>> diff --git a/arm/run b/arm/run
>> index a390ca5ae0ba..cc5890e7fec4 100755
>> --- a/arm/run
>> +++ b/arm/run
>> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then
>>  	source config.mak
>>  	source scripts/arch-run.bash
>>  fi
>> -processor="$PROCESSOR"
>>  
>> -ACCEL=$(get_qemu_accelerator) ||
>> -	exit $?
>> +run_test_qemu()
>> +{
>> +    processor="$PROCESSOR"
>>  
>> -qemu=$(search_qemu_binary) ||
>> -	exit $?
>> +    ACCEL=$(get_qemu_accelerator) ||
>> +        exit $?
>>  
>> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
>> -	echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
>> -	exit 2
>> -fi
>> +    qemu=$(search_qemu_binary) ||
>> +        exit $?
>>  
>> -M='-machine virt'
>> +    if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
>> +        echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
>> +        exit 2
>> +    fi
>>  
>> -if [ "$ACCEL" = "kvm" ]; then
>> -	if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
>> -		M+=',gic-version=host'
>> -	fi
>> -	if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
>> -		processor="host"
>> -		if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
>> -			processor+=",aarch64=off"
>> -		fi
>> -	fi
>> -fi
>> +    M='-machine virt'
>>  
>> -if [ "$ARCH" = "arm" ]; then
>> -	M+=",highmem=off"
>> -fi
>> +    if [ "$ACCEL" = "kvm" ]; then
>> +        if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
>> +            M+=',gic-version=host'
>> +        fi
>> +        if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
>> +            processor="host"
>> +            if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
>> +                processor+=",aarch64=off"
>> +            fi
>> +        fi
>> +    fi
>>  
>> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
>> -	echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
>> -	exit 2
>> -fi
>> +    if [ "$ARCH" = "arm" ]; then
>> +        M+=",highmem=off"
>> +    fi
>>  
>> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
>> -		| grep backend > /dev/null; then
>> -	echo "$qemu doesn't support chr-testdev. Exiting."
>> -	exit 2
>> -fi
>> +    if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
>> +        echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
>> +        exit 2
>> +    fi
>>  
>> -chr_testdev='-device virtio-serial-device'
>> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
>> +    if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
>> +            | grep backend > /dev/null; then
>> +        echo "$qemu doesn't support chr-testdev. Exiting."
>> +        exit 2
>> +    fi
>>  
>> -pci_testdev=
>> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
>> -	pci_testdev="-device pci-testdev"
>> -fi
>> +    chr_testdev='-device virtio-serial-device'
>> +    chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
>> +
>> +    pci_testdev=
>> +    if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
>> +        pci_testdev="-device pci-testdev"
>> +    fi
>> +
>> +    M+=",accel=$ACCEL"
>> +    command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
>> +    command+=" -display none -serial stdio -kernel"
>> +    command="$(migration_cmd) $(timeout_cmd) $command"
>> +
>> +    run_qemu $command "$@"
>> +}
>> +
>> +run_test_kvmtool()
>> +{
>> +    kvmtool=$(search_kvmtool_binary) ||
>> +        exit $?
>>  
>> -M+=",accel=$ACCEL"
>> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
>> -command+=" -display none -serial stdio -kernel"
>> -command="$(migration_cmd) $(timeout_cmd) $command"
>> +    local command="$(timeout_cmd) $kvmtool run --firmware "
>> +    run_test_status $command "$@"
>> +}
>>  
>> -run_qemu $command "$@"
>> +case "$TARGET" in
>> +    "qemu")
>> +        run_test_qemu "$@"
>> +        ;;
>> +    "kvmtool")
>> +        run_test_kvmtool "$@"
>> +        ;;
>> +esac
>> diff --git a/run_tests.sh b/run_tests.sh
>> index 65108e73a2c0..b010ee3ab348 100755
>> --- a/run_tests.sh
>> +++ b/run_tests.sh
>> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t]
>>      -t, --tap13     Output test results in TAP format
>>  
>>  Set the environment variable QEMU=/path/to/qemu-system-ARCH to
>> -specify the appropriate qemu binary for ARCH-run.
>> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool
>> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool
>> +can be used to specify a custom location for the kvmtool binary.
>>  
>>  EOF
>>  }
>> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then
>>      exit 1
>>  fi
>>  
>> +if [ "$TARGET" = "kvmtool" ]; then
>> +    if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then
>> +        echo "kvmtool supports only the kvm accelerator"
>> +        exit 1
>> +    fi
>> +fi
>> +
>>  only_tests=""
>>  args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*`
>>  [ $? -ne 0 ] && exit 2;

Andrew Jones Sept. 6, 2021, 11:01 a.m. UTC | #3

On Mon, Sep 06, 2021 at 11:28:28AM +0100, Alexandru Elisei wrote:
> Hi Andre,
> 
> Thanks for having a look!
> 
> On 7/12/21 5:52 PM, Andre Przywara wrote:
> > On Fri,  2 Jul 2021 17:31:20 +0100
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >
> > Hi Alex,
> >
> >> Modify run_tests.sh to use kvmtool instead of qemu to run tests when
> >> kvm-unit-tests has been configured with --target=kvmtool.
> >>
> >> Example invocation:
> >>
> >> $ ./configure --target=kvmtool
> >> $ make clean && make
> >> $ ./run_scripts.sh
> >>
> >> A custom location for the kvmtool binary can be set using the environment
> >> variable KVMTOOL:
> >>
> >> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh
> >>
> >> Standalone test support is absent, but will be added in subsequent patches.
> > So I skimmed over this, and it's at least better than what I had tried
> > a few years back ;-)
> > And while there might be other ways to sort out command line
> > differences between the two, this fixup_kvmtool_opts() looks like a
> > clever solution to the problem.
> >
> > The only problem with this patch is that it's rather big, I wonder if
> > this could be split up? For instance move any QEMU specific
> > functionality into separate functions first (like run_test_qemu()),
> > also use 'if [ "$TARGET" = "qemu" ]' in this first (or second?) patch.
> > Then later on just add the kvmtool specifics.
> 
> You're right, this patch looks very big and difficult to review, it needs to be
> split into smaller chunks for the next iteration.
> 
> Drew, did you manage to take a quick look at this patch? Do you think this is the
> right direction? I would prefer to hear your opinion about it before I start
> reworking it.

Afraid not. I'll prioritize finishing my review of this series though and
try to do it today or tomorrow.

Thanks,
drew

> 
> Thanks,
> 
> Alex
> 
> >
> > Cheers,
> > Andre
> >
> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >> ---
> >>  scripts/arch-run.bash   |  48 ++++++++++++++++--
> >>  scripts/runtime.bash    |  94 ++++++++++++++++++++++++++++------
> >>  scripts/mkstandalone.sh |   5 ++
> >>  arm/run                 | 110 ++++++++++++++++++++++++----------------
> >>  run_tests.sh            |  11 +++-
> >>  5 files changed, 204 insertions(+), 64 deletions(-)
> >>
> >> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> >> index 8ceed53ed7f8..b916b0e79aca 100644
> >> --- a/scripts/arch-run.bash
> >> +++ b/scripts/arch-run.bash
> >> @@ -69,16 +69,39 @@ run_qemu ()
> >>  	return $ret
> >>  }
> >>  
> >> +run_kvmtool()
> >> +{
> >> +	local stdout errors ret sig
> >> +
> >> +	# kvmtool doesn't allow an initrd argument with --firmware, but configure
> >> +	# sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target.
> >> +
> >> +	# stdout to {stdout}, stderr to $errors and stderr
> >> +	exec {stdout}>&1
> >> +	errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout)
> >> +	ret=$?
> >> +	exec {stdout}>&-
> >> +
> >> +	# ret=0 success, everything else is failure.
> >> +	return $ret
> >> +}
> >> +
> >>  run_test_status ()
> >>  {
> >> -	local stdout ret
> >> +	local stdout ret exit_status
> >>  
> >>  	exec {stdout}>&1
> >> -	lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
> >> +	if [ "$TARGET" = "kvmtool" ]; then
> >> +		lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout))
> >> +		exit_status=0
> >> +	else
> >> +		lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
> >> +		exit_status=1
> >> +	fi
> >>  	ret=$?
> >>  	exec {stdout}>&-
> >>  
> >> -	if [ $ret -eq 1 ]; then
> >> +	if [ $ret -eq $exit_status ]; then
> >>  		testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
> >>  		if [ "$testret" ]; then
> >>  			if [ $testret -eq 1 ]; then
> >> @@ -193,6 +216,25 @@ search_qemu_binary ()
> >>  	export PATH=$save_path
> >>  }
> >>  
> >> +search_kvmtool_binary ()
> >> +{
> >> +	local lkvm kvmtool
> >> +
> >> +	for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do
> >> +		if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then
> >> +			kvmtool="$lkvm"
> >> +			break
> >> +		fi
> >> +	done
> >> +
> >> +	if [ -z "$kvmtool" ]; then
> >> +		echo "A kvmtool binary was not found." >&2
> >> +		echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2
> >> +		return 2
> >> +	fi
> >> +	command -v $kvmtool
> >> +}
> >> +
> >>  initrd_create ()
> >>  {
> >>  	if [ "$ENVIRON_DEFAULT" = "yes" ]; then
> >> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> >> index 132389c7dd59..23b238a6ab6f 100644
> >> --- a/scripts/runtime.bash
> >> +++ b/scripts/runtime.bash
> >> @@ -12,14 +12,19 @@ extract_summary()
> >>      tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/'
> >>  }
> >>  
> >> -# We assume that QEMU is going to work if it tried to load the kernel
> >> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel
> >>  premature_failure()
> >>  {
> >>      local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)"
> >>  
> >> -    echo "$log" | grep "_NO_FILE_4Uhere_" |
> >> -        grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
> >> -        return 1
> >> +    if [ "$TARGET" = "kvmtool" ]; then
> >> +        echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" &&
> >> +            return 1
> >> +    else
> >> +        echo "$log" | grep "_NO_FILE_4Uhere_" |
> >> +            grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
> >> +            return 1
> >> +    fi
> >>  
> >>      RUNTIME_log_stderr <<< "$log"
> >>  
> >> @@ -30,7 +35,14 @@ premature_failure()
> >>  get_cmdline()
> >>  {
> >>      local kernel=$1
> >> -    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
> >> +    local smp_param
> >> +
> >> +    if [ "$TARGET" = "kvmtool" ]; then
> >> +        smp_param="--cpus $smp"
> >> +    else
> >> +        smp_param="-smp $smp"
> >> +    fi
> >> +    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts"
> >>  }
> >>  
> >>  skip_nodefault()
> >> @@ -70,6 +82,35 @@ function find_word()
> >>      grep -Fq " $1 " <<< " $2 "
> >>  }
> >>  
> >> +fixup_kvmtool_opts()
> >> +{
> >> +    local opts=$1
> >> +    local groups=$2
> >> +    local gic
> >> +    local gic_version
> >> +
> >> +    if find_word "pmu" $groups; then
> >> +        opts+=" --pmu"
> >> +    fi
> >> +
> >> +    if find_word "its" $groups; then
> >> +        gic_version=3
> >> +        gic="gicv3-its"
> >> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
> >> +        gic_version="${BASH_REMATCH[1]}"
> >> +        gic="gicv$gic_version"
> >> +    fi
> >> +
> >> +    if [ -n "$gic" ]; then
> >> +        opts=${opts/-machine gic-version=$gic_version/}
> >> +        opts+=" --irqchip=$gic"
> >> +    fi
> >> +
> >> +    opts=${opts/-append/--params}
> >> +
> >> +    echo "$opts"
> >> +}
> >> +
> >>  function run()
> >>  {
> >>      local testname="$1"
> >> @@ -105,7 +146,12 @@ function run()
> >>          return 2
> >>      fi
> >>  
> >> -    if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
> >> +    if [ "$TARGET" = "kvmtool" ]; then
> >> +        if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then
> >> +            print_result "SKIP" $testname "" "$accel not supported by kvmtool"
> >> +            return 2
> >> +        fi
> >> +    elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
> >>          print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL"
> >>          return 2
> >>      elif [ -n "$ACCEL" ]; then
> >> @@ -126,6 +172,10 @@ function run()
> >>          done
> >>      fi
> >>  
> >> +    if [ "$TARGET" = "kvmtool" ]; then
> >> +        opts=$(fixup_kvmtool_opts "$opts" "$groups")
> >> +    fi
> >> +
> >>      last_line=$(premature_failure > >(tail -1)) && {
> >>          print_result "SKIP" $testname "" "$last_line"
> >>          return 77
> >> @@ -165,13 +215,25 @@ function run()
> >>  #
> >>  # Probe for MAX_SMP, in case it's less than the number of host cpus.
> >>  #
> >> -# This probing currently only works for ARM, as x86 bails on another
> >> -# error first. Also, this probing isn't necessary for any ARM hosts
> >> -# running kernels later than v4.3, i.e. those including ef748917b52
> >> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
> >> -# point when maintaining the while loop gets too tiresome, we can
> >> -# just remove it...
> >> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> >> -		|& grep -qi 'exceeds max CPUs'; do
> >> -	MAX_SMP=$((MAX_SMP >> 1))
> >> -done
> >> +# This probing currently only works for ARM, as x86 bails on another error
> >> +# first. Also, this probing isn't necessary for any ARM hosts running kernels
> >> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove
> >> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop
> >> +# gets too tiresome, we can just remove it...
> >> +#
> >> +# We don't need this check for kvmtool, as kvmtool will automatically limit the
> >> +# number of VCPUs to what the host supports instead of exiting with an error.
> >> +# kvmtool prints a message when that happens, but it's harmless and the chance
> >> +# of running a kernel so old that the number of VCPUs is smaller than the number
> >> +# of physical CPUs is vanishingly small.
> >> +#
> >> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0,
> >> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test
> >> +# is running on a recent x86 machine, there's a fairly good chance that more
> >> +# than 8 logical CPUs are available.
> >> +if [ "$TARGET" = "qemu" ]; then
> >> +    while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> >> +            |& grep -qi 'exceeds max CPUs'; do
> >> +        MAX_SMP=$((MAX_SMP >> 1))
> >> +    done
> >> +fi
> >> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> >> index cefdec30cb33..16f461c06842 100755
> >> --- a/scripts/mkstandalone.sh
> >> +++ b/scripts/mkstandalone.sh
> >> @@ -95,6 +95,11 @@ function mkstandalone()
> >>  	echo Written $standalone.
> >>  }
> >>  
> >> +if [ "$TARGET" = "kvmtool" ]; then
> >> +	echo "Standalone tests not supported with kvmtool"
> >> +	exit 2
> >> +fi
> >> +
> >>  if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then
> >>  	echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2
> >>  	exit 2
> >> diff --git a/arm/run b/arm/run
> >> index a390ca5ae0ba..cc5890e7fec4 100755
> >> --- a/arm/run
> >> +++ b/arm/run
> >> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then
> >>  	source config.mak
> >>  	source scripts/arch-run.bash
> >>  fi
> >> -processor="$PROCESSOR"
> >>  
> >> -ACCEL=$(get_qemu_accelerator) ||
> >> -	exit $?
> >> +run_test_qemu()
> >> +{
> >> +    processor="$PROCESSOR"
> >>  
> >> -qemu=$(search_qemu_binary) ||
> >> -	exit $?
> >> +    ACCEL=$(get_qemu_accelerator) ||
> >> +        exit $?
> >>  
> >> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
> >> -	echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
> >> -	exit 2
> >> -fi
> >> +    qemu=$(search_qemu_binary) ||
> >> +        exit $?
> >>  
> >> -M='-machine virt'
> >> +    if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
> >> +        echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
> >> +        exit 2
> >> +    fi
> >>  
> >> -if [ "$ACCEL" = "kvm" ]; then
> >> -	if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
> >> -		M+=',gic-version=host'
> >> -	fi
> >> -	if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
> >> -		processor="host"
> >> -		if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
> >> -			processor+=",aarch64=off"
> >> -		fi
> >> -	fi
> >> -fi
> >> +    M='-machine virt'
> >>  
> >> -if [ "$ARCH" = "arm" ]; then
> >> -	M+=",highmem=off"
> >> -fi
> >> +    if [ "$ACCEL" = "kvm" ]; then
> >> +        if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
> >> +            M+=',gic-version=host'
> >> +        fi
> >> +        if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
> >> +            processor="host"
> >> +            if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
> >> +                processor+=",aarch64=off"
> >> +            fi
> >> +        fi
> >> +    fi
> >>  
> >> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
> >> -	echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
> >> -	exit 2
> >> -fi
> >> +    if [ "$ARCH" = "arm" ]; then
> >> +        M+=",highmem=off"
> >> +    fi
> >>  
> >> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
> >> -		| grep backend > /dev/null; then
> >> -	echo "$qemu doesn't support chr-testdev. Exiting."
> >> -	exit 2
> >> -fi
> >> +    if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
> >> +        echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
> >> +        exit 2
> >> +    fi
> >>  
> >> -chr_testdev='-device virtio-serial-device'
> >> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
> >> +    if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
> >> +            | grep backend > /dev/null; then
> >> +        echo "$qemu doesn't support chr-testdev. Exiting."
> >> +        exit 2
> >> +    fi
> >>  
> >> -pci_testdev=
> >> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
> >> -	pci_testdev="-device pci-testdev"
> >> -fi
> >> +    chr_testdev='-device virtio-serial-device'
> >> +    chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
> >> +
> >> +    pci_testdev=
> >> +    if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
> >> +        pci_testdev="-device pci-testdev"
> >> +    fi
> >> +
> >> +    M+=",accel=$ACCEL"
> >> +    command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
> >> +    command+=" -display none -serial stdio -kernel"
> >> +    command="$(migration_cmd) $(timeout_cmd) $command"
> >> +
> >> +    run_qemu $command "$@"
> >> +}
> >> +
> >> +run_test_kvmtool()
> >> +{
> >> +    kvmtool=$(search_kvmtool_binary) ||
> >> +        exit $?
> >>  
> >> -M+=",accel=$ACCEL"
> >> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
> >> -command+=" -display none -serial stdio -kernel"
> >> -command="$(migration_cmd) $(timeout_cmd) $command"
> >> +    local command="$(timeout_cmd) $kvmtool run --firmware "
> >> +    run_test_status $command "$@"
> >> +}
> >>  
> >> -run_qemu $command "$@"
> >> +case "$TARGET" in
> >> +    "qemu")
> >> +        run_test_qemu "$@"
> >> +        ;;
> >> +    "kvmtool")
> >> +        run_test_kvmtool "$@"
> >> +        ;;
> >> +esac
> >> diff --git a/run_tests.sh b/run_tests.sh
> >> index 65108e73a2c0..b010ee3ab348 100755
> >> --- a/run_tests.sh
> >> +++ b/run_tests.sh
> >> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t]
> >>      -t, --tap13     Output test results in TAP format
> >>  
> >>  Set the environment variable QEMU=/path/to/qemu-system-ARCH to
> >> -specify the appropriate qemu binary for ARCH-run.
> >> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool
> >> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool
> >> +can be used to specify a custom location for the kvmtool binary.
> >>  
> >>  EOF
> >>  }
> >> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then
> >>      exit 1
> >>  fi
> >>  
> >> +if [ "$TARGET" = "kvmtool" ]; then
> >> +    if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then
> >> +        echo "kvmtool supports only the kvm accelerator"
> >> +        exit 1
> >> +    fi
> >> +fi
> >> +
> >>  only_tests=""
> >>  args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*`
> >>  [ $? -ne 0 ] && exit 2;
>

Alexandru Elisei Sept. 6, 2021, 11:07 a.m. UTC | #4

Hi Drew,

On 9/6/21 12:01 PM, Andrew Jones wrote:
> On Mon, Sep 06, 2021 at 11:28:28AM +0100, Alexandru Elisei wrote:
>> Hi Andre,
>>
>> Thanks for having a look!
>>
>> On 7/12/21 5:52 PM, Andre Przywara wrote:
>>> On Fri,  2 Jul 2021 17:31:20 +0100
>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>
>>> Hi Alex,
>>>
>>>> Modify run_tests.sh to use kvmtool instead of qemu to run tests when
>>>> kvm-unit-tests has been configured with --target=kvmtool.
>>>>
>>>> Example invocation:
>>>>
>>>> $ ./configure --target=kvmtool
>>>> $ make clean && make
>>>> $ ./run_scripts.sh
>>>>
>>>> A custom location for the kvmtool binary can be set using the environment
>>>> variable KVMTOOL:
>>>>
>>>> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh
>>>>
>>>> Standalone test support is absent, but will be added in subsequent patches.
>>> So I skimmed over this, and it's at least better than what I had tried
>>> a few years back ;-)
>>> And while there might be other ways to sort out command line
>>> differences between the two, this fixup_kvmtool_opts() looks like a
>>> clever solution to the problem.
>>>
>>> The only problem with this patch is that it's rather big, I wonder if
>>> this could be split up? For instance move any QEMU specific
>>> functionality into separate functions first (like run_test_qemu()),
>>> also use 'if [ "$TARGET" = "qemu" ]' in this first (or second?) patch.
>>> Then later on just add the kvmtool specifics.
>> You're right, this patch looks very big and difficult to review, it needs to be
>> split into smaller chunks for the next iteration.
>>
>> Drew, did you manage to take a quick look at this patch? Do you think this is the
>> right direction? I would prefer to hear your opinion about it before I start
>> reworking it.
> Afraid not. I'll prioritize finishing my review of this series though and
> try to do it today or tomorrow.

Sounds great, thank you!

Thanks,

Alex

>
> Thanks,
> drew
>
>> Thanks,
>>
>> Alex
>>
>>> Cheers,
>>> Andre
>>>
>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>> ---
>>>>  scripts/arch-run.bash   |  48 ++++++++++++++++--
>>>>  scripts/runtime.bash    |  94 ++++++++++++++++++++++++++++------
>>>>  scripts/mkstandalone.sh |   5 ++
>>>>  arm/run                 | 110 ++++++++++++++++++++++++----------------
>>>>  run_tests.sh            |  11 +++-
>>>>  5 files changed, 204 insertions(+), 64 deletions(-)
>>>>
>>>> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
>>>> index 8ceed53ed7f8..b916b0e79aca 100644
>>>> --- a/scripts/arch-run.bash
>>>> +++ b/scripts/arch-run.bash
>>>> @@ -69,16 +69,39 @@ run_qemu ()
>>>>  	return $ret
>>>>  }
>>>>  
>>>> +run_kvmtool()
>>>> +{
>>>> +	local stdout errors ret sig
>>>> +
>>>> +	# kvmtool doesn't allow an initrd argument with --firmware, but configure
>>>> +	# sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target.
>>>> +
>>>> +	# stdout to {stdout}, stderr to $errors and stderr
>>>> +	exec {stdout}>&1
>>>> +	errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout)
>>>> +	ret=$?
>>>> +	exec {stdout}>&-
>>>> +
>>>> +	# ret=0 success, everything else is failure.
>>>> +	return $ret
>>>> +}
>>>> +
>>>>  run_test_status ()
>>>>  {
>>>> -	local stdout ret
>>>> +	local stdout ret exit_status
>>>>  
>>>>  	exec {stdout}>&1
>>>> -	lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
>>>> +	if [ "$TARGET" = "kvmtool" ]; then
>>>> +		lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout))
>>>> +		exit_status=0
>>>> +	else
>>>> +		lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
>>>> +		exit_status=1
>>>> +	fi
>>>>  	ret=$?
>>>>  	exec {stdout}>&-
>>>>  
>>>> -	if [ $ret -eq 1 ]; then
>>>> +	if [ $ret -eq $exit_status ]; then
>>>>  		testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
>>>>  		if [ "$testret" ]; then
>>>>  			if [ $testret -eq 1 ]; then
>>>> @@ -193,6 +216,25 @@ search_qemu_binary ()
>>>>  	export PATH=$save_path
>>>>  }
>>>>  
>>>> +search_kvmtool_binary ()
>>>> +{
>>>> +	local lkvm kvmtool
>>>> +
>>>> +	for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do
>>>> +		if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then
>>>> +			kvmtool="$lkvm"
>>>> +			break
>>>> +		fi
>>>> +	done
>>>> +
>>>> +	if [ -z "$kvmtool" ]; then
>>>> +		echo "A kvmtool binary was not found." >&2
>>>> +		echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2
>>>> +		return 2
>>>> +	fi
>>>> +	command -v $kvmtool
>>>> +}
>>>> +
>>>>  initrd_create ()
>>>>  {
>>>>  	if [ "$ENVIRON_DEFAULT" = "yes" ]; then
>>>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>>>> index 132389c7dd59..23b238a6ab6f 100644
>>>> --- a/scripts/runtime.bash
>>>> +++ b/scripts/runtime.bash
>>>> @@ -12,14 +12,19 @@ extract_summary()
>>>>      tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/'
>>>>  }
>>>>  
>>>> -# We assume that QEMU is going to work if it tried to load the kernel
>>>> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel
>>>>  premature_failure()
>>>>  {
>>>>      local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)"
>>>>  
>>>> -    echo "$log" | grep "_NO_FILE_4Uhere_" |
>>>> -        grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
>>>> -        return 1
>>>> +    if [ "$TARGET" = "kvmtool" ]; then
>>>> +        echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" &&
>>>> +            return 1
>>>> +    else
>>>> +        echo "$log" | grep "_NO_FILE_4Uhere_" |
>>>> +            grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
>>>> +            return 1
>>>> +    fi
>>>>  
>>>>      RUNTIME_log_stderr <<< "$log"
>>>>  
>>>> @@ -30,7 +35,14 @@ premature_failure()
>>>>  get_cmdline()
>>>>  {
>>>>      local kernel=$1
>>>> -    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
>>>> +    local smp_param
>>>> +
>>>> +    if [ "$TARGET" = "kvmtool" ]; then
>>>> +        smp_param="--cpus $smp"
>>>> +    else
>>>> +        smp_param="-smp $smp"
>>>> +    fi
>>>> +    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts"
>>>>  }
>>>>  
>>>>  skip_nodefault()
>>>> @@ -70,6 +82,35 @@ function find_word()
>>>>      grep -Fq " $1 " <<< " $2 "
>>>>  }
>>>>  
>>>> +fixup_kvmtool_opts()
>>>> +{
>>>> +    local opts=$1
>>>> +    local groups=$2
>>>> +    local gic
>>>> +    local gic_version
>>>> +
>>>> +    if find_word "pmu" $groups; then
>>>> +        opts+=" --pmu"
>>>> +    fi
>>>> +
>>>> +    if find_word "its" $groups; then
>>>> +        gic_version=3
>>>> +        gic="gicv3-its"
>>>> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
>>>> +        gic_version="${BASH_REMATCH[1]}"
>>>> +        gic="gicv$gic_version"
>>>> +    fi
>>>> +
>>>> +    if [ -n "$gic" ]; then
>>>> +        opts=${opts/-machine gic-version=$gic_version/}
>>>> +        opts+=" --irqchip=$gic"
>>>> +    fi
>>>> +
>>>> +    opts=${opts/-append/--params}
>>>> +
>>>> +    echo "$opts"
>>>> +}
>>>> +
>>>>  function run()
>>>>  {
>>>>      local testname="$1"
>>>> @@ -105,7 +146,12 @@ function run()
>>>>          return 2
>>>>      fi
>>>>  
>>>> -    if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
>>>> +    if [ "$TARGET" = "kvmtool" ]; then
>>>> +        if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then
>>>> +            print_result "SKIP" $testname "" "$accel not supported by kvmtool"
>>>> +            return 2
>>>> +        fi
>>>> +    elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
>>>>          print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL"
>>>>          return 2
>>>>      elif [ -n "$ACCEL" ]; then
>>>> @@ -126,6 +172,10 @@ function run()
>>>>          done
>>>>      fi
>>>>  
>>>> +    if [ "$TARGET" = "kvmtool" ]; then
>>>> +        opts=$(fixup_kvmtool_opts "$opts" "$groups")
>>>> +    fi
>>>> +
>>>>      last_line=$(premature_failure > >(tail -1)) && {
>>>>          print_result "SKIP" $testname "" "$last_line"
>>>>          return 77
>>>> @@ -165,13 +215,25 @@ function run()
>>>>  #
>>>>  # Probe for MAX_SMP, in case it's less than the number of host cpus.
>>>>  #
>>>> -# This probing currently only works for ARM, as x86 bails on another
>>>> -# error first. Also, this probing isn't necessary for any ARM hosts
>>>> -# running kernels later than v4.3, i.e. those including ef748917b52
>>>> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
>>>> -# point when maintaining the while loop gets too tiresome, we can
>>>> -# just remove it...
>>>> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
>>>> -		|& grep -qi 'exceeds max CPUs'; do
>>>> -	MAX_SMP=$((MAX_SMP >> 1))
>>>> -done
>>>> +# This probing currently only works for ARM, as x86 bails on another error
>>>> +# first. Also, this probing isn't necessary for any ARM hosts running kernels
>>>> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove
>>>> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop
>>>> +# gets too tiresome, we can just remove it...
>>>> +#
>>>> +# We don't need this check for kvmtool, as kvmtool will automatically limit the
>>>> +# number of VCPUs to what the host supports instead of exiting with an error.
>>>> +# kvmtool prints a message when that happens, but it's harmless and the chance
>>>> +# of running a kernel so old that the number of VCPUs is smaller than the number
>>>> +# of physical CPUs is vanishingly small.
>>>> +#
>>>> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0,
>>>> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test
>>>> +# is running on a recent x86 machine, there's a fairly good chance that more
>>>> +# than 8 logical CPUs are available.
>>>> +if [ "$TARGET" = "qemu" ]; then
>>>> +    while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
>>>> +            |& grep -qi 'exceeds max CPUs'; do
>>>> +        MAX_SMP=$((MAX_SMP >> 1))
>>>> +    done
>>>> +fi
>>>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>>>> index cefdec30cb33..16f461c06842 100755
>>>> --- a/scripts/mkstandalone.sh
>>>> +++ b/scripts/mkstandalone.sh
>>>> @@ -95,6 +95,11 @@ function mkstandalone()
>>>>  	echo Written $standalone.
>>>>  }
>>>>  
>>>> +if [ "$TARGET" = "kvmtool" ]; then
>>>> +	echo "Standalone tests not supported with kvmtool"
>>>> +	exit 2
>>>> +fi
>>>> +
>>>>  if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then
>>>>  	echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2
>>>>  	exit 2
>>>> diff --git a/arm/run b/arm/run
>>>> index a390ca5ae0ba..cc5890e7fec4 100755
>>>> --- a/arm/run
>>>> +++ b/arm/run
>>>> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then
>>>>  	source config.mak
>>>>  	source scripts/arch-run.bash
>>>>  fi
>>>> -processor="$PROCESSOR"
>>>>  
>>>> -ACCEL=$(get_qemu_accelerator) ||
>>>> -	exit $?
>>>> +run_test_qemu()
>>>> +{
>>>> +    processor="$PROCESSOR"
>>>>  
>>>> -qemu=$(search_qemu_binary) ||
>>>> -	exit $?
>>>> +    ACCEL=$(get_qemu_accelerator) ||
>>>> +        exit $?
>>>>  
>>>> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
>>>> -	echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
>>>> -	exit 2
>>>> -fi
>>>> +    qemu=$(search_qemu_binary) ||
>>>> +        exit $?
>>>>  
>>>> -M='-machine virt'
>>>> +    if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
>>>> +        echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
>>>> +        exit 2
>>>> +    fi
>>>>  
>>>> -if [ "$ACCEL" = "kvm" ]; then
>>>> -	if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
>>>> -		M+=',gic-version=host'
>>>> -	fi
>>>> -	if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
>>>> -		processor="host"
>>>> -		if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
>>>> -			processor+=",aarch64=off"
>>>> -		fi
>>>> -	fi
>>>> -fi
>>>> +    M='-machine virt'
>>>>  
>>>> -if [ "$ARCH" = "arm" ]; then
>>>> -	M+=",highmem=off"
>>>> -fi
>>>> +    if [ "$ACCEL" = "kvm" ]; then
>>>> +        if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
>>>> +            M+=',gic-version=host'
>>>> +        fi
>>>> +        if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
>>>> +            processor="host"
>>>> +            if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
>>>> +                processor+=",aarch64=off"
>>>> +            fi
>>>> +        fi
>>>> +    fi
>>>>  
>>>> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
>>>> -	echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
>>>> -	exit 2
>>>> -fi
>>>> +    if [ "$ARCH" = "arm" ]; then
>>>> +        M+=",highmem=off"
>>>> +    fi
>>>>  
>>>> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
>>>> -		| grep backend > /dev/null; then
>>>> -	echo "$qemu doesn't support chr-testdev. Exiting."
>>>> -	exit 2
>>>> -fi
>>>> +    if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
>>>> +        echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
>>>> +        exit 2
>>>> +    fi
>>>>  
>>>> -chr_testdev='-device virtio-serial-device'
>>>> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
>>>> +    if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
>>>> +            | grep backend > /dev/null; then
>>>> +        echo "$qemu doesn't support chr-testdev. Exiting."
>>>> +        exit 2
>>>> +    fi
>>>>  
>>>> -pci_testdev=
>>>> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
>>>> -	pci_testdev="-device pci-testdev"
>>>> -fi
>>>> +    chr_testdev='-device virtio-serial-device'
>>>> +    chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
>>>> +
>>>> +    pci_testdev=
>>>> +    if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
>>>> +        pci_testdev="-device pci-testdev"
>>>> +    fi
>>>> +
>>>> +    M+=",accel=$ACCEL"
>>>> +    command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
>>>> +    command+=" -display none -serial stdio -kernel"
>>>> +    command="$(migration_cmd) $(timeout_cmd) $command"
>>>> +
>>>> +    run_qemu $command "$@"
>>>> +}
>>>> +
>>>> +run_test_kvmtool()
>>>> +{
>>>> +    kvmtool=$(search_kvmtool_binary) ||
>>>> +        exit $?
>>>>  
>>>> -M+=",accel=$ACCEL"
>>>> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
>>>> -command+=" -display none -serial stdio -kernel"
>>>> -command="$(migration_cmd) $(timeout_cmd) $command"
>>>> +    local command="$(timeout_cmd) $kvmtool run --firmware "
>>>> +    run_test_status $command "$@"
>>>> +}
>>>>  
>>>> -run_qemu $command "$@"
>>>> +case "$TARGET" in
>>>> +    "qemu")
>>>> +        run_test_qemu "$@"
>>>> +        ;;
>>>> +    "kvmtool")
>>>> +        run_test_kvmtool "$@"
>>>> +        ;;
>>>> +esac
>>>> diff --git a/run_tests.sh b/run_tests.sh
>>>> index 65108e73a2c0..b010ee3ab348 100755
>>>> --- a/run_tests.sh
>>>> +++ b/run_tests.sh
>>>> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t]
>>>>      -t, --tap13     Output test results in TAP format
>>>>  
>>>>  Set the environment variable QEMU=/path/to/qemu-system-ARCH to
>>>> -specify the appropriate qemu binary for ARCH-run.
>>>> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool
>>>> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool
>>>> +can be used to specify a custom location for the kvmtool binary.
>>>>  
>>>>  EOF
>>>>  }
>>>> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then
>>>>      exit 1
>>>>  fi
>>>>  
>>>> +if [ "$TARGET" = "kvmtool" ]; then
>>>> +    if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then
>>>> +        echo "kvmtool supports only the kvm accelerator"
>>>> +        exit 1
>>>> +    fi
>>>> +fi
>>>> +
>>>>  only_tests=""
>>>>  args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*`
>>>>  [ $? -ne 0 ] && exit 2;

Andrew Jones Sept. 7, 2021, 10:17 a.m. UTC | #5

On Fri, Jul 02, 2021 at 05:31:20PM +0100, Alexandru Elisei wrote:
> Modify run_tests.sh to use kvmtool instead of qemu to run tests when
> kvm-unit-tests has been configured with --target=kvmtool.
> 
> Example invocation:
> 
> $ ./configure --target=kvmtool
> $ make clean && make
> $ ./run_scripts.sh
> 
> A custom location for the kvmtool binary can be set using the environment
> variable KVMTOOL:
> 
> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh
> 
> Standalone test support is absent, but will be added in subsequent patches.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  scripts/arch-run.bash   |  48 ++++++++++++++++--
>  scripts/runtime.bash    |  94 ++++++++++++++++++++++++++++------
>  scripts/mkstandalone.sh |   5 ++
>  arm/run                 | 110 ++++++++++++++++++++++++----------------
>  run_tests.sh            |  11 +++-
>  5 files changed, 204 insertions(+), 64 deletions(-)
> 
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index 8ceed53ed7f8..b916b0e79aca 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -69,16 +69,39 @@ run_qemu ()
>  	return $ret
>  }
>  
> +run_kvmtool()
> +{
> +	local stdout errors ret sig
> +
> +	# kvmtool doesn't allow an initrd argument with --firmware, but configure
> +	# sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target.
> +
> +	# stdout to {stdout}, stderr to $errors and stderr
> +	exec {stdout}>&1
> +	errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout)
> +	ret=$?
> +	exec {stdout}>&-
> +
> +	# ret=0 success, everything else is failure.
> +	return $ret
> +}
> +
>  run_test_status ()
>  {
> -	local stdout ret
> +	local stdout ret exit_status
>  
>  	exec {stdout}>&1
> -	lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
> +	if [ "$TARGET" = "kvmtool" ]; then
> +		lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout))
> +		exit_status=0
> +	else
> +		lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
> +		exit_status=1
> +	fi

Now that we're adding a second VMM, let's assume that we may want to add
more. (Actually, I have low priority aspirations to try and also use a
Rust VMM, for example). To prepare for another VMM, everywhere we add a
kvmtool case like this, we should use a case statement instead, e.g.

   case "$TARGET" in
   qemu)
     lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
     exit_status=1
     ;;
   kvmtool)
     lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout))
     exit_status=0
   *) echo "$BASH_SOURCE $LINENO"; exit 2;;
   esac

This means we also need to modify configure to unconditionally do

  echo "TARGET=$target" >> config.mak

Right now it only does that for arm/arm64.

>  	ret=$?
>  	exec {stdout}>&-
>  
> -	if [ $ret -eq 1 ]; then
> +	if [ $ret -eq $exit_status ]; then
>  		testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
>  		if [ "$testret" ]; then
>  			if [ $testret -eq 1 ]; then
> @@ -193,6 +216,25 @@ search_qemu_binary ()
>  	export PATH=$save_path
>  }
>  
> +search_kvmtool_binary ()
> +{
> +	local lkvm kvmtool
> +
> +	for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do
> +		if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then
> +			kvmtool="$lkvm"
> +			break
> +		fi
> +	done
> +
> +	if [ -z "$kvmtool" ]; then
> +		echo "A kvmtool binary was not found." >&2
> +		echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2
> +		return 2
> +	fi
> +	command -v $kvmtool
> +}
> +
>  initrd_create ()
>  {
>  	if [ "$ENVIRON_DEFAULT" = "yes" ]; then
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index 132389c7dd59..23b238a6ab6f 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -12,14 +12,19 @@ extract_summary()
>      tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/'
>  }
>  
> -# We assume that QEMU is going to work if it tried to load the kernel
> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel

assume that the VMM

>  premature_failure()
>  {
>      local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)"
>  
> -    echo "$log" | grep "_NO_FILE_4Uhere_" |
> -        grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
> -        return 1
> +    if [ "$TARGET" = "kvmtool" ]; then
> +        echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" &&
> +            return 1
> +    else
> +        echo "$log" | grep "_NO_FILE_4Uhere_" |
> +            grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
> +            return 1
> +    fi

Another place for a switch.

>  
>      RUNTIME_log_stderr <<< "$log"
>  
> @@ -30,7 +35,14 @@ premature_failure()
>  get_cmdline()
>  {
>      local kernel=$1
> -    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
> +    local smp_param
> +
> +    if [ "$TARGET" = "kvmtool" ]; then
> +        smp_param="--cpus $smp"
> +    else
> +        smp_param="-smp $smp"
> +    fi

switch

> +    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts"
>  }
>  
>  skip_nodefault()
> @@ -70,6 +82,35 @@ function find_word()
>      grep -Fq " $1 " <<< " $2 "
>  }
>  
> +fixup_kvmtool_opts()
> +{
> +    local opts=$1
> +    local groups=$2
> +    local gic
> +    local gic_version
> +
> +    if find_word "pmu" $groups; then
> +        opts+=" --pmu"
> +    fi
> +
> +    if find_word "its" $groups; then
> +        gic_version=3
> +        gic="gicv3-its"
> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
> +        gic_version="${BASH_REMATCH[1]}"
> +        gic="gicv$gic_version"
> +    fi
> +
> +    if [ -n "$gic" ]; then
> +        opts=${opts/-machine gic-version=$gic_version/}
> +        opts+=" --irqchip=$gic"
> +    fi
> +
> +    opts=${opts/-append/--params}
> +
> +    echo "$opts"
> +}

Hmm, I don't think we want to write a QEMU parameter translator for
all other VMMs, and all other VMM architectures, that we want to
support. I think we should add new "extra_params" variables to the
unittest configuration instead, e.g. "kvmtool_params", where the
extra parameters can be listed correctly and explicitly. While at
it, I would create an alias for "extra_params", which would be
"qemu_params" allowing unittests that support more than one VMM
to clearly show what's what.

> +
>  function run()
>  {
>      local testname="$1"
> @@ -105,7 +146,12 @@ function run()
>          return 2
>      fi
>  
> -    if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
> +    if [ "$TARGET" = "kvmtool" ]; then
> +        if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then
> +            print_result "SKIP" $testname "" "$accel not supported by kvmtool"
> +            return 2
> +        fi
> +    elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then

switch

>          print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL"
>          return 2
>      elif [ -n "$ACCEL" ]; then
> @@ -126,6 +172,10 @@ function run()
>          done
>      fi
>  
> +    if [ "$TARGET" = "kvmtool" ]; then
> +        opts=$(fixup_kvmtool_opts "$opts" "$groups")
> +    fi
> +
>      last_line=$(premature_failure > >(tail -1)) && {
>          print_result "SKIP" $testname "" "$last_line"
>          return 77
> @@ -165,13 +215,25 @@ function run()
>  #
>  # Probe for MAX_SMP, in case it's less than the number of host cpus.
>  #
> -# This probing currently only works for ARM, as x86 bails on another
> -# error first. Also, this probing isn't necessary for any ARM hosts
> -# running kernels later than v4.3, i.e. those including ef748917b52
> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
> -# point when maintaining the while loop gets too tiresome, we can
> -# just remove it...
> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> -		|& grep -qi 'exceeds max CPUs'; do
> -	MAX_SMP=$((MAX_SMP >> 1))
> -done
> +# This probing currently only works for ARM, as x86 bails on another error
> +# first. Also, this probing isn't necessary for any ARM hosts running kernels
> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove
> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop
> +# gets too tiresome, we can just remove it...
> +#
> +# We don't need this check for kvmtool, as kvmtool will automatically limit the
> +# number of VCPUs to what the host supports instead of exiting with an error.
> +# kvmtool prints a message when that happens, but it's harmless and the chance
> +# of running a kernel so old that the number of VCPUs is smaller than the number
> +# of physical CPUs is vanishingly small.
> +#
> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0,
> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test
> +# is running on a recent x86 machine, there's a fairly good chance that more
> +# than 8 logical CPUs are available.
> +if [ "$TARGET" = "qemu" ]; then
> +    while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
> +            |& grep -qi 'exceeds max CPUs'; do
> +        MAX_SMP=$((MAX_SMP >> 1))
> +    done
> +fi
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index cefdec30cb33..16f461c06842 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -95,6 +95,11 @@ function mkstandalone()
>  	echo Written $standalone.
>  }
>  
> +if [ "$TARGET" = "kvmtool" ]; then
> +	echo "Standalone tests not supported with kvmtool"
> +	exit 2
> +fi
> +
>  if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then
>  	echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2
>  	exit 2
> diff --git a/arm/run b/arm/run
> index a390ca5ae0ba..cc5890e7fec4 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then
>  	source config.mak
>  	source scripts/arch-run.bash
>  fi
> -processor="$PROCESSOR"
>  
> -ACCEL=$(get_qemu_accelerator) ||
> -	exit $?
> +run_test_qemu()
> +{
> +    processor="$PROCESSOR"
>  
> -qemu=$(search_qemu_binary) ||
> -	exit $?
> +    ACCEL=$(get_qemu_accelerator) ||
> +        exit $?
>  
> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
> -	echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
> -	exit 2
> -fi
> +    qemu=$(search_qemu_binary) ||
> +        exit $?
>  
> -M='-machine virt'
> +    if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
> +        echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
> +        exit 2
> +    fi
>  
> -if [ "$ACCEL" = "kvm" ]; then
> -	if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
> -		M+=',gic-version=host'
> -	fi
> -	if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
> -		processor="host"
> -		if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
> -			processor+=",aarch64=off"
> -		fi
> -	fi
> -fi
> +    M='-machine virt'
>  
> -if [ "$ARCH" = "arm" ]; then
> -	M+=",highmem=off"
> -fi
> +    if [ "$ACCEL" = "kvm" ]; then
> +        if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
> +            M+=',gic-version=host'
> +        fi
> +        if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
> +            processor="host"
> +            if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
> +                processor+=",aarch64=off"
> +            fi
> +        fi
> +    fi
>  
> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
> -	echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
> -	exit 2
> -fi
> +    if [ "$ARCH" = "arm" ]; then
> +        M+=",highmem=off"
> +    fi
>  
> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
> -		| grep backend > /dev/null; then
> -	echo "$qemu doesn't support chr-testdev. Exiting."
> -	exit 2
> -fi
> +    if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
> +        echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
> +        exit 2
> +    fi
>  
> -chr_testdev='-device virtio-serial-device'
> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
> +    if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
> +            | grep backend > /dev/null; then
> +        echo "$qemu doesn't support chr-testdev. Exiting."
> +        exit 2
> +    fi
>  
> -pci_testdev=
> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
> -	pci_testdev="-device pci-testdev"
> -fi
> +    chr_testdev='-device virtio-serial-device'
> +    chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
> +
> +    pci_testdev=
> +    if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
> +        pci_testdev="-device pci-testdev"
> +    fi
> +
> +    M+=",accel=$ACCEL"
> +    command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
> +    command+=" -display none -serial stdio -kernel"
> +    command="$(migration_cmd) $(timeout_cmd) $command"
> +
> +    run_qemu $command "$@"
> +}
> +
> +run_test_kvmtool()
> +{
> +    kvmtool=$(search_kvmtool_binary) ||
> +        exit $?
>  
> -M+=",accel=$ACCEL"
> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
> -command+=" -display none -serial stdio -kernel"
> -command="$(migration_cmd) $(timeout_cmd) $command"
> +    local command="$(timeout_cmd) $kvmtool run --firmware "
> +    run_test_status $command "$@"
> +}
>  
> -run_qemu $command "$@"
> +case "$TARGET" in
> +    "qemu")
> +        run_test_qemu "$@"
> +        ;;
> +    "kvmtool")
> +        run_test_kvmtool "$@"
> +        ;;
> +esac

Hey, here's a switch :-) But I'd add a default case that errors out for
good measure.

> diff --git a/run_tests.sh b/run_tests.sh
> index 65108e73a2c0..b010ee3ab348 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t]
>      -t, --tap13     Output test results in TAP format
>  
>  Set the environment variable QEMU=/path/to/qemu-system-ARCH to
> -specify the appropriate qemu binary for ARCH-run.
> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool
> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool
> +can be used to specify a custom location for the kvmtool binary.

How about changing this last sentence (as a separate patch first) in usage
to

ENVIRONMENT
    QEMU            Path to QEMU binary for ARCH-run
    ACCEL           QEMU accelerator to use, e.g. "kvm", "hvf" or "tcg"
    TIMEOUT         Timeout duration for the timeout(1) command

And then in this patch adds

    KVMTOOL         Path to KVMTOOL binary for ARCH-run

>  
>  EOF
>  }
> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then
>      exit 1
>  fi
>  
> +if [ "$TARGET" = "kvmtool" ]; then
> +    if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then
> +        echo "kvmtool supports only the kvm accelerator"
> +        exit 1
> +    fi
> +fi

I think this belongs in arm/run, because run_tests.sh isn't always used to
run the tests.

> +
>  only_tests=""
>  args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*`
>  [ $? -ne 0 ] && exit 2;
> -- 
> 2.32.0
> 

Thanks,
drew

Alexandru Elisei Sept. 8, 2021, 2:33 p.m. UTC | #6

Hi Drew,

Thanks for having a look so quickly!

On 9/7/21 11:17 AM, Andrew Jones wrote:
> On Fri, Jul 02, 2021 at 05:31:20PM +0100, Alexandru Elisei wrote:
>> Modify run_tests.sh to use kvmtool instead of qemu to run tests when
>> kvm-unit-tests has been configured with --target=kvmtool.
>>
>> Example invocation:
>>
>> $ ./configure --target=kvmtool
>> $ make clean && make
>> $ ./run_scripts.sh
>>
>> A custom location for the kvmtool binary can be set using the environment
>> variable KVMTOOL:
>>
>> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh
>>
>> Standalone test support is absent, but will be added in subsequent patches.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  scripts/arch-run.bash   |  48 ++++++++++++++++--
>>  scripts/runtime.bash    |  94 ++++++++++++++++++++++++++++------
>>  scripts/mkstandalone.sh |   5 ++
>>  arm/run                 | 110 ++++++++++++++++++++++++----------------
>>  run_tests.sh            |  11 +++-
>>  5 files changed, 204 insertions(+), 64 deletions(-)
>>
>> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
>> index 8ceed53ed7f8..b916b0e79aca 100644
>> --- a/scripts/arch-run.bash
>> +++ b/scripts/arch-run.bash
>> @@ -69,16 +69,39 @@ run_qemu ()
>>  	return $ret
>>  }
>>  
>> +run_kvmtool()
>> +{
>> +	local stdout errors ret sig
>> +
>> +	# kvmtool doesn't allow an initrd argument with --firmware, but configure
>> +	# sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target.
>> +
>> +	# stdout to {stdout}, stderr to $errors and stderr
>> +	exec {stdout}>&1
>> +	errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout)
>> +	ret=$?
>> +	exec {stdout}>&-
>> +
>> +	# ret=0 success, everything else is failure.
>> +	return $ret
>> +}
>> +
>>  run_test_status ()
>>  {
>> -	local stdout ret
>> +	local stdout ret exit_status
>>  
>>  	exec {stdout}>&1
>> -	lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
>> +	if [ "$TARGET" = "kvmtool" ]; then
>> +		lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout))
>> +		exit_status=0
>> +	else
>> +		lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
>> +		exit_status=1
>> +	fi
> Now that we're adding a second VMM, let's assume that we may want to add
> more. (Actually, I have low priority aspirations to try and also use a
> Rust VMM, for example). To prepare for another VMM, everywhere we add a
> kvmtool case like this, we should use a case statement instead, e.g.
>
>    case "$TARGET" in
>    qemu)
>      lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout))
>      exit_status=1
>      ;;
>    kvmtool)
>      lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout))
>      exit_status=0
>    *) echo "$BASH_SOURCE $LINENO"; exit 2;;
>    esac
>
> This means we also need to modify configure to unconditionally do
>
>   echo "TARGET=$target" >> config.mak
>
> Right now it only does that for arm/arm64.

Yep, using a case statement makes more sense. I'll update everywhere you've
pointed out.

I'll modify configure to set the TARGET variable unconditionally in config.mak.

>
>>  	ret=$?
>>  	exec {stdout}>&-
>>  
>> -	if [ $ret -eq 1 ]; then
>> +	if [ $ret -eq $exit_status ]; then
>>  		testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
>>  		if [ "$testret" ]; then
>>  			if [ $testret -eq 1 ]; then
>> @@ -193,6 +216,25 @@ search_qemu_binary ()
>>  	export PATH=$save_path
>>  }
>>  
>> +search_kvmtool_binary ()
>> +{
>> +	local lkvm kvmtool
>> +
>> +	for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do
>> +		if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then
>> +			kvmtool="$lkvm"
>> +			break
>> +		fi
>> +	done
>> +
>> +	if [ -z "$kvmtool" ]; then
>> +		echo "A kvmtool binary was not found." >&2
>> +		echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2
>> +		return 2
>> +	fi
>> +	command -v $kvmtool
>> +}
>> +
>>  initrd_create ()
>>  {
>>  	if [ "$ENVIRON_DEFAULT" = "yes" ]; then
>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>> index 132389c7dd59..23b238a6ab6f 100644
>> --- a/scripts/runtime.bash
>> +++ b/scripts/runtime.bash
>> @@ -12,14 +12,19 @@ extract_summary()
>>      tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/'
>>  }
>>  
>> -# We assume that QEMU is going to work if it tried to load the kernel
>> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel
> assume that the VMM

Got it.

>
>>  premature_failure()
>>  {
>>      local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)"
>>  
>> -    echo "$log" | grep "_NO_FILE_4Uhere_" |
>> -        grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
>> -        return 1
>> +    if [ "$TARGET" = "kvmtool" ]; then
>> +        echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" &&
>> +            return 1
>> +    else
>> +        echo "$log" | grep "_NO_FILE_4Uhere_" |
>> +            grep -q -e "could not \(load\|open\) kernel" -e "error loading" &&
>> +            return 1
>> +    fi
> Another place for a switch.
>
>>  
>>      RUNTIME_log_stderr <<< "$log"
>>  
>> @@ -30,7 +35,14 @@ premature_failure()
>>  get_cmdline()
>>  {
>>      local kernel=$1
>> -    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
>> +    local smp_param
>> +
>> +    if [ "$TARGET" = "kvmtool" ]; then
>> +        smp_param="--cpus $smp"
>> +    else
>> +        smp_param="-smp $smp"
>> +    fi
> switch
>
>> +    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts"
>>  }
>>  
>>  skip_nodefault()
>> @@ -70,6 +82,35 @@ function find_word()
>>      grep -Fq " $1 " <<< " $2 "
>>  }
>>  
>> +fixup_kvmtool_opts()
>> +{
>> +    local opts=$1
>> +    local groups=$2
>> +    local gic
>> +    local gic_version
>> +
>> +    if find_word "pmu" $groups; then
>> +        opts+=" --pmu"
>> +    fi
>> +
>> +    if find_word "its" $groups; then
>> +        gic_version=3
>> +        gic="gicv3-its"
>> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
>> +        gic_version="${BASH_REMATCH[1]}"
>> +        gic="gicv$gic_version"
>> +    fi
>> +
>> +    if [ -n "$gic" ]; then
>> +        opts=${opts/-machine gic-version=$gic_version/}
>> +        opts+=" --irqchip=$gic"
>> +    fi
>> +
>> +    opts=${opts/-append/--params}
>> +
>> +    echo "$opts"
>> +}
> Hmm, I don't think we want to write a QEMU parameter translator for
> all other VMMs, and all other VMM architectures, that we want to
> support. I think we should add new "extra_params" variables to the
> unittest configuration instead, e.g. "kvmtool_params", where the
> extra parameters can be listed correctly and explicitly. While at
> it, I would create an alias for "extra_params", which would be
> "qemu_params" allowing unittests that support more than one VMM
> to clearly show what's what.

I agree, this is a much better idea than a parameter translator. Using a dedicated
variable in unittests.cfg will make it easier for new tests to get support for all
VMMs (for example, writing a list of parameters in unittests.cfg should be easier
than digging through the scripts to figure exactly how and where to add a
translation for a new parameter), and it allow us to express parameters for other
VMMs which don't have a direct correspondent in qemu.

By creating an alias, do you mean replacing extra_params with qemu_params in
arm/unittests.cfg? Or something else?

>
>> +
>>  function run()
>>  {
>>      local testname="$1"
>> @@ -105,7 +146,12 @@ function run()
>>          return 2
>>      fi
>>  
>> -    if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
>> +    if [ "$TARGET" = "kvmtool" ]; then
>> +        if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then
>> +            print_result "SKIP" $testname "" "$accel not supported by kvmtool"
>> +            return 2
>> +        fi
>> +    elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then
> switch
>
>>          print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL"
>>          return 2
>>      elif [ -n "$ACCEL" ]; then
>> @@ -126,6 +172,10 @@ function run()
>>          done
>>      fi
>>  
>> +    if [ "$TARGET" = "kvmtool" ]; then
>> +        opts=$(fixup_kvmtool_opts "$opts" "$groups")
>> +    fi
>> +
>>      last_line=$(premature_failure > >(tail -1)) && {
>>          print_result "SKIP" $testname "" "$last_line"
>>          return 77
>> @@ -165,13 +215,25 @@ function run()
>>  #
>>  # Probe for MAX_SMP, in case it's less than the number of host cpus.
>>  #
>> -# This probing currently only works for ARM, as x86 bails on another
>> -# error first. Also, this probing isn't necessary for any ARM hosts
>> -# running kernels later than v4.3, i.e. those including ef748917b52
>> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some
>> -# point when maintaining the while loop gets too tiresome, we can
>> -# just remove it...
>> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
>> -		|& grep -qi 'exceeds max CPUs'; do
>> -	MAX_SMP=$((MAX_SMP >> 1))
>> -done
>> +# This probing currently only works for ARM, as x86 bails on another error
>> +# first. Also, this probing isn't necessary for any ARM hosts running kernels
>> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove
>> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop
>> +# gets too tiresome, we can just remove it...
>> +#
>> +# We don't need this check for kvmtool, as kvmtool will automatically limit the
>> +# number of VCPUs to what the host supports instead of exiting with an error.
>> +# kvmtool prints a message when that happens, but it's harmless and the chance
>> +# of running a kernel so old that the number of VCPUs is smaller than the number
>> +# of physical CPUs is vanishingly small.
>> +#
>> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0,
>> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test
>> +# is running on a recent x86 machine, there's a fairly good chance that more
>> +# than 8 logical CPUs are available.
>> +if [ "$TARGET" = "qemu" ]; then
>> +    while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \
>> +            |& grep -qi 'exceeds max CPUs'; do
>> +        MAX_SMP=$((MAX_SMP >> 1))
>> +    done
>> +fi
>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>> index cefdec30cb33..16f461c06842 100755
>> --- a/scripts/mkstandalone.sh
>> +++ b/scripts/mkstandalone.sh
>> @@ -95,6 +95,11 @@ function mkstandalone()
>>  	echo Written $standalone.
>>  }
>>  
>> +if [ "$TARGET" = "kvmtool" ]; then
>> +	echo "Standalone tests not supported with kvmtool"
>> +	exit 2
>> +fi
>> +
>>  if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then
>>  	echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2
>>  	exit 2
>> diff --git a/arm/run b/arm/run
>> index a390ca5ae0ba..cc5890e7fec4 100755
>> --- a/arm/run
>> +++ b/arm/run
>> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then
>>  	source config.mak
>>  	source scripts/arch-run.bash
>>  fi
>> -processor="$PROCESSOR"
>>  
>> -ACCEL=$(get_qemu_accelerator) ||
>> -	exit $?
>> +run_test_qemu()
>> +{
>> +    processor="$PROCESSOR"
>>  
>> -qemu=$(search_qemu_binary) ||
>> -	exit $?
>> +    ACCEL=$(get_qemu_accelerator) ||
>> +        exit $?
>>  
>> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
>> -	echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
>> -	exit 2
>> -fi
>> +    qemu=$(search_qemu_binary) ||
>> +        exit $?
>>  
>> -M='-machine virt'
>> +    if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
>> +        echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
>> +        exit 2
>> +    fi
>>  
>> -if [ "$ACCEL" = "kvm" ]; then
>> -	if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
>> -		M+=',gic-version=host'
>> -	fi
>> -	if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
>> -		processor="host"
>> -		if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
>> -			processor+=",aarch64=off"
>> -		fi
>> -	fi
>> -fi
>> +    M='-machine virt'
>>  
>> -if [ "$ARCH" = "arm" ]; then
>> -	M+=",highmem=off"
>> -fi
>> +    if [ "$ACCEL" = "kvm" ]; then
>> +        if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then
>> +            M+=',gic-version=host'
>> +        fi
>> +        if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then
>> +            processor="host"
>> +            if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then
>> +                processor+=",aarch64=off"
>> +            fi
>> +        fi
>> +    fi
>>  
>> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
>> -	echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
>> -	exit 2
>> -fi
>> +    if [ "$ARCH" = "arm" ]; then
>> +        M+=",highmem=off"
>> +    fi
>>  
>> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
>> -		| grep backend > /dev/null; then
>> -	echo "$qemu doesn't support chr-testdev. Exiting."
>> -	exit 2
>> -fi
>> +    if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then
>> +        echo "$qemu doesn't support virtio-console for chr-testdev. Exiting."
>> +        exit 2
>> +    fi
>>  
>> -chr_testdev='-device virtio-serial-device'
>> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
>> +    if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
>> +            | grep backend > /dev/null; then
>> +        echo "$qemu doesn't support chr-testdev. Exiting."
>> +        exit 2
>> +    fi
>>  
>> -pci_testdev=
>> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
>> -	pci_testdev="-device pci-testdev"
>> -fi
>> +    chr_testdev='-device virtio-serial-device'
>> +    chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
>> +
>> +    pci_testdev=
>> +    if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
>> +        pci_testdev="-device pci-testdev"
>> +    fi
>> +
>> +    M+=",accel=$ACCEL"
>> +    command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
>> +    command+=" -display none -serial stdio -kernel"
>> +    command="$(migration_cmd) $(timeout_cmd) $command"
>> +
>> +    run_qemu $command "$@"
>> +}
>> +
>> +run_test_kvmtool()
>> +{
>> +    kvmtool=$(search_kvmtool_binary) ||
>> +        exit $?
>>  
>> -M+=",accel=$ACCEL"
>> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev"
>> -command+=" -display none -serial stdio -kernel"
>> -command="$(migration_cmd) $(timeout_cmd) $command"
>> +    local command="$(timeout_cmd) $kvmtool run --firmware "
>> +    run_test_status $command "$@"
>> +}
>>  
>> -run_qemu $command "$@"
>> +case "$TARGET" in
>> +    "qemu")
>> +        run_test_qemu "$@"
>> +        ;;
>> +    "kvmtool")
>> +        run_test_kvmtool "$@"
>> +        ;;
>> +esac
> Hey, here's a switch :-) But I'd add a default case that errors out for
> good measure.

Yep, I think I made a mental note to replace if/else with this kind of switch
statement, but it slipped my mind in the end.

>
>> diff --git a/run_tests.sh b/run_tests.sh
>> index 65108e73a2c0..b010ee3ab348 100755
>> --- a/run_tests.sh
>> +++ b/run_tests.sh
>> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t]
>>      -t, --tap13     Output test results in TAP format
>>  
>>  Set the environment variable QEMU=/path/to/qemu-system-ARCH to
>> -specify the appropriate qemu binary for ARCH-run.
>> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool
>> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool
>> +can be used to specify a custom location for the kvmtool binary.
> How about changing this last sentence (as a separate patch first) in usage
> to
>
> ENVIRONMENT
>     QEMU            Path to QEMU binary for ARCH-run
>     ACCEL           QEMU accelerator to use, e.g. "kvm", "hvf" or "tcg"
>     TIMEOUT         Timeout duration for the timeout(1) command
>
> And then in this patch adds
>
>     KVMTOOL         Path to KVMTOOL binary for ARCH-run

Sounds good, will do.

>
>>  
>>  EOF
>>  }
>> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then
>>      exit 1
>>  fi
>>  
>> +if [ "$TARGET" = "kvmtool" ]; then
>> +    if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then
>> +        echo "kvmtool supports only the kvm accelerator"
>> +        exit 1
>> +    fi
>> +fi
> I think this belongs in arm/run, because run_tests.sh isn't always used to
> run the tests.

Ok, I'll have a look and move this where it belongs.

Thanks,

Alex

>
>> +
>>  only_tests=""
>>  args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*`
>>  [ $? -ne 0 ] && exit 2;
>> -- 
>> 2.32.0
>>
> Thanks,
> drew
>

Andrew Jones Sept. 8, 2021, 3:09 p.m. UTC | #7

On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote:
...
> >> +fixup_kvmtool_opts()
> >> +{
> >> +    local opts=$1
> >> +    local groups=$2
> >> +    local gic
> >> +    local gic_version
> >> +
> >> +    if find_word "pmu" $groups; then
> >> +        opts+=" --pmu"
> >> +    fi
> >> +
> >> +    if find_word "its" $groups; then
> >> +        gic_version=3
> >> +        gic="gicv3-its"
> >> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
> >> +        gic_version="${BASH_REMATCH[1]}"
> >> +        gic="gicv$gic_version"
> >> +    fi
> >> +
> >> +    if [ -n "$gic" ]; then
> >> +        opts=${opts/-machine gic-version=$gic_version/}
> >> +        opts+=" --irqchip=$gic"
> >> +    fi
> >> +
> >> +    opts=${opts/-append/--params}
> >> +
> >> +    echo "$opts"
> >> +}
> > Hmm, I don't think we want to write a QEMU parameter translator for
> > all other VMMs, and all other VMM architectures, that we want to
> > support. I think we should add new "extra_params" variables to the
> > unittest configuration instead, e.g. "kvmtool_params", where the
> > extra parameters can be listed correctly and explicitly. While at
> > it, I would create an alias for "extra_params", which would be
> > "qemu_params" allowing unittests that support more than one VMM
> > to clearly show what's what.
> 
> I agree, this is a much better idea than a parameter translator. Using a dedicated
> variable in unittests.cfg will make it easier for new tests to get support for all
> VMMs (for example, writing a list of parameters in unittests.cfg should be easier
> than digging through the scripts to figure exactly how and where to add a
> translation for a new parameter), and it allow us to express parameters for other
> VMMs which don't have a direct correspondent in qemu.
> 
> By creating an alias, do you mean replacing extra_params with qemu_params in
> arm/unittests.cfg? Or something else?

Probably something like this

diff --git a/scripts/common.bash b/scripts/common.bash
index 7b983f7d6dd6..e5119ff216e5 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -37,7 +37,12 @@ function for_each_unittest()
                elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
                        smp=${BASH_REMATCH[1]}
                elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
-                       opts=${BASH_REMATCH[1]}
+               elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
+                       qemu_opts=${BASH_REMATCH[1]}
+               elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then
+                       qemu_opts=${BASH_REMATCH[1]}
+               elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then
+                       kvmtool_opts=${BASH_REMATCH[1]}
                elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then
                        groups=${BASH_REMATCH[1]}
                elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then

and all other changes needed to support the s/opts/qemu_opts/ change
should work. Also, an addition to the unittests.cfg documentation.

The above diff doesn't consider that a unittests.cfg file could have
both an 'extra_params' and a 'qemu_params' field, but I'm not sure
we care about that. Users should read the documentation and we
should review changes to the committed unittests.cfg files to avoid
that.

Thanks,
drew

Alexandru Elisei Sept. 8, 2021, 3:46 p.m. UTC | #8

Hi Drew,

On 9/8/21 4:09 PM, Andrew Jones wrote:
> On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote:
> ...
>>>> +fixup_kvmtool_opts()
>>>> +{
>>>> +    local opts=$1
>>>> +    local groups=$2
>>>> +    local gic
>>>> +    local gic_version
>>>> +
>>>> +    if find_word "pmu" $groups; then
>>>> +        opts+=" --pmu"
>>>> +    fi
>>>> +
>>>> +    if find_word "its" $groups; then
>>>> +        gic_version=3
>>>> +        gic="gicv3-its"
>>>> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
>>>> +        gic_version="${BASH_REMATCH[1]}"
>>>> +        gic="gicv$gic_version"
>>>> +    fi
>>>> +
>>>> +    if [ -n "$gic" ]; then
>>>> +        opts=${opts/-machine gic-version=$gic_version/}
>>>> +        opts+=" --irqchip=$gic"
>>>> +    fi
>>>> +
>>>> +    opts=${opts/-append/--params}
>>>> +
>>>> +    echo "$opts"
>>>> +}
>>> Hmm, I don't think we want to write a QEMU parameter translator for
>>> all other VMMs, and all other VMM architectures, that we want to
>>> support. I think we should add new "extra_params" variables to the
>>> unittest configuration instead, e.g. "kvmtool_params", where the
>>> extra parameters can be listed correctly and explicitly. While at
>>> it, I would create an alias for "extra_params", which would be
>>> "qemu_params" allowing unittests that support more than one VMM
>>> to clearly show what's what.
>> I agree, this is a much better idea than a parameter translator. Using a dedicated
>> variable in unittests.cfg will make it easier for new tests to get support for all
>> VMMs (for example, writing a list of parameters in unittests.cfg should be easier
>> than digging through the scripts to figure exactly how and where to add a
>> translation for a new parameter), and it allow us to express parameters for other
>> VMMs which don't have a direct correspondent in qemu.
>>
>> By creating an alias, do you mean replacing extra_params with qemu_params in
>> arm/unittests.cfg? Or something else?
> Probably something like this
>
> diff --git a/scripts/common.bash b/scripts/common.bash
> index 7b983f7d6dd6..e5119ff216e5 100644
> --- a/scripts/common.bash
> +++ b/scripts/common.bash
> @@ -37,7 +37,12 @@ function for_each_unittest()
>                 elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
>                         smp=${BASH_REMATCH[1]}
>                 elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
> -                       opts=${BASH_REMATCH[1]}
> +               elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
> +                       qemu_opts=${BASH_REMATCH[1]}
> +               elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then
> +                       qemu_opts=${BASH_REMATCH[1]}
> +               elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then
> +                       kvmtool_opts=${BASH_REMATCH[1]}
>                 elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then
>                         groups=${BASH_REMATCH[1]}
>                 elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
>
> and all other changes needed to support the s/opts/qemu_opts/ change
> should work. Also, an addition to the unittests.cfg documentation.

Got it, replace extra_opts with qemu_opts in the scripts.

Yes, the documentation for unittests.cfg (at the top of the file) should
definitely be updated to document the new configuration option, kvmtool_params.

>
> The above diff doesn't consider that a unittests.cfg file could have
> both an 'extra_params' and a 'qemu_params' field, but I'm not sure
> we care about that. Users should read the documentation and we
> should review changes to the committed unittests.cfg files to avoid
> that.

What do you feel about renaming extra_params -> qemu_params in unittests.cfg? I'm
thinking it would make the usage clearer, improve consistency (we would have
qemu_params and kvmtool_params, instead of extra_params and kvmtool_params), and
remove any confusions regarding when they are used (I can see someone thinking
that extra_params are used all the time, and are appended to kvmtool_params when
--target=kvmtool). On the other hand, this could be problematic for people using
out-of-tree scripts that parse the unittest.cfg file for whatever reason (are
there people that do that?).

Thanks,

Alex

>
> Thanks,
> drew
>

Andrew Jones Sept. 8, 2021, 3:49 p.m. UTC | #9

On Wed, Sep 08, 2021 at 04:46:19PM +0100, Alexandru Elisei wrote:
> Hi Drew,
> 
> On 9/8/21 4:09 PM, Andrew Jones wrote:
> > On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote:
> > ...
> >>>> +fixup_kvmtool_opts()
> >>>> +{
> >>>> +    local opts=$1
> >>>> +    local groups=$2
> >>>> +    local gic
> >>>> +    local gic_version
> >>>> +
> >>>> +    if find_word "pmu" $groups; then
> >>>> +        opts+=" --pmu"
> >>>> +    fi
> >>>> +
> >>>> +    if find_word "its" $groups; then
> >>>> +        gic_version=3
> >>>> +        gic="gicv3-its"
> >>>> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
> >>>> +        gic_version="${BASH_REMATCH[1]}"
> >>>> +        gic="gicv$gic_version"
> >>>> +    fi
> >>>> +
> >>>> +    if [ -n "$gic" ]; then
> >>>> +        opts=${opts/-machine gic-version=$gic_version/}
> >>>> +        opts+=" --irqchip=$gic"
> >>>> +    fi
> >>>> +
> >>>> +    opts=${opts/-append/--params}
> >>>> +
> >>>> +    echo "$opts"
> >>>> +}
> >>> Hmm, I don't think we want to write a QEMU parameter translator for
> >>> all other VMMs, and all other VMM architectures, that we want to
> >>> support. I think we should add new "extra_params" variables to the
> >>> unittest configuration instead, e.g. "kvmtool_params", where the
> >>> extra parameters can be listed correctly and explicitly. While at
> >>> it, I would create an alias for "extra_params", which would be
> >>> "qemu_params" allowing unittests that support more than one VMM
> >>> to clearly show what's what.
> >> I agree, this is a much better idea than a parameter translator. Using a dedicated
> >> variable in unittests.cfg will make it easier for new tests to get support for all
> >> VMMs (for example, writing a list of parameters in unittests.cfg should be easier
> >> than digging through the scripts to figure exactly how and where to add a
> >> translation for a new parameter), and it allow us to express parameters for other
> >> VMMs which don't have a direct correspondent in qemu.
> >>
> >> By creating an alias, do you mean replacing extra_params with qemu_params in
> >> arm/unittests.cfg? Or something else?
> > Probably something like this
> >
> > diff --git a/scripts/common.bash b/scripts/common.bash
> > index 7b983f7d6dd6..e5119ff216e5 100644
> > --- a/scripts/common.bash
> > +++ b/scripts/common.bash
> > @@ -37,7 +37,12 @@ function for_each_unittest()
> >                 elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
> >                         smp=${BASH_REMATCH[1]}
> >                 elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
> > -                       opts=${BASH_REMATCH[1]}
> > +               elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
> > +                       qemu_opts=${BASH_REMATCH[1]}
> > +               elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then
> > +                       qemu_opts=${BASH_REMATCH[1]}
> > +               elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then
> > +                       kvmtool_opts=${BASH_REMATCH[1]}
> >                 elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then
> >                         groups=${BASH_REMATCH[1]}
> >                 elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
> >
> > and all other changes needed to support the s/opts/qemu_opts/ change
> > should work. Also, an addition to the unittests.cfg documentation.
> 
> Got it, replace extra_opts with qemu_opts in the scripts.
> 
> Yes, the documentation for unittests.cfg (at the top of the file) should
> definitely be updated to document the new configuration option, kvmtool_params.
> 
> >
> > The above diff doesn't consider that a unittests.cfg file could have
> > both an 'extra_params' and a 'qemu_params' field, but I'm not sure
> > we care about that. Users should read the documentation and we
> > should review changes to the committed unittests.cfg files to avoid
> > that.
> 
> What do you feel about renaming extra_params -> qemu_params in unittests.cfg?

Yes, that's what I would expect the patch to do.

> I'm
> thinking it would make the usage clearer, improve consistency (we would have
> qemu_params and kvmtool_params, instead of extra_params and kvmtool_params), and
> remove any confusions regarding when they are used (I can see someone thinking
> that extra_params are used all the time, and are appended to kvmtool_params when
> --target=kvmtool). On the other hand, this could be problematic for people using
> out-of-tree scripts that parse the unittest.cfg file for whatever reason (are
> there people that do that?).

I'm not as worried about that as about people using out-of-tree
unittests.cfg files that will break when the 'extra_params' field
disappears. That's why I suggested to make 'extra_params' an alias.

Thanks,
drew

> 
> Thanks,
> 
> Alex
> 
> >
> > Thanks,
> > drew
> >
>

Alexandru Elisei Sept. 9, 2021, 11:33 a.m. UTC | #10

Hi Drew,

On 9/8/21 4:49 PM, Andrew Jones wrote:
> On Wed, Sep 08, 2021 at 04:46:19PM +0100, Alexandru Elisei wrote:
>> Hi Drew,
>>
>> On 9/8/21 4:09 PM, Andrew Jones wrote:
>>> On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote:
>>> ...
>>>>>> +fixup_kvmtool_opts()
>>>>>> +{
>>>>>> +    local opts=$1
>>>>>> +    local groups=$2
>>>>>> +    local gic
>>>>>> +    local gic_version
>>>>>> +
>>>>>> +    if find_word "pmu" $groups; then
>>>>>> +        opts+=" --pmu"
>>>>>> +    fi
>>>>>> +
>>>>>> +    if find_word "its" $groups; then
>>>>>> +        gic_version=3
>>>>>> +        gic="gicv3-its"
>>>>>> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
>>>>>> +        gic_version="${BASH_REMATCH[1]}"
>>>>>> +        gic="gicv$gic_version"
>>>>>> +    fi
>>>>>> +
>>>>>> +    if [ -n "$gic" ]; then
>>>>>> +        opts=${opts/-machine gic-version=$gic_version/}
>>>>>> +        opts+=" --irqchip=$gic"
>>>>>> +    fi
>>>>>> +
>>>>>> +    opts=${opts/-append/--params}
>>>>>> +
>>>>>> +    echo "$opts"
>>>>>> +}
>>>>> Hmm, I don't think we want to write a QEMU parameter translator for
>>>>> all other VMMs, and all other VMM architectures, that we want to
>>>>> support. I think we should add new "extra_params" variables to the
>>>>> unittest configuration instead, e.g. "kvmtool_params", where the
>>>>> extra parameters can be listed correctly and explicitly. While at
>>>>> it, I would create an alias for "extra_params", which would be
>>>>> "qemu_params" allowing unittests that support more than one VMM
>>>>> to clearly show what's what.
>>>> I agree, this is a much better idea than a parameter translator. Using a dedicated
>>>> variable in unittests.cfg will make it easier for new tests to get support for all
>>>> VMMs (for example, writing a list of parameters in unittests.cfg should be easier
>>>> than digging through the scripts to figure exactly how and where to add a
>>>> translation for a new parameter), and it allow us to express parameters for other
>>>> VMMs which don't have a direct correspondent in qemu.
>>>>
>>>> By creating an alias, do you mean replacing extra_params with qemu_params in
>>>> arm/unittests.cfg? Or something else?
>>> Probably something like this
>>>
>>> diff --git a/scripts/common.bash b/scripts/common.bash
>>> index 7b983f7d6dd6..e5119ff216e5 100644
>>> --- a/scripts/common.bash
>>> +++ b/scripts/common.bash
>>> @@ -37,7 +37,12 @@ function for_each_unittest()
>>>                 elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
>>>                         smp=${BASH_REMATCH[1]}
>>>                 elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
>>> -                       opts=${BASH_REMATCH[1]}
>>> +               elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
>>> +                       qemu_opts=${BASH_REMATCH[1]}
>>> +               elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then
>>> +                       qemu_opts=${BASH_REMATCH[1]}
>>> +               elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then
>>> +                       kvmtool_opts=${BASH_REMATCH[1]}
>>>                 elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then
>>>                         groups=${BASH_REMATCH[1]}
>>>                 elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
>>>
>>> and all other changes needed to support the s/opts/qemu_opts/ change
>>> should work. Also, an addition to the unittests.cfg documentation.
>> Got it, replace extra_opts with qemu_opts in the scripts.
>>
>> Yes, the documentation for unittests.cfg (at the top of the file) should
>> definitely be updated to document the new configuration option, kvmtool_params.
>>
>>> The above diff doesn't consider that a unittests.cfg file could have
>>> both an 'extra_params' and a 'qemu_params' field, but I'm not sure
>>> we care about that. Users should read the documentation and we
>>> should review changes to the committed unittests.cfg files to avoid
>>> that.
>> What do you feel about renaming extra_params -> qemu_params in unittests.cfg?
> Yes, that's what I would expect the patch to do.
>
>> I'm
>> thinking it would make the usage clearer, improve consistency (we would have
>> qemu_params and kvmtool_params, instead of extra_params and kvmtool_params), and
>> remove any confusions regarding when they are used (I can see someone thinking
>> that extra_params are used all the time, and are appended to kvmtool_params when
>> --target=kvmtool). On the other hand, this could be problematic for people using
>> out-of-tree scripts that parse the unittest.cfg file for whatever reason (are
>> there people that do that?).
> I'm not as worried about that as about people using out-of-tree
> unittests.cfg files that will break when the 'extra_params' field
> disappears. That's why I suggested to make 'extra_params' an alias.

I'm sorry, but I'm still having trouble parsing what alias means in this context.
Do you mean keep extra_params for current tests, encourage qemu_params for new
tests, document that they mean the same thing and going forward qemu_params should
be used?

Thanks,

Alex

>
> Thanks,
> drew
>
>> Thanks,
>>
>> Alex
>>
>>> Thanks,
>>> drew
>>>

Andrew Jones Sept. 9, 2021, 12:49 p.m. UTC | #11

On Thu, Sep 09, 2021 at 12:33:11PM +0100, Alexandru Elisei wrote:
> Hi Drew,
> 
> On 9/8/21 4:49 PM, Andrew Jones wrote:
> > On Wed, Sep 08, 2021 at 04:46:19PM +0100, Alexandru Elisei wrote:
> >> Hi Drew,
> >>
> >> On 9/8/21 4:09 PM, Andrew Jones wrote:
> >>> On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote:
> >>> ...
> >>>>>> +fixup_kvmtool_opts()
> >>>>>> +{
> >>>>>> +    local opts=$1
> >>>>>> +    local groups=$2
> >>>>>> +    local gic
> >>>>>> +    local gic_version
> >>>>>> +
> >>>>>> +    if find_word "pmu" $groups; then
> >>>>>> +        opts+=" --pmu"
> >>>>>> +    fi
> >>>>>> +
> >>>>>> +    if find_word "its" $groups; then
> >>>>>> +        gic_version=3
> >>>>>> +        gic="gicv3-its"
> >>>>>> +    elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then
> >>>>>> +        gic_version="${BASH_REMATCH[1]}"
> >>>>>> +        gic="gicv$gic_version"
> >>>>>> +    fi
> >>>>>> +
> >>>>>> +    if [ -n "$gic" ]; then
> >>>>>> +        opts=${opts/-machine gic-version=$gic_version/}
> >>>>>> +        opts+=" --irqchip=$gic"
> >>>>>> +    fi
> >>>>>> +
> >>>>>> +    opts=${opts/-append/--params}
> >>>>>> +
> >>>>>> +    echo "$opts"
> >>>>>> +}
> >>>>> Hmm, I don't think we want to write a QEMU parameter translator for
> >>>>> all other VMMs, and all other VMM architectures, that we want to
> >>>>> support. I think we should add new "extra_params" variables to the
> >>>>> unittest configuration instead, e.g. "kvmtool_params", where the
> >>>>> extra parameters can be listed correctly and explicitly. While at
> >>>>> it, I would create an alias for "extra_params", which would be
> >>>>> "qemu_params" allowing unittests that support more than one VMM
> >>>>> to clearly show what's what.
> >>>> I agree, this is a much better idea than a parameter translator. Using a dedicated
> >>>> variable in unittests.cfg will make it easier for new tests to get support for all
> >>>> VMMs (for example, writing a list of parameters in unittests.cfg should be easier
> >>>> than digging through the scripts to figure exactly how and where to add a
> >>>> translation for a new parameter), and it allow us to express parameters for other
> >>>> VMMs which don't have a direct correspondent in qemu.
> >>>>
> >>>> By creating an alias, do you mean replacing extra_params with qemu_params in
> >>>> arm/unittests.cfg? Or something else?
> >>> Probably something like this
> >>>
> >>> diff --git a/scripts/common.bash b/scripts/common.bash
> >>> index 7b983f7d6dd6..e5119ff216e5 100644
> >>> --- a/scripts/common.bash
> >>> +++ b/scripts/common.bash
> >>> @@ -37,7 +37,12 @@ function for_each_unittest()
> >>>                 elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
> >>>                         smp=${BASH_REMATCH[1]}
> >>>                 elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
> >>> -                       opts=${BASH_REMATCH[1]}
> >>> +               elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
> >>> +                       qemu_opts=${BASH_REMATCH[1]}
> >>> +               elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then
> >>> +                       qemu_opts=${BASH_REMATCH[1]}
> >>> +               elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then
> >>> +                       kvmtool_opts=${BASH_REMATCH[1]}
> >>>                 elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then
> >>>                         groups=${BASH_REMATCH[1]}
> >>>                 elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
> >>>
> >>> and all other changes needed to support the s/opts/qemu_opts/ change
> >>> should work. Also, an addition to the unittests.cfg documentation.
> >> Got it, replace extra_opts with qemu_opts in the scripts.
> >>
> >> Yes, the documentation for unittests.cfg (at the top of the file) should
> >> definitely be updated to document the new configuration option, kvmtool_params.
> >>
> >>> The above diff doesn't consider that a unittests.cfg file could have
> >>> both an 'extra_params' and a 'qemu_params' field, but I'm not sure
> >>> we care about that. Users should read the documentation and we
> >>> should review changes to the committed unittests.cfg files to avoid
> >>> that.
> >> What do you feel about renaming extra_params -> qemu_params in unittests.cfg?
> > Yes, that's what I would expect the patch to do.
> >
> >> I'm
> >> thinking it would make the usage clearer, improve consistency (we would have
> >> qemu_params and kvmtool_params, instead of extra_params and kvmtool_params), and
> >> remove any confusions regarding when they are used (I can see someone thinking
> >> that extra_params are used all the time, and are appended to kvmtool_params when
> >> --target=kvmtool). On the other hand, this could be problematic for people using
> >> out-of-tree scripts that parse the unittest.cfg file for whatever reason (are
> >> there people that do that?).
> > I'm not as worried about that as about people using out-of-tree
> > unittests.cfg files that will break when the 'extra_params' field
> > disappears. That's why I suggested to make 'extra_params' an alias.
> 
> I'm sorry, but I'm still having trouble parsing what alias means in this context.
> Do you mean keep extra_params for current tests, encourage qemu_params for new
> tests, document that they mean the same thing and going forward qemu_params should
> be used?

Exactly, which just amounts to keeping the parsing line

           elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then
                 qemu_opts=${BASH_REMATCH[1]}

and some documentation changes.

Thanks,
drew

> 
> Thanks,
> 
> Alex
> 
> >
> > Thanks,
> > drew
> >
> >> Thanks,
> >>
> >> Alex
> >>
> >>> Thanks,
> >>> drew
> >>>
>

[kvm-unit-tests,RFC,3/5] run_tests.sh: Add kvmtool support

Commit Message

Comments

Patch