| Message ID | 20210702163122.96110-2-alexandru.elisei@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Add kvmtool to the runner script | expand |
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
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; > +}
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; > > +} >
> 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
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 >
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 > > >
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 >>>
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; +}
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(-)