[PATCH kvm-unit-tests] arch-run: Introduce QEMU_ARCH

[PATCH kvm-unit-tests] arch-run: Introduce QEMU_ARCH

[Andrew Jones]

Add QEMU_ARCH, which allows run scripts to specify which architecture
of QEMU should be used. This is useful on AArch64 when running with
KVM and running AArch32 tests. For those tests, we *don't* want to
select the 'arm' QEMU, as would have been selected, but rather the
$HOST ('aarch64') QEMU.

To use this new variable, simply ensure it's set prior to calling
search_qemu_binary().

Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/run               | 4 ++++
 scripts/arch-run.bash | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arm/run b/arm/run
index 0629b69a117c..28a0b4ad2729 100755
--- a/arm/run
+++ b/arm/run
@@ -13,6 +13,10 @@ processor="$PROCESSOR"
 ACCEL=$(get_qemu_accelerator) ||
 	exit $?
 
+if [ "$ACCEL" = "kvm" ]; then
+	QEMU_ARCH=$HOST
+fi
+
 qemu=$(search_qemu_binary) ||
 	exit $?
 
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index aae552321f9b..0dfaf017db0a 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -176,8 +176,10 @@ search_qemu_binary ()
 	local save_path=$PATH
 	local qemucmd qemu
 
+	: "${QEMU_ARCH:=$ARCH_NAME}"
+
 	export PATH=$PATH:/usr/libexec
-	for qemucmd in ${QEMU:-qemu-system-$ARCH_NAME qemu-kvm}; do
+	for qemucmd in ${QEMU:-qemu-system-$QEMU_ARCH qemu-kvm}; do
 		if $qemucmd --help 2>/dev/null | grep -q 'QEMU'; then
 			qemu="$qemucmd"
 			break
-- 
2.34.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH kvm-unit-tests] arch-run: Introduce QEMU_ARCH

[Alexandru Elisei]

Hi,

On Tue, Mar 15, 2022 at 09:01:52AM +0100, Andrew Jones wrote:
> Add QEMU_ARCH, which allows run scripts to specify which architecture
> of QEMU should be used. This is useful on AArch64 when running with
> KVM and running AArch32 tests. For those tests, we *don't* want to
> select the 'arm' QEMU, as would have been selected, but rather the
> $HOST ('aarch64') QEMU.
> 
> To use this new variable, simply ensure it's set prior to calling
> search_qemu_binary().

Looks good, tested on an arm64 machine, with ACCEL set to tcg -
run_tests.sh selects qemu-system-arm; ACCEL unset - run_tests.sh selects
ACCEL=kvm and qemu-system-aarch64; also tested on an x86 machine -
run_tests.sh selects ACCEL=tcg and qemu-system-arm:

Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

One thing I noticed is that if the user sets QEMU=qemu-system-arm on an arm64
machine, run_tests.sh still selects ACCEL=kvm which leads to the following
failure:

SKIP selftest-setup (qemu-system-arm: -accel kvm: invalid accelerator kvm)

I'm not sure if this deserves a fix, if the user set the QEMU variable I
believe it is probable that the user is also aware of the ACCEL variable
and the error message does a good job explaining what is wrong. Just in
case, this is what I did to make kvm-unit-tests pick the right accelerator
(copied-and-pasted the find_word function from scripts/runtime.bash):

diff --git a/arm/run b/arm/run
index 94adcddb7399..b0c9613b8d28 100755
--- a/arm/run
+++ b/arm/run
@@ -10,6 +15,10 @@ if [ -z "$KUT_STANDALONE" ]; then
 fi
 processor="$PROCESSOR"

+if [ -z $ACCEL ] && [ "$HOST" = "aarch64" ] && ! find_word "qemu-system-arm" "$QEMU"; then
+       ACCEL=tcg
+fi
+

Thanks,
Alex

> 
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/run               | 4 ++++
>  scripts/arch-run.bash | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/run b/arm/run
> index 0629b69a117c..28a0b4ad2729 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -13,6 +13,10 @@ processor="$PROCESSOR"
>  ACCEL=$(get_qemu_accelerator) ||
>  	exit $?
>  
> +if [ "$ACCEL" = "kvm" ]; then
> +	QEMU_ARCH=$HOST
> +fi
> +
>  qemu=$(search_qemu_binary) ||
>  	exit $?
>  
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index aae552321f9b..0dfaf017db0a 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -176,8 +176,10 @@ search_qemu_binary ()
>  	local save_path=$PATH
>  	local qemucmd qemu
>  
> +	: "${QEMU_ARCH:=$ARCH_NAME}"
> +
>  	export PATH=$PATH:/usr/libexec
> -	for qemucmd in ${QEMU:-qemu-system-$ARCH_NAME qemu-kvm}; do
> +	for qemucmd in ${QEMU:-qemu-system-$QEMU_ARCH qemu-kvm}; do
>  		if $qemucmd --help 2>/dev/null | grep -q 'QEMU'; then
>  			qemu="$qemucmd"
>  			break
> -- 
> 2.34.1
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH kvm-unit-tests] arch-run: Introduce QEMU_ARCH

