| Message ID | 20200213143300.32141-3-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | runtime: Allow additional VMM parameters to be provided | expand |
On 13.02.20 15:33, Andrew Jones wrote: > Users may need to temporarily provide additional VMM parameters. > The VMM_PARAMS environment variable provides a way to do that. > We take care to make sure when executing ./run_tests.sh that > the VMM_PARAMS parameters come last, allowing unittests.cfg > parameters to be overridden. However, when running a command > line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override > $VMM_PARAMS, so we ensure that too. While reading this, I was wondering why not simply "$QEMU_PARAMS"?
On 13/02/20 15:33, Andrew Jones wrote: > Users may need to temporarily provide additional VMM parameters. > The VMM_PARAMS environment variable provides a way to do that. > We take care to make sure when executing ./run_tests.sh that > the VMM_PARAMS parameters come last, allowing unittests.cfg > parameters to be overridden. However, when running a command > line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override > $VMM_PARAMS, so we ensure that too. > > Additional QEMU parameters can still be provided by appending > them to the $QEMU environment variable when it provides the > path to QEMU, but as those parameters will then be the first > in the command line they cannot override anything, and may > themselves be overridden. What about looking for "--" and passing to QEMU all parameters after it? Paolo
On Fri, Feb 14, 2020 at 11:14:49AM +0100, David Hildenbrand wrote: > On 13.02.20 15:33, Andrew Jones wrote: > > Users may need to temporarily provide additional VMM parameters. > > The VMM_PARAMS environment variable provides a way to do that. > > We take care to make sure when executing ./run_tests.sh that > > the VMM_PARAMS parameters come last, allowing unittests.cfg > > parameters to be overridden. However, when running a command > > line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override > > $VMM_PARAMS, so we ensure that too. > > While reading this, I was wondering why not simply "$QEMU_PARAMS"? I'd like to slowly move away from assuming QEMU is the VMM. We already have support for kvmtool (to some degree) and I'd like to see that support increase. Also, maybe we'll eventually add support for a rust-vmm reference VMM to drive kvm-unit-tests. IOW, rather than add yet another QEMU named variable I'd like to start a trend of using VMM named variables. Actually, we could add VMM named alternatives for the QEMU named ones we have now and start using them in the scripts. We'd just need to remap the old names for backward compatibility early in the run. Thanks, drew
On 14.02.20 11:30, Andrew Jones wrote: > On Fri, Feb 14, 2020 at 11:14:49AM +0100, David Hildenbrand wrote: >> On 13.02.20 15:33, Andrew Jones wrote: >>> Users may need to temporarily provide additional VMM parameters. >>> The VMM_PARAMS environment variable provides a way to do that. >>> We take care to make sure when executing ./run_tests.sh that >>> the VMM_PARAMS parameters come last, allowing unittests.cfg >>> parameters to be overridden. However, when running a command >>> line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override >>> $VMM_PARAMS, so we ensure that too. >> >> While reading this, I was wondering why not simply "$QEMU_PARAMS"? > > I'd like to slowly move away from assuming QEMU is the VMM. We > already have support for kvmtool (to some degree) and I'd like > to see that support increase. Also, maybe we'll eventually add > support for a rust-vmm reference VMM to drive kvm-unit-tests. > IOW, rather than add yet another QEMU named variable I'd like > to start a trend of using VMM named variables. Actually, we > could add VMM named alternatives for the QEMU named ones we have > now and start using them in the scripts. We'd just need to remap > the old names for backward compatibility early in the run. And we do expect other VMMs to eat the same set of parameters? If it's QEMU specific, I think we should make this clear. But no strong opinion.
On Fri, Feb 14, 2020 at 11:27:17AM +0100, Paolo Bonzini wrote: > On 13/02/20 15:33, Andrew Jones wrote: > > Users may need to temporarily provide additional VMM parameters. > > The VMM_PARAMS environment variable provides a way to do that. > > We take care to make sure when executing ./run_tests.sh that > > the VMM_PARAMS parameters come last, allowing unittests.cfg > > parameters to be overridden. However, when running a command > > line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override > > $VMM_PARAMS, so we ensure that too. > > > > Additional QEMU parameters can still be provided by appending > > them to the $QEMU environment variable when it provides the > > path to QEMU, but as those parameters will then be the first > > in the command line they cannot override anything, and may > > themselves be overridden. > > What about looking for "--" and passing to QEMU all parameters after it? > That was the way we originally planned on doing it when Alex Bennee posted his patch. However since d4d34e648482 ("run_tests: fix command line options handling") the "--" has become the divider between run_tests.sh parameter inputs and individually specified tests. We'd have to change that behavior, potentially breaking command lines, to go back to the "--" approach. Also, the nice thing about using an environment variable is that it works with standalone tests too. VMM_PARAMS="-foo bar" tests/mytest will run the test with "-foo bar" appended to the command line. We could modify mkstandalone.sh to get that feature too (allowing the additional parameters to be given after tests/mytest), but with VMM_PARAMS we don't have to. Thanks, drew
On Fri, Feb 14, 2020 at 11:33:55AM +0100, David Hildenbrand wrote: > On 14.02.20 11:30, Andrew Jones wrote: > > On Fri, Feb 14, 2020 at 11:14:49AM +0100, David Hildenbrand wrote: > >> On 13.02.20 15:33, Andrew Jones wrote: > >>> Users may need to temporarily provide additional VMM parameters. > >>> The VMM_PARAMS environment variable provides a way to do that. > >>> We take care to make sure when executing ./run_tests.sh that > >>> the VMM_PARAMS parameters come last, allowing unittests.cfg > >>> parameters to be overridden. However, when running a command > >>> line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override > >>> $VMM_PARAMS, so we ensure that too. > >> > >> While reading this, I was wondering why not simply "$QEMU_PARAMS"? > > > > I'd like to slowly move away from assuming QEMU is the VMM. We > > already have support for kvmtool (to some degree) and I'd like > > to see that support increase. Also, maybe we'll eventually add > > support for a rust-vmm reference VMM to drive kvm-unit-tests. > > IOW, rather than add yet another QEMU named variable I'd like > > to start a trend of using VMM named variables. Actually, we > > could add VMM named alternatives for the QEMU named ones we have > > now and start using them in the scripts. We'd just need to remap > > the old names for backward compatibility early in the run. > > And we do expect other VMMs to eat the same set of parameters? If it's > QEMU specific, I think we should make this clear. It won't matter. The parameters are VMM specific. Whether or not where they show up on the command line has the same semantics as QEMU could be an issue, but what the parameters are is up to the user, and they should know which vmm they're currently using. kvmtool support isn't complete because the run scripts haven't yet provided a way for non-QEMU vmms to use the unittests.cfg files. Andre had some patches once, though. So maybe someday. Thanks, drew
On 14/02/20 11:38, Andrew Jones wrote: > That was the way we originally planned on doing it when Alex Bennee posted > his patch. However since d4d34e648482 ("run_tests: fix command line > options handling") the "--" has become the divider between run_tests.sh > parameter inputs and individually specified tests. Hmm, more precisely that is how getopt separates options from other parameters. Since we don't expect test names starting with a dash, we could do something like (untested): diff --git a/run_tests.sh b/run_tests.sh index 01e36dc..8b71cf2 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -35,7 +35,20 @@ RUNTIME_arch_run="./$TEST_DIR/run" source scripts/runtime.bash only_tests="" -args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*` +args="" +vmm_args="" +while [ $# -gt 0 ]; do + if test "$1" = --; then + shift + vmm_args=$* + break + else + args="args $1" + shift + fi +done + +args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $args` [ $? -ne 0 ] && exit 2; set -- $args; while [ $# -gt 0 ]; do > will run the test with "-foo bar" appended to the command line. We could > modify mkstandalone.sh to get that feature too (allowing the additional > parameters to be given after tests/mytest), but with VMM_PARAMS we don't > have to. Yes, having consistency between standalone and run_tests.sh is a good thing. Paolo
On Fri, Feb 14, 2020 at 11:50:08AM +0100, Paolo Bonzini wrote: > On 14/02/20 11:38, Andrew Jones wrote: > > That was the way we originally planned on doing it when Alex Bennee posted > > his patch. However since d4d34e648482 ("run_tests: fix command line > > options handling") the "--" has become the divider between run_tests.sh > > parameter inputs and individually specified tests. > > Hmm, more precisely that is how getopt separates options from other > parameters. > > Since we don't expect test names starting with a dash, we could do > something like (untested): > > diff --git a/run_tests.sh b/run_tests.sh > index 01e36dc..8b71cf2 100755 > --- a/run_tests.sh > +++ b/run_tests.sh > @@ -35,7 +35,20 @@ RUNTIME_arch_run="./$TEST_DIR/run" > source scripts/runtime.bash > > only_tests="" > -args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*` > +args="" > +vmm_args="" > +while [ $# -gt 0 ]; do > + if test "$1" = --; then > + shift > + vmm_args=$* > + break > + else > + args="args $1" > + shift > + fi > +done > + > +args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $args` > [ $? -ne 0 ] && exit 2; > set -- $args; > while [ $# -gt 0 ]; do Unfortunately it regresses the current command line. Here's what I tested Before ------ $ ./run_tests.sh -j 2 -v pmu psci VMM_PARAMS= TESTNAME=pmu TIMEOUT=90s ACCEL= ./arm/run arm/pmu.flat -smp 1 VMM_PARAMS= TESTNAME=psci TIMEOUT=90s ACCEL= ./arm/run arm/psci.flat -smp $MAX_SMP PASS pmu (3 tests) PASS psci (4 tests) $ ./run_tests.sh pmu psci -j 2 -v VMM_PARAMS= TESTNAME=pmu TIMEOUT=90s ACCEL= ./arm/run arm/pmu.flat -smp 1 VMM_PARAMS= TESTNAME=psci TIMEOUT=90s ACCEL= ./arm/run arm/psci.flat -smp $MAX_SMP PASS pmu (3 tests) PASS psci (4 tests) $ ./run_tests.sh -j 2 -v -- pmu psci VMM_PARAMS= TESTNAME=pmu TIMEOUT=90s ACCEL= ./arm/run arm/pmu.flat -smp 1 VMM_PARAMS= TESTNAME=psci TIMEOUT=90s ACCEL= ./arm/run arm/psci.flat -smp $MAX_SMP PASS pmu (3 tests) PASS psci (4 tests) After ----- $ ./run_tests.sh -j 2 -v pmu psci PASS psci (4 tests) $ ./run_tests.sh pmu psci -j 2 -v $ (no output) $ ./run_tests.sh -j 2 -v -- pmu psci $ (no output) > > > will run the test with "-foo bar" appended to the command line. We could > > modify mkstandalone.sh to get that feature too (allowing the additional > > parameters to be given after tests/mytest), but with VMM_PARAMS we don't > > have to. > > Yes, having consistency between standalone and run_tests.sh is a good thing. > So should we: 1) try to get "--" working, but also keep the environment variable as an alternative which works with standalone? 2) drop the environment variable, get "--" working and modify mkstandalone.sh? 3) drop the environment variable, get "--" working, but forget about standalone? 4) just keep the VMM_PARAMS approach and forget about "--"? Thanks, drew
On 14/02/20 12:50, Andrew Jones wrote: > After > ----- > > $ ./run_tests.sh -j 2 -v pmu psci > PASS psci (4 tests) > > $ ./run_tests.sh pmu psci -j 2 -v > $ > > (no output) Bugs. :) > $ ./run_tests.sh -j 2 -v -- pmu psci > $ > > (no output) This is intended, it shouldn't be a big deal because we don't have tests starting with a dash. Paolo
diff --git a/README.md b/README.md index 396fbf809a4e..834d6202ac97 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,11 @@ environment variable: QEMU=/tmp/qemu/x86_64-softmmu/qemu-system-x86_64 ./x86-run ./x86/msr.flat +If QEMU additional parameters are needed for all tests, then they may be +provided in the VMM_PARAMS environment variable: + + VMM_PARAMS="-additional parameters -go here" ./run_tests.sh + To select an accelerator, for example "kvm" or "tcg", specify the ACCEL=name environment variable: diff --git a/arm/run b/arm/run index 277db9bb4a02..a8a93591b825 100755 --- a/arm/run +++ b/arm/run @@ -60,7 +60,9 @@ fi M+=",accel=$ACCEL" command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" -command+=" -display none -serial stdio -kernel" +command+=" -display none -serial stdio" +command+=" $VMM_PARAMS" +command+=" -kernel" command="$(timeout_cmd) $command" run_qemu $command "$@" diff --git a/powerpc/run b/powerpc/run index 597ab96ed8a8..b07aa18f26bf 100755 --- a/powerpc/run +++ b/powerpc/run @@ -23,7 +23,9 @@ fi M='-machine pseries' M+=",accel=$ACCEL" command="$qemu -nodefaults $M -bios $FIRMWARE" -command+=" -display none -serial stdio -kernel" +command+=" -display none -serial stdio" +command+=" $VMM_PARAMS" +command+=" -kernel" command="$(migration_cmd) $(timeout_cmd) $command" # powerpc tests currently exit with rtas-poweroff, which exits with 0. diff --git a/s390x/run b/s390x/run index 0980504448ce..9bfb95397064 100755 --- a/s390x/run +++ b/s390x/run @@ -19,6 +19,7 @@ M='-machine s390-ccw-virtio' M+=",accel=$ACCEL" command="$qemu -nodefaults -nographic $M" command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0" +command+=" $VMM_PARAMS" command+=" -kernel" command="$(timeout_cmd) $command" diff --git a/scripts/runtime.bash b/scripts/runtime.bash index eb6089091e23..5ea3772d9b2b 100644 --- a/scripts/runtime.bash +++ b/scripts/runtime.bash @@ -30,7 +30,9 @@ premature_failure() get_cmdline() { local kernel=$1 - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" + + # Move VMM_PARAMS to the end to override parameters provided in unittests.cfg:extra_params + echo "VMM_PARAMS= TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts $VMM_PARAMS" } skip_nodefault() diff --git a/x86/run b/x86/run index 1ac91f1d880f..3770c16ad4e6 100755 --- a/x86/run +++ b/x86/run @@ -38,7 +38,9 @@ else fi command="${qemu} -nodefaults $pc_testdev -vnc none -serial stdio $pci_testdev" -command+=" -machine accel=$ACCEL -kernel" +command+=" -machine accel=$ACCEL" +command+=" $VMM_PARAMS" +command+=" -kernel" command="$(timeout_cmd) $command" run_qemu ${command} "$@"
Users may need to temporarily provide additional VMM parameters. The VMM_PARAMS environment variable provides a way to do that. We take care to make sure when executing ./run_tests.sh that the VMM_PARAMS parameters come last, allowing unittests.cfg parameters to be overridden. However, when running a command line like `$ARCH/run $TEST $PARAMS` we want $PARAMS to override $VMM_PARAMS, so we ensure that too. Additional QEMU parameters can still be provided by appending them to the $QEMU environment variable when it provides the path to QEMU, but as those parameters will then be the first in the command line they cannot override anything, and may themselves be overridden. Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Alex Bennée <alex.bennee@linaro.org> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Laurent Vivier <lvivier@redhat.com> Cc: Thomas Huth <thuth@redhat.com> Cc: David Hildenbrand <david@redhat.com> Cc: Janosch Frank <frankja@linux.ibm.com> Signed-off-by: Andrew Jones <drjones@redhat.com> --- README.md | 5 +++++ arm/run | 4 +++- powerpc/run | 4 +++- s390x/run | 1 + scripts/runtime.bash | 4 +++- x86/run | 4 +++- 6 files changed, 18 insertions(+), 4 deletions(-)