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@xxxxxxx>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>
> 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
