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

[Alexandru Elisei <alexandru.elisei@arm.com>]

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