[Andrew Jones]

On Tue, Mar 15, 2022 at 12:33:17PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Tue, Mar 15, 2022 at 09:01:52AM +0100, Andrew Jones wrote:
> > Add QEMU_ARCH, which allows run scripts to specify which architecture
> > of QEMU should be used. This is useful on AArch64 when running with
> > KVM and running AArch32 tests. For those tests, we *don't* want to
> > select the 'arm' QEMU, as would have been selected, but rather the
> > $HOST ('aarch64') QEMU.
> > 
> > To use this new variable, simply ensure it's set prior to calling
> > search_qemu_binary().
> 
> Looks good, tested on an arm64 machine, with ACCEL set to tcg -
> run_tests.sh selects qemu-system-arm; ACCEL unset - run_tests.sh selects
> ACCEL=kvm and qemu-system-aarch64; also tested on an x86 machine -
> run_tests.sh selects ACCEL=tcg and qemu-system-arm:
> 
> Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> One thing I noticed is that if the user sets QEMU=qemu-system-arm on an arm64
> machine, run_tests.sh still selects ACCEL=kvm which leads to the following
> failure:
> 
> SKIP selftest-setup (qemu-system-arm: -accel kvm: invalid accelerator kvm)
> 
> I'm not sure if this deserves a fix, if the user set the QEMU variable I
> believe it is probable that the user is also aware of the ACCEL variable
> and the error message does a good job explaining what is wrong.

Yes, we assume the user selected the wrong qemu, rather than assuming the
user didn't expect KVM to be enabled. If we're wrong, then the error
message should hopefully imply to the user that they need to do

 QEMU=qemu-system-arm ACCEL=tcg ...

> Just in
> case, this is what I did to make kvm-unit-tests pick the right accelerator
> (copied-and-pasted the find_word function from scripts/runtime.bash):
> 
> diff --git a/arm/run b/arm/run
> index 94adcddb7399..b0c9613b8d28 100755
> --- a/arm/run
> +++ b/arm/run
> @@ -10,6 +15,10 @@ if [ -z "$KUT_STANDALONE" ]; then
>  fi
>  processor="$PROCESSOR"
> 
> +if [ -z $ACCEL ] && [ "$HOST" = "aarch64" ] && ! find_word "qemu-system-arm" "$QEMU"; then

Instead of find_word,

 [ "$QEMU" ] && [ "$(basename $QEMU)" = "qemu-system-arm" ]

> +       ACCEL=tcg
> +fi
> +

When ACCEL is unset, we currently set it to kvm when we have /dev/kvm and
$HOST == $ARCH_NAME or ($HOST == aarch64 && $ARCH == arm) and tcg
otherwise. Adding logic like the above would allow overriding the
"set to kvm" logic when $QEMU == qemu-system-arm. That makes sense to
me, but we trade one assumption for another. We would now assume that
$QEMU is correct and the user wants to run with TCG, rather than that
$QEMU is wrong and the user wanted to run with KVM.

I think I'd prefer not adding the special case override. I think it's
more likely the user expects to run with KVM when running on an AArch64
host and that they mistakenly selected the wrong qemu, than that they
wanted TCG with qemu-system-arm. We also avoid a few more lines of code
and a change in behavior by maintaining the old assumption.

I've pushed this to arm/queue -- and MR coming up.

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH kvm-unit-tests] arch-run: Introduce QEMU_ARCH

[Alexandru Elisei]

Hi,

On Tue, Mar 15, 2022 at 04:16:30PM +0100, Andrew Jones wrote:
> On Tue, Mar 15, 2022 at 12:33:17PM +0000, Alexandru Elisei wrote:
> > Hi,
> > 
> > On Tue, Mar 15, 2022 at 09:01:52AM +0100, Andrew Jones wrote:
> > > Add QEMU_ARCH, which allows run scripts to specify which architecture
> > > of QEMU should be used. This is useful on AArch64 when running with
> > > KVM and running AArch32 tests. For those tests, we *don't* want to
> > > select the 'arm' QEMU, as would have been selected, but rather the
> > > $HOST ('aarch64') QEMU.
> > > 
> > > To use this new variable, simply ensure it's set prior to calling
> > > search_qemu_binary().
> > 
> > Looks good, tested on an arm64 machine, with ACCEL set to tcg -
> > run_tests.sh selects qemu-system-arm; ACCEL unset - run_tests.sh selects
> > ACCEL=kvm and qemu-system-aarch64; also tested on an x86 machine -
> > run_tests.sh selects ACCEL=tcg and qemu-system-arm:
> > 
> > Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > 
> > One thing I noticed is that if the user sets QEMU=qemu-system-arm on an arm64
> > machine, run_tests.sh still selects ACCEL=kvm which leads to the following
> > failure:
> > 
> > SKIP selftest-setup (qemu-system-arm: -accel kvm: invalid accelerator kvm)
> > 
> > I'm not sure if this deserves a fix, if the user set the QEMU variable I
> > believe it is probable that the user is also aware of the ACCEL variable
> > and the error message does a good job explaining what is wrong.
> 
> Yes, we assume the user selected the wrong qemu, rather than assuming the
> user didn't expect KVM to be enabled. If we're wrong, then the error
> message should hopefully imply to the user that they need to do
> 
>  QEMU=qemu-system-arm ACCEL=tcg ...

