* [kvm-unit-tests RFC PATCH 0/5] arm: Add kvmtool to the runner script @ 2021-07-02 16:31 Alexandru Elisei 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available Alexandru Elisei ` (4 more replies) 0 siblings, 5 replies; 36+ messages in thread From: Alexandru Elisei @ 2021-07-02 16:31 UTC (permalink / raw) To: drjones, thuth, pbonzini, lvivier, kvm-ppc, david, frankja, cohuck, imbrenda, linux-s390, kvm, kvmarm Cc: vivek.gautam, andre.przywara, maz kvm-unit-tests has support for running arm64 tests under kvmtool since 2019. However, each test had to be run manually since the runner script was blissfully unware of other VMMs beside qemu. This series aims to support running all the tests automatically with kvmtool, like it has always been possible to do with qemu by invoking: $ ./run_tests.sh The runner script will choose kvmtool as the VMM if the tests has been configured with the kvmtool target. I see several advantages to being able to use kvmtool alongside qemu: - kvmtool is smaller and a lot easier to hack than qemu, which means it may be possible for developers to prefer it over qemu when adding new features to KVM. Being able to run all the tests reliably and automatically is useful in the development process. - kvmtool runs all the tests faster than qemu (roughly 3 times faster on 4xA53s). I think this is another nice feature for development. - kvmtool does things differently than qemu: different memory layout, different uart, PMU emulation is optional, etc. This makes it a good testing vehicule for kvm-unit-tests. This series is an RFC for various reasons: - The migration tests work under kvmtool because when kvm-unit-tests writes something to the UART (like "Now migrate the VM, then press a key a key to continue...\n"), a read will return the last character that was written (newline, in this case). I don't know if this is a feature or a bug with the kvm-unit-tests UART mini-driver or with kvmtool, and I'm investigating it. - I've tried to keep the changes as small as possible, but I would like some feedback about my approach, as I am not very familiar with bash scripting. - The series needs more testing. I've only tested the patches on a rockpro64 with qemu and kvmtool, and on my Ryzen amd64 machine (qemu only, --target=kvmtool is available only for arm/arm64). TODO: - More testing, especially on powerpc and s390x which are touched in patch #2. - README changes to reflect kvmtool support for the runner script. - Figure out how to handle migration tests under kvmtool. Comments welcome and much appreciated. Alexandru Elisei (5): lib: arm: Print test exit status on exit if chr-testdev is not available scripts: Rename run_qemu_status -> run_test_status run_tests.sh: Add kvmtool support scripts: Generate kvmtool standalone tests configure: Ignore --erratatxt when --target=kvmtool scripts/arch-run.bash | 50 ++++++++++++++++-- scripts/runtime.bash | 94 ++++++++++++++++++++++++++++------ scripts/mkstandalone.sh | 9 +++- arm/run | 110 ++++++++++++++++++++++++---------------- powerpc/run | 2 +- s390x/run | 2 +- run_tests.sh | 11 +++- configure | 26 +++++++--- lib/chr-testdev.h | 1 + lib/arm/io.c | 10 +++- lib/chr-testdev.c | 5 ++ 11 files changed, 243 insertions(+), 77 deletions(-) -- 2.32.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available 2021-07-02 16:31 [kvm-unit-tests RFC PATCH 0/5] arm: Add kvmtool to the runner script Alexandru Elisei @ 2021-07-02 16:31 ` Alexandru Elisei 2021-07-12 16:36 ` Andrew Jones 2021-07-12 16:51 ` Andre Przywara 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status Alexandru Elisei ` (3 subsequent siblings) 4 siblings, 2 replies; 36+ messages in thread From: Alexandru Elisei @ 2021-07-02 16:31 UTC (permalink / raw) To: drjones, thuth, pbonzini, lvivier, kvm-ppc, david, frankja, cohuck, imbrenda, linux-s390, kvm, kvmarm Cc: vivek.gautam, andre.przywara, maz The arm64 tests can be run under kvmtool, which doesn't emulate a chr-testdev device. In preparation for adding run script support for kvmtool, print the test exit status so the scripts can pick it up and correctly mark the test as pass or fail. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- lib/chr-testdev.h | 1 + lib/arm/io.c | 10 +++++++++- lib/chr-testdev.c | 5 +++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h index ffd9a851aa9b..09b4b424670e 100644 --- a/lib/chr-testdev.h +++ b/lib/chr-testdev.h @@ -11,4 +11,5 @@ */ extern void chr_testdev_init(void); extern void chr_testdev_exit(int code); +extern bool chr_testdev_available(void); #endif diff --git a/lib/arm/io.c b/lib/arm/io.c index 343e10822263..9e62b571a91b 100644 --- a/lib/arm/io.c +++ b/lib/arm/io.c @@ -125,7 +125,15 @@ extern void halt(int code); void exit(int code) { - chr_testdev_exit(code); + if (chr_testdev_available()) { + chr_testdev_exit(code); + } else { + /* + * Print the test return code in the format used by chr-testdev + * so the runner script can parse it. + */ + printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); + } psci_system_off(); halt(code); __builtin_unreachable(); diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c index b3c641a833fe..301e73a6c064 100644 --- a/lib/chr-testdev.c +++ b/lib/chr-testdev.c @@ -68,3 +68,8 @@ void chr_testdev_init(void) in_vq = vqs[0]; out_vq = vqs[1]; } + +bool chr_testdev_available(void) +{ + return vcon != NULL; +} -- 2.32.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available Alexandru Elisei @ 2021-07-12 16:36 ` Andrew Jones 2021-09-06 10:20 ` Alexandru Elisei 2021-07-12 16:51 ` Andre Przywara 1 sibling, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-07-12 16:36 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Fri, Jul 02, 2021 at 05:31:18PM +0100, Alexandru Elisei wrote: > The arm64 tests can be run under kvmtool, which doesn't emulate a > chr-testdev device. In preparation for adding run script support for > kvmtool, print the test exit status so the scripts can pick it up and > correctly mark the test as pass or fail. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > lib/chr-testdev.h | 1 + > lib/arm/io.c | 10 +++++++++- > lib/chr-testdev.c | 5 +++++ > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h > index ffd9a851aa9b..09b4b424670e 100644 > --- a/lib/chr-testdev.h > +++ b/lib/chr-testdev.h > @@ -11,4 +11,5 @@ > */ > extern void chr_testdev_init(void); > extern void chr_testdev_exit(int code); > +extern bool chr_testdev_available(void); > #endif > diff --git a/lib/arm/io.c b/lib/arm/io.c > index 343e10822263..9e62b571a91b 100644 > --- a/lib/arm/io.c > +++ b/lib/arm/io.c > @@ -125,7 +125,15 @@ extern void halt(int code); > > void exit(int code) > { > - chr_testdev_exit(code); > + if (chr_testdev_available()) { > + chr_testdev_exit(code); chr_testdev_exit() already has a 'if !vcon goto out' in it, so you can just call it unconditionally. No need for chr_testdev_available(). > + } else { > + /* > + * Print the test return code in the format used by chr-testdev > + * so the runner script can parse it. > + */ > + printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); > + } > psci_system_off(); > halt(code); > __builtin_unreachable(); > diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c > index b3c641a833fe..301e73a6c064 100644 > --- a/lib/chr-testdev.c > +++ b/lib/chr-testdev.c > @@ -68,3 +68,8 @@ void chr_testdev_init(void) > in_vq = vqs[0]; > out_vq = vqs[1]; > } > + > +bool chr_testdev_available(void) > +{ > + return vcon != NULL; > +} > -- > 2.32.0 > Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available 2021-07-12 16:36 ` Andrew Jones @ 2021-09-06 10:20 ` Alexandru Elisei 2021-09-06 10:58 ` Andrew Jones 0 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-09-06 10:20 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, Sorry for taking so long to reply, been busy with other things. On 7/12/21 5:36 PM, Andrew Jones wrote: > On Fri, Jul 02, 2021 at 05:31:18PM +0100, Alexandru Elisei wrote: >> The arm64 tests can be run under kvmtool, which doesn't emulate a >> chr-testdev device. In preparation for adding run script support for >> kvmtool, print the test exit status so the scripts can pick it up and >> correctly mark the test as pass or fail. >> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >> --- >> lib/chr-testdev.h | 1 + >> lib/arm/io.c | 10 +++++++++- >> lib/chr-testdev.c | 5 +++++ >> 3 files changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h >> index ffd9a851aa9b..09b4b424670e 100644 >> --- a/lib/chr-testdev.h >> +++ b/lib/chr-testdev.h >> @@ -11,4 +11,5 @@ >> */ >> extern void chr_testdev_init(void); >> extern void chr_testdev_exit(int code); >> +extern bool chr_testdev_available(void); >> #endif >> diff --git a/lib/arm/io.c b/lib/arm/io.c >> index 343e10822263..9e62b571a91b 100644 >> --- a/lib/arm/io.c >> +++ b/lib/arm/io.c >> @@ -125,7 +125,15 @@ extern void halt(int code); >> >> void exit(int code) >> { >> - chr_testdev_exit(code); >> + if (chr_testdev_available()) { >> + chr_testdev_exit(code); > chr_testdev_exit() already has a 'if !vcon goto out' in it, so you can > just call it unconditionally. No need for chr_testdev_available(). I'm not sure what you mean. There has to be a way to check if chr-testdev is available, and if it's not present on the system, to print the EXIT: STATUS message, and vcon is static in chr-testdev.c. Are you suggesting that we move the message to chr_testdev_exit(code)? Thanks, Alex > >> + } else { >> + /* >> + * Print the test return code in the format used by chr-testdev >> + * so the runner script can parse it. >> + */ >> + printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); >> + } >> psci_system_off(); >> halt(code); >> __builtin_unreachable(); >> diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c >> index b3c641a833fe..301e73a6c064 100644 >> --- a/lib/chr-testdev.c >> +++ b/lib/chr-testdev.c >> @@ -68,3 +68,8 @@ void chr_testdev_init(void) >> in_vq = vqs[0]; >> out_vq = vqs[1]; >> } >> + >> +bool chr_testdev_available(void) >> +{ >> + return vcon != NULL; >> +} >> -- >> 2.32.0 >> > Thanks, > drew > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available 2021-09-06 10:20 ` Alexandru Elisei @ 2021-09-06 10:58 ` Andrew Jones 2021-09-06 11:06 ` Alexandru Elisei 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-06 10:58 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Mon, Sep 06, 2021 at 11:20:31AM +0100, Alexandru Elisei wrote: > Hi Drew, > > Sorry for taking so long to reply, been busy with other things. > > On 7/12/21 5:36 PM, Andrew Jones wrote: > > On Fri, Jul 02, 2021 at 05:31:18PM +0100, Alexandru Elisei wrote: > >> The arm64 tests can be run under kvmtool, which doesn't emulate a > >> chr-testdev device. In preparation for adding run script support for > >> kvmtool, print the test exit status so the scripts can pick it up and > >> correctly mark the test as pass or fail. > >> > >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > >> --- > >> lib/chr-testdev.h | 1 + > >> lib/arm/io.c | 10 +++++++++- > >> lib/chr-testdev.c | 5 +++++ > >> 3 files changed, 15 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h > >> index ffd9a851aa9b..09b4b424670e 100644 > >> --- a/lib/chr-testdev.h > >> +++ b/lib/chr-testdev.h > >> @@ -11,4 +11,5 @@ > >> */ > >> extern void chr_testdev_init(void); > >> extern void chr_testdev_exit(int code); > >> +extern bool chr_testdev_available(void); > >> #endif > >> diff --git a/lib/arm/io.c b/lib/arm/io.c > >> index 343e10822263..9e62b571a91b 100644 > >> --- a/lib/arm/io.c > >> +++ b/lib/arm/io.c > >> @@ -125,7 +125,15 @@ extern void halt(int code); > >> > >> void exit(int code) > >> { > >> - chr_testdev_exit(code); > >> + if (chr_testdev_available()) { > >> + chr_testdev_exit(code); > > chr_testdev_exit() already has a 'if !vcon goto out' in it, so you can > > just call it unconditionally. No need for chr_testdev_available(). > > I'm not sure what you mean. There has to be a way to check if chr-testdev is > available, and if it's not present on the system, to print the EXIT: STATUS > message, and vcon is static in chr-testdev.c. > > Are you suggesting that we move the message to chr_testdev_exit(code)? I'm saying you can unconditionally call chr_testdev_exit(), because it only conditionally does anything, and on the same condition that you're adding (vcon != NULL). $ /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,accel=tcg -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/selftest.flat ABORT: selftest: no test specified SUMMARY: 0 tests $ echo $? 127 $ /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,accel=tcg -cpu cortex-a57 -display none -serial stdio -kernel arm/selftest.flat ABORT: selftest: no test specified SUMMARY: 0 tests $ echo $? 0 See, no explosions when the device is removed. Just a lack of return code. Also, since chr_testdev_exit() exits, any calls after it won't happen. So the exit print statement doesn't need to be in an else clause. That said, I think the print statement should come first in order to also put it in the qemu output logs. We might as well have consistent output between qemu and kvmtool. Thanks, drew > > Thanks, > > Alex > > > > >> + } else { > >> + /* > >> + * Print the test return code in the format used by chr-testdev > >> + * so the runner script can parse it. > >> + */ > >> + printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); > >> + } > >> psci_system_off(); > >> halt(code); > >> __builtin_unreachable(); > >> diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c > >> index b3c641a833fe..301e73a6c064 100644 > >> --- a/lib/chr-testdev.c > >> +++ b/lib/chr-testdev.c > >> @@ -68,3 +68,8 @@ void chr_testdev_init(void) > >> in_vq = vqs[0]; > >> out_vq = vqs[1]; > >> } > >> + > >> +bool chr_testdev_available(void) > >> +{ > >> + return vcon != NULL; > >> +} > >> -- > >> 2.32.0 > >> > > Thanks, > > drew > > > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available 2021-09-06 10:58 ` Andrew Jones @ 2021-09-06 11:06 ` Alexandru Elisei 0 siblings, 0 replies; 36+ messages in thread From: Alexandru Elisei @ 2021-09-06 11:06 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, On 9/6/21 11:58 AM, Andrew Jones wrote: > On Mon, Sep 06, 2021 at 11:20:31AM +0100, Alexandru Elisei wrote: >> Hi Drew, >> >> Sorry for taking so long to reply, been busy with other things. >> >> On 7/12/21 5:36 PM, Andrew Jones wrote: >>> On Fri, Jul 02, 2021 at 05:31:18PM +0100, Alexandru Elisei wrote: >>>> The arm64 tests can be run under kvmtool, which doesn't emulate a >>>> chr-testdev device. In preparation for adding run script support for >>>> kvmtool, print the test exit status so the scripts can pick it up and >>>> correctly mark the test as pass or fail. >>>> >>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >>>> --- >>>> lib/chr-testdev.h | 1 + >>>> lib/arm/io.c | 10 +++++++++- >>>> lib/chr-testdev.c | 5 +++++ >>>> 3 files changed, 15 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h >>>> index ffd9a851aa9b..09b4b424670e 100644 >>>> --- a/lib/chr-testdev.h >>>> +++ b/lib/chr-testdev.h >>>> @@ -11,4 +11,5 @@ >>>> */ >>>> extern void chr_testdev_init(void); >>>> extern void chr_testdev_exit(int code); >>>> +extern bool chr_testdev_available(void); >>>> #endif >>>> diff --git a/lib/arm/io.c b/lib/arm/io.c >>>> index 343e10822263..9e62b571a91b 100644 >>>> --- a/lib/arm/io.c >>>> +++ b/lib/arm/io.c >>>> @@ -125,7 +125,15 @@ extern void halt(int code); >>>> >>>> void exit(int code) >>>> { >>>> - chr_testdev_exit(code); >>>> + if (chr_testdev_available()) { >>>> + chr_testdev_exit(code); >>> chr_testdev_exit() already has a 'if !vcon goto out' in it, so you can >>> just call it unconditionally. No need for chr_testdev_available(). >> I'm not sure what you mean. There has to be a way to check if chr-testdev is >> available, and if it's not present on the system, to print the EXIT: STATUS >> message, and vcon is static in chr-testdev.c. >> >> Are you suggesting that we move the message to chr_testdev_exit(code)? > I'm saying you can unconditionally call chr_testdev_exit(), because it > only conditionally does anything, and on the same condition that you're > adding (vcon != NULL). > > $ /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,accel=tcg -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/selftest.flat > ABORT: selftest: no test specified > SUMMARY: 0 tests > $ echo $? > 127 > $ /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,accel=tcg -cpu cortex-a57 -display none -serial stdio -kernel arm/selftest.flat > ABORT: selftest: no test specified > SUMMARY: 0 tests > $ echo $? > 0 > > See, no explosions when the device is removed. Just a lack of return code. Yup, this makes sense, this is exactly what happens today with kvmtool. > > Also, since chr_testdev_exit() exits, any calls after it won't happen. So > the exit print statement doesn't need to be in an else clause. That said, > I think the print statement should come first in order to also put it in > the qemu output logs. We might as well have consistent output between qemu > and kvmtool. Makes sense, I'll move the printf before chr_testdev_exit(). Thanks for the quick reply! Thanks, Alex > > Thanks, > drew > > >> Thanks, >> >> Alex >> >>>> + } else { >>>> + /* >>>> + * Print the test return code in the format used by chr-testdev >>>> + * so the runner script can parse it. >>>> + */ >>>> + printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); >>>> + } >>>> psci_system_off(); >>>> halt(code); >>>> __builtin_unreachable(); >>>> diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c >>>> index b3c641a833fe..301e73a6c064 100644 >>>> --- a/lib/chr-testdev.c >>>> +++ b/lib/chr-testdev.c >>>> @@ -68,3 +68,8 @@ void chr_testdev_init(void) >>>> in_vq = vqs[0]; >>>> out_vq = vqs[1]; >>>> } >>>> + >>>> +bool chr_testdev_available(void) >>>> +{ >>>> + return vcon != NULL; >>>> +} >>>> -- >>>> 2.32.0 >>>> >>> Thanks, >>> drew >>> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available Alexandru Elisei 2021-07-12 16:36 ` Andrew Jones @ 2021-07-12 16:51 ` Andre Przywara 2021-07-12 17:07 ` Andrew Jones 1 sibling, 1 reply; 36+ messages in thread From: Andre Przywara @ 2021-07-12 16:51 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, frankja, kvm, david, maz, cohuck, thuth, kvm-ppc, vivek.gautam, pbonzini, imbrenda, kvmarm On Fri, 2 Jul 2021 17:31:18 +0100 Alexandru Elisei <alexandru.elisei@arm.com> wrote: Hi, > The arm64 tests can be run under kvmtool, which doesn't emulate a > chr-testdev device. In preparation for adding run script support for > kvmtool, print the test exit status so the scripts can pick it up and > correctly mark the test as pass or fail. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > lib/chr-testdev.h | 1 + > lib/arm/io.c | 10 +++++++++- > lib/chr-testdev.c | 5 +++++ > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h > index ffd9a851aa9b..09b4b424670e 100644 > --- a/lib/chr-testdev.h > +++ b/lib/chr-testdev.h > @@ -11,4 +11,5 @@ > */ > extern void chr_testdev_init(void); > extern void chr_testdev_exit(int code); > +extern bool chr_testdev_available(void); > #endif > diff --git a/lib/arm/io.c b/lib/arm/io.c > index 343e10822263..9e62b571a91b 100644 > --- a/lib/arm/io.c > +++ b/lib/arm/io.c > @@ -125,7 +125,15 @@ extern void halt(int code); > > void exit(int code) > { > - chr_testdev_exit(code); > + if (chr_testdev_available()) { > + chr_testdev_exit(code); > + } else { > + /* > + * Print the test return code in the format used by chr-testdev > + * so the runner script can parse it. > + */ > + printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); It's more me being clueless here rather than a problem, but where does this "EXIT: STATUS" line come from? In lib/chr-testdev.c I see "%dq", so it this coming from QEMU (but I couldn't find it in there)? But anyways the patch looks good and matches what PPC and s390 do. Cheers, Andre > + } > psci_system_off(); > halt(code); > __builtin_unreachable(); > diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c > index b3c641a833fe..301e73a6c064 100644 > --- a/lib/chr-testdev.c > +++ b/lib/chr-testdev.c > @@ -68,3 +68,8 @@ void chr_testdev_init(void) > in_vq = vqs[0]; > out_vq = vqs[1]; > } > + > +bool chr_testdev_available(void) > +{ > + return vcon != NULL; > +} _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available 2021-07-12 16:51 ` Andre Przywara @ 2021-07-12 17:07 ` Andrew Jones 2021-07-12 17:12 ` Nadav Amit 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-07-12 17:07 UTC (permalink / raw) To: Andre Przywara Cc: lvivier, linux-s390, thuth, frankja, kvm, david, maz, cohuck, kvm-ppc, vivek.gautam, pbonzini, imbrenda, kvmarm On Mon, Jul 12, 2021 at 05:51:55PM +0100, Andre Przywara wrote: > On Fri, 2 Jul 2021 17:31:18 +0100 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi, > > > The arm64 tests can be run under kvmtool, which doesn't emulate a > > chr-testdev device. In preparation for adding run script support for > > kvmtool, print the test exit status so the scripts can pick it up and > > correctly mark the test as pass or fail. > > > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > > --- > > lib/chr-testdev.h | 1 + > > lib/arm/io.c | 10 +++++++++- > > lib/chr-testdev.c | 5 +++++ > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h > > index ffd9a851aa9b..09b4b424670e 100644 > > --- a/lib/chr-testdev.h > > +++ b/lib/chr-testdev.h > > @@ -11,4 +11,5 @@ > > */ > > extern void chr_testdev_init(void); > > extern void chr_testdev_exit(int code); > > +extern bool chr_testdev_available(void); > > #endif > > diff --git a/lib/arm/io.c b/lib/arm/io.c > > index 343e10822263..9e62b571a91b 100644 > > --- a/lib/arm/io.c > > +++ b/lib/arm/io.c > > @@ -125,7 +125,15 @@ extern void halt(int code); > > > > void exit(int code) > > { > > - chr_testdev_exit(code); > > + if (chr_testdev_available()) { > > + chr_testdev_exit(code); > > + } else { > > + /* > > + * Print the test return code in the format used by chr-testdev > > + * so the runner script can parse it. > > + */ > > + printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); > > It's more me being clueless here rather than a problem, but where does > this "EXIT: STATUS" line come from? In lib/chr-testdev.c I see "%dq", > so it this coming from QEMU (but I couldn't find it in there)? > > But anyways the patch looks good and matches what PPC and s390 do. I invented the 'EXIT: STATUS' format for PPC, which didn't/doesn't have an exit code testdev. Now that it has also been adopted by s390 I guess we've got a kvm-unit-tests standard to follow for arm :-) Thanks, drew > > Cheers, > Andre > > > > + } > > psci_system_off(); > > halt(code); > > __builtin_unreachable(); > > diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c > > index b3c641a833fe..301e73a6c064 100644 > > --- a/lib/chr-testdev.c > > +++ b/lib/chr-testdev.c > > @@ -68,3 +68,8 @@ void chr_testdev_init(void) > > in_vq = vqs[0]; > > out_vq = vqs[1]; > > } > > + > > +bool chr_testdev_available(void) > > +{ > > + return vcon != NULL; > > +} > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available 2021-07-12 17:07 ` Andrew Jones @ 2021-07-12 17:12 ` Nadav Amit 0 siblings, 0 replies; 36+ messages in thread From: Nadav Amit @ 2021-07-12 17:12 UTC (permalink / raw) To: Andrew Jones Cc: Laurent Vivier, linux-s390, Thomas Huth, Janosch Frank, KVM, David Hildenbrand, Andre Przywara, cohuck, kvm-ppc, vivek.gautam, maz, Paolo Bonzini, Claudio Imbrenda, kvmarm [-- Attachment #1.1: Type: text/plain, Size: 2398 bytes --] > On Jul 12, 2021, at 10:07 AM, Andrew Jones <drjones@redhat.com> wrote: > > On Mon, Jul 12, 2021 at 05:51:55PM +0100, Andre Przywara wrote: >> On Fri, 2 Jul 2021 17:31:18 +0100 >> Alexandru Elisei <alexandru.elisei@arm.com> wrote: >> >> Hi, >> >>> The arm64 tests can be run under kvmtool, which doesn't emulate a >>> chr-testdev device. In preparation for adding run script support for >>> kvmtool, print the test exit status so the scripts can pick it up and >>> correctly mark the test as pass or fail. >>> >>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >>> --- >>> lib/chr-testdev.h | 1 + >>> lib/arm/io.c | 10 +++++++++- >>> lib/chr-testdev.c | 5 +++++ >>> 3 files changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h >>> index ffd9a851aa9b..09b4b424670e 100644 >>> --- a/lib/chr-testdev.h >>> +++ b/lib/chr-testdev.h >>> @@ -11,4 +11,5 @@ >>> */ >>> extern void chr_testdev_init(void); >>> extern void chr_testdev_exit(int code); >>> +extern bool chr_testdev_available(void); >>> #endif >>> diff --git a/lib/arm/io.c b/lib/arm/io.c >>> index 343e10822263..9e62b571a91b 100644 >>> --- a/lib/arm/io.c >>> +++ b/lib/arm/io.c >>> @@ -125,7 +125,15 @@ extern void halt(int code); >>> >>> void exit(int code) >>> { >>> - chr_testdev_exit(code); >>> + if (chr_testdev_available()) { >>> + chr_testdev_exit(code); >>> + } else { >>> + /* >>> + * Print the test return code in the format used by chr-testdev >>> + * so the runner script can parse it. >>> + */ >>> + printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); >> >> It's more me being clueless here rather than a problem, but where does >> this "EXIT: STATUS" line come from? In lib/chr-testdev.c I see "%dq", >> so it this coming from QEMU (but I couldn't find it in there)? >> >> But anyways the patch looks good and matches what PPC and s390 do. > > I invented the 'EXIT: STATUS' format for PPC, which didn't/doesn't have an > exit code testdev. Now that it has also been adopted by s390 I guess we've > got a kvm-unit-tests standard to follow for arm :-) I was unaware of this “standard” and I mistakenly used a different format for x86, in case someone wants to fix it. [1] [1] https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/commit/5747945371b47c51cb16187a26111d06f58f06b2 [-- Attachment #1.2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 151 bytes --] _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status 2021-07-02 16:31 [kvm-unit-tests RFC PATCH 0/5] arm: Add kvmtool to the runner script Alexandru Elisei 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available Alexandru Elisei @ 2021-07-02 16:31 ` Alexandru Elisei 2021-07-12 16:37 ` Andrew Jones 2021-07-13 7:45 ` Thomas Huth 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support Alexandru Elisei ` (2 subsequent siblings) 4 siblings, 2 replies; 36+ messages in thread From: Alexandru Elisei @ 2021-07-02 16:31 UTC (permalink / raw) To: drjones, thuth, pbonzini, lvivier, kvm-ppc, david, frankja, cohuck, imbrenda, linux-s390, kvm, kvmarm Cc: vivek.gautam, andre.przywara, maz kvm-unit-tests will get support for running tests automatically under kvmtool, rename the function to make it more generic. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- scripts/arch-run.bash | 2 +- powerpc/run | 2 +- s390x/run | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 5997e384019b..8ceed53ed7f8 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -69,7 +69,7 @@ run_qemu () return $ret } -run_qemu_status () +run_test_status () { local stdout ret diff --git a/powerpc/run b/powerpc/run index 597ab96ed8a8..312576006504 100755 --- a/powerpc/run +++ b/powerpc/run @@ -31,4 +31,4 @@ command="$(migration_cmd) $(timeout_cmd) $command" # to fixup the fixup below by parsing the true exit code from the output. # The second fixup is also a FIXME, because once we add chr-testdev # support for powerpc, we won't need the second fixup. -run_qemu_status $command "$@" +run_test_status $command "$@" diff --git a/s390x/run b/s390x/run index c615caa1b772..5a4bb3bda805 100755 --- a/s390x/run +++ b/s390x/run @@ -28,4 +28,4 @@ command+=" -kernel" command="$(timeout_cmd) $command" # We return the exit code via stdout, not via the QEMU return code -run_qemu_status $command "$@" +run_test_status $command "$@" -- 2.32.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status Alexandru Elisei @ 2021-07-12 16:37 ` Andrew Jones 2021-07-13 7:45 ` Thomas Huth 1 sibling, 0 replies; 36+ messages in thread From: Andrew Jones @ 2021-07-12 16:37 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Fri, Jul 02, 2021 at 05:31:19PM +0100, Alexandru Elisei wrote: > kvm-unit-tests will get support for running tests automatically under > kvmtool, rename the function to make it more generic. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > scripts/arch-run.bash | 2 +- > powerpc/run | 2 +- > s390x/run | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index 5997e384019b..8ceed53ed7f8 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -69,7 +69,7 @@ run_qemu () > return $ret > } > > -run_qemu_status () > +run_test_status () > { > local stdout ret > > diff --git a/powerpc/run b/powerpc/run > index 597ab96ed8a8..312576006504 100755 > --- a/powerpc/run > +++ b/powerpc/run > @@ -31,4 +31,4 @@ command="$(migration_cmd) $(timeout_cmd) $command" > # to fixup the fixup below by parsing the true exit code from the output. > # The second fixup is also a FIXME, because once we add chr-testdev > # support for powerpc, we won't need the second fixup. > -run_qemu_status $command "$@" > +run_test_status $command "$@" > diff --git a/s390x/run b/s390x/run > index c615caa1b772..5a4bb3bda805 100755 > --- a/s390x/run > +++ b/s390x/run > @@ -28,4 +28,4 @@ command+=" -kernel" > command="$(timeout_cmd) $command" > > # We return the exit code via stdout, not via the QEMU return code > -run_qemu_status $command "$@" > +run_test_status $command "$@" > -- > 2.32.0 > Reviewed-by: Andrew Jones <drjones@redhat.com> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status Alexandru Elisei 2021-07-12 16:37 ` Andrew Jones @ 2021-07-13 7:45 ` Thomas Huth 1 sibling, 0 replies; 36+ messages in thread From: Thomas Huth @ 2021-07-13 7:45 UTC (permalink / raw) To: Alexandru Elisei, drjones, pbonzini, lvivier, kvm-ppc, david, frankja, cohuck, imbrenda, linux-s390, kvm, kvmarm Cc: vivek.gautam, andre.przywara, maz On 02/07/2021 18.31, Alexandru Elisei wrote: > kvm-unit-tests will get support for running tests automatically under > kvmtool, rename the function to make it more generic. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > scripts/arch-run.bash | 2 +- > powerpc/run | 2 +- > s390x/run | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index 5997e384019b..8ceed53ed7f8 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -69,7 +69,7 @@ run_qemu () > return $ret > } > > -run_qemu_status () > +run_test_status () > { > local stdout ret > > diff --git a/powerpc/run b/powerpc/run > index 597ab96ed8a8..312576006504 100755 > --- a/powerpc/run > +++ b/powerpc/run > @@ -31,4 +31,4 @@ command="$(migration_cmd) $(timeout_cmd) $command" > # to fixup the fixup below by parsing the true exit code from the output. > # The second fixup is also a FIXME, because once we add chr-testdev > # support for powerpc, we won't need the second fixup. > -run_qemu_status $command "$@" > +run_test_status $command "$@" > diff --git a/s390x/run b/s390x/run > index c615caa1b772..5a4bb3bda805 100755 > --- a/s390x/run > +++ b/s390x/run > @@ -28,4 +28,4 @@ command+=" -kernel" > command="$(timeout_cmd) $command" > > # We return the exit code via stdout, not via the QEMU return code > -run_qemu_status $command "$@" > +run_test_status $command "$@" > Reviewed-by: Thomas Huth <thuth@redhat.com> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-07-02 16:31 [kvm-unit-tests RFC PATCH 0/5] arm: Add kvmtool to the runner script Alexandru Elisei 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available Alexandru Elisei 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status Alexandru Elisei @ 2021-07-02 16:31 ` Alexandru Elisei 2021-07-12 16:52 ` Andre Przywara 2021-09-07 10:17 ` Andrew Jones 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests Alexandru Elisei 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool Alexandru Elisei 4 siblings, 2 replies; 36+ messages in thread From: Alexandru Elisei @ 2021-07-02 16:31 UTC (permalink / raw) To: drjones, thuth, pbonzini, lvivier, kvm-ppc, david, frankja, cohuck, imbrenda, linux-s390, kvm, kvmarm Cc: vivek.gautam, andre.przywara, maz Modify run_tests.sh to use kvmtool instead of qemu to run tests when kvm-unit-tests has been configured with --target=kvmtool. Example invocation: $ ./configure --target=kvmtool $ make clean && make $ ./run_scripts.sh A custom location for the kvmtool binary can be set using the environment variable KVMTOOL: $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh Standalone test support is absent, but will be added in subsequent patches. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- scripts/arch-run.bash | 48 ++++++++++++++++-- scripts/runtime.bash | 94 ++++++++++++++++++++++++++++------ scripts/mkstandalone.sh | 5 ++ arm/run | 110 ++++++++++++++++++++++++---------------- run_tests.sh | 11 +++- 5 files changed, 204 insertions(+), 64 deletions(-) diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 8ceed53ed7f8..b916b0e79aca 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -69,16 +69,39 @@ run_qemu () return $ret } +run_kvmtool() +{ + local stdout errors ret sig + + # kvmtool doesn't allow an initrd argument with --firmware, but configure + # sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target. + + # stdout to {stdout}, stderr to $errors and stderr + exec {stdout}>&1 + errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout) + ret=$? + exec {stdout}>&- + + # ret=0 success, everything else is failure. + return $ret +} + run_test_status () { - local stdout ret + local stdout ret exit_status exec {stdout}>&1 - lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) + if [ "$TARGET" = "kvmtool" ]; then + lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout)) + exit_status=0 + else + lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) + exit_status=1 + fi ret=$? exec {stdout}>&- - if [ $ret -eq 1 ]; then + if [ $ret -eq $exit_status ]; then testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/') if [ "$testret" ]; then if [ $testret -eq 1 ]; then @@ -193,6 +216,25 @@ search_qemu_binary () export PATH=$save_path } +search_kvmtool_binary () +{ + local lkvm kvmtool + + for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do + if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then + kvmtool="$lkvm" + break + fi + done + + if [ -z "$kvmtool" ]; then + echo "A kvmtool binary was not found." >&2 + echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2 + return 2 + fi + command -v $kvmtool +} + initrd_create () { if [ "$ENVIRON_DEFAULT" = "yes" ]; then diff --git a/scripts/runtime.bash b/scripts/runtime.bash index 132389c7dd59..23b238a6ab6f 100644 --- a/scripts/runtime.bash +++ b/scripts/runtime.bash @@ -12,14 +12,19 @@ extract_summary() tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/' } -# We assume that QEMU is going to work if it tried to load the kernel +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel premature_failure() { local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)" - echo "$log" | grep "_NO_FILE_4Uhere_" | - grep -q -e "could not \(load\|open\) kernel" -e "error loading" && - return 1 + if [ "$TARGET" = "kvmtool" ]; then + echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" && + return 1 + else + echo "$log" | grep "_NO_FILE_4Uhere_" | + grep -q -e "could not \(load\|open\) kernel" -e "error loading" && + return 1 + fi RUNTIME_log_stderr <<< "$log" @@ -30,7 +35,14 @@ premature_failure() get_cmdline() { local kernel=$1 - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" + local smp_param + + if [ "$TARGET" = "kvmtool" ]; then + smp_param="--cpus $smp" + else + smp_param="-smp $smp" + fi + echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts" } skip_nodefault() @@ -70,6 +82,35 @@ function find_word() grep -Fq " $1 " <<< " $2 " } +fixup_kvmtool_opts() +{ + local opts=$1 + local groups=$2 + local gic + local gic_version + + if find_word "pmu" $groups; then + opts+=" --pmu" + fi + + if find_word "its" $groups; then + gic_version=3 + gic="gicv3-its" + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then + gic_version="${BASH_REMATCH[1]}" + gic="gicv$gic_version" + fi + + if [ -n "$gic" ]; then + opts=${opts/-machine gic-version=$gic_version/} + opts+=" --irqchip=$gic" + fi + + opts=${opts/-append/--params} + + echo "$opts" +} + function run() { local testname="$1" @@ -105,7 +146,12 @@ function run() return 2 fi - if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then + if [ "$TARGET" = "kvmtool" ]; then + if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then + print_result "SKIP" $testname "" "$accel not supported by kvmtool" + return 2 + fi + elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL" return 2 elif [ -n "$ACCEL" ]; then @@ -126,6 +172,10 @@ function run() done fi + if [ "$TARGET" = "kvmtool" ]; then + opts=$(fixup_kvmtool_opts "$opts" "$groups") + fi + last_line=$(premature_failure > >(tail -1)) && { print_result "SKIP" $testname "" "$last_line" return 77 @@ -165,13 +215,25 @@ function run() # # Probe for MAX_SMP, in case it's less than the number of host cpus. # -# This probing currently only works for ARM, as x86 bails on another -# error first. Also, this probing isn't necessary for any ARM hosts -# running kernels later than v4.3, i.e. those including ef748917b52 -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some -# point when maintaining the while loop gets too tiresome, we can -# just remove it... -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ - |& grep -qi 'exceeds max CPUs'; do - MAX_SMP=$((MAX_SMP >> 1)) -done +# This probing currently only works for ARM, as x86 bails on another error +# first. Also, this probing isn't necessary for any ARM hosts running kernels +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop +# gets too tiresome, we can just remove it... +# +# We don't need this check for kvmtool, as kvmtool will automatically limit the +# number of VCPUs to what the host supports instead of exiting with an error. +# kvmtool prints a message when that happens, but it's harmless and the chance +# of running a kernel so old that the number of VCPUs is smaller than the number +# of physical CPUs is vanishingly small. +# +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0, +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test +# is running on a recent x86 machine, there's a fairly good chance that more +# than 8 logical CPUs are available. +if [ "$TARGET" = "qemu" ]; then + while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ + |& grep -qi 'exceeds max CPUs'; do + MAX_SMP=$((MAX_SMP >> 1)) + done +fi diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh index cefdec30cb33..16f461c06842 100755 --- a/scripts/mkstandalone.sh +++ b/scripts/mkstandalone.sh @@ -95,6 +95,11 @@ function mkstandalone() echo Written $standalone. } +if [ "$TARGET" = "kvmtool" ]; then + echo "Standalone tests not supported with kvmtool" + exit 2 +fi + if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 exit 2 diff --git a/arm/run b/arm/run index a390ca5ae0ba..cc5890e7fec4 100755 --- a/arm/run +++ b/arm/run @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then source config.mak source scripts/arch-run.bash fi -processor="$PROCESSOR" -ACCEL=$(get_qemu_accelerator) || - exit $? +run_test_qemu() +{ + processor="$PROCESSOR" -qemu=$(search_qemu_binary) || - exit $? + ACCEL=$(get_qemu_accelerator) || + exit $? -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then - echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." - exit 2 -fi + qemu=$(search_qemu_binary) || + exit $? -M='-machine virt' + if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then + echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." + exit 2 + fi -if [ "$ACCEL" = "kvm" ]; then - if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then - M+=',gic-version=host' - fi - if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then - processor="host" - if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then - processor+=",aarch64=off" - fi - fi -fi + M='-machine virt' -if [ "$ARCH" = "arm" ]; then - M+=",highmem=off" -fi + if [ "$ACCEL" = "kvm" ]; then + if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then + M+=',gic-version=host' + fi + if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then + processor="host" + if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then + processor+=",aarch64=off" + fi + fi + fi -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then - echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." - exit 2 -fi + if [ "$ARCH" = "arm" ]; then + M+=",highmem=off" + fi -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ - | grep backend > /dev/null; then - echo "$qemu doesn't support chr-testdev. Exiting." - exit 2 -fi + if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then + echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." + exit 2 + fi -chr_testdev='-device virtio-serial-device' -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' + if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ + | grep backend > /dev/null; then + echo "$qemu doesn't support chr-testdev. Exiting." + exit 2 + fi -pci_testdev= -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then - pci_testdev="-device pci-testdev" -fi + chr_testdev='-device virtio-serial-device' + chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' + + pci_testdev= + if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then + pci_testdev="-device pci-testdev" + fi + + M+=",accel=$ACCEL" + command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" + command+=" -display none -serial stdio -kernel" + command="$(migration_cmd) $(timeout_cmd) $command" + + run_qemu $command "$@" +} + +run_test_kvmtool() +{ + kvmtool=$(search_kvmtool_binary) || + exit $? -M+=",accel=$ACCEL" -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" -command+=" -display none -serial stdio -kernel" -command="$(migration_cmd) $(timeout_cmd) $command" + local command="$(timeout_cmd) $kvmtool run --firmware " + run_test_status $command "$@" +} -run_qemu $command "$@" +case "$TARGET" in + "qemu") + run_test_qemu "$@" + ;; + "kvmtool") + run_test_kvmtool "$@" + ;; +esac diff --git a/run_tests.sh b/run_tests.sh index 65108e73a2c0..b010ee3ab348 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t] -t, --tap13 Output test results in TAP format Set the environment variable QEMU=/path/to/qemu-system-ARCH to -specify the appropriate qemu binary for ARCH-run. +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool +is also supported and the environment variable KVMTOOL=/path/to/kvmtool +can be used to specify a custom location for the kvmtool binary. EOF } @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then exit 1 fi +if [ "$TARGET" = "kvmtool" ]; then + if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then + echo "kvmtool supports only the kvm accelerator" + exit 1 + fi +fi + only_tests="" args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*` [ $? -ne 0 ] && exit 2; -- 2.32.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support Alexandru Elisei @ 2021-07-12 16:52 ` Andre Przywara 2021-09-06 10:28 ` Alexandru Elisei 2021-09-07 10:17 ` Andrew Jones 1 sibling, 1 reply; 36+ messages in thread From: Andre Przywara @ 2021-07-12 16:52 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, frankja, kvm, david, maz, cohuck, thuth, kvm-ppc, vivek.gautam, pbonzini, imbrenda, kvmarm On Fri, 2 Jul 2021 17:31:20 +0100 Alexandru Elisei <alexandru.elisei@arm.com> wrote: Hi Alex, > Modify run_tests.sh to use kvmtool instead of qemu to run tests when > kvm-unit-tests has been configured with --target=kvmtool. > > Example invocation: > > $ ./configure --target=kvmtool > $ make clean && make > $ ./run_scripts.sh > > A custom location for the kvmtool binary can be set using the environment > variable KVMTOOL: > > $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh > > Standalone test support is absent, but will be added in subsequent patches. So I skimmed over this, and it's at least better than what I had tried a few years back ;-) And while there might be other ways to sort out command line differences between the two, this fixup_kvmtool_opts() looks like a clever solution to the problem. The only problem with this patch is that it's rather big, I wonder if this could be split up? For instance move any QEMU specific functionality into separate functions first (like run_test_qemu()), also use 'if [ "$TARGET" = "qemu" ]' in this first (or second?) patch. Then later on just add the kvmtool specifics. Cheers, Andre > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > scripts/arch-run.bash | 48 ++++++++++++++++-- > scripts/runtime.bash | 94 ++++++++++++++++++++++++++++------ > scripts/mkstandalone.sh | 5 ++ > arm/run | 110 ++++++++++++++++++++++++---------------- > run_tests.sh | 11 +++- > 5 files changed, 204 insertions(+), 64 deletions(-) > > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index 8ceed53ed7f8..b916b0e79aca 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -69,16 +69,39 @@ run_qemu () > return $ret > } > > +run_kvmtool() > +{ > + local stdout errors ret sig > + > + # kvmtool doesn't allow an initrd argument with --firmware, but configure > + # sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target. > + > + # stdout to {stdout}, stderr to $errors and stderr > + exec {stdout}>&1 > + errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout) > + ret=$? > + exec {stdout}>&- > + > + # ret=0 success, everything else is failure. > + return $ret > +} > + > run_test_status () > { > - local stdout ret > + local stdout ret exit_status > > exec {stdout}>&1 > - lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) > + if [ "$TARGET" = "kvmtool" ]; then > + lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout)) > + exit_status=0 > + else > + lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) > + exit_status=1 > + fi > ret=$? > exec {stdout}>&- > > - if [ $ret -eq 1 ]; then > + if [ $ret -eq $exit_status ]; then > testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/') > if [ "$testret" ]; then > if [ $testret -eq 1 ]; then > @@ -193,6 +216,25 @@ search_qemu_binary () > export PATH=$save_path > } > > +search_kvmtool_binary () > +{ > + local lkvm kvmtool > + > + for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do > + if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then > + kvmtool="$lkvm" > + break > + fi > + done > + > + if [ -z "$kvmtool" ]; then > + echo "A kvmtool binary was not found." >&2 > + echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2 > + return 2 > + fi > + command -v $kvmtool > +} > + > initrd_create () > { > if [ "$ENVIRON_DEFAULT" = "yes" ]; then > diff --git a/scripts/runtime.bash b/scripts/runtime.bash > index 132389c7dd59..23b238a6ab6f 100644 > --- a/scripts/runtime.bash > +++ b/scripts/runtime.bash > @@ -12,14 +12,19 @@ extract_summary() > tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/' > } > > -# We assume that QEMU is going to work if it tried to load the kernel > +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel > premature_failure() > { > local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)" > > - echo "$log" | grep "_NO_FILE_4Uhere_" | > - grep -q -e "could not \(load\|open\) kernel" -e "error loading" && > - return 1 > + if [ "$TARGET" = "kvmtool" ]; then > + echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" && > + return 1 > + else > + echo "$log" | grep "_NO_FILE_4Uhere_" | > + grep -q -e "could not \(load\|open\) kernel" -e "error loading" && > + return 1 > + fi > > RUNTIME_log_stderr <<< "$log" > > @@ -30,7 +35,14 @@ premature_failure() > get_cmdline() > { > local kernel=$1 > - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" > + local smp_param > + > + if [ "$TARGET" = "kvmtool" ]; then > + smp_param="--cpus $smp" > + else > + smp_param="-smp $smp" > + fi > + echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts" > } > > skip_nodefault() > @@ -70,6 +82,35 @@ function find_word() > grep -Fq " $1 " <<< " $2 " > } > > +fixup_kvmtool_opts() > +{ > + local opts=$1 > + local groups=$2 > + local gic > + local gic_version > + > + if find_word "pmu" $groups; then > + opts+=" --pmu" > + fi > + > + if find_word "its" $groups; then > + gic_version=3 > + gic="gicv3-its" > + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then > + gic_version="${BASH_REMATCH[1]}" > + gic="gicv$gic_version" > + fi > + > + if [ -n "$gic" ]; then > + opts=${opts/-machine gic-version=$gic_version/} > + opts+=" --irqchip=$gic" > + fi > + > + opts=${opts/-append/--params} > + > + echo "$opts" > +} > + > function run() > { > local testname="$1" > @@ -105,7 +146,12 @@ function run() > return 2 > fi > > - if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then > + if [ "$TARGET" = "kvmtool" ]; then > + if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then > + print_result "SKIP" $testname "" "$accel not supported by kvmtool" > + return 2 > + fi > + elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then > print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL" > return 2 > elif [ -n "$ACCEL" ]; then > @@ -126,6 +172,10 @@ function run() > done > fi > > + if [ "$TARGET" = "kvmtool" ]; then > + opts=$(fixup_kvmtool_opts "$opts" "$groups") > + fi > + > last_line=$(premature_failure > >(tail -1)) && { > print_result "SKIP" $testname "" "$last_line" > return 77 > @@ -165,13 +215,25 @@ function run() > # > # Probe for MAX_SMP, in case it's less than the number of host cpus. > # > -# This probing currently only works for ARM, as x86 bails on another > -# error first. Also, this probing isn't necessary for any ARM hosts > -# running kernels later than v4.3, i.e. those including ef748917b52 > -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some > -# point when maintaining the while loop gets too tiresome, we can > -# just remove it... > -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ > - |& grep -qi 'exceeds max CPUs'; do > - MAX_SMP=$((MAX_SMP >> 1)) > -done > +# This probing currently only works for ARM, as x86 bails on another error > +# first. Also, this probing isn't necessary for any ARM hosts running kernels > +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove > +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop > +# gets too tiresome, we can just remove it... > +# > +# We don't need this check for kvmtool, as kvmtool will automatically limit the > +# number of VCPUs to what the host supports instead of exiting with an error. > +# kvmtool prints a message when that happens, but it's harmless and the chance > +# of running a kernel so old that the number of VCPUs is smaller than the number > +# of physical CPUs is vanishingly small. > +# > +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0, > +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test > +# is running on a recent x86 machine, there's a fairly good chance that more > +# than 8 logical CPUs are available. > +if [ "$TARGET" = "qemu" ]; then > + while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ > + |& grep -qi 'exceeds max CPUs'; do > + MAX_SMP=$((MAX_SMP >> 1)) > + done > +fi > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh > index cefdec30cb33..16f461c06842 100755 > --- a/scripts/mkstandalone.sh > +++ b/scripts/mkstandalone.sh > @@ -95,6 +95,11 @@ function mkstandalone() > echo Written $standalone. > } > > +if [ "$TARGET" = "kvmtool" ]; then > + echo "Standalone tests not supported with kvmtool" > + exit 2 > +fi > + > if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 > exit 2 > diff --git a/arm/run b/arm/run > index a390ca5ae0ba..cc5890e7fec4 100755 > --- a/arm/run > +++ b/arm/run > @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then > source config.mak > source scripts/arch-run.bash > fi > -processor="$PROCESSOR" > > -ACCEL=$(get_qemu_accelerator) || > - exit $? > +run_test_qemu() > +{ > + processor="$PROCESSOR" > > -qemu=$(search_qemu_binary) || > - exit $? > + ACCEL=$(get_qemu_accelerator) || > + exit $? > > -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then > - echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." > - exit 2 > -fi > + qemu=$(search_qemu_binary) || > + exit $? > > -M='-machine virt' > + if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then > + echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." > + exit 2 > + fi > > -if [ "$ACCEL" = "kvm" ]; then > - if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then > - M+=',gic-version=host' > - fi > - if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then > - processor="host" > - if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then > - processor+=",aarch64=off" > - fi > - fi > -fi > + M='-machine virt' > > -if [ "$ARCH" = "arm" ]; then > - M+=",highmem=off" > -fi > + if [ "$ACCEL" = "kvm" ]; then > + if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then > + M+=',gic-version=host' > + fi > + if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then > + processor="host" > + if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then > + processor+=",aarch64=off" > + fi > + fi > + fi > > -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then > - echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." > - exit 2 > -fi > + if [ "$ARCH" = "arm" ]; then > + M+=",highmem=off" > + fi > > -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ > - | grep backend > /dev/null; then > - echo "$qemu doesn't support chr-testdev. Exiting." > - exit 2 > -fi > + if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then > + echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." > + exit 2 > + fi > > -chr_testdev='-device virtio-serial-device' > -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' > + if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ > + | grep backend > /dev/null; then > + echo "$qemu doesn't support chr-testdev. Exiting." > + exit 2 > + fi > > -pci_testdev= > -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then > - pci_testdev="-device pci-testdev" > -fi > + chr_testdev='-device virtio-serial-device' > + chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' > + > + pci_testdev= > + if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then > + pci_testdev="-device pci-testdev" > + fi > + > + M+=",accel=$ACCEL" > + command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" > + command+=" -display none -serial stdio -kernel" > + command="$(migration_cmd) $(timeout_cmd) $command" > + > + run_qemu $command "$@" > +} > + > +run_test_kvmtool() > +{ > + kvmtool=$(search_kvmtool_binary) || > + exit $? > > -M+=",accel=$ACCEL" > -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" > -command+=" -display none -serial stdio -kernel" > -command="$(migration_cmd) $(timeout_cmd) $command" > + local command="$(timeout_cmd) $kvmtool run --firmware " > + run_test_status $command "$@" > +} > > -run_qemu $command "$@" > +case "$TARGET" in > + "qemu") > + run_test_qemu "$@" > + ;; > + "kvmtool") > + run_test_kvmtool "$@" > + ;; > +esac > diff --git a/run_tests.sh b/run_tests.sh > index 65108e73a2c0..b010ee3ab348 100755 > --- a/run_tests.sh > +++ b/run_tests.sh > @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t] > -t, --tap13 Output test results in TAP format > > Set the environment variable QEMU=/path/to/qemu-system-ARCH to > -specify the appropriate qemu binary for ARCH-run. > +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool > +is also supported and the environment variable KVMTOOL=/path/to/kvmtool > +can be used to specify a custom location for the kvmtool binary. > > EOF > } > @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then > exit 1 > fi > > +if [ "$TARGET" = "kvmtool" ]; then > + if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then > + echo "kvmtool supports only the kvm accelerator" > + exit 1 > + fi > +fi > + > only_tests="" > args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*` > [ $? -ne 0 ] && exit 2; _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-07-12 16:52 ` Andre Przywara @ 2021-09-06 10:28 ` Alexandru Elisei 2021-09-06 11:01 ` Andrew Jones 0 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-09-06 10:28 UTC (permalink / raw) To: Andre Przywara Cc: lvivier, linux-s390, frankja, kvm, david, maz, cohuck, thuth, kvm-ppc, vivek.gautam, pbonzini, imbrenda, kvmarm Hi Andre, Thanks for having a look! On 7/12/21 5:52 PM, Andre Przywara wrote: > On Fri, 2 Jul 2021 17:31:20 +0100 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi Alex, > >> Modify run_tests.sh to use kvmtool instead of qemu to run tests when >> kvm-unit-tests has been configured with --target=kvmtool. >> >> Example invocation: >> >> $ ./configure --target=kvmtool >> $ make clean && make >> $ ./run_scripts.sh >> >> A custom location for the kvmtool binary can be set using the environment >> variable KVMTOOL: >> >> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh >> >> Standalone test support is absent, but will be added in subsequent patches. > So I skimmed over this, and it's at least better than what I had tried > a few years back ;-) > And while there might be other ways to sort out command line > differences between the two, this fixup_kvmtool_opts() looks like a > clever solution to the problem. > > The only problem with this patch is that it's rather big, I wonder if > this could be split up? For instance move any QEMU specific > functionality into separate functions first (like run_test_qemu()), > also use 'if [ "$TARGET" = "qemu" ]' in this first (or second?) patch. > Then later on just add the kvmtool specifics. You're right, this patch looks very big and difficult to review, it needs to be split into smaller chunks for the next iteration. Drew, did you manage to take a quick look at this patch? Do you think this is the right direction? I would prefer to hear your opinion about it before I start reworking it. Thanks, Alex > > Cheers, > Andre > >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >> --- >> scripts/arch-run.bash | 48 ++++++++++++++++-- >> scripts/runtime.bash | 94 ++++++++++++++++++++++++++++------ >> scripts/mkstandalone.sh | 5 ++ >> arm/run | 110 ++++++++++++++++++++++++---------------- >> run_tests.sh | 11 +++- >> 5 files changed, 204 insertions(+), 64 deletions(-) >> >> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash >> index 8ceed53ed7f8..b916b0e79aca 100644 >> --- a/scripts/arch-run.bash >> +++ b/scripts/arch-run.bash >> @@ -69,16 +69,39 @@ run_qemu () >> return $ret >> } >> >> +run_kvmtool() >> +{ >> + local stdout errors ret sig >> + >> + # kvmtool doesn't allow an initrd argument with --firmware, but configure >> + # sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target. >> + >> + # stdout to {stdout}, stderr to $errors and stderr >> + exec {stdout}>&1 >> + errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout) >> + ret=$? >> + exec {stdout}>&- >> + >> + # ret=0 success, everything else is failure. >> + return $ret >> +} >> + >> run_test_status () >> { >> - local stdout ret >> + local stdout ret exit_status >> >> exec {stdout}>&1 >> - lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) >> + if [ "$TARGET" = "kvmtool" ]; then >> + lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout)) >> + exit_status=0 >> + else >> + lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) >> + exit_status=1 >> + fi >> ret=$? >> exec {stdout}>&- >> >> - if [ $ret -eq 1 ]; then >> + if [ $ret -eq $exit_status ]; then >> testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/') >> if [ "$testret" ]; then >> if [ $testret -eq 1 ]; then >> @@ -193,6 +216,25 @@ search_qemu_binary () >> export PATH=$save_path >> } >> >> +search_kvmtool_binary () >> +{ >> + local lkvm kvmtool >> + >> + for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do >> + if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then >> + kvmtool="$lkvm" >> + break >> + fi >> + done >> + >> + if [ -z "$kvmtool" ]; then >> + echo "A kvmtool binary was not found." >&2 >> + echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2 >> + return 2 >> + fi >> + command -v $kvmtool >> +} >> + >> initrd_create () >> { >> if [ "$ENVIRON_DEFAULT" = "yes" ]; then >> diff --git a/scripts/runtime.bash b/scripts/runtime.bash >> index 132389c7dd59..23b238a6ab6f 100644 >> --- a/scripts/runtime.bash >> +++ b/scripts/runtime.bash >> @@ -12,14 +12,19 @@ extract_summary() >> tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/' >> } >> >> -# We assume that QEMU is going to work if it tried to load the kernel >> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel >> premature_failure() >> { >> local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)" >> >> - echo "$log" | grep "_NO_FILE_4Uhere_" | >> - grep -q -e "could not \(load\|open\) kernel" -e "error loading" && >> - return 1 >> + if [ "$TARGET" = "kvmtool" ]; then >> + echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" && >> + return 1 >> + else >> + echo "$log" | grep "_NO_FILE_4Uhere_" | >> + grep -q -e "could not \(load\|open\) kernel" -e "error loading" && >> + return 1 >> + fi >> >> RUNTIME_log_stderr <<< "$log" >> >> @@ -30,7 +35,14 @@ premature_failure() >> get_cmdline() >> { >> local kernel=$1 >> - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" >> + local smp_param >> + >> + if [ "$TARGET" = "kvmtool" ]; then >> + smp_param="--cpus $smp" >> + else >> + smp_param="-smp $smp" >> + fi >> + echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts" >> } >> >> skip_nodefault() >> @@ -70,6 +82,35 @@ function find_word() >> grep -Fq " $1 " <<< " $2 " >> } >> >> +fixup_kvmtool_opts() >> +{ >> + local opts=$1 >> + local groups=$2 >> + local gic >> + local gic_version >> + >> + if find_word "pmu" $groups; then >> + opts+=" --pmu" >> + fi >> + >> + if find_word "its" $groups; then >> + gic_version=3 >> + gic="gicv3-its" >> + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then >> + gic_version="${BASH_REMATCH[1]}" >> + gic="gicv$gic_version" >> + fi >> + >> + if [ -n "$gic" ]; then >> + opts=${opts/-machine gic-version=$gic_version/} >> + opts+=" --irqchip=$gic" >> + fi >> + >> + opts=${opts/-append/--params} >> + >> + echo "$opts" >> +} >> + >> function run() >> { >> local testname="$1" >> @@ -105,7 +146,12 @@ function run() >> return 2 >> fi >> >> - if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then >> + if [ "$TARGET" = "kvmtool" ]; then >> + if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then >> + print_result "SKIP" $testname "" "$accel not supported by kvmtool" >> + return 2 >> + fi >> + elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then >> print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL" >> return 2 >> elif [ -n "$ACCEL" ]; then >> @@ -126,6 +172,10 @@ function run() >> done >> fi >> >> + if [ "$TARGET" = "kvmtool" ]; then >> + opts=$(fixup_kvmtool_opts "$opts" "$groups") >> + fi >> + >> last_line=$(premature_failure > >(tail -1)) && { >> print_result "SKIP" $testname "" "$last_line" >> return 77 >> @@ -165,13 +215,25 @@ function run() >> # >> # Probe for MAX_SMP, in case it's less than the number of host cpus. >> # >> -# This probing currently only works for ARM, as x86 bails on another >> -# error first. Also, this probing isn't necessary for any ARM hosts >> -# running kernels later than v4.3, i.e. those including ef748917b52 >> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some >> -# point when maintaining the while loop gets too tiresome, we can >> -# just remove it... >> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ >> - |& grep -qi 'exceeds max CPUs'; do >> - MAX_SMP=$((MAX_SMP >> 1)) >> -done >> +# This probing currently only works for ARM, as x86 bails on another error >> +# first. Also, this probing isn't necessary for any ARM hosts running kernels >> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove >> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop >> +# gets too tiresome, we can just remove it... >> +# >> +# We don't need this check for kvmtool, as kvmtool will automatically limit the >> +# number of VCPUs to what the host supports instead of exiting with an error. >> +# kvmtool prints a message when that happens, but it's harmless and the chance >> +# of running a kernel so old that the number of VCPUs is smaller than the number >> +# of physical CPUs is vanishingly small. >> +# >> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0, >> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test >> +# is running on a recent x86 machine, there's a fairly good chance that more >> +# than 8 logical CPUs are available. >> +if [ "$TARGET" = "qemu" ]; then >> + while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ >> + |& grep -qi 'exceeds max CPUs'; do >> + MAX_SMP=$((MAX_SMP >> 1)) >> + done >> +fi >> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh >> index cefdec30cb33..16f461c06842 100755 >> --- a/scripts/mkstandalone.sh >> +++ b/scripts/mkstandalone.sh >> @@ -95,6 +95,11 @@ function mkstandalone() >> echo Written $standalone. >> } >> >> +if [ "$TARGET" = "kvmtool" ]; then >> + echo "Standalone tests not supported with kvmtool" >> + exit 2 >> +fi >> + >> if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 >> exit 2 >> diff --git a/arm/run b/arm/run >> index a390ca5ae0ba..cc5890e7fec4 100755 >> --- a/arm/run >> +++ b/arm/run >> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then >> source config.mak >> source scripts/arch-run.bash >> fi >> -processor="$PROCESSOR" >> >> -ACCEL=$(get_qemu_accelerator) || >> - exit $? >> +run_test_qemu() >> +{ >> + processor="$PROCESSOR" >> >> -qemu=$(search_qemu_binary) || >> - exit $? >> + ACCEL=$(get_qemu_accelerator) || >> + exit $? >> >> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then >> - echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." >> - exit 2 >> -fi >> + qemu=$(search_qemu_binary) || >> + exit $? >> >> -M='-machine virt' >> + if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then >> + echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." >> + exit 2 >> + fi >> >> -if [ "$ACCEL" = "kvm" ]; then >> - if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then >> - M+=',gic-version=host' >> - fi >> - if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then >> - processor="host" >> - if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then >> - processor+=",aarch64=off" >> - fi >> - fi >> -fi >> + M='-machine virt' >> >> -if [ "$ARCH" = "arm" ]; then >> - M+=",highmem=off" >> -fi >> + if [ "$ACCEL" = "kvm" ]; then >> + if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then >> + M+=',gic-version=host' >> + fi >> + if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then >> + processor="host" >> + if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then >> + processor+=",aarch64=off" >> + fi >> + fi >> + fi >> >> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then >> - echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." >> - exit 2 >> -fi >> + if [ "$ARCH" = "arm" ]; then >> + M+=",highmem=off" >> + fi >> >> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ >> - | grep backend > /dev/null; then >> - echo "$qemu doesn't support chr-testdev. Exiting." >> - exit 2 >> -fi >> + if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then >> + echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." >> + exit 2 >> + fi >> >> -chr_testdev='-device virtio-serial-device' >> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' >> + if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ >> + | grep backend > /dev/null; then >> + echo "$qemu doesn't support chr-testdev. Exiting." >> + exit 2 >> + fi >> >> -pci_testdev= >> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then >> - pci_testdev="-device pci-testdev" >> -fi >> + chr_testdev='-device virtio-serial-device' >> + chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' >> + >> + pci_testdev= >> + if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then >> + pci_testdev="-device pci-testdev" >> + fi >> + >> + M+=",accel=$ACCEL" >> + command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" >> + command+=" -display none -serial stdio -kernel" >> + command="$(migration_cmd) $(timeout_cmd) $command" >> + >> + run_qemu $command "$@" >> +} >> + >> +run_test_kvmtool() >> +{ >> + kvmtool=$(search_kvmtool_binary) || >> + exit $? >> >> -M+=",accel=$ACCEL" >> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" >> -command+=" -display none -serial stdio -kernel" >> -command="$(migration_cmd) $(timeout_cmd) $command" >> + local command="$(timeout_cmd) $kvmtool run --firmware " >> + run_test_status $command "$@" >> +} >> >> -run_qemu $command "$@" >> +case "$TARGET" in >> + "qemu") >> + run_test_qemu "$@" >> + ;; >> + "kvmtool") >> + run_test_kvmtool "$@" >> + ;; >> +esac >> diff --git a/run_tests.sh b/run_tests.sh >> index 65108e73a2c0..b010ee3ab348 100755 >> --- a/run_tests.sh >> +++ b/run_tests.sh >> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t] >> -t, --tap13 Output test results in TAP format >> >> Set the environment variable QEMU=/path/to/qemu-system-ARCH to >> -specify the appropriate qemu binary for ARCH-run. >> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool >> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool >> +can be used to specify a custom location for the kvmtool binary. >> >> EOF >> } >> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then >> exit 1 >> fi >> >> +if [ "$TARGET" = "kvmtool" ]; then >> + if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then >> + echo "kvmtool supports only the kvm accelerator" >> + exit 1 >> + fi >> +fi >> + >> only_tests="" >> args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*` >> [ $? -ne 0 ] && exit 2; _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-09-06 10:28 ` Alexandru Elisei @ 2021-09-06 11:01 ` Andrew Jones 2021-09-06 11:07 ` Alexandru Elisei 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-06 11:01 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, Andre Przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Mon, Sep 06, 2021 at 11:28:28AM +0100, Alexandru Elisei wrote: > Hi Andre, > > Thanks for having a look! > > On 7/12/21 5:52 PM, Andre Przywara wrote: > > On Fri, 2 Jul 2021 17:31:20 +0100 > > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > > > Hi Alex, > > > >> Modify run_tests.sh to use kvmtool instead of qemu to run tests when > >> kvm-unit-tests has been configured with --target=kvmtool. > >> > >> Example invocation: > >> > >> $ ./configure --target=kvmtool > >> $ make clean && make > >> $ ./run_scripts.sh > >> > >> A custom location for the kvmtool binary can be set using the environment > >> variable KVMTOOL: > >> > >> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh > >> > >> Standalone test support is absent, but will be added in subsequent patches. > > So I skimmed over this, and it's at least better than what I had tried > > a few years back ;-) > > And while there might be other ways to sort out command line > > differences between the two, this fixup_kvmtool_opts() looks like a > > clever solution to the problem. > > > > The only problem with this patch is that it's rather big, I wonder if > > this could be split up? For instance move any QEMU specific > > functionality into separate functions first (like run_test_qemu()), > > also use 'if [ "$TARGET" = "qemu" ]' in this first (or second?) patch. > > Then later on just add the kvmtool specifics. > > You're right, this patch looks very big and difficult to review, it needs to be > split into smaller chunks for the next iteration. > > Drew, did you manage to take a quick look at this patch? Do you think this is the > right direction? I would prefer to hear your opinion about it before I start > reworking it. Afraid not. I'll prioritize finishing my review of this series though and try to do it today or tomorrow. Thanks, drew > > Thanks, > > Alex > > > > > Cheers, > > Andre > > > >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > >> --- > >> scripts/arch-run.bash | 48 ++++++++++++++++-- > >> scripts/runtime.bash | 94 ++++++++++++++++++++++++++++------ > >> scripts/mkstandalone.sh | 5 ++ > >> arm/run | 110 ++++++++++++++++++++++++---------------- > >> run_tests.sh | 11 +++- > >> 5 files changed, 204 insertions(+), 64 deletions(-) > >> > >> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > >> index 8ceed53ed7f8..b916b0e79aca 100644 > >> --- a/scripts/arch-run.bash > >> +++ b/scripts/arch-run.bash > >> @@ -69,16 +69,39 @@ run_qemu () > >> return $ret > >> } > >> > >> +run_kvmtool() > >> +{ > >> + local stdout errors ret sig > >> + > >> + # kvmtool doesn't allow an initrd argument with --firmware, but configure > >> + # sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target. > >> + > >> + # stdout to {stdout}, stderr to $errors and stderr > >> + exec {stdout}>&1 > >> + errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout) > >> + ret=$? > >> + exec {stdout}>&- > >> + > >> + # ret=0 success, everything else is failure. > >> + return $ret > >> +} > >> + > >> run_test_status () > >> { > >> - local stdout ret > >> + local stdout ret exit_status > >> > >> exec {stdout}>&1 > >> - lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) > >> + if [ "$TARGET" = "kvmtool" ]; then > >> + lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout)) > >> + exit_status=0 > >> + else > >> + lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) > >> + exit_status=1 > >> + fi > >> ret=$? > >> exec {stdout}>&- > >> > >> - if [ $ret -eq 1 ]; then > >> + if [ $ret -eq $exit_status ]; then > >> testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/') > >> if [ "$testret" ]; then > >> if [ $testret -eq 1 ]; then > >> @@ -193,6 +216,25 @@ search_qemu_binary () > >> export PATH=$save_path > >> } > >> > >> +search_kvmtool_binary () > >> +{ > >> + local lkvm kvmtool > >> + > >> + for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do > >> + if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then > >> + kvmtool="$lkvm" > >> + break > >> + fi > >> + done > >> + > >> + if [ -z "$kvmtool" ]; then > >> + echo "A kvmtool binary was not found." >&2 > >> + echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2 > >> + return 2 > >> + fi > >> + command -v $kvmtool > >> +} > >> + > >> initrd_create () > >> { > >> if [ "$ENVIRON_DEFAULT" = "yes" ]; then > >> diff --git a/scripts/runtime.bash b/scripts/runtime.bash > >> index 132389c7dd59..23b238a6ab6f 100644 > >> --- a/scripts/runtime.bash > >> +++ b/scripts/runtime.bash > >> @@ -12,14 +12,19 @@ extract_summary() > >> tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/' > >> } > >> > >> -# We assume that QEMU is going to work if it tried to load the kernel > >> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel > >> premature_failure() > >> { > >> local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)" > >> > >> - echo "$log" | grep "_NO_FILE_4Uhere_" | > >> - grep -q -e "could not \(load\|open\) kernel" -e "error loading" && > >> - return 1 > >> + if [ "$TARGET" = "kvmtool" ]; then > >> + echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" && > >> + return 1 > >> + else > >> + echo "$log" | grep "_NO_FILE_4Uhere_" | > >> + grep -q -e "could not \(load\|open\) kernel" -e "error loading" && > >> + return 1 > >> + fi > >> > >> RUNTIME_log_stderr <<< "$log" > >> > >> @@ -30,7 +35,14 @@ premature_failure() > >> get_cmdline() > >> { > >> local kernel=$1 > >> - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" > >> + local smp_param > >> + > >> + if [ "$TARGET" = "kvmtool" ]; then > >> + smp_param="--cpus $smp" > >> + else > >> + smp_param="-smp $smp" > >> + fi > >> + echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts" > >> } > >> > >> skip_nodefault() > >> @@ -70,6 +82,35 @@ function find_word() > >> grep -Fq " $1 " <<< " $2 " > >> } > >> > >> +fixup_kvmtool_opts() > >> +{ > >> + local opts=$1 > >> + local groups=$2 > >> + local gic > >> + local gic_version > >> + > >> + if find_word "pmu" $groups; then > >> + opts+=" --pmu" > >> + fi > >> + > >> + if find_word "its" $groups; then > >> + gic_version=3 > >> + gic="gicv3-its" > >> + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then > >> + gic_version="${BASH_REMATCH[1]}" > >> + gic="gicv$gic_version" > >> + fi > >> + > >> + if [ -n "$gic" ]; then > >> + opts=${opts/-machine gic-version=$gic_version/} > >> + opts+=" --irqchip=$gic" > >> + fi > >> + > >> + opts=${opts/-append/--params} > >> + > >> + echo "$opts" > >> +} > >> + > >> function run() > >> { > >> local testname="$1" > >> @@ -105,7 +146,12 @@ function run() > >> return 2 > >> fi > >> > >> - if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then > >> + if [ "$TARGET" = "kvmtool" ]; then > >> + if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then > >> + print_result "SKIP" $testname "" "$accel not supported by kvmtool" > >> + return 2 > >> + fi > >> + elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then > >> print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL" > >> return 2 > >> elif [ -n "$ACCEL" ]; then > >> @@ -126,6 +172,10 @@ function run() > >> done > >> fi > >> > >> + if [ "$TARGET" = "kvmtool" ]; then > >> + opts=$(fixup_kvmtool_opts "$opts" "$groups") > >> + fi > >> + > >> last_line=$(premature_failure > >(tail -1)) && { > >> print_result "SKIP" $testname "" "$last_line" > >> return 77 > >> @@ -165,13 +215,25 @@ function run() > >> # > >> # Probe for MAX_SMP, in case it's less than the number of host cpus. > >> # > >> -# This probing currently only works for ARM, as x86 bails on another > >> -# error first. Also, this probing isn't necessary for any ARM hosts > >> -# running kernels later than v4.3, i.e. those including ef748917b52 > >> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some > >> -# point when maintaining the while loop gets too tiresome, we can > >> -# just remove it... > >> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ > >> - |& grep -qi 'exceeds max CPUs'; do > >> - MAX_SMP=$((MAX_SMP >> 1)) > >> -done > >> +# This probing currently only works for ARM, as x86 bails on another error > >> +# first. Also, this probing isn't necessary for any ARM hosts running kernels > >> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove > >> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop > >> +# gets too tiresome, we can just remove it... > >> +# > >> +# We don't need this check for kvmtool, as kvmtool will automatically limit the > >> +# number of VCPUs to what the host supports instead of exiting with an error. > >> +# kvmtool prints a message when that happens, but it's harmless and the chance > >> +# of running a kernel so old that the number of VCPUs is smaller than the number > >> +# of physical CPUs is vanishingly small. > >> +# > >> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0, > >> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test > >> +# is running on a recent x86 machine, there's a fairly good chance that more > >> +# than 8 logical CPUs are available. > >> +if [ "$TARGET" = "qemu" ]; then > >> + while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ > >> + |& grep -qi 'exceeds max CPUs'; do > >> + MAX_SMP=$((MAX_SMP >> 1)) > >> + done > >> +fi > >> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh > >> index cefdec30cb33..16f461c06842 100755 > >> --- a/scripts/mkstandalone.sh > >> +++ b/scripts/mkstandalone.sh > >> @@ -95,6 +95,11 @@ function mkstandalone() > >> echo Written $standalone. > >> } > >> > >> +if [ "$TARGET" = "kvmtool" ]; then > >> + echo "Standalone tests not supported with kvmtool" > >> + exit 2 > >> +fi > >> + > >> if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > >> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 > >> exit 2 > >> diff --git a/arm/run b/arm/run > >> index a390ca5ae0ba..cc5890e7fec4 100755 > >> --- a/arm/run > >> +++ b/arm/run > >> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then > >> source config.mak > >> source scripts/arch-run.bash > >> fi > >> -processor="$PROCESSOR" > >> > >> -ACCEL=$(get_qemu_accelerator) || > >> - exit $? > >> +run_test_qemu() > >> +{ > >> + processor="$PROCESSOR" > >> > >> -qemu=$(search_qemu_binary) || > >> - exit $? > >> + ACCEL=$(get_qemu_accelerator) || > >> + exit $? > >> > >> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then > >> - echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." > >> - exit 2 > >> -fi > >> + qemu=$(search_qemu_binary) || > >> + exit $? > >> > >> -M='-machine virt' > >> + if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then > >> + echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." > >> + exit 2 > >> + fi > >> > >> -if [ "$ACCEL" = "kvm" ]; then > >> - if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then > >> - M+=',gic-version=host' > >> - fi > >> - if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then > >> - processor="host" > >> - if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then > >> - processor+=",aarch64=off" > >> - fi > >> - fi > >> -fi > >> + M='-machine virt' > >> > >> -if [ "$ARCH" = "arm" ]; then > >> - M+=",highmem=off" > >> -fi > >> + if [ "$ACCEL" = "kvm" ]; then > >> + if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then > >> + M+=',gic-version=host' > >> + fi > >> + if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then > >> + processor="host" > >> + if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then > >> + processor+=",aarch64=off" > >> + fi > >> + fi > >> + fi > >> > >> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then > >> - echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." > >> - exit 2 > >> -fi > >> + if [ "$ARCH" = "arm" ]; then > >> + M+=",highmem=off" > >> + fi > >> > >> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ > >> - | grep backend > /dev/null; then > >> - echo "$qemu doesn't support chr-testdev. Exiting." > >> - exit 2 > >> -fi > >> + if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then > >> + echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." > >> + exit 2 > >> + fi > >> > >> -chr_testdev='-device virtio-serial-device' > >> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' > >> + if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ > >> + | grep backend > /dev/null; then > >> + echo "$qemu doesn't support chr-testdev. Exiting." > >> + exit 2 > >> + fi > >> > >> -pci_testdev= > >> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then > >> - pci_testdev="-device pci-testdev" > >> -fi > >> + chr_testdev='-device virtio-serial-device' > >> + chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' > >> + > >> + pci_testdev= > >> + if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then > >> + pci_testdev="-device pci-testdev" > >> + fi > >> + > >> + M+=",accel=$ACCEL" > >> + command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" > >> + command+=" -display none -serial stdio -kernel" > >> + command="$(migration_cmd) $(timeout_cmd) $command" > >> + > >> + run_qemu $command "$@" > >> +} > >> + > >> +run_test_kvmtool() > >> +{ > >> + kvmtool=$(search_kvmtool_binary) || > >> + exit $? > >> > >> -M+=",accel=$ACCEL" > >> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" > >> -command+=" -display none -serial stdio -kernel" > >> -command="$(migration_cmd) $(timeout_cmd) $command" > >> + local command="$(timeout_cmd) $kvmtool run --firmware " > >> + run_test_status $command "$@" > >> +} > >> > >> -run_qemu $command "$@" > >> +case "$TARGET" in > >> + "qemu") > >> + run_test_qemu "$@" > >> + ;; > >> + "kvmtool") > >> + run_test_kvmtool "$@" > >> + ;; > >> +esac > >> diff --git a/run_tests.sh b/run_tests.sh > >> index 65108e73a2c0..b010ee3ab348 100755 > >> --- a/run_tests.sh > >> +++ b/run_tests.sh > >> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t] > >> -t, --tap13 Output test results in TAP format > >> > >> Set the environment variable QEMU=/path/to/qemu-system-ARCH to > >> -specify the appropriate qemu binary for ARCH-run. > >> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool > >> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool > >> +can be used to specify a custom location for the kvmtool binary. > >> > >> EOF > >> } > >> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then > >> exit 1 > >> fi > >> > >> +if [ "$TARGET" = "kvmtool" ]; then > >> + if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then > >> + echo "kvmtool supports only the kvm accelerator" > >> + exit 1 > >> + fi > >> +fi > >> + > >> only_tests="" > >> args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*` > >> [ $? -ne 0 ] && exit 2; > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-09-06 11:01 ` Andrew Jones @ 2021-09-06 11:07 ` Alexandru Elisei 0 siblings, 0 replies; 36+ messages in thread From: Alexandru Elisei @ 2021-09-06 11:07 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, Andre Przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, On 9/6/21 12:01 PM, Andrew Jones wrote: > On Mon, Sep 06, 2021 at 11:28:28AM +0100, Alexandru Elisei wrote: >> Hi Andre, >> >> Thanks for having a look! >> >> On 7/12/21 5:52 PM, Andre Przywara wrote: >>> On Fri, 2 Jul 2021 17:31:20 +0100 >>> Alexandru Elisei <alexandru.elisei@arm.com> wrote: >>> >>> Hi Alex, >>> >>>> Modify run_tests.sh to use kvmtool instead of qemu to run tests when >>>> kvm-unit-tests has been configured with --target=kvmtool. >>>> >>>> Example invocation: >>>> >>>> $ ./configure --target=kvmtool >>>> $ make clean && make >>>> $ ./run_scripts.sh >>>> >>>> A custom location for the kvmtool binary can be set using the environment >>>> variable KVMTOOL: >>>> >>>> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh >>>> >>>> Standalone test support is absent, but will be added in subsequent patches. >>> So I skimmed over this, and it's at least better than what I had tried >>> a few years back ;-) >>> And while there might be other ways to sort out command line >>> differences between the two, this fixup_kvmtool_opts() looks like a >>> clever solution to the problem. >>> >>> The only problem with this patch is that it's rather big, I wonder if >>> this could be split up? For instance move any QEMU specific >>> functionality into separate functions first (like run_test_qemu()), >>> also use 'if [ "$TARGET" = "qemu" ]' in this first (or second?) patch. >>> Then later on just add the kvmtool specifics. >> You're right, this patch looks very big and difficult to review, it needs to be >> split into smaller chunks for the next iteration. >> >> Drew, did you manage to take a quick look at this patch? Do you think this is the >> right direction? I would prefer to hear your opinion about it before I start >> reworking it. > Afraid not. I'll prioritize finishing my review of this series though and > try to do it today or tomorrow. Sounds great, thank you! Thanks, Alex > > Thanks, > drew > >> Thanks, >> >> Alex >> >>> Cheers, >>> Andre >>> >>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >>>> --- >>>> scripts/arch-run.bash | 48 ++++++++++++++++-- >>>> scripts/runtime.bash | 94 ++++++++++++++++++++++++++++------ >>>> scripts/mkstandalone.sh | 5 ++ >>>> arm/run | 110 ++++++++++++++++++++++++---------------- >>>> run_tests.sh | 11 +++- >>>> 5 files changed, 204 insertions(+), 64 deletions(-) >>>> >>>> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash >>>> index 8ceed53ed7f8..b916b0e79aca 100644 >>>> --- a/scripts/arch-run.bash >>>> +++ b/scripts/arch-run.bash >>>> @@ -69,16 +69,39 @@ run_qemu () >>>> return $ret >>>> } >>>> >>>> +run_kvmtool() >>>> +{ >>>> + local stdout errors ret sig >>>> + >>>> + # kvmtool doesn't allow an initrd argument with --firmware, but configure >>>> + # sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target. >>>> + >>>> + # stdout to {stdout}, stderr to $errors and stderr >>>> + exec {stdout}>&1 >>>> + errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout) >>>> + ret=$? >>>> + exec {stdout}>&- >>>> + >>>> + # ret=0 success, everything else is failure. >>>> + return $ret >>>> +} >>>> + >>>> run_test_status () >>>> { >>>> - local stdout ret >>>> + local stdout ret exit_status >>>> >>>> exec {stdout}>&1 >>>> - lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) >>>> + if [ "$TARGET" = "kvmtool" ]; then >>>> + lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout)) >>>> + exit_status=0 >>>> + else >>>> + lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) >>>> + exit_status=1 >>>> + fi >>>> ret=$? >>>> exec {stdout}>&- >>>> >>>> - if [ $ret -eq 1 ]; then >>>> + if [ $ret -eq $exit_status ]; then >>>> testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/') >>>> if [ "$testret" ]; then >>>> if [ $testret -eq 1 ]; then >>>> @@ -193,6 +216,25 @@ search_qemu_binary () >>>> export PATH=$save_path >>>> } >>>> >>>> +search_kvmtool_binary () >>>> +{ >>>> + local lkvm kvmtool >>>> + >>>> + for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do >>>> + if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then >>>> + kvmtool="$lkvm" >>>> + break >>>> + fi >>>> + done >>>> + >>>> + if [ -z "$kvmtool" ]; then >>>> + echo "A kvmtool binary was not found." >&2 >>>> + echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2 >>>> + return 2 >>>> + fi >>>> + command -v $kvmtool >>>> +} >>>> + >>>> initrd_create () >>>> { >>>> if [ "$ENVIRON_DEFAULT" = "yes" ]; then >>>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash >>>> index 132389c7dd59..23b238a6ab6f 100644 >>>> --- a/scripts/runtime.bash >>>> +++ b/scripts/runtime.bash >>>> @@ -12,14 +12,19 @@ extract_summary() >>>> tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/' >>>> } >>>> >>>> -# We assume that QEMU is going to work if it tried to load the kernel >>>> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel >>>> premature_failure() >>>> { >>>> local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)" >>>> >>>> - echo "$log" | grep "_NO_FILE_4Uhere_" | >>>> - grep -q -e "could not \(load\|open\) kernel" -e "error loading" && >>>> - return 1 >>>> + if [ "$TARGET" = "kvmtool" ]; then >>>> + echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" && >>>> + return 1 >>>> + else >>>> + echo "$log" | grep "_NO_FILE_4Uhere_" | >>>> + grep -q -e "could not \(load\|open\) kernel" -e "error loading" && >>>> + return 1 >>>> + fi >>>> >>>> RUNTIME_log_stderr <<< "$log" >>>> >>>> @@ -30,7 +35,14 @@ premature_failure() >>>> get_cmdline() >>>> { >>>> local kernel=$1 >>>> - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" >>>> + local smp_param >>>> + >>>> + if [ "$TARGET" = "kvmtool" ]; then >>>> + smp_param="--cpus $smp" >>>> + else >>>> + smp_param="-smp $smp" >>>> + fi >>>> + echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts" >>>> } >>>> >>>> skip_nodefault() >>>> @@ -70,6 +82,35 @@ function find_word() >>>> grep -Fq " $1 " <<< " $2 " >>>> } >>>> >>>> +fixup_kvmtool_opts() >>>> +{ >>>> + local opts=$1 >>>> + local groups=$2 >>>> + local gic >>>> + local gic_version >>>> + >>>> + if find_word "pmu" $groups; then >>>> + opts+=" --pmu" >>>> + fi >>>> + >>>> + if find_word "its" $groups; then >>>> + gic_version=3 >>>> + gic="gicv3-its" >>>> + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then >>>> + gic_version="${BASH_REMATCH[1]}" >>>> + gic="gicv$gic_version" >>>> + fi >>>> + >>>> + if [ -n "$gic" ]; then >>>> + opts=${opts/-machine gic-version=$gic_version/} >>>> + opts+=" --irqchip=$gic" >>>> + fi >>>> + >>>> + opts=${opts/-append/--params} >>>> + >>>> + echo "$opts" >>>> +} >>>> + >>>> function run() >>>> { >>>> local testname="$1" >>>> @@ -105,7 +146,12 @@ function run() >>>> return 2 >>>> fi >>>> >>>> - if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then >>>> + if [ "$TARGET" = "kvmtool" ]; then >>>> + if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then >>>> + print_result "SKIP" $testname "" "$accel not supported by kvmtool" >>>> + return 2 >>>> + fi >>>> + elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then >>>> print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL" >>>> return 2 >>>> elif [ -n "$ACCEL" ]; then >>>> @@ -126,6 +172,10 @@ function run() >>>> done >>>> fi >>>> >>>> + if [ "$TARGET" = "kvmtool" ]; then >>>> + opts=$(fixup_kvmtool_opts "$opts" "$groups") >>>> + fi >>>> + >>>> last_line=$(premature_failure > >(tail -1)) && { >>>> print_result "SKIP" $testname "" "$last_line" >>>> return 77 >>>> @@ -165,13 +215,25 @@ function run() >>>> # >>>> # Probe for MAX_SMP, in case it's less than the number of host cpus. >>>> # >>>> -# This probing currently only works for ARM, as x86 bails on another >>>> -# error first. Also, this probing isn't necessary for any ARM hosts >>>> -# running kernels later than v4.3, i.e. those including ef748917b52 >>>> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some >>>> -# point when maintaining the while loop gets too tiresome, we can >>>> -# just remove it... >>>> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ >>>> - |& grep -qi 'exceeds max CPUs'; do >>>> - MAX_SMP=$((MAX_SMP >> 1)) >>>> -done >>>> +# This probing currently only works for ARM, as x86 bails on another error >>>> +# first. Also, this probing isn't necessary for any ARM hosts running kernels >>>> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove >>>> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop >>>> +# gets too tiresome, we can just remove it... >>>> +# >>>> +# We don't need this check for kvmtool, as kvmtool will automatically limit the >>>> +# number of VCPUs to what the host supports instead of exiting with an error. >>>> +# kvmtool prints a message when that happens, but it's harmless and the chance >>>> +# of running a kernel so old that the number of VCPUs is smaller than the number >>>> +# of physical CPUs is vanishingly small. >>>> +# >>>> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0, >>>> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test >>>> +# is running on a recent x86 machine, there's a fairly good chance that more >>>> +# than 8 logical CPUs are available. >>>> +if [ "$TARGET" = "qemu" ]; then >>>> + while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ >>>> + |& grep -qi 'exceeds max CPUs'; do >>>> + MAX_SMP=$((MAX_SMP >> 1)) >>>> + done >>>> +fi >>>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh >>>> index cefdec30cb33..16f461c06842 100755 >>>> --- a/scripts/mkstandalone.sh >>>> +++ b/scripts/mkstandalone.sh >>>> @@ -95,6 +95,11 @@ function mkstandalone() >>>> echo Written $standalone. >>>> } >>>> >>>> +if [ "$TARGET" = "kvmtool" ]; then >>>> + echo "Standalone tests not supported with kvmtool" >>>> + exit 2 >>>> +fi >>>> + >>>> if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >>>> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 >>>> exit 2 >>>> diff --git a/arm/run b/arm/run >>>> index a390ca5ae0ba..cc5890e7fec4 100755 >>>> --- a/arm/run >>>> +++ b/arm/run >>>> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then >>>> source config.mak >>>> source scripts/arch-run.bash >>>> fi >>>> -processor="$PROCESSOR" >>>> >>>> -ACCEL=$(get_qemu_accelerator) || >>>> - exit $? >>>> +run_test_qemu() >>>> +{ >>>> + processor="$PROCESSOR" >>>> >>>> -qemu=$(search_qemu_binary) || >>>> - exit $? >>>> + ACCEL=$(get_qemu_accelerator) || >>>> + exit $? >>>> >>>> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then >>>> - echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." >>>> - exit 2 >>>> -fi >>>> + qemu=$(search_qemu_binary) || >>>> + exit $? >>>> >>>> -M='-machine virt' >>>> + if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then >>>> + echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." >>>> + exit 2 >>>> + fi >>>> >>>> -if [ "$ACCEL" = "kvm" ]; then >>>> - if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then >>>> - M+=',gic-version=host' >>>> - fi >>>> - if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then >>>> - processor="host" >>>> - if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then >>>> - processor+=",aarch64=off" >>>> - fi >>>> - fi >>>> -fi >>>> + M='-machine virt' >>>> >>>> -if [ "$ARCH" = "arm" ]; then >>>> - M+=",highmem=off" >>>> -fi >>>> + if [ "$ACCEL" = "kvm" ]; then >>>> + if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then >>>> + M+=',gic-version=host' >>>> + fi >>>> + if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then >>>> + processor="host" >>>> + if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then >>>> + processor+=",aarch64=off" >>>> + fi >>>> + fi >>>> + fi >>>> >>>> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then >>>> - echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." >>>> - exit 2 >>>> -fi >>>> + if [ "$ARCH" = "arm" ]; then >>>> + M+=",highmem=off" >>>> + fi >>>> >>>> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ >>>> - | grep backend > /dev/null; then >>>> - echo "$qemu doesn't support chr-testdev. Exiting." >>>> - exit 2 >>>> -fi >>>> + if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then >>>> + echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." >>>> + exit 2 >>>> + fi >>>> >>>> -chr_testdev='-device virtio-serial-device' >>>> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' >>>> + if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ >>>> + | grep backend > /dev/null; then >>>> + echo "$qemu doesn't support chr-testdev. Exiting." >>>> + exit 2 >>>> + fi >>>> >>>> -pci_testdev= >>>> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then >>>> - pci_testdev="-device pci-testdev" >>>> -fi >>>> + chr_testdev='-device virtio-serial-device' >>>> + chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' >>>> + >>>> + pci_testdev= >>>> + if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then >>>> + pci_testdev="-device pci-testdev" >>>> + fi >>>> + >>>> + M+=",accel=$ACCEL" >>>> + command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" >>>> + command+=" -display none -serial stdio -kernel" >>>> + command="$(migration_cmd) $(timeout_cmd) $command" >>>> + >>>> + run_qemu $command "$@" >>>> +} >>>> + >>>> +run_test_kvmtool() >>>> +{ >>>> + kvmtool=$(search_kvmtool_binary) || >>>> + exit $? >>>> >>>> -M+=",accel=$ACCEL" >>>> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" >>>> -command+=" -display none -serial stdio -kernel" >>>> -command="$(migration_cmd) $(timeout_cmd) $command" >>>> + local command="$(timeout_cmd) $kvmtool run --firmware " >>>> + run_test_status $command "$@" >>>> +} >>>> >>>> -run_qemu $command "$@" >>>> +case "$TARGET" in >>>> + "qemu") >>>> + run_test_qemu "$@" >>>> + ;; >>>> + "kvmtool") >>>> + run_test_kvmtool "$@" >>>> + ;; >>>> +esac >>>> diff --git a/run_tests.sh b/run_tests.sh >>>> index 65108e73a2c0..b010ee3ab348 100755 >>>> --- a/run_tests.sh >>>> +++ b/run_tests.sh >>>> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t] >>>> -t, --tap13 Output test results in TAP format >>>> >>>> Set the environment variable QEMU=/path/to/qemu-system-ARCH to >>>> -specify the appropriate qemu binary for ARCH-run. >>>> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool >>>> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool >>>> +can be used to specify a custom location for the kvmtool binary. >>>> >>>> EOF >>>> } >>>> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then >>>> exit 1 >>>> fi >>>> >>>> +if [ "$TARGET" = "kvmtool" ]; then >>>> + if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then >>>> + echo "kvmtool supports only the kvm accelerator" >>>> + exit 1 >>>> + fi >>>> +fi >>>> + >>>> only_tests="" >>>> args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*` >>>> [ $? -ne 0 ] && exit 2; _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support Alexandru Elisei 2021-07-12 16:52 ` Andre Przywara @ 2021-09-07 10:17 ` Andrew Jones 2021-09-08 14:33 ` Alexandru Elisei 1 sibling, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-07 10:17 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Fri, Jul 02, 2021 at 05:31:20PM +0100, Alexandru Elisei wrote: > Modify run_tests.sh to use kvmtool instead of qemu to run tests when > kvm-unit-tests has been configured with --target=kvmtool. > > Example invocation: > > $ ./configure --target=kvmtool > $ make clean && make > $ ./run_scripts.sh > > A custom location for the kvmtool binary can be set using the environment > variable KVMTOOL: > > $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh > > Standalone test support is absent, but will be added in subsequent patches. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > scripts/arch-run.bash | 48 ++++++++++++++++-- > scripts/runtime.bash | 94 ++++++++++++++++++++++++++++------ > scripts/mkstandalone.sh | 5 ++ > arm/run | 110 ++++++++++++++++++++++++---------------- > run_tests.sh | 11 +++- > 5 files changed, 204 insertions(+), 64 deletions(-) > > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index 8ceed53ed7f8..b916b0e79aca 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -69,16 +69,39 @@ run_qemu () > return $ret > } > > +run_kvmtool() > +{ > + local stdout errors ret sig > + > + # kvmtool doesn't allow an initrd argument with --firmware, but configure > + # sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target. > + > + # stdout to {stdout}, stderr to $errors and stderr > + exec {stdout}>&1 > + errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout) > + ret=$? > + exec {stdout}>&- > + > + # ret=0 success, everything else is failure. > + return $ret > +} > + > run_test_status () > { > - local stdout ret > + local stdout ret exit_status > > exec {stdout}>&1 > - lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) > + if [ "$TARGET" = "kvmtool" ]; then > + lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout)) > + exit_status=0 > + else > + lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) > + exit_status=1 > + fi Now that we're adding a second VMM, let's assume that we may want to add more. (Actually, I have low priority aspirations to try and also use a Rust VMM, for example). To prepare for another VMM, everywhere we add a kvmtool case like this, we should use a case statement instead, e.g. case "$TARGET" in qemu) lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) exit_status=1 ;; kvmtool) lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout)) exit_status=0 *) echo "$BASH_SOURCE $LINENO"; exit 2;; esac This means we also need to modify configure to unconditionally do echo "TARGET=$target" >> config.mak Right now it only does that for arm/arm64. > ret=$? > exec {stdout}>&- > > - if [ $ret -eq 1 ]; then > + if [ $ret -eq $exit_status ]; then > testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/') > if [ "$testret" ]; then > if [ $testret -eq 1 ]; then > @@ -193,6 +216,25 @@ search_qemu_binary () > export PATH=$save_path > } > > +search_kvmtool_binary () > +{ > + local lkvm kvmtool > + > + for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do > + if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then > + kvmtool="$lkvm" > + break > + fi > + done > + > + if [ -z "$kvmtool" ]; then > + echo "A kvmtool binary was not found." >&2 > + echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2 > + return 2 > + fi > + command -v $kvmtool > +} > + > initrd_create () > { > if [ "$ENVIRON_DEFAULT" = "yes" ]; then > diff --git a/scripts/runtime.bash b/scripts/runtime.bash > index 132389c7dd59..23b238a6ab6f 100644 > --- a/scripts/runtime.bash > +++ b/scripts/runtime.bash > @@ -12,14 +12,19 @@ extract_summary() > tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/' > } > > -# We assume that QEMU is going to work if it tried to load the kernel > +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel assume that the VMM > premature_failure() > { > local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)" > > - echo "$log" | grep "_NO_FILE_4Uhere_" | > - grep -q -e "could not \(load\|open\) kernel" -e "error loading" && > - return 1 > + if [ "$TARGET" = "kvmtool" ]; then > + echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" && > + return 1 > + else > + echo "$log" | grep "_NO_FILE_4Uhere_" | > + grep -q -e "could not \(load\|open\) kernel" -e "error loading" && > + return 1 > + fi Another place for a switch. > > RUNTIME_log_stderr <<< "$log" > > @@ -30,7 +35,14 @@ premature_failure() > get_cmdline() > { > local kernel=$1 > - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" > + local smp_param > + > + if [ "$TARGET" = "kvmtool" ]; then > + smp_param="--cpus $smp" > + else > + smp_param="-smp $smp" > + fi switch > + echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts" > } > > skip_nodefault() > @@ -70,6 +82,35 @@ function find_word() > grep -Fq " $1 " <<< " $2 " > } > > +fixup_kvmtool_opts() > +{ > + local opts=$1 > + local groups=$2 > + local gic > + local gic_version > + > + if find_word "pmu" $groups; then > + opts+=" --pmu" > + fi > + > + if find_word "its" $groups; then > + gic_version=3 > + gic="gicv3-its" > + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then > + gic_version="${BASH_REMATCH[1]}" > + gic="gicv$gic_version" > + fi > + > + if [ -n "$gic" ]; then > + opts=${opts/-machine gic-version=$gic_version/} > + opts+=" --irqchip=$gic" > + fi > + > + opts=${opts/-append/--params} > + > + echo "$opts" > +} Hmm, I don't think we want to write a QEMU parameter translator for all other VMMs, and all other VMM architectures, that we want to support. I think we should add new "extra_params" variables to the unittest configuration instead, e.g. "kvmtool_params", where the extra parameters can be listed correctly and explicitly. While at it, I would create an alias for "extra_params", which would be "qemu_params" allowing unittests that support more than one VMM to clearly show what's what. > + > function run() > { > local testname="$1" > @@ -105,7 +146,12 @@ function run() > return 2 > fi > > - if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then > + if [ "$TARGET" = "kvmtool" ]; then > + if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then > + print_result "SKIP" $testname "" "$accel not supported by kvmtool" > + return 2 > + fi > + elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then switch > print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL" > return 2 > elif [ -n "$ACCEL" ]; then > @@ -126,6 +172,10 @@ function run() > done > fi > > + if [ "$TARGET" = "kvmtool" ]; then > + opts=$(fixup_kvmtool_opts "$opts" "$groups") > + fi > + > last_line=$(premature_failure > >(tail -1)) && { > print_result "SKIP" $testname "" "$last_line" > return 77 > @@ -165,13 +215,25 @@ function run() > # > # Probe for MAX_SMP, in case it's less than the number of host cpus. > # > -# This probing currently only works for ARM, as x86 bails on another > -# error first. Also, this probing isn't necessary for any ARM hosts > -# running kernels later than v4.3, i.e. those including ef748917b52 > -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some > -# point when maintaining the while loop gets too tiresome, we can > -# just remove it... > -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ > - |& grep -qi 'exceeds max CPUs'; do > - MAX_SMP=$((MAX_SMP >> 1)) > -done > +# This probing currently only works for ARM, as x86 bails on another error > +# first. Also, this probing isn't necessary for any ARM hosts running kernels > +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove > +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop > +# gets too tiresome, we can just remove it... > +# > +# We don't need this check for kvmtool, as kvmtool will automatically limit the > +# number of VCPUs to what the host supports instead of exiting with an error. > +# kvmtool prints a message when that happens, but it's harmless and the chance > +# of running a kernel so old that the number of VCPUs is smaller than the number > +# of physical CPUs is vanishingly small. > +# > +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0, > +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test > +# is running on a recent x86 machine, there's a fairly good chance that more > +# than 8 logical CPUs are available. > +if [ "$TARGET" = "qemu" ]; then > + while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ > + |& grep -qi 'exceeds max CPUs'; do > + MAX_SMP=$((MAX_SMP >> 1)) > + done > +fi > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh > index cefdec30cb33..16f461c06842 100755 > --- a/scripts/mkstandalone.sh > +++ b/scripts/mkstandalone.sh > @@ -95,6 +95,11 @@ function mkstandalone() > echo Written $standalone. > } > > +if [ "$TARGET" = "kvmtool" ]; then > + echo "Standalone tests not supported with kvmtool" > + exit 2 > +fi > + > if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 > exit 2 > diff --git a/arm/run b/arm/run > index a390ca5ae0ba..cc5890e7fec4 100755 > --- a/arm/run > +++ b/arm/run > @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then > source config.mak > source scripts/arch-run.bash > fi > -processor="$PROCESSOR" > > -ACCEL=$(get_qemu_accelerator) || > - exit $? > +run_test_qemu() > +{ > + processor="$PROCESSOR" > > -qemu=$(search_qemu_binary) || > - exit $? > + ACCEL=$(get_qemu_accelerator) || > + exit $? > > -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then > - echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." > - exit 2 > -fi > + qemu=$(search_qemu_binary) || > + exit $? > > -M='-machine virt' > + if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then > + echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." > + exit 2 > + fi > > -if [ "$ACCEL" = "kvm" ]; then > - if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then > - M+=',gic-version=host' > - fi > - if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then > - processor="host" > - if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then > - processor+=",aarch64=off" > - fi > - fi > -fi > + M='-machine virt' > > -if [ "$ARCH" = "arm" ]; then > - M+=",highmem=off" > -fi > + if [ "$ACCEL" = "kvm" ]; then > + if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then > + M+=',gic-version=host' > + fi > + if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then > + processor="host" > + if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then > + processor+=",aarch64=off" > + fi > + fi > + fi > > -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then > - echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." > - exit 2 > -fi > + if [ "$ARCH" = "arm" ]; then > + M+=",highmem=off" > + fi > > -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ > - | grep backend > /dev/null; then > - echo "$qemu doesn't support chr-testdev. Exiting." > - exit 2 > -fi > + if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then > + echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." > + exit 2 > + fi > > -chr_testdev='-device virtio-serial-device' > -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' > + if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ > + | grep backend > /dev/null; then > + echo "$qemu doesn't support chr-testdev. Exiting." > + exit 2 > + fi > > -pci_testdev= > -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then > - pci_testdev="-device pci-testdev" > -fi > + chr_testdev='-device virtio-serial-device' > + chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' > + > + pci_testdev= > + if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then > + pci_testdev="-device pci-testdev" > + fi > + > + M+=",accel=$ACCEL" > + command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" > + command+=" -display none -serial stdio -kernel" > + command="$(migration_cmd) $(timeout_cmd) $command" > + > + run_qemu $command "$@" > +} > + > +run_test_kvmtool() > +{ > + kvmtool=$(search_kvmtool_binary) || > + exit $? > > -M+=",accel=$ACCEL" > -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" > -command+=" -display none -serial stdio -kernel" > -command="$(migration_cmd) $(timeout_cmd) $command" > + local command="$(timeout_cmd) $kvmtool run --firmware " > + run_test_status $command "$@" > +} > > -run_qemu $command "$@" > +case "$TARGET" in > + "qemu") > + run_test_qemu "$@" > + ;; > + "kvmtool") > + run_test_kvmtool "$@" > + ;; > +esac Hey, here's a switch :-) But I'd add a default case that errors out for good measure. > diff --git a/run_tests.sh b/run_tests.sh > index 65108e73a2c0..b010ee3ab348 100755 > --- a/run_tests.sh > +++ b/run_tests.sh > @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t] > -t, --tap13 Output test results in TAP format > > Set the environment variable QEMU=/path/to/qemu-system-ARCH to > -specify the appropriate qemu binary for ARCH-run. > +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool > +is also supported and the environment variable KVMTOOL=/path/to/kvmtool > +can be used to specify a custom location for the kvmtool binary. How about changing this last sentence (as a separate patch first) in usage to ENVIRONMENT QEMU Path to QEMU binary for ARCH-run ACCEL QEMU accelerator to use, e.g. "kvm", "hvf" or "tcg" TIMEOUT Timeout duration for the timeout(1) command And then in this patch adds KVMTOOL Path to KVMTOOL binary for ARCH-run > > EOF > } > @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then > exit 1 > fi > > +if [ "$TARGET" = "kvmtool" ]; then > + if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then > + echo "kvmtool supports only the kvm accelerator" > + exit 1 > + fi > +fi I think this belongs in arm/run, because run_tests.sh isn't always used to run the tests. > + > only_tests="" > args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*` > [ $? -ne 0 ] && exit 2; > -- > 2.32.0 > Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-09-07 10:17 ` Andrew Jones @ 2021-09-08 14:33 ` Alexandru Elisei 2021-09-08 15:09 ` Andrew Jones 0 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-09-08 14:33 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, Thanks for having a look so quickly! On 9/7/21 11:17 AM, Andrew Jones wrote: > On Fri, Jul 02, 2021 at 05:31:20PM +0100, Alexandru Elisei wrote: >> Modify run_tests.sh to use kvmtool instead of qemu to run tests when >> kvm-unit-tests has been configured with --target=kvmtool. >> >> Example invocation: >> >> $ ./configure --target=kvmtool >> $ make clean && make >> $ ./run_scripts.sh >> >> A custom location for the kvmtool binary can be set using the environment >> variable KVMTOOL: >> >> $ KVMTOOL=/path/to/kvmtool/binary ./run_scripts.sh >> >> Standalone test support is absent, but will be added in subsequent patches. >> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >> --- >> scripts/arch-run.bash | 48 ++++++++++++++++-- >> scripts/runtime.bash | 94 ++++++++++++++++++++++++++++------ >> scripts/mkstandalone.sh | 5 ++ >> arm/run | 110 ++++++++++++++++++++++++---------------- >> run_tests.sh | 11 +++- >> 5 files changed, 204 insertions(+), 64 deletions(-) >> >> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash >> index 8ceed53ed7f8..b916b0e79aca 100644 >> --- a/scripts/arch-run.bash >> +++ b/scripts/arch-run.bash >> @@ -69,16 +69,39 @@ run_qemu () >> return $ret >> } >> >> +run_kvmtool() >> +{ >> + local stdout errors ret sig >> + >> + # kvmtool doesn't allow an initrd argument with --firmware, but configure >> + # sets CONFIG_ERRATA_FORCE in lib/config.h for the kvmtool target. >> + >> + # stdout to {stdout}, stderr to $errors and stderr >> + exec {stdout}>&1 >> + errors=$("${@}" </dev/null 2> >(tee /dev/stderr) > /dev/fd/$stdout) >> + ret=$? >> + exec {stdout}>&- >> + >> + # ret=0 success, everything else is failure. >> + return $ret >> +} >> + >> run_test_status () >> { >> - local stdout ret >> + local stdout ret exit_status >> >> exec {stdout}>&1 >> - lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) >> + if [ "$TARGET" = "kvmtool" ]; then >> + lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout)) >> + exit_status=0 >> + else >> + lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) >> + exit_status=1 >> + fi > Now that we're adding a second VMM, let's assume that we may want to add > more. (Actually, I have low priority aspirations to try and also use a > Rust VMM, for example). To prepare for another VMM, everywhere we add a > kvmtool case like this, we should use a case statement instead, e.g. > > case "$TARGET" in > qemu) > lines=$(run_qemu "$@" > >(tee /dev/fd/$stdout)) > exit_status=1 > ;; > kvmtool) > lines=$(run_kvmtool "$@" > >(tee /dev/fd/$stdout)) > exit_status=0 > *) echo "$BASH_SOURCE $LINENO"; exit 2;; > esac > > This means we also need to modify configure to unconditionally do > > echo "TARGET=$target" >> config.mak > > Right now it only does that for arm/arm64. Yep, using a case statement makes more sense. I'll update everywhere you've pointed out. I'll modify configure to set the TARGET variable unconditionally in config.mak. > >> ret=$? >> exec {stdout}>&- >> >> - if [ $ret -eq 1 ]; then >> + if [ $ret -eq $exit_status ]; then >> testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/') >> if [ "$testret" ]; then >> if [ $testret -eq 1 ]; then >> @@ -193,6 +216,25 @@ search_qemu_binary () >> export PATH=$save_path >> } >> >> +search_kvmtool_binary () >> +{ >> + local lkvm kvmtool >> + >> + for lkvm in ${KVMTOOL:-lkvm vm lkvm-static}; do >> + if $lkvm --help 2>/dev/null | grep -q 'The most commonly used'; then >> + kvmtool="$lkvm" >> + break >> + fi >> + done >> + >> + if [ -z "$kvmtool" ]; then >> + echo "A kvmtool binary was not found." >&2 >> + echo "You can set a custom location by using the KVMTOOL=<path> environment variable." >&2 >> + return 2 >> + fi >> + command -v $kvmtool >> +} >> + >> initrd_create () >> { >> if [ "$ENVIRON_DEFAULT" = "yes" ]; then >> diff --git a/scripts/runtime.bash b/scripts/runtime.bash >> index 132389c7dd59..23b238a6ab6f 100644 >> --- a/scripts/runtime.bash >> +++ b/scripts/runtime.bash >> @@ -12,14 +12,19 @@ extract_summary() >> tail -3 | grep '^SUMMARY: ' | sed 's/^SUMMARY: /(/;s/'"$cr"'\{0,1\}$/)/' >> } >> >> -# We assume that QEMU is going to work if it tried to load the kernel >> +# We assume that QEMU/kvmtool is going to work if it tried to load the kernel > assume that the VMM Got it. > >> premature_failure() >> { >> local log="$(eval $(get_cmdline _NO_FILE_4Uhere_) 2>&1)" >> >> - echo "$log" | grep "_NO_FILE_4Uhere_" | >> - grep -q -e "could not \(load\|open\) kernel" -e "error loading" && >> - return 1 >> + if [ "$TARGET" = "kvmtool" ]; then >> + echo "$log" | grep "Fatal: unable to load firmware image _NO_FILE_4Uhere_" && >> + return 1 >> + else >> + echo "$log" | grep "_NO_FILE_4Uhere_" | >> + grep -q -e "could not \(load\|open\) kernel" -e "error loading" && >> + return 1 >> + fi > Another place for a switch. > >> >> RUNTIME_log_stderr <<< "$log" >> >> @@ -30,7 +35,14 @@ premature_failure() >> get_cmdline() >> { >> local kernel=$1 >> - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" >> + local smp_param >> + >> + if [ "$TARGET" = "kvmtool" ]; then >> + smp_param="--cpus $smp" >> + else >> + smp_param="-smp $smp" >> + fi > switch > >> + echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel $smp_param $opts" >> } >> >> skip_nodefault() >> @@ -70,6 +82,35 @@ function find_word() >> grep -Fq " $1 " <<< " $2 " >> } >> >> +fixup_kvmtool_opts() >> +{ >> + local opts=$1 >> + local groups=$2 >> + local gic >> + local gic_version >> + >> + if find_word "pmu" $groups; then >> + opts+=" --pmu" >> + fi >> + >> + if find_word "its" $groups; then >> + gic_version=3 >> + gic="gicv3-its" >> + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then >> + gic_version="${BASH_REMATCH[1]}" >> + gic="gicv$gic_version" >> + fi >> + >> + if [ -n "$gic" ]; then >> + opts=${opts/-machine gic-version=$gic_version/} >> + opts+=" --irqchip=$gic" >> + fi >> + >> + opts=${opts/-append/--params} >> + >> + echo "$opts" >> +} > Hmm, I don't think we want to write a QEMU parameter translator for > all other VMMs, and all other VMM architectures, that we want to > support. I think we should add new "extra_params" variables to the > unittest configuration instead, e.g. "kvmtool_params", where the > extra parameters can be listed correctly and explicitly. While at > it, I would create an alias for "extra_params", which would be > "qemu_params" allowing unittests that support more than one VMM > to clearly show what's what. I agree, this is a much better idea than a parameter translator. Using a dedicated variable in unittests.cfg will make it easier for new tests to get support for all VMMs (for example, writing a list of parameters in unittests.cfg should be easier than digging through the scripts to figure exactly how and where to add a translation for a new parameter), and it allow us to express parameters for other VMMs which don't have a direct correspondent in qemu. By creating an alias, do you mean replacing extra_params with qemu_params in arm/unittests.cfg? Or something else? > >> + >> function run() >> { >> local testname="$1" >> @@ -105,7 +146,12 @@ function run() >> return 2 >> fi >> >> - if [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then >> + if [ "$TARGET" = "kvmtool" ]; then >> + if [ -n "$accel" ] && [ "$accel" != "kvm" ]; then >> + print_result "SKIP" $testname "" "$accel not supported by kvmtool" >> + return 2 >> + fi >> + elif [ -n "$accel" ] && [ -n "$ACCEL" ] && [ "$accel" != "$ACCEL" ]; then > switch > >> print_result "SKIP" $testname "" "$accel only, but ACCEL=$ACCEL" >> return 2 >> elif [ -n "$ACCEL" ]; then >> @@ -126,6 +172,10 @@ function run() >> done >> fi >> >> + if [ "$TARGET" = "kvmtool" ]; then >> + opts=$(fixup_kvmtool_opts "$opts" "$groups") >> + fi >> + >> last_line=$(premature_failure > >(tail -1)) && { >> print_result "SKIP" $testname "" "$last_line" >> return 77 >> @@ -165,13 +215,25 @@ function run() >> # >> # Probe for MAX_SMP, in case it's less than the number of host cpus. >> # >> -# This probing currently only works for ARM, as x86 bails on another >> -# error first. Also, this probing isn't necessary for any ARM hosts >> -# running kernels later than v4.3, i.e. those including ef748917b52 >> -# "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some >> -# point when maintaining the while loop gets too tiresome, we can >> -# just remove it... >> -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ >> - |& grep -qi 'exceeds max CPUs'; do >> - MAX_SMP=$((MAX_SMP >> 1)) >> -done >> +# This probing currently only works for ARM, as x86 bails on another error >> +# first. Also, this probing isn't necessary for any ARM hosts running kernels >> +# later than v4.3, i.e. those including ef748917b52 "arm/arm64: KVM: Remove >> +# 'config KVM_ARM_MAX_VCPUS'". So, at some point when maintaining the while loop >> +# gets too tiresome, we can just remove it... >> +# >> +# We don't need this check for kvmtool, as kvmtool will automatically limit the >> +# number of VCPUs to what the host supports instead of exiting with an error. >> +# kvmtool prints a message when that happens, but it's harmless and the chance >> +# of running a kernel so old that the number of VCPUs is smaller than the number >> +# of physical CPUs is vanishingly small. >> +# >> +# For qemu this check is still needed. For qemu-system-aarch64 version 6.0.0, >> +# using TCG, the maximum number of VCPUs that mach-virt supports is 8. If a test >> +# is running on a recent x86 machine, there's a fairly good chance that more >> +# than 8 logical CPUs are available. >> +if [ "$TARGET" = "qemu" ]; then >> + while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ >> + |& grep -qi 'exceeds max CPUs'; do >> + MAX_SMP=$((MAX_SMP >> 1)) >> + done >> +fi >> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh >> index cefdec30cb33..16f461c06842 100755 >> --- a/scripts/mkstandalone.sh >> +++ b/scripts/mkstandalone.sh >> @@ -95,6 +95,11 @@ function mkstandalone() >> echo Written $standalone. >> } >> >> +if [ "$TARGET" = "kvmtool" ]; then >> + echo "Standalone tests not supported with kvmtool" >> + exit 2 >> +fi >> + >> if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 >> exit 2 >> diff --git a/arm/run b/arm/run >> index a390ca5ae0ba..cc5890e7fec4 100755 >> --- a/arm/run >> +++ b/arm/run >> @@ -8,59 +8,81 @@ if [ -z "$STANDALONE" ]; then >> source config.mak >> source scripts/arch-run.bash >> fi >> -processor="$PROCESSOR" >> >> -ACCEL=$(get_qemu_accelerator) || >> - exit $? >> +run_test_qemu() >> +{ >> + processor="$PROCESSOR" >> >> -qemu=$(search_qemu_binary) || >> - exit $? >> + ACCEL=$(get_qemu_accelerator) || >> + exit $? >> >> -if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then >> - echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." >> - exit 2 >> -fi >> + qemu=$(search_qemu_binary) || >> + exit $? >> >> -M='-machine virt' >> + if ! $qemu -machine '?' 2>&1 | grep 'ARM Virtual Machine' > /dev/null; then >> + echo "$qemu doesn't support mach-virt ('-machine virt'). Exiting." >> + exit 2 >> + fi >> >> -if [ "$ACCEL" = "kvm" ]; then >> - if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then >> - M+=',gic-version=host' >> - fi >> - if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then >> - processor="host" >> - if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then >> - processor+=",aarch64=off" >> - fi >> - fi >> -fi >> + M='-machine virt' >> >> -if [ "$ARCH" = "arm" ]; then >> - M+=",highmem=off" >> -fi >> + if [ "$ACCEL" = "kvm" ]; then >> + if $qemu $M,\? 2>&1 | grep gic-version > /dev/null; then >> + M+=',gic-version=host' >> + fi >> + if [ "$HOST" = "aarch64" ] || [ "$HOST" = "arm" ]; then >> + processor="host" >> + if [ "$ARCH" = "arm" ] && [ "$HOST" = "aarch64" ]; then >> + processor+=",aarch64=off" >> + fi >> + fi >> + fi >> >> -if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then >> - echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." >> - exit 2 >> -fi >> + if [ "$ARCH" = "arm" ]; then >> + M+=",highmem=off" >> + fi >> >> -if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ >> - | grep backend > /dev/null; then >> - echo "$qemu doesn't support chr-testdev. Exiting." >> - exit 2 >> -fi >> + if ! $qemu $M -device '?' 2>&1 | grep virtconsole > /dev/null; then >> + echo "$qemu doesn't support virtio-console for chr-testdev. Exiting." >> + exit 2 >> + fi >> >> -chr_testdev='-device virtio-serial-device' >> -chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' >> + if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ >> + | grep backend > /dev/null; then >> + echo "$qemu doesn't support chr-testdev. Exiting." >> + exit 2 >> + fi >> >> -pci_testdev= >> -if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then >> - pci_testdev="-device pci-testdev" >> -fi >> + chr_testdev='-device virtio-serial-device' >> + chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' >> + >> + pci_testdev= >> + if $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then >> + pci_testdev="-device pci-testdev" >> + fi >> + >> + M+=",accel=$ACCEL" >> + command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" >> + command+=" -display none -serial stdio -kernel" >> + command="$(migration_cmd) $(timeout_cmd) $command" >> + >> + run_qemu $command "$@" >> +} >> + >> +run_test_kvmtool() >> +{ >> + kvmtool=$(search_kvmtool_binary) || >> + exit $? >> >> -M+=",accel=$ACCEL" >> -command="$qemu -nodefaults $M -cpu $processor $chr_testdev $pci_testdev" >> -command+=" -display none -serial stdio -kernel" >> -command="$(migration_cmd) $(timeout_cmd) $command" >> + local command="$(timeout_cmd) $kvmtool run --firmware " >> + run_test_status $command "$@" >> +} >> >> -run_qemu $command "$@" >> +case "$TARGET" in >> + "qemu") >> + run_test_qemu "$@" >> + ;; >> + "kvmtool") >> + run_test_kvmtool "$@" >> + ;; >> +esac > Hey, here's a switch :-) But I'd add a default case that errors out for > good measure. Yep, I think I made a mental note to replace if/else with this kind of switch statement, but it slipped my mind in the end. > >> diff --git a/run_tests.sh b/run_tests.sh >> index 65108e73a2c0..b010ee3ab348 100755 >> --- a/run_tests.sh >> +++ b/run_tests.sh >> @@ -26,7 +26,9 @@ Usage: $0 [-h] [-v] [-a] [-g group] [-j NUM-TASKS] [-t] >> -t, --tap13 Output test results in TAP format >> >> Set the environment variable QEMU=/path/to/qemu-system-ARCH to >> -specify the appropriate qemu binary for ARCH-run. >> +specify the appropriate qemu binary for ARCH-run. For arm/arm64, kvmtool >> +is also supported and the environment variable KVMTOOL=/path/to/kvmtool >> +can be used to specify a custom location for the kvmtool binary. > How about changing this last sentence (as a separate patch first) in usage > to > > ENVIRONMENT > QEMU Path to QEMU binary for ARCH-run > ACCEL QEMU accelerator to use, e.g. "kvm", "hvf" or "tcg" > TIMEOUT Timeout duration for the timeout(1) command > > And then in this patch adds > > KVMTOOL Path to KVMTOOL binary for ARCH-run Sounds good, will do. > >> >> EOF >> } >> @@ -41,6 +43,13 @@ if [ $? -ne 4 ]; then >> exit 1 >> fi >> >> +if [ "$TARGET" = "kvmtool" ]; then >> + if [ -n "$ACCEL" ] && [ "$ACCEL" != "kvm" ]; then >> + echo "kvmtool supports only the kvm accelerator" >> + exit 1 >> + fi >> +fi > I think this belongs in arm/run, because run_tests.sh isn't always used to > run the tests. Ok, I'll have a look and move this where it belongs. Thanks, Alex > >> + >> only_tests="" >> args=`getopt -u -o ag:htj:v -l all,group:,help,tap13,parallel:,verbose -- $*` >> [ $? -ne 0 ] && exit 2; >> -- >> 2.32.0 >> > Thanks, > drew > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-09-08 14:33 ` Alexandru Elisei @ 2021-09-08 15:09 ` Andrew Jones 2021-09-08 15:46 ` Alexandru Elisei 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-08 15:09 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote: ... > >> +fixup_kvmtool_opts() > >> +{ > >> + local opts=$1 > >> + local groups=$2 > >> + local gic > >> + local gic_version > >> + > >> + if find_word "pmu" $groups; then > >> + opts+=" --pmu" > >> + fi > >> + > >> + if find_word "its" $groups; then > >> + gic_version=3 > >> + gic="gicv3-its" > >> + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then > >> + gic_version="${BASH_REMATCH[1]}" > >> + gic="gicv$gic_version" > >> + fi > >> + > >> + if [ -n "$gic" ]; then > >> + opts=${opts/-machine gic-version=$gic_version/} > >> + opts+=" --irqchip=$gic" > >> + fi > >> + > >> + opts=${opts/-append/--params} > >> + > >> + echo "$opts" > >> +} > > Hmm, I don't think we want to write a QEMU parameter translator for > > all other VMMs, and all other VMM architectures, that we want to > > support. I think we should add new "extra_params" variables to the > > unittest configuration instead, e.g. "kvmtool_params", where the > > extra parameters can be listed correctly and explicitly. While at > > it, I would create an alias for "extra_params", which would be > > "qemu_params" allowing unittests that support more than one VMM > > to clearly show what's what. > > I agree, this is a much better idea than a parameter translator. Using a dedicated > variable in unittests.cfg will make it easier for new tests to get support for all > VMMs (for example, writing a list of parameters in unittests.cfg should be easier > than digging through the scripts to figure exactly how and where to add a > translation for a new parameter), and it allow us to express parameters for other > VMMs which don't have a direct correspondent in qemu. > > By creating an alias, do you mean replacing extra_params with qemu_params in > arm/unittests.cfg? Or something else? Probably something like this diff --git a/scripts/common.bash b/scripts/common.bash index 7b983f7d6dd6..e5119ff216e5 100644 --- a/scripts/common.bash +++ b/scripts/common.bash @@ -37,7 +37,12 @@ function for_each_unittest() elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then smp=${BASH_REMATCH[1]} elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then - opts=${BASH_REMATCH[1]} + elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then + qemu_opts=${BASH_REMATCH[1]} + elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then + qemu_opts=${BASH_REMATCH[1]} + elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then + kvmtool_opts=${BASH_REMATCH[1]} elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then groups=${BASH_REMATCH[1]} elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then and all other changes needed to support the s/opts/qemu_opts/ change should work. Also, an addition to the unittests.cfg documentation. The above diff doesn't consider that a unittests.cfg file could have both an 'extra_params' and a 'qemu_params' field, but I'm not sure we care about that. Users should read the documentation and we should review changes to the committed unittests.cfg files to avoid that. Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-09-08 15:09 ` Andrew Jones @ 2021-09-08 15:46 ` Alexandru Elisei 2021-09-08 15:49 ` Andrew Jones 0 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-09-08 15:46 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, On 9/8/21 4:09 PM, Andrew Jones wrote: > On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote: > ... >>>> +fixup_kvmtool_opts() >>>> +{ >>>> + local opts=$1 >>>> + local groups=$2 >>>> + local gic >>>> + local gic_version >>>> + >>>> + if find_word "pmu" $groups; then >>>> + opts+=" --pmu" >>>> + fi >>>> + >>>> + if find_word "its" $groups; then >>>> + gic_version=3 >>>> + gic="gicv3-its" >>>> + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then >>>> + gic_version="${BASH_REMATCH[1]}" >>>> + gic="gicv$gic_version" >>>> + fi >>>> + >>>> + if [ -n "$gic" ]; then >>>> + opts=${opts/-machine gic-version=$gic_version/} >>>> + opts+=" --irqchip=$gic" >>>> + fi >>>> + >>>> + opts=${opts/-append/--params} >>>> + >>>> + echo "$opts" >>>> +} >>> Hmm, I don't think we want to write a QEMU parameter translator for >>> all other VMMs, and all other VMM architectures, that we want to >>> support. I think we should add new "extra_params" variables to the >>> unittest configuration instead, e.g. "kvmtool_params", where the >>> extra parameters can be listed correctly and explicitly. While at >>> it, I would create an alias for "extra_params", which would be >>> "qemu_params" allowing unittests that support more than one VMM >>> to clearly show what's what. >> I agree, this is a much better idea than a parameter translator. Using a dedicated >> variable in unittests.cfg will make it easier for new tests to get support for all >> VMMs (for example, writing a list of parameters in unittests.cfg should be easier >> than digging through the scripts to figure exactly how and where to add a >> translation for a new parameter), and it allow us to express parameters for other >> VMMs which don't have a direct correspondent in qemu. >> >> By creating an alias, do you mean replacing extra_params with qemu_params in >> arm/unittests.cfg? Or something else? > Probably something like this > > diff --git a/scripts/common.bash b/scripts/common.bash > index 7b983f7d6dd6..e5119ff216e5 100644 > --- a/scripts/common.bash > +++ b/scripts/common.bash > @@ -37,7 +37,12 @@ function for_each_unittest() > elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then > smp=${BASH_REMATCH[1]} > elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then > - opts=${BASH_REMATCH[1]} > + elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then > + qemu_opts=${BASH_REMATCH[1]} > + elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then > + qemu_opts=${BASH_REMATCH[1]} > + elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then > + kvmtool_opts=${BASH_REMATCH[1]} > elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then > groups=${BASH_REMATCH[1]} > elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then > > and all other changes needed to support the s/opts/qemu_opts/ change > should work. Also, an addition to the unittests.cfg documentation. Got it, replace extra_opts with qemu_opts in the scripts. Yes, the documentation for unittests.cfg (at the top of the file) should definitely be updated to document the new configuration option, kvmtool_params. > > The above diff doesn't consider that a unittests.cfg file could have > both an 'extra_params' and a 'qemu_params' field, but I'm not sure > we care about that. Users should read the documentation and we > should review changes to the committed unittests.cfg files to avoid > that. What do you feel about renaming extra_params -> qemu_params in unittests.cfg? I'm thinking it would make the usage clearer, improve consistency (we would have qemu_params and kvmtool_params, instead of extra_params and kvmtool_params), and remove any confusions regarding when they are used (I can see someone thinking that extra_params are used all the time, and are appended to kvmtool_params when --target=kvmtool). On the other hand, this could be problematic for people using out-of-tree scripts that parse the unittest.cfg file for whatever reason (are there people that do that?). Thanks, Alex > > Thanks, > drew > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-09-08 15:46 ` Alexandru Elisei @ 2021-09-08 15:49 ` Andrew Jones 2021-09-09 11:33 ` Alexandru Elisei 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-08 15:49 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Wed, Sep 08, 2021 at 04:46:19PM +0100, Alexandru Elisei wrote: > Hi Drew, > > On 9/8/21 4:09 PM, Andrew Jones wrote: > > On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote: > > ... > >>>> +fixup_kvmtool_opts() > >>>> +{ > >>>> + local opts=$1 > >>>> + local groups=$2 > >>>> + local gic > >>>> + local gic_version > >>>> + > >>>> + if find_word "pmu" $groups; then > >>>> + opts+=" --pmu" > >>>> + fi > >>>> + > >>>> + if find_word "its" $groups; then > >>>> + gic_version=3 > >>>> + gic="gicv3-its" > >>>> + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then > >>>> + gic_version="${BASH_REMATCH[1]}" > >>>> + gic="gicv$gic_version" > >>>> + fi > >>>> + > >>>> + if [ -n "$gic" ]; then > >>>> + opts=${opts/-machine gic-version=$gic_version/} > >>>> + opts+=" --irqchip=$gic" > >>>> + fi > >>>> + > >>>> + opts=${opts/-append/--params} > >>>> + > >>>> + echo "$opts" > >>>> +} > >>> Hmm, I don't think we want to write a QEMU parameter translator for > >>> all other VMMs, and all other VMM architectures, that we want to > >>> support. I think we should add new "extra_params" variables to the > >>> unittest configuration instead, e.g. "kvmtool_params", where the > >>> extra parameters can be listed correctly and explicitly. While at > >>> it, I would create an alias for "extra_params", which would be > >>> "qemu_params" allowing unittests that support more than one VMM > >>> to clearly show what's what. > >> I agree, this is a much better idea than a parameter translator. Using a dedicated > >> variable in unittests.cfg will make it easier for new tests to get support for all > >> VMMs (for example, writing a list of parameters in unittests.cfg should be easier > >> than digging through the scripts to figure exactly how and where to add a > >> translation for a new parameter), and it allow us to express parameters for other > >> VMMs which don't have a direct correspondent in qemu. > >> > >> By creating an alias, do you mean replacing extra_params with qemu_params in > >> arm/unittests.cfg? Or something else? > > Probably something like this > > > > diff --git a/scripts/common.bash b/scripts/common.bash > > index 7b983f7d6dd6..e5119ff216e5 100644 > > --- a/scripts/common.bash > > +++ b/scripts/common.bash > > @@ -37,7 +37,12 @@ function for_each_unittest() > > elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then > > smp=${BASH_REMATCH[1]} > > elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then > > - opts=${BASH_REMATCH[1]} > > + elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then > > + qemu_opts=${BASH_REMATCH[1]} > > + elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then > > + qemu_opts=${BASH_REMATCH[1]} > > + elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then > > + kvmtool_opts=${BASH_REMATCH[1]} > > elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then > > groups=${BASH_REMATCH[1]} > > elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then > > > > and all other changes needed to support the s/opts/qemu_opts/ change > > should work. Also, an addition to the unittests.cfg documentation. > > Got it, replace extra_opts with qemu_opts in the scripts. > > Yes, the documentation for unittests.cfg (at the top of the file) should > definitely be updated to document the new configuration option, kvmtool_params. > > > > > The above diff doesn't consider that a unittests.cfg file could have > > both an 'extra_params' and a 'qemu_params' field, but I'm not sure > > we care about that. Users should read the documentation and we > > should review changes to the committed unittests.cfg files to avoid > > that. > > What do you feel about renaming extra_params -> qemu_params in unittests.cfg? Yes, that's what I would expect the patch to do. > I'm > thinking it would make the usage clearer, improve consistency (we would have > qemu_params and kvmtool_params, instead of extra_params and kvmtool_params), and > remove any confusions regarding when they are used (I can see someone thinking > that extra_params are used all the time, and are appended to kvmtool_params when > --target=kvmtool). On the other hand, this could be problematic for people using > out-of-tree scripts that parse the unittest.cfg file for whatever reason (are > there people that do that?). I'm not as worried about that as about people using out-of-tree unittests.cfg files that will break when the 'extra_params' field disappears. That's why I suggested to make 'extra_params' an alias. Thanks, drew > > Thanks, > > Alex > > > > > Thanks, > > drew > > > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-09-08 15:49 ` Andrew Jones @ 2021-09-09 11:33 ` Alexandru Elisei 2021-09-09 12:49 ` Andrew Jones 0 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-09-09 11:33 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, On 9/8/21 4:49 PM, Andrew Jones wrote: > On Wed, Sep 08, 2021 at 04:46:19PM +0100, Alexandru Elisei wrote: >> Hi Drew, >> >> On 9/8/21 4:09 PM, Andrew Jones wrote: >>> On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote: >>> ... >>>>>> +fixup_kvmtool_opts() >>>>>> +{ >>>>>> + local opts=$1 >>>>>> + local groups=$2 >>>>>> + local gic >>>>>> + local gic_version >>>>>> + >>>>>> + if find_word "pmu" $groups; then >>>>>> + opts+=" --pmu" >>>>>> + fi >>>>>> + >>>>>> + if find_word "its" $groups; then >>>>>> + gic_version=3 >>>>>> + gic="gicv3-its" >>>>>> + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then >>>>>> + gic_version="${BASH_REMATCH[1]}" >>>>>> + gic="gicv$gic_version" >>>>>> + fi >>>>>> + >>>>>> + if [ -n "$gic" ]; then >>>>>> + opts=${opts/-machine gic-version=$gic_version/} >>>>>> + opts+=" --irqchip=$gic" >>>>>> + fi >>>>>> + >>>>>> + opts=${opts/-append/--params} >>>>>> + >>>>>> + echo "$opts" >>>>>> +} >>>>> Hmm, I don't think we want to write a QEMU parameter translator for >>>>> all other VMMs, and all other VMM architectures, that we want to >>>>> support. I think we should add new "extra_params" variables to the >>>>> unittest configuration instead, e.g. "kvmtool_params", where the >>>>> extra parameters can be listed correctly and explicitly. While at >>>>> it, I would create an alias for "extra_params", which would be >>>>> "qemu_params" allowing unittests that support more than one VMM >>>>> to clearly show what's what. >>>> I agree, this is a much better idea than a parameter translator. Using a dedicated >>>> variable in unittests.cfg will make it easier for new tests to get support for all >>>> VMMs (for example, writing a list of parameters in unittests.cfg should be easier >>>> than digging through the scripts to figure exactly how and where to add a >>>> translation for a new parameter), and it allow us to express parameters for other >>>> VMMs which don't have a direct correspondent in qemu. >>>> >>>> By creating an alias, do you mean replacing extra_params with qemu_params in >>>> arm/unittests.cfg? Or something else? >>> Probably something like this >>> >>> diff --git a/scripts/common.bash b/scripts/common.bash >>> index 7b983f7d6dd6..e5119ff216e5 100644 >>> --- a/scripts/common.bash >>> +++ b/scripts/common.bash >>> @@ -37,7 +37,12 @@ function for_each_unittest() >>> elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then >>> smp=${BASH_REMATCH[1]} >>> elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then >>> - opts=${BASH_REMATCH[1]} >>> + elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then >>> + qemu_opts=${BASH_REMATCH[1]} >>> + elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then >>> + qemu_opts=${BASH_REMATCH[1]} >>> + elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then >>> + kvmtool_opts=${BASH_REMATCH[1]} >>> elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then >>> groups=${BASH_REMATCH[1]} >>> elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then >>> >>> and all other changes needed to support the s/opts/qemu_opts/ change >>> should work. Also, an addition to the unittests.cfg documentation. >> Got it, replace extra_opts with qemu_opts in the scripts. >> >> Yes, the documentation for unittests.cfg (at the top of the file) should >> definitely be updated to document the new configuration option, kvmtool_params. >> >>> The above diff doesn't consider that a unittests.cfg file could have >>> both an 'extra_params' and a 'qemu_params' field, but I'm not sure >>> we care about that. Users should read the documentation and we >>> should review changes to the committed unittests.cfg files to avoid >>> that. >> What do you feel about renaming extra_params -> qemu_params in unittests.cfg? > Yes, that's what I would expect the patch to do. > >> I'm >> thinking it would make the usage clearer, improve consistency (we would have >> qemu_params and kvmtool_params, instead of extra_params and kvmtool_params), and >> remove any confusions regarding when they are used (I can see someone thinking >> that extra_params are used all the time, and are appended to kvmtool_params when >> --target=kvmtool). On the other hand, this could be problematic for people using >> out-of-tree scripts that parse the unittest.cfg file for whatever reason (are >> there people that do that?). > I'm not as worried about that as about people using out-of-tree > unittests.cfg files that will break when the 'extra_params' field > disappears. That's why I suggested to make 'extra_params' an alias. I'm sorry, but I'm still having trouble parsing what alias means in this context. Do you mean keep extra_params for current tests, encourage qemu_params for new tests, document that they mean the same thing and going forward qemu_params should be used? Thanks, Alex > > Thanks, > drew > >> Thanks, >> >> Alex >> >>> Thanks, >>> drew >>> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support 2021-09-09 11:33 ` Alexandru Elisei @ 2021-09-09 12:49 ` Andrew Jones 0 siblings, 0 replies; 36+ messages in thread From: Andrew Jones @ 2021-09-09 12:49 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Thu, Sep 09, 2021 at 12:33:11PM +0100, Alexandru Elisei wrote: > Hi Drew, > > On 9/8/21 4:49 PM, Andrew Jones wrote: > > On Wed, Sep 08, 2021 at 04:46:19PM +0100, Alexandru Elisei wrote: > >> Hi Drew, > >> > >> On 9/8/21 4:09 PM, Andrew Jones wrote: > >>> On Wed, Sep 08, 2021 at 03:33:19PM +0100, Alexandru Elisei wrote: > >>> ... > >>>>>> +fixup_kvmtool_opts() > >>>>>> +{ > >>>>>> + local opts=$1 > >>>>>> + local groups=$2 > >>>>>> + local gic > >>>>>> + local gic_version > >>>>>> + > >>>>>> + if find_word "pmu" $groups; then > >>>>>> + opts+=" --pmu" > >>>>>> + fi > >>>>>> + > >>>>>> + if find_word "its" $groups; then > >>>>>> + gic_version=3 > >>>>>> + gic="gicv3-its" > >>>>>> + elif [[ "$opts" =~ -machine\ *gic-version=(2|3) ]]; then > >>>>>> + gic_version="${BASH_REMATCH[1]}" > >>>>>> + gic="gicv$gic_version" > >>>>>> + fi > >>>>>> + > >>>>>> + if [ -n "$gic" ]; then > >>>>>> + opts=${opts/-machine gic-version=$gic_version/} > >>>>>> + opts+=" --irqchip=$gic" > >>>>>> + fi > >>>>>> + > >>>>>> + opts=${opts/-append/--params} > >>>>>> + > >>>>>> + echo "$opts" > >>>>>> +} > >>>>> Hmm, I don't think we want to write a QEMU parameter translator for > >>>>> all other VMMs, and all other VMM architectures, that we want to > >>>>> support. I think we should add new "extra_params" variables to the > >>>>> unittest configuration instead, e.g. "kvmtool_params", where the > >>>>> extra parameters can be listed correctly and explicitly. While at > >>>>> it, I would create an alias for "extra_params", which would be > >>>>> "qemu_params" allowing unittests that support more than one VMM > >>>>> to clearly show what's what. > >>>> I agree, this is a much better idea than a parameter translator. Using a dedicated > >>>> variable in unittests.cfg will make it easier for new tests to get support for all > >>>> VMMs (for example, writing a list of parameters in unittests.cfg should be easier > >>>> than digging through the scripts to figure exactly how and where to add a > >>>> translation for a new parameter), and it allow us to express parameters for other > >>>> VMMs which don't have a direct correspondent in qemu. > >>>> > >>>> By creating an alias, do you mean replacing extra_params with qemu_params in > >>>> arm/unittests.cfg? Or something else? > >>> Probably something like this > >>> > >>> diff --git a/scripts/common.bash b/scripts/common.bash > >>> index 7b983f7d6dd6..e5119ff216e5 100644 > >>> --- a/scripts/common.bash > >>> +++ b/scripts/common.bash > >>> @@ -37,7 +37,12 @@ function for_each_unittest() > >>> elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then > >>> smp=${BASH_REMATCH[1]} > >>> elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then > >>> - opts=${BASH_REMATCH[1]} > >>> + elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then > >>> + qemu_opts=${BASH_REMATCH[1]} > >>> + elif [[ $line =~ ^qemu_params\ *=\ *(.*)$ ]]; then > >>> + qemu_opts=${BASH_REMATCH[1]} > >>> + elif [[ $line =~ ^kvmtool_params\ *=\ *(.*)$ ]]; then > >>> + kvmtool_opts=${BASH_REMATCH[1]} > >>> elif [[ $line =~ ^groups\ *=\ *(.*)$ ]]; then > >>> groups=${BASH_REMATCH[1]} > >>> elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then > >>> > >>> and all other changes needed to support the s/opts/qemu_opts/ change > >>> should work. Also, an addition to the unittests.cfg documentation. > >> Got it, replace extra_opts with qemu_opts in the scripts. > >> > >> Yes, the documentation for unittests.cfg (at the top of the file) should > >> definitely be updated to document the new configuration option, kvmtool_params. > >> > >>> The above diff doesn't consider that a unittests.cfg file could have > >>> both an 'extra_params' and a 'qemu_params' field, but I'm not sure > >>> we care about that. Users should read the documentation and we > >>> should review changes to the committed unittests.cfg files to avoid > >>> that. > >> What do you feel about renaming extra_params -> qemu_params in unittests.cfg? > > Yes, that's what I would expect the patch to do. > > > >> I'm > >> thinking it would make the usage clearer, improve consistency (we would have > >> qemu_params and kvmtool_params, instead of extra_params and kvmtool_params), and > >> remove any confusions regarding when they are used (I can see someone thinking > >> that extra_params are used all the time, and are appended to kvmtool_params when > >> --target=kvmtool). On the other hand, this could be problematic for people using > >> out-of-tree scripts that parse the unittest.cfg file for whatever reason (are > >> there people that do that?). > > I'm not as worried about that as about people using out-of-tree > > unittests.cfg files that will break when the 'extra_params' field > > disappears. That's why I suggested to make 'extra_params' an alias. > > I'm sorry, but I'm still having trouble parsing what alias means in this context. > Do you mean keep extra_params for current tests, encourage qemu_params for new > tests, document that they mean the same thing and going forward qemu_params should > be used? Exactly, which just amounts to keeping the parsing line elif [[ $line =~ ^extra_params\ *=\ *(.*)$ ]]; then qemu_opts=${BASH_REMATCH[1]} and some documentation changes. Thanks, drew > > Thanks, > > Alex > > > > > Thanks, > > drew > > > >> Thanks, > >> > >> Alex > >> > >>> Thanks, > >>> drew > >>> > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests 2021-07-02 16:31 [kvm-unit-tests RFC PATCH 0/5] arm: Add kvmtool to the runner script Alexandru Elisei ` (2 preceding siblings ...) 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support Alexandru Elisei @ 2021-07-02 16:31 ` Alexandru Elisei 2021-09-07 10:21 ` Andrew Jones 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool Alexandru Elisei 4 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-07-02 16:31 UTC (permalink / raw) To: drjones, thuth, pbonzini, lvivier, kvm-ppc, david, frankja, cohuck, imbrenda, linux-s390, kvm, kvmarm Cc: vivek.gautam, andre.przywara, maz Add support for the standalone target when running kvm-unit-tests under kvmtool. Example command line invocation: $ ./configure --target=kvmtool $ make clean && make standalone Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- scripts/mkstandalone.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh index 16f461c06842..d84bdb7e278c 100755 --- a/scripts/mkstandalone.sh +++ b/scripts/mkstandalone.sh @@ -44,6 +44,10 @@ generate_test () config_export ARCH_NAME config_export PROCESSOR + if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then + config_export TARGET + fi + echo "echo BUILD_HEAD=$(cat build-head)" if [ ! -f $kernel ]; then @@ -59,7 +63,7 @@ generate_test () echo 'export FIRMWARE' fi - if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then + if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then temp_file ERRATATXT "$ERRATATXT" echo 'export ERRATATXT' fi @@ -95,12 +99,8 @@ function mkstandalone() echo Written $standalone. } -if [ "$TARGET" = "kvmtool" ]; then - echo "Standalone tests not supported with kvmtool" - exit 2 -fi - -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \ + [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 exit 2 fi -- 2.32.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests Alexandru Elisei @ 2021-09-07 10:21 ` Andrew Jones 2021-09-08 15:37 ` Alexandru Elisei 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-07 10:21 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Fri, Jul 02, 2021 at 05:31:21PM +0100, Alexandru Elisei wrote: > Add support for the standalone target when running kvm-unit-tests under > kvmtool. > > Example command line invocation: > > $ ./configure --target=kvmtool > $ make clean && make standalone > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > scripts/mkstandalone.sh | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh > index 16f461c06842..d84bdb7e278c 100755 > --- a/scripts/mkstandalone.sh > +++ b/scripts/mkstandalone.sh > @@ -44,6 +44,10 @@ generate_test () > config_export ARCH_NAME > config_export PROCESSOR > > + if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then > + config_export TARGET > + fi Should export unconditionally, since we'll want TARGET set unconditionally. > + > echo "echo BUILD_HEAD=$(cat build-head)" > > if [ ! -f $kernel ]; then > @@ -59,7 +63,7 @@ generate_test () > echo 'export FIRMWARE' > fi > > - if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then > + if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then I think it would be better to ensure that ENVIRON_DEFAULT is "no" for TARGET=kvmtool in configure. > temp_file ERRATATXT "$ERRATATXT" > echo 'export ERRATATXT' > fi > @@ -95,12 +99,8 @@ function mkstandalone() > echo Written $standalone. > } > > -if [ "$TARGET" = "kvmtool" ]; then > - echo "Standalone tests not supported with kvmtool" > - exit 2 > -fi > - > -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \ > + [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 > exit 2 > fi > -- > 2.32.0 > Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests 2021-09-07 10:21 ` Andrew Jones @ 2021-09-08 15:37 ` Alexandru Elisei 2021-09-08 16:07 ` Andrew Jones 0 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-09-08 15:37 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, On 9/7/21 11:21 AM, Andrew Jones wrote: > On Fri, Jul 02, 2021 at 05:31:21PM +0100, Alexandru Elisei wrote: >> Add support for the standalone target when running kvm-unit-tests under >> kvmtool. >> >> Example command line invocation: >> >> $ ./configure --target=kvmtool >> $ make clean && make standalone >> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >> --- >> scripts/mkstandalone.sh | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh >> index 16f461c06842..d84bdb7e278c 100755 >> --- a/scripts/mkstandalone.sh >> +++ b/scripts/mkstandalone.sh >> @@ -44,6 +44,10 @@ generate_test () >> config_export ARCH_NAME >> config_export PROCESSOR >> >> + if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then >> + config_export TARGET >> + fi > Should export unconditionally, since we'll want TARGET set > unconditionally. Yes, will do. > >> + >> echo "echo BUILD_HEAD=$(cat build-head)" >> >> if [ ! -f $kernel ]; then >> @@ -59,7 +63,7 @@ generate_test () >> echo 'export FIRMWARE' >> fi >> >> - if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then >> + if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then > I think it would be better to ensure that ENVIRON_DEFAULT is "no" for > TARGET=kvmtool in configure. From looking at the code, it is my understanding that with ENVIRON_DEFAULT=yes, an initrd file is generated with the contents of erratatxt and other information, in a key=value pair format. This initrd is then passed on to the test (please correct me if I'm wrong). With ENVIRON_DEFAULT=no (set via ./configure --disable-default-environ), this initrd is not generated. kvmtool doesn't have support for passing an initrd when loading firmware, so yes, I believe the default should be no. However, I have two questions: 1. What happens when the user specifically enables the default environ via ./configure --enable-default-environ --target=kvmtool? In my opinion, that should be an error because the user wants something that is not possible with kvmtool (loading an image with --firmware in kvmtool means that the initrd image it not loaded into the guest memory and no node is generated for it in the dtb), but I would like to hear your thoughts about it. 2. If the default environment is disabled, is it still possible for an user to pass an initrd via other means? I couldn't find where that is implemented, so I'm guessing it's not possible. Thanks, Alex > > >> temp_file ERRATATXT "$ERRATATXT" >> echo 'export ERRATATXT' >> fi >> @@ -95,12 +99,8 @@ function mkstandalone() >> echo Written $standalone. >> } >> >> -if [ "$TARGET" = "kvmtool" ]; then >> - echo "Standalone tests not supported with kvmtool" >> - exit 2 >> -fi >> - >> -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >> +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \ >> + [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 >> exit 2 >> fi >> -- >> 2.32.0 >> > Thanks, > drew > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests 2021-09-08 15:37 ` Alexandru Elisei @ 2021-09-08 16:07 ` Andrew Jones 2021-09-09 11:11 ` Alexandru Elisei 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-08 16:07 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Wed, Sep 08, 2021 at 04:37:39PM +0100, Alexandru Elisei wrote: > Hi Drew, > > On 9/7/21 11:21 AM, Andrew Jones wrote: > > On Fri, Jul 02, 2021 at 05:31:21PM +0100, Alexandru Elisei wrote: > >> Add support for the standalone target when running kvm-unit-tests under > >> kvmtool. > >> > >> Example command line invocation: > >> > >> $ ./configure --target=kvmtool > >> $ make clean && make standalone > >> > >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > >> --- > >> scripts/mkstandalone.sh | 14 +++++++------- > >> 1 file changed, 7 insertions(+), 7 deletions(-) > >> > >> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh > >> index 16f461c06842..d84bdb7e278c 100755 > >> --- a/scripts/mkstandalone.sh > >> +++ b/scripts/mkstandalone.sh > >> @@ -44,6 +44,10 @@ generate_test () > >> config_export ARCH_NAME > >> config_export PROCESSOR > >> > >> + if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then > >> + config_export TARGET > >> + fi > > Should export unconditionally, since we'll want TARGET set > > unconditionally. > > Yes, will do. > > > > >> + > >> echo "echo BUILD_HEAD=$(cat build-head)" > >> > >> if [ ! -f $kernel ]; then > >> @@ -59,7 +63,7 @@ generate_test () > >> echo 'export FIRMWARE' > >> fi > >> > >> - if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then > >> + if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then > > I think it would be better to ensure that ENVIRON_DEFAULT is "no" for > > TARGET=kvmtool in configure. > > From looking at the code, it is my understanding that with ENVIRON_DEFAULT=yes, an > initrd file is generated with the contents of erratatxt and other information, in > a key=value pair format. This initrd is then passed on to the test (please correct > me if I'm wrong). With ENVIRON_DEFAULT=no (set via ./configure > --disable-default-environ), this initrd is not generated. > > kvmtool doesn't have support for passing an initrd when loading firmware, so yes, > I believe the default should be no. > > However, I have two questions: > > 1. What happens when the user specifically enables the default environ via > ./configure --enable-default-environ --target=kvmtool? In my opinion, that should > be an error because the user wants something that is not possible with kvmtool > (loading an image with --firmware in kvmtool means that the initrd image it not > loaded into the guest memory and no node is generated for it in the dtb), but I > would like to hear your thoughts about it. As part of the forcing ENVIRON_DEFAULT to "no" for kvmtool in configure an error should be generated if a user tries to explicitly enable it. > > 2. If the default environment is disabled, is it still possible for an user to > pass an initrd via other means? I couldn't find where that is implemented, so I'm > guessing it's not possible. Yes, a user could have a KVM_UNIT_TESTS_ENV environment variable set when they launch the tests. If that variable points to a file then it will get passed as an initrd. I guess you should also report a warning in arm/run if KVM_UNIT_TESTS_ENV is set which states that the environment file will be ignored when running with kvmtool. There aren't currently any other ways to invoke the addition of the -initrd command line option, because so far we only support passing a single file to test (the environment "file"). If we ever want to pass more files, then we'd need to create a simple file system on the initrd and make it possible to add -initrd even when no environment is desired. But, that may never happen. Thanks, drew > > Thanks, > > Alex > > > > > > >> temp_file ERRATATXT "$ERRATATXT" > >> echo 'export ERRATATXT' > >> fi > >> @@ -95,12 +99,8 @@ function mkstandalone() > >> echo Written $standalone. > >> } > >> > >> -if [ "$TARGET" = "kvmtool" ]; then > >> - echo "Standalone tests not supported with kvmtool" > >> - exit 2 > >> -fi > >> - > >> -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > >> +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \ > >> + [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > >> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 > >> exit 2 > >> fi > >> -- > >> 2.32.0 > >> > > Thanks, > > drew > > > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests 2021-09-08 16:07 ` Andrew Jones @ 2021-09-09 11:11 ` Alexandru Elisei 2021-09-09 13:05 ` Andrew Jones 0 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-09-09 11:11 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, On 9/8/21 5:07 PM, Andrew Jones wrote: > On Wed, Sep 08, 2021 at 04:37:39PM +0100, Alexandru Elisei wrote: >> Hi Drew, >> >> On 9/7/21 11:21 AM, Andrew Jones wrote: >>> On Fri, Jul 02, 2021 at 05:31:21PM +0100, Alexandru Elisei wrote: >>>> Add support for the standalone target when running kvm-unit-tests under >>>> kvmtool. >>>> >>>> Example command line invocation: >>>> >>>> $ ./configure --target=kvmtool >>>> $ make clean && make standalone >>>> >>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >>>> --- >>>> scripts/mkstandalone.sh | 14 +++++++------- >>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh >>>> index 16f461c06842..d84bdb7e278c 100755 >>>> --- a/scripts/mkstandalone.sh >>>> +++ b/scripts/mkstandalone.sh >>>> @@ -44,6 +44,10 @@ generate_test () >>>> config_export ARCH_NAME >>>> config_export PROCESSOR >>>> >>>> + if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then >>>> + config_export TARGET >>>> + fi >>> Should export unconditionally, since we'll want TARGET set >>> unconditionally. >> Yes, will do. >> >>>> + >>>> echo "echo BUILD_HEAD=$(cat build-head)" >>>> >>>> if [ ! -f $kernel ]; then >>>> @@ -59,7 +63,7 @@ generate_test () >>>> echo 'export FIRMWARE' >>>> fi >>>> >>>> - if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then >>>> + if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then >>> I think it would be better to ensure that ENVIRON_DEFAULT is "no" for >>> TARGET=kvmtool in configure. >> From looking at the code, it is my understanding that with ENVIRON_DEFAULT=yes, an >> initrd file is generated with the contents of erratatxt and other information, in >> a key=value pair format. This initrd is then passed on to the test (please correct >> me if I'm wrong). With ENVIRON_DEFAULT=no (set via ./configure >> --disable-default-environ), this initrd is not generated. >> >> kvmtool doesn't have support for passing an initrd when loading firmware, so yes, >> I believe the default should be no. >> >> However, I have two questions: >> >> 1. What happens when the user specifically enables the default environ via >> ./configure --enable-default-environ --target=kvmtool? In my opinion, that should >> be an error because the user wants something that is not possible with kvmtool >> (loading an image with --firmware in kvmtool means that the initrd image it not >> loaded into the guest memory and no node is generated for it in the dtb), but I >> would like to hear your thoughts about it. > As part of the forcing ENVIRON_DEFAULT to "no" for kvmtool in configure an > error should be generated if a user tries to explicitly enable it. > >> 2. If the default environment is disabled, is it still possible for an user to >> pass an initrd via other means? I couldn't find where that is implemented, so I'm >> guessing it's not possible. > Yes, a user could have a KVM_UNIT_TESTS_ENV environment variable set when > they launch the tests. If that variable points to a file then it will get > passed as an initrd. I guess you should also report a warning in arm/run > if KVM_UNIT_TESTS_ENV is set which states that the environment file will > be ignored when running with kvmtool. Thank you for explaining it, I had looked at scripts/arch-run.bash::initrd_create(), but it didn't click that setting the KVM_UNIT_TESTS_ENV environment variable is enough to generate and use the initrd. After looking at the code some more, in the logs the -initrd argument is shown as a comment, instead of an actual argument that is passed to qemu: timeout -k 1s --foreground 90s /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host,accel=kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/cache.flat -smp 1 # -initrd /tmp/tmp.rUIZ3h9KLJ QEMU_ACCEL = kvm INFO: IDC-DIC: dcache clean to PoU required INFO: IDC-DIC: icache invalidation to PoU required PASS: IDC-DIC: code generation SUMMARY: 1 tests This is done intentionally in scripts/arch-run.bash::run_qemu(). I don't understand the reason for that. When I first looked at the logs, I was sure that no initrd is passed to the test. I had to go dig through the scripts to figure out that the "#" sign (which marks the beginning of a comment) is not present in the qemu invocation. Thanks, Alex > > There aren't currently any other ways to invoke the addition of the > -initrd command line option, because so far we only support passing a > single file to test (the environment "file"). If we ever want to pass > more files, then we'd need to create a simple file system on the initrd > and make it possible to add -initrd even when no environment is desired. > But, that may never happen. > > Thanks, > drew > >> Thanks, >> >> Alex >> >>> >>>> temp_file ERRATATXT "$ERRATATXT" >>>> echo 'export ERRATATXT' >>>> fi >>>> @@ -95,12 +99,8 @@ function mkstandalone() >>>> echo Written $standalone. >>>> } >>>> >>>> -if [ "$TARGET" = "kvmtool" ]; then >>>> - echo "Standalone tests not supported with kvmtool" >>>> - exit 2 >>>> -fi >>>> - >>>> -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >>>> +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \ >>>> + [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >>>> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 >>>> exit 2 >>>> fi >>>> -- >>>> 2.32.0 >>>> >>> Thanks, >>> drew >>> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests 2021-09-09 11:11 ` Alexandru Elisei @ 2021-09-09 13:05 ` Andrew Jones 2021-09-09 13:47 ` Alexandru Elisei 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-09 13:05 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Thu, Sep 09, 2021 at 12:11:52PM +0100, Alexandru Elisei wrote: > Hi Drew, > > On 9/8/21 5:07 PM, Andrew Jones wrote: > > On Wed, Sep 08, 2021 at 04:37:39PM +0100, Alexandru Elisei wrote: > >> Hi Drew, > >> > >> On 9/7/21 11:21 AM, Andrew Jones wrote: > >>> On Fri, Jul 02, 2021 at 05:31:21PM +0100, Alexandru Elisei wrote: > >>>> Add support for the standalone target when running kvm-unit-tests under > >>>> kvmtool. > >>>> > >>>> Example command line invocation: > >>>> > >>>> $ ./configure --target=kvmtool > >>>> $ make clean && make standalone > >>>> > >>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > >>>> --- > >>>> scripts/mkstandalone.sh | 14 +++++++------- > >>>> 1 file changed, 7 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh > >>>> index 16f461c06842..d84bdb7e278c 100755 > >>>> --- a/scripts/mkstandalone.sh > >>>> +++ b/scripts/mkstandalone.sh > >>>> @@ -44,6 +44,10 @@ generate_test () > >>>> config_export ARCH_NAME > >>>> config_export PROCESSOR > >>>> > >>>> + if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then > >>>> + config_export TARGET > >>>> + fi > >>> Should export unconditionally, since we'll want TARGET set > >>> unconditionally. > >> Yes, will do. > >> > >>>> + > >>>> echo "echo BUILD_HEAD=$(cat build-head)" > >>>> > >>>> if [ ! -f $kernel ]; then > >>>> @@ -59,7 +63,7 @@ generate_test () > >>>> echo 'export FIRMWARE' > >>>> fi > >>>> > >>>> - if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then > >>>> + if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then > >>> I think it would be better to ensure that ENVIRON_DEFAULT is "no" for > >>> TARGET=kvmtool in configure. > >> From looking at the code, it is my understanding that with ENVIRON_DEFAULT=yes, an > >> initrd file is generated with the contents of erratatxt and other information, in > >> a key=value pair format. This initrd is then passed on to the test (please correct > >> me if I'm wrong). With ENVIRON_DEFAULT=no (set via ./configure > >> --disable-default-environ), this initrd is not generated. > >> > >> kvmtool doesn't have support for passing an initrd when loading firmware, so yes, > >> I believe the default should be no. > >> > >> However, I have two questions: > >> > >> 1. What happens when the user specifically enables the default environ via > >> ./configure --enable-default-environ --target=kvmtool? In my opinion, that should > >> be an error because the user wants something that is not possible with kvmtool > >> (loading an image with --firmware in kvmtool means that the initrd image it not > >> loaded into the guest memory and no node is generated for it in the dtb), but I > >> would like to hear your thoughts about it. > > As part of the forcing ENVIRON_DEFAULT to "no" for kvmtool in configure an > > error should be generated if a user tries to explicitly enable it. > > > >> 2. If the default environment is disabled, is it still possible for an user to > >> pass an initrd via other means? I couldn't find where that is implemented, so I'm > >> guessing it's not possible. > > Yes, a user could have a KVM_UNIT_TESTS_ENV environment variable set when > > they launch the tests. If that variable points to a file then it will get > > passed as an initrd. I guess you should also report a warning in arm/run > > if KVM_UNIT_TESTS_ENV is set which states that the environment file will > > be ignored when running with kvmtool. > > Thank you for explaining it, I had looked at > scripts/arch-run.bash::initrd_create(), but it didn't click that setting the > KVM_UNIT_TESTS_ENV environment variable is enough to generate and use the initrd. > > After looking at the code some more, in the logs the -initrd argument is shown as > a comment, instead of an actual argument that is passed to qemu: > > timeout -k 1s --foreground 90s /usr/bin/qemu-system-aarch64 -nodefaults -machine > virt,gic-version=host,accel=kvm -cpu host -device virtio-serial-device -device > virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none > -serial stdio -kernel arm/cache.flat -smp 1 # -initrd /tmp/tmp.rUIZ3h9KLJ > QEMU_ACCEL = kvm > INFO: IDC-DIC: dcache clean to PoU required > INFO: IDC-DIC: icache invalidation to PoU required > PASS: IDC-DIC: code generation > SUMMARY: 1 tests > > This is done intentionally in scripts/arch-run.bash::run_qemu(). I don't > understand the reason for that. When I first looked at the logs, I was sure that > no initrd is passed to the test. I had to go dig through the scripts to figure out > that the "#" sign (which marks the beginning of a comment) is not present in the > qemu invocation. It's commented out because if you want to copy+paste the command line to use it again it'll fail to run because the temp file will be gone. Of course somebody depending on the environment for their test run will have other problems when it's gone, but those people can use the KVM_UNIT_TESTS_ENV variable to specify a non-temp file which includes the default environment and then configure without the default environment. The command line won't get the # in that case. Thanks, drew > > Thanks, > > Alex > > > > > There aren't currently any other ways to invoke the addition of the > > -initrd command line option, because so far we only support passing a > > single file to test (the environment "file"). If we ever want to pass > > more files, then we'd need to create a simple file system on the initrd > > and make it possible to add -initrd even when no environment is desired. > > But, that may never happen. > > > > Thanks, > > drew > > > >> Thanks, > >> > >> Alex > >> > >>> > >>>> temp_file ERRATATXT "$ERRATATXT" > >>>> echo 'export ERRATATXT' > >>>> fi > >>>> @@ -95,12 +99,8 @@ function mkstandalone() > >>>> echo Written $standalone. > >>>> } > >>>> > >>>> -if [ "$TARGET" = "kvmtool" ]; then > >>>> - echo "Standalone tests not supported with kvmtool" > >>>> - exit 2 > >>>> -fi > >>>> - > >>>> -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > >>>> +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \ > >>>> + [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > >>>> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 > >>>> exit 2 > >>>> fi > >>>> -- > >>>> 2.32.0 > >>>> > >>> Thanks, > >>> drew > >>> > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests 2021-09-09 13:05 ` Andrew Jones @ 2021-09-09 13:47 ` Alexandru Elisei 2021-09-09 13:54 ` Andrew Jones 0 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-09-09 13:47 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, On 9/9/21 2:05 PM, Andrew Jones wrote: > On Thu, Sep 09, 2021 at 12:11:52PM +0100, Alexandru Elisei wrote: >> Hi Drew, >> >> On 9/8/21 5:07 PM, Andrew Jones wrote: >>> On Wed, Sep 08, 2021 at 04:37:39PM +0100, Alexandru Elisei wrote: >>>> Hi Drew, >>>> >>>> On 9/7/21 11:21 AM, Andrew Jones wrote: >>>>> On Fri, Jul 02, 2021 at 05:31:21PM +0100, Alexandru Elisei wrote: >>>>>> Add support for the standalone target when running kvm-unit-tests under >>>>>> kvmtool. >>>>>> >>>>>> Example command line invocation: >>>>>> >>>>>> $ ./configure --target=kvmtool >>>>>> $ make clean && make standalone >>>>>> >>>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >>>>>> --- >>>>>> scripts/mkstandalone.sh | 14 +++++++------- >>>>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh >>>>>> index 16f461c06842..d84bdb7e278c 100755 >>>>>> --- a/scripts/mkstandalone.sh >>>>>> +++ b/scripts/mkstandalone.sh >>>>>> @@ -44,6 +44,10 @@ generate_test () >>>>>> config_export ARCH_NAME >>>>>> config_export PROCESSOR >>>>>> >>>>>> + if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then >>>>>> + config_export TARGET >>>>>> + fi >>>>> Should export unconditionally, since we'll want TARGET set >>>>> unconditionally. >>>> Yes, will do. >>>> >>>>>> + >>>>>> echo "echo BUILD_HEAD=$(cat build-head)" >>>>>> >>>>>> if [ ! -f $kernel ]; then >>>>>> @@ -59,7 +63,7 @@ generate_test () >>>>>> echo 'export FIRMWARE' >>>>>> fi >>>>>> >>>>>> - if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then >>>>>> + if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then >>>>> I think it would be better to ensure that ENVIRON_DEFAULT is "no" for >>>>> TARGET=kvmtool in configure. >>>> From looking at the code, it is my understanding that with ENVIRON_DEFAULT=yes, an >>>> initrd file is generated with the contents of erratatxt and other information, in >>>> a key=value pair format. This initrd is then passed on to the test (please correct >>>> me if I'm wrong). With ENVIRON_DEFAULT=no (set via ./configure >>>> --disable-default-environ), this initrd is not generated. >>>> >>>> kvmtool doesn't have support for passing an initrd when loading firmware, so yes, >>>> I believe the default should be no. >>>> >>>> However, I have two questions: >>>> >>>> 1. What happens when the user specifically enables the default environ via >>>> ./configure --enable-default-environ --target=kvmtool? In my opinion, that should >>>> be an error because the user wants something that is not possible with kvmtool >>>> (loading an image with --firmware in kvmtool means that the initrd image it not >>>> loaded into the guest memory and no node is generated for it in the dtb), but I >>>> would like to hear your thoughts about it. >>> As part of the forcing ENVIRON_DEFAULT to "no" for kvmtool in configure an >>> error should be generated if a user tries to explicitly enable it. >>> >>>> 2. If the default environment is disabled, is it still possible for an user to >>>> pass an initrd via other means? I couldn't find where that is implemented, so I'm >>>> guessing it's not possible. >>> Yes, a user could have a KVM_UNIT_TESTS_ENV environment variable set when >>> they launch the tests. If that variable points to a file then it will get >>> passed as an initrd. I guess you should also report a warning in arm/run >>> if KVM_UNIT_TESTS_ENV is set which states that the environment file will >>> be ignored when running with kvmtool. >> Thank you for explaining it, I had looked at >> scripts/arch-run.bash::initrd_create(), but it didn't click that setting the >> KVM_UNIT_TESTS_ENV environment variable is enough to generate and use the initrd. >> >> After looking at the code some more, in the logs the -initrd argument is shown as >> a comment, instead of an actual argument that is passed to qemu: >> >> timeout -k 1s --foreground 90s /usr/bin/qemu-system-aarch64 -nodefaults -machine >> virt,gic-version=host,accel=kvm -cpu host -device virtio-serial-device -device >> virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none >> -serial stdio -kernel arm/cache.flat -smp 1 # -initrd /tmp/tmp.rUIZ3h9KLJ >> QEMU_ACCEL = kvm >> INFO: IDC-DIC: dcache clean to PoU required >> INFO: IDC-DIC: icache invalidation to PoU required >> PASS: IDC-DIC: code generation >> SUMMARY: 1 tests >> >> This is done intentionally in scripts/arch-run.bash::run_qemu(). I don't >> understand the reason for that. When I first looked at the logs, I was sure that >> no initrd is passed to the test. I had to go dig through the scripts to figure out >> that the "#" sign (which marks the beginning of a comment) is not present in the >> qemu invocation. > It's commented out because if you want to copy+paste the command line to > use it again it'll fail to run because the temp file will be gone. Of > course somebody depending on the environment for their test run will have > other problems when it's gone, but those people can use the > KVM_UNIT_TESTS_ENV variable to specify a non-temp file which includes the > default environment and then configure without the default environment. > The command line won't get the # in that case. Hmm... wouldn't it make more sense then to generate the initrd in the logs directory, and keep it there? To ensure the test runs can be reproduced manually, if needed? Thanks, Alex > > Thanks, > drew > >> Thanks, >> >> Alex >> >>> There aren't currently any other ways to invoke the addition of the >>> -initrd command line option, because so far we only support passing a >>> single file to test (the environment "file"). If we ever want to pass >>> more files, then we'd need to create a simple file system on the initrd >>> and make it possible to add -initrd even when no environment is desired. >>> But, that may never happen. >>> >>> Thanks, >>> drew >>> >>>> Thanks, >>>> >>>> Alex >>>> >>>>>> temp_file ERRATATXT "$ERRATATXT" >>>>>> echo 'export ERRATATXT' >>>>>> fi >>>>>> @@ -95,12 +99,8 @@ function mkstandalone() >>>>>> echo Written $standalone. >>>>>> } >>>>>> >>>>>> -if [ "$TARGET" = "kvmtool" ]; then >>>>>> - echo "Standalone tests not supported with kvmtool" >>>>>> - exit 2 >>>>>> -fi >>>>>> - >>>>>> -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >>>>>> +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \ >>>>>> + [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >>>>>> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 >>>>>> exit 2 >>>>>> fi >>>>>> -- >>>>>> 2.32.0 >>>>>> >>>>> Thanks, >>>>> drew >>>>> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests 2021-09-09 13:47 ` Alexandru Elisei @ 2021-09-09 13:54 ` Andrew Jones 2021-09-09 14:42 ` Alexandru Elisei 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-09 13:54 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Thu, Sep 09, 2021 at 02:47:57PM +0100, Alexandru Elisei wrote: > Hi Drew, > > On 9/9/21 2:05 PM, Andrew Jones wrote: > > On Thu, Sep 09, 2021 at 12:11:52PM +0100, Alexandru Elisei wrote: > >> Hi Drew, > >> > >> On 9/8/21 5:07 PM, Andrew Jones wrote: > >>> On Wed, Sep 08, 2021 at 04:37:39PM +0100, Alexandru Elisei wrote: > >>>> Hi Drew, > >>>> > >>>> On 9/7/21 11:21 AM, Andrew Jones wrote: > >>>>> On Fri, Jul 02, 2021 at 05:31:21PM +0100, Alexandru Elisei wrote: > >>>>>> Add support for the standalone target when running kvm-unit-tests under > >>>>>> kvmtool. > >>>>>> > >>>>>> Example command line invocation: > >>>>>> > >>>>>> $ ./configure --target=kvmtool > >>>>>> $ make clean && make standalone > >>>>>> > >>>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > >>>>>> --- > >>>>>> scripts/mkstandalone.sh | 14 +++++++------- > >>>>>> 1 file changed, 7 insertions(+), 7 deletions(-) > >>>>>> > >>>>>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh > >>>>>> index 16f461c06842..d84bdb7e278c 100755 > >>>>>> --- a/scripts/mkstandalone.sh > >>>>>> +++ b/scripts/mkstandalone.sh > >>>>>> @@ -44,6 +44,10 @@ generate_test () > >>>>>> config_export ARCH_NAME > >>>>>> config_export PROCESSOR > >>>>>> > >>>>>> + if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then > >>>>>> + config_export TARGET > >>>>>> + fi > >>>>> Should export unconditionally, since we'll want TARGET set > >>>>> unconditionally. > >>>> Yes, will do. > >>>> > >>>>>> + > >>>>>> echo "echo BUILD_HEAD=$(cat build-head)" > >>>>>> > >>>>>> if [ ! -f $kernel ]; then > >>>>>> @@ -59,7 +63,7 @@ generate_test () > >>>>>> echo 'export FIRMWARE' > >>>>>> fi > >>>>>> > >>>>>> - if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then > >>>>>> + if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then > >>>>> I think it would be better to ensure that ENVIRON_DEFAULT is "no" for > >>>>> TARGET=kvmtool in configure. > >>>> From looking at the code, it is my understanding that with ENVIRON_DEFAULT=yes, an > >>>> initrd file is generated with the contents of erratatxt and other information, in > >>>> a key=value pair format. This initrd is then passed on to the test (please correct > >>>> me if I'm wrong). With ENVIRON_DEFAULT=no (set via ./configure > >>>> --disable-default-environ), this initrd is not generated. > >>>> > >>>> kvmtool doesn't have support for passing an initrd when loading firmware, so yes, > >>>> I believe the default should be no. > >>>> > >>>> However, I have two questions: > >>>> > >>>> 1. What happens when the user specifically enables the default environ via > >>>> ./configure --enable-default-environ --target=kvmtool? In my opinion, that should > >>>> be an error because the user wants something that is not possible with kvmtool > >>>> (loading an image with --firmware in kvmtool means that the initrd image it not > >>>> loaded into the guest memory and no node is generated for it in the dtb), but I > >>>> would like to hear your thoughts about it. > >>> As part of the forcing ENVIRON_DEFAULT to "no" for kvmtool in configure an > >>> error should be generated if a user tries to explicitly enable it. > >>> > >>>> 2. If the default environment is disabled, is it still possible for an user to > >>>> pass an initrd via other means? I couldn't find where that is implemented, so I'm > >>>> guessing it's not possible. > >>> Yes, a user could have a KVM_UNIT_TESTS_ENV environment variable set when > >>> they launch the tests. If that variable points to a file then it will get > >>> passed as an initrd. I guess you should also report a warning in arm/run > >>> if KVM_UNIT_TESTS_ENV is set which states that the environment file will > >>> be ignored when running with kvmtool. > >> Thank you for explaining it, I had looked at > >> scripts/arch-run.bash::initrd_create(), but it didn't click that setting the > >> KVM_UNIT_TESTS_ENV environment variable is enough to generate and use the initrd. > >> > >> After looking at the code some more, in the logs the -initrd argument is shown as > >> a comment, instead of an actual argument that is passed to qemu: > >> > >> timeout -k 1s --foreground 90s /usr/bin/qemu-system-aarch64 -nodefaults -machine > >> virt,gic-version=host,accel=kvm -cpu host -device virtio-serial-device -device > >> virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none > >> -serial stdio -kernel arm/cache.flat -smp 1 # -initrd /tmp/tmp.rUIZ3h9KLJ > >> QEMU_ACCEL = kvm > >> INFO: IDC-DIC: dcache clean to PoU required > >> INFO: IDC-DIC: icache invalidation to PoU required > >> PASS: IDC-DIC: code generation > >> SUMMARY: 1 tests > >> > >> This is done intentionally in scripts/arch-run.bash::run_qemu(). I don't > >> understand the reason for that. When I first looked at the logs, I was sure that > >> no initrd is passed to the test. I had to go dig through the scripts to figure out > >> that the "#" sign (which marks the beginning of a comment) is not present in the > >> qemu invocation. > > It's commented out because if you want to copy+paste the command line to > > use it again it'll fail to run because the temp file will be gone. Of > > course somebody depending on the environment for their test run will have > > other problems when it's gone, but those people can use the > > KVM_UNIT_TESTS_ENV variable to specify a non-temp file which includes the > > default environment and then configure without the default environment. > > The command line won't get the # in that case. > > Hmm... wouldn't it make more sense then to generate the initrd in the logs > directory, and keep it there? To ensure the test runs can be reproduced manually, > if needed? Well, there's no logs directory for standalone tests, but I do like the idea of capturing the environment when possible. Possibly the best thing to do is to provide an option that, when enabled, says to dump the environment into the log before executing the test. That would be similar to how BUILD_HEAD is output first when running the tests standalone. Anyway, this is a good idea, but probably outside the scope of your kvmtool work unless the initrd thing is blocking you and you need to rework it anyway. Thanks, drew > > Thanks, > > Alex > > > > > Thanks, > > drew > > > >> Thanks, > >> > >> Alex > >> > >>> There aren't currently any other ways to invoke the addition of the > >>> -initrd command line option, because so far we only support passing a > >>> single file to test (the environment "file"). If we ever want to pass > >>> more files, then we'd need to create a simple file system on the initrd > >>> and make it possible to add -initrd even when no environment is desired. > >>> But, that may never happen. > >>> > >>> Thanks, > >>> drew > >>> > >>>> Thanks, > >>>> > >>>> Alex > >>>> > >>>>>> temp_file ERRATATXT "$ERRATATXT" > >>>>>> echo 'export ERRATATXT' > >>>>>> fi > >>>>>> @@ -95,12 +99,8 @@ function mkstandalone() > >>>>>> echo Written $standalone. > >>>>>> } > >>>>>> > >>>>>> -if [ "$TARGET" = "kvmtool" ]; then > >>>>>> - echo "Standalone tests not supported with kvmtool" > >>>>>> - exit 2 > >>>>>> -fi > >>>>>> - > >>>>>> -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > >>>>>> +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \ > >>>>>> + [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then > >>>>>> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 > >>>>>> exit 2 > >>>>>> fi > >>>>>> -- > >>>>>> 2.32.0 > >>>>>> > >>>>> Thanks, > >>>>> drew > >>>>> > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests 2021-09-09 13:54 ` Andrew Jones @ 2021-09-09 14:42 ` Alexandru Elisei 0 siblings, 0 replies; 36+ messages in thread From: Alexandru Elisei @ 2021-09-09 14:42 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, On 9/9/21 2:54 PM, Andrew Jones wrote: > On Thu, Sep 09, 2021 at 02:47:57PM +0100, Alexandru Elisei wrote: >> Hi Drew, >> >> On 9/9/21 2:05 PM, Andrew Jones wrote: >>> On Thu, Sep 09, 2021 at 12:11:52PM +0100, Alexandru Elisei wrote: >>>> Hi Drew, >>>> >>>> On 9/8/21 5:07 PM, Andrew Jones wrote: >>>>> On Wed, Sep 08, 2021 at 04:37:39PM +0100, Alexandru Elisei wrote: >>>>>> Hi Drew, >>>>>> >>>>>> On 9/7/21 11:21 AM, Andrew Jones wrote: >>>>>>> On Fri, Jul 02, 2021 at 05:31:21PM +0100, Alexandru Elisei wrote: >>>>>>>> Add support for the standalone target when running kvm-unit-tests under >>>>>>>> kvmtool. >>>>>>>> >>>>>>>> Example command line invocation: >>>>>>>> >>>>>>>> $ ./configure --target=kvmtool >>>>>>>> $ make clean && make standalone >>>>>>>> >>>>>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >>>>>>>> --- >>>>>>>> scripts/mkstandalone.sh | 14 +++++++------- >>>>>>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh >>>>>>>> index 16f461c06842..d84bdb7e278c 100755 >>>>>>>> --- a/scripts/mkstandalone.sh >>>>>>>> +++ b/scripts/mkstandalone.sh >>>>>>>> @@ -44,6 +44,10 @@ generate_test () >>>>>>>> config_export ARCH_NAME >>>>>>>> config_export PROCESSOR >>>>>>>> >>>>>>>> + if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then >>>>>>>> + config_export TARGET >>>>>>>> + fi >>>>>>> Should export unconditionally, since we'll want TARGET set >>>>>>> unconditionally. >>>>>> Yes, will do. >>>>>> >>>>>>>> + >>>>>>>> echo "echo BUILD_HEAD=$(cat build-head)" >>>>>>>> >>>>>>>> if [ ! -f $kernel ]; then >>>>>>>> @@ -59,7 +63,7 @@ generate_test () >>>>>>>> echo 'export FIRMWARE' >>>>>>>> fi >>>>>>>> >>>>>>>> - if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then >>>>>>>> + if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then >>>>>>> I think it would be better to ensure that ENVIRON_DEFAULT is "no" for >>>>>>> TARGET=kvmtool in configure. >>>>>> From looking at the code, it is my understanding that with ENVIRON_DEFAULT=yes, an >>>>>> initrd file is generated with the contents of erratatxt and other information, in >>>>>> a key=value pair format. This initrd is then passed on to the test (please correct >>>>>> me if I'm wrong). With ENVIRON_DEFAULT=no (set via ./configure >>>>>> --disable-default-environ), this initrd is not generated. >>>>>> >>>>>> kvmtool doesn't have support for passing an initrd when loading firmware, so yes, >>>>>> I believe the default should be no. >>>>>> >>>>>> However, I have two questions: >>>>>> >>>>>> 1. What happens when the user specifically enables the default environ via >>>>>> ./configure --enable-default-environ --target=kvmtool? In my opinion, that should >>>>>> be an error because the user wants something that is not possible with kvmtool >>>>>> (loading an image with --firmware in kvmtool means that the initrd image it not >>>>>> loaded into the guest memory and no node is generated for it in the dtb), but I >>>>>> would like to hear your thoughts about it. >>>>> As part of the forcing ENVIRON_DEFAULT to "no" for kvmtool in configure an >>>>> error should be generated if a user tries to explicitly enable it. >>>>> >>>>>> 2. If the default environment is disabled, is it still possible for an user to >>>>>> pass an initrd via other means? I couldn't find where that is implemented, so I'm >>>>>> guessing it's not possible. >>>>> Yes, a user could have a KVM_UNIT_TESTS_ENV environment variable set when >>>>> they launch the tests. If that variable points to a file then it will get >>>>> passed as an initrd. I guess you should also report a warning in arm/run >>>>> if KVM_UNIT_TESTS_ENV is set which states that the environment file will >>>>> be ignored when running with kvmtool. >>>> Thank you for explaining it, I had looked at >>>> scripts/arch-run.bash::initrd_create(), but it didn't click that setting the >>>> KVM_UNIT_TESTS_ENV environment variable is enough to generate and use the initrd. >>>> >>>> After looking at the code some more, in the logs the -initrd argument is shown as >>>> a comment, instead of an actual argument that is passed to qemu: >>>> >>>> timeout -k 1s --foreground 90s /usr/bin/qemu-system-aarch64 -nodefaults -machine >>>> virt,gic-version=host,accel=kvm -cpu host -device virtio-serial-device -device >>>> virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none >>>> -serial stdio -kernel arm/cache.flat -smp 1 # -initrd /tmp/tmp.rUIZ3h9KLJ >>>> QEMU_ACCEL = kvm >>>> INFO: IDC-DIC: dcache clean to PoU required >>>> INFO: IDC-DIC: icache invalidation to PoU required >>>> PASS: IDC-DIC: code generation >>>> SUMMARY: 1 tests >>>> >>>> This is done intentionally in scripts/arch-run.bash::run_qemu(). I don't >>>> understand the reason for that. When I first looked at the logs, I was sure that >>>> no initrd is passed to the test. I had to go dig through the scripts to figure out >>>> that the "#" sign (which marks the beginning of a comment) is not present in the >>>> qemu invocation. >>> It's commented out because if you want to copy+paste the command line to >>> use it again it'll fail to run because the temp file will be gone. Of >>> course somebody depending on the environment for their test run will have >>> other problems when it's gone, but those people can use the >>> KVM_UNIT_TESTS_ENV variable to specify a non-temp file which includes the >>> default environment and then configure without the default environment. >>> The command line won't get the # in that case. >> Hmm... wouldn't it make more sense then to generate the initrd in the logs >> directory, and keep it there? To ensure the test runs can be reproduced manually, >> if needed? > Well, there's no logs directory for standalone tests, but I do like the > idea of capturing the environment when possible. Possibly the best thing > to do is to provide an option that, when enabled, says to dump the > environment into the log before executing the test. That would be similar > to how BUILD_HEAD is output first when running the tests standalone. > Anyway, this is a good idea, but probably outside the scope of your > kvmtool work unless the initrd thing is blocking you and you need to > rework it anyway. I don't need to change anything about how initrd works in kvm-unit-tests for my kvmtool series, I was just curious to understand more about it. Thank you for the explanations! Thanks, Alex > > Thanks, > drew > >> Thanks, >> >> Alex >> >>> Thanks, >>> drew >>> >>>> Thanks, >>>> >>>> Alex >>>> >>>>> There aren't currently any other ways to invoke the addition of the >>>>> -initrd command line option, because so far we only support passing a >>>>> single file to test (the environment "file"). If we ever want to pass >>>>> more files, then we'd need to create a simple file system on the initrd >>>>> and make it possible to add -initrd even when no environment is desired. >>>>> But, that may never happen. >>>>> >>>>> Thanks, >>>>> drew >>>>> >>>>>> Thanks, >>>>>> >>>>>> Alex >>>>>> >>>>>>>> temp_file ERRATATXT "$ERRATATXT" >>>>>>>> echo 'export ERRATATXT' >>>>>>>> fi >>>>>>>> @@ -95,12 +99,8 @@ function mkstandalone() >>>>>>>> echo Written $standalone. >>>>>>>> } >>>>>>>> >>>>>>>> -if [ "$TARGET" = "kvmtool" ]; then >>>>>>>> - echo "Standalone tests not supported with kvmtool" >>>>>>>> - exit 2 >>>>>>>> -fi >>>>>>>> - >>>>>>>> -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >>>>>>>> +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \ >>>>>>>> + [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then >>>>>>>> echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2 >>>>>>>> exit 2 >>>>>>>> fi >>>>>>>> -- >>>>>>>> 2.32.0 >>>>>>>> >>>>>>> Thanks, >>>>>>> drew >>>>>>> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* [kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool 2021-07-02 16:31 [kvm-unit-tests RFC PATCH 0/5] arm: Add kvmtool to the runner script Alexandru Elisei ` (3 preceding siblings ...) 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests Alexandru Elisei @ 2021-07-02 16:31 ` Alexandru Elisei 2021-09-07 10:25 ` Andrew Jones 4 siblings, 1 reply; 36+ messages in thread From: Alexandru Elisei @ 2021-07-02 16:31 UTC (permalink / raw) To: drjones, thuth, pbonzini, lvivier, kvm-ppc, david, frankja, cohuck, imbrenda, linux-s390, kvm, kvmarm Cc: vivek.gautam, andre.przywara, maz kvmtool runs a test using the -f/--firmware argument, which doesn't load an initrd, making specifying an errata file useless. Instead, configure forces all erratas to be enabled via the CONFIG_ERRATA_FORCE define in lib/config.h. Forbid the --erratatxt option when kvm-unit-tests is configured for kvmtool and let the user know that all erratas are enabled by default. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- I'm not sure if printing an error is too strong here and a simple warning would suffice. Suggestions welcome! configure | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/configure b/configure index 395c809c9c02..acd288239f80 100755 --- a/configure +++ b/configure @@ -24,7 +24,8 @@ u32_long= wa_divide= target= errata_force=0 -erratatxt="$srcdir/errata.txt" +erratatxt_default="$srcdir/errata.txt" +erratatxt="_NO_FILE_4Uhere_" host_key_document= page_size= earlycon= @@ -50,7 +51,8 @@ usage() { enable or disable the generation of a default environ when no environ is provided by the user (enabled by default) --erratatxt=FILE specify a file to use instead of errata.txt. Use - '--erratatxt=' to ensure no file is used. + '--erratatxt=' to ensure no file is used. This option is + invalid for arm/arm64 when target=kvmtool. --host-key-document=HOST_KEY_DOCUMENT Specify the machine-specific host-key document for creating a PVM image with 'genprotimg' (s390x only) @@ -147,11 +149,6 @@ if [ -n "$host_key_document" ] && [ ! -f "$host_key_document" ]; then exit 1 fi -if [ "$erratatxt" ] && [ ! -f "$erratatxt" ]; then - echo "erratatxt: $erratatxt does not exist or is not a regular file" - exit 1 -fi - arch_name=$arch [ "$arch" = "aarch64" ] && arch="arm64" [ "$arch_name" = "arm64" ] && arch_name="aarch64" @@ -184,6 +181,21 @@ else fi fi +if [ "$target" = "kvmtool" ]; then + if [ "$erratatxt" ] && [ "$erratatxt" != "_NO_FILE_4Uhere_" ]; then + echo "--erratatxt is not supported for target=kvmtool (all erratas enabled by default)" + usage + fi +else + if [ "$erratatxt" = "_NO_FILE_4Uhere_" ]; then + erratatxt=$erratatxt_default + fi + if [ "$erratatxt" ] && [ ! -f "$erratatxt" ]; then + echo "erratatxt: $erratatxt does not exist or is not a regular file" + exit 1 + fi +fi + [ -z "$processor" ] && processor="$arch" if [ "$processor" = "arm64" ]; then -- 2.32.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool Alexandru Elisei @ 2021-09-07 10:25 ` Andrew Jones 2021-09-08 16:13 ` Alexandru Elisei 0 siblings, 1 reply; 36+ messages in thread From: Andrew Jones @ 2021-09-07 10:25 UTC (permalink / raw) To: Alexandru Elisei Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm On Fri, Jul 02, 2021 at 05:31:22PM +0100, Alexandru Elisei wrote: > kvmtool runs a test using the -f/--firmware argument, which doesn't load an > initrd, making specifying an errata file useless. Instead, configure forces > all erratas to be enabled via the CONFIG_ERRATA_FORCE define in > lib/config.h. > > Forbid the --erratatxt option when kvm-unit-tests is configured for kvmtool > and let the user know that all erratas are enabled by default. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > I'm not sure if printing an error is too strong here and a simple warning would > suffice. Suggestions welcome! > > configure | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/configure b/configure > index 395c809c9c02..acd288239f80 100755 > --- a/configure > +++ b/configure > @@ -24,7 +24,8 @@ u32_long= > wa_divide= > target= > errata_force=0 > -erratatxt="$srcdir/errata.txt" > +erratatxt_default="$srcdir/errata.txt" > +erratatxt="_NO_FILE_4Uhere_" > host_key_document= > page_size= > earlycon= > @@ -50,7 +51,8 @@ usage() { > enable or disable the generation of a default environ when > no environ is provided by the user (enabled by default) > --erratatxt=FILE specify a file to use instead of errata.txt. Use > - '--erratatxt=' to ensure no file is used. > + '--erratatxt=' to ensure no file is used. This option is > + invalid for arm/arm64 when target=kvmtool. Do we need to always specifiy arm/arm64 when talking about target=kvmtool? How much more effort would an x86 kvmtool target be to add? > --host-key-document=HOST_KEY_DOCUMENT > Specify the machine-specific host-key document for creating > a PVM image with 'genprotimg' (s390x only) > @@ -147,11 +149,6 @@ if [ -n "$host_key_document" ] && [ ! -f "$host_key_document" ]; then > exit 1 > fi > > -if [ "$erratatxt" ] && [ ! -f "$erratatxt" ]; then > - echo "erratatxt: $erratatxt does not exist or is not a regular file" > - exit 1 > -fi > - > arch_name=$arch > [ "$arch" = "aarch64" ] && arch="arm64" > [ "$arch_name" = "arm64" ] && arch_name="aarch64" > @@ -184,6 +181,21 @@ else > fi > fi > > +if [ "$target" = "kvmtool" ]; then > + if [ "$erratatxt" ] && [ "$erratatxt" != "_NO_FILE_4Uhere_" ]; then > + echo "--erratatxt is not supported for target=kvmtool (all erratas enabled by default)" > + usage > + fi > +else > + if [ "$erratatxt" = "_NO_FILE_4Uhere_" ]; then > + erratatxt=$erratatxt_default > + fi > + if [ "$erratatxt" ] && [ ! -f "$erratatxt" ]; then > + echo "erratatxt: $erratatxt does not exist or is not a regular file" > + exit 1 > + fi > +fi switch > + > [ -z "$processor" ] && processor="$arch" > > if [ "$processor" = "arm64" ]; then > -- > 2.32.0 > Otherwise looks good to me. Thanks, drew _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool 2021-09-07 10:25 ` Andrew Jones @ 2021-09-08 16:13 ` Alexandru Elisei 0 siblings, 0 replies; 36+ messages in thread From: Alexandru Elisei @ 2021-09-08 16:13 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, frankja, kvm, david, andre.przywara, cohuck, kvm-ppc, vivek.gautam, maz, pbonzini, imbrenda, kvmarm Hi Drew, On 9/7/21 11:25 AM, Andrew Jones wrote: > On Fri, Jul 02, 2021 at 05:31:22PM +0100, Alexandru Elisei wrote: >> kvmtool runs a test using the -f/--firmware argument, which doesn't load an >> initrd, making specifying an errata file useless. Instead, configure forces >> all erratas to be enabled via the CONFIG_ERRATA_FORCE define in >> lib/config.h. >> >> Forbid the --erratatxt option when kvm-unit-tests is configured for kvmtool >> and let the user know that all erratas are enabled by default. >> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >> --- >> I'm not sure if printing an error is too strong here and a simple warning would >> suffice. Suggestions welcome! >> >> configure | 26 +++++++++++++++++++------- >> 1 file changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/configure b/configure >> index 395c809c9c02..acd288239f80 100755 >> --- a/configure >> +++ b/configure >> @@ -24,7 +24,8 @@ u32_long= >> wa_divide= >> target= >> errata_force=0 >> -erratatxt="$srcdir/errata.txt" >> +erratatxt_default="$srcdir/errata.txt" >> +erratatxt="_NO_FILE_4Uhere_" >> host_key_document= >> page_size= >> earlycon= >> @@ -50,7 +51,8 @@ usage() { >> enable or disable the generation of a default environ when >> no environ is provided by the user (enabled by default) >> --erratatxt=FILE specify a file to use instead of errata.txt. Use >> - '--erratatxt=' to ensure no file is used. >> + '--erratatxt=' to ensure no file is used. This option is >> + invalid for arm/arm64 when target=kvmtool. > Do we need to always specifiy arm/arm64 when talking about target=kvmtool? > How much more effort would an x86 kvmtool target be to add? Err.. there haven't been any commits to the x86 code, other than those caused by refactoring in the other parts of the code, since 2015. Ballpark estimate, it's going to be very hard even to make 50% of the tests run on kvmtool. I think adding adding --target=uefi is much more likely to happen for x86 than --target=kvmtool. > >> --host-key-document=HOST_KEY_DOCUMENT >> Specify the machine-specific host-key document for creating >> a PVM image with 'genprotimg' (s390x only) >> @@ -147,11 +149,6 @@ if [ -n "$host_key_document" ] && [ ! -f "$host_key_document" ]; then >> exit 1 >> fi >> >> -if [ "$erratatxt" ] && [ ! -f "$erratatxt" ]; then >> - echo "erratatxt: $erratatxt does not exist or is not a regular file" >> - exit 1 >> -fi >> - >> arch_name=$arch >> [ "$arch" = "aarch64" ] && arch="arm64" >> [ "$arch_name" = "arm64" ] && arch_name="aarch64" >> @@ -184,6 +181,21 @@ else >> fi >> fi >> >> +if [ "$target" = "kvmtool" ]; then >> + if [ "$erratatxt" ] && [ "$erratatxt" != "_NO_FILE_4Uhere_" ]; then >> + echo "--erratatxt is not supported for target=kvmtool (all erratas enabled by default)" >> + usage >> + fi >> +else >> + if [ "$erratatxt" = "_NO_FILE_4Uhere_" ]; then >> + erratatxt=$erratatxt_default >> + fi >> + if [ "$erratatxt" ] && [ ! -f "$erratatxt" ]; then >> + echo "erratatxt: $erratatxt does not exist or is not a regular file" >> + exit 1 >> + fi >> +fi > switch Sure. Thanks, Alex > >> + >> [ -z "$processor" ] && processor="$arch" >> >> if [ "$processor" = "arm64" ]; then >> -- >> 2.32.0 >> > Otherwise looks good to me. > > Thanks, > drew > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2021-09-09 14:41 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-02 16:31 [kvm-unit-tests RFC PATCH 0/5] arm: Add kvmtool to the runner script Alexandru Elisei 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available Alexandru Elisei 2021-07-12 16:36 ` Andrew Jones 2021-09-06 10:20 ` Alexandru Elisei 2021-09-06 10:58 ` Andrew Jones 2021-09-06 11:06 ` Alexandru Elisei 2021-07-12 16:51 ` Andre Przywara 2021-07-12 17:07 ` Andrew Jones 2021-07-12 17:12 ` Nadav Amit 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status Alexandru Elisei 2021-07-12 16:37 ` Andrew Jones 2021-07-13 7:45 ` Thomas Huth 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support Alexandru Elisei 2021-07-12 16:52 ` Andre Przywara 2021-09-06 10:28 ` Alexandru Elisei 2021-09-06 11:01 ` Andrew Jones 2021-09-06 11:07 ` Alexandru Elisei 2021-09-07 10:17 ` Andrew Jones 2021-09-08 14:33 ` Alexandru Elisei 2021-09-08 15:09 ` Andrew Jones 2021-09-08 15:46 ` Alexandru Elisei 2021-09-08 15:49 ` Andrew Jones 2021-09-09 11:33 ` Alexandru Elisei 2021-09-09 12:49 ` Andrew Jones 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests Alexandru Elisei 2021-09-07 10:21 ` Andrew Jones 2021-09-08 15:37 ` Alexandru Elisei 2021-09-08 16:07 ` Andrew Jones 2021-09-09 11:11 ` Alexandru Elisei 2021-09-09 13:05 ` Andrew Jones 2021-09-09 13:47 ` Alexandru Elisei 2021-09-09 13:54 ` Andrew Jones 2021-09-09 14:42 ` Alexandru Elisei 2021-07-02 16:31 ` [kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool Alexandru Elisei 2021-09-07 10:25 ` Andrew Jones 2021-09-08 16:13 ` Alexandru Elisei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).