| Message ID | 20220315080152.224606-1-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] arch-run: Introduce QEMU_ARCH | expand |
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 >
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
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
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
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
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(-)