Yep, it was very easy to figure out what needs to be done to get the tests
running again.

> 
> > Just in
> > case, this is what I did to make kvm-unit-tests pick the right accelerator
> > (copied-and-pasted the find_word function from scripts/runtime.bash):
> > 
> > diff --git a/arm/run b/arm/run
> > index 94adcddb7399..b0c9613b8d28 100755
> > --- a/arm/run
> > +++ b/arm/run
> > @@ -10,6 +15,10 @@ if [ -z "$KUT_STANDALONE" ]; then
> >  fi
> >  processor="$PROCESSOR"
> > 
> > +if [ -z $ACCEL ] && [ "$HOST" = "aarch64" ] && ! find_word "qemu-system-arm" "$QEMU"; then
> 
> Instead of find_word,
> 
>  [ "$QEMU" ] && [ "$(basename $QEMU)" = "qemu-system-arm" ]
> 
> > +       ACCEL=tcg
> > +fi
> > +
> 
> When ACCEL is unset, we currently set it to kvm when we have /dev/kvm and
> $HOST == $ARCH_NAME or ($HOST == aarch64 && $ARCH == arm) and tcg
> otherwise. Adding logic like the above would allow overriding the
> "set to kvm" logic when $QEMU == qemu-system-arm. That makes sense to
> me, but we trade one assumption for another. We would now assume that
> $QEMU is correct and the user wants to run with TCG, rather than that
> $QEMU is wrong and the user wanted to run with KVM.
> 
> I think I'd prefer not adding the special case override. I think it's
> more likely the user expects to run with KVM when running on an AArch64
> host and that they mistakenly selected the wrong qemu, than that they
> wanted TCG with qemu-system-arm. We also avoid a few more lines of code
> and a change in behavior by maintaining the old assumption.

Well, kvm-unit-tests selects KVM or TCG under the hood without the user
being involved at all. In my opinion, it's slightly better from an
usability perspective for kvm-unit-tests to do its best to run the tests
based on what the user specifically set (QEMU=qemu-system-arm) than fail to
run the tests because of an internal heuristic of which the user might be
entirely ignorant (if arm64 and /dev/kvm is available, pick ACCEL=kvm).

Regardless, I don't have a strong opinion either way, and it's trivial for
a user to figure out that ACCEL=tcg will make the tests run. So from my
side this is mostly academic and the test runner can stay as it is if you
don't see a reason to change it.

Thanks,
Alex
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH kvm-unit-tests] arch-run: Introduce QEMU_ARCH

[Andrew Jones]

On Tue, Mar 15, 2022 at 04:31:34PM +0000, Alexandru Elisei wrote:
> Well, kvm-unit-tests selects KVM or TCG under the hood without the user
> being involved at all.

The under the hood aspect isn't great. It's best for testers to know what
they're testing. It's pretty obvious, though, that if you choose
ARCH != HOST that you'll end up on TCG. And, since KVM has historically
been the primary focus of kvm-unit-tests, then it's probably reasonable
to assume KVM is used when ARCH == HOST. However, we still silently fall
back to TCG, even when ARCH == HOST, if /dev/kvm isn't available! And,
the whole AArch32 guest support on AArch64 hosts with KVM requiring a
different qemu binary muddies things further...

Anyway, I hope serious test runners always specify ACCEL and QEMU to
whatever they plan to test.

> In my opinion, it's slightly better from an
> usability perspective for kvm-unit-tests to do its best to run the tests
> based on what the user specifically set (QEMU=qemu-system-arm) than fail to
> run the tests because of an internal heuristic of which the user might be
> entirely ignorant (if arm64 and /dev/kvm is available, pick ACCEL=kvm).

If you'd like to post a patch for it, then I'd prefer something like
below, which spells out the condition that the override is applied
and also allows $QEMU to be checked by search_qemu_binary() before
using it to make decisions.

Thanks,
drew

diff --git a/arm/run b/arm/run
index 28a0b4ad2729..128489125dcb 100755
--- a/arm/run
+++ b/arm/run
@@ -10,16 +10,24 @@ if [ -z "$KUT_STANDALONE" ]; then
 fi
 processor="$PROCESSOR"
 
-ACCEL=$(get_qemu_accelerator) ||
+accel=$(get_qemu_accelerator) ||
        exit $?
 
-if [ "$ACCEL" = "kvm" ]; then
+if [ "$accel" = "kvm" ]; then
        QEMU_ARCH=$HOST
 fi
 
 qemu=$(search_qemu_binary) ||
        exit $?
 
+if [ "$QEMU" ] && [ -z "$ACCEL" ] &&
+   [ "$HOST" = "aarch64" ] && [ "$ARCH" = "arm" ] &&
+   [ "$(basename $QEMU)" = "qemu-system-arm" ]; then
+       accel=tcg
+fi
+
+ACCEL=$accel
+
 if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then
        echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting."
        exit 2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm