[kvm-unit-tests RFC PATCH 0/5] arm: Add kvmtool to the runner script

[kvm-unit-tests RFC PATCH 0/5] arm: Add kvmtool to the runner script

[Alexandru Elisei]

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

[kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available

[Alexandru Elisei]

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

Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available

[Andrew Jones]

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 

Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available

[Alexandru Elisei]

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 
>

Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available

[Andrew Jones]

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 
> >
> 

Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available

[Alexandru Elisei]

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 
>>>

Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available

[Andre Przywara]

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;
> +}

Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available

[Andrew Jones]

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;
> > +}
> 

Re: [kvm-unit-tests RFC PATCH 1/5] lib: arm: Print test exit status on exit if chr-testdev is not available

[Nadav Amit]

[-- Attachment #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 #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status

[Alexandru Elisei]

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

Re: [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status

[Andrew Jones]

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>

Re: [kvm-unit-tests RFC PATCH 2/5] scripts: Rename run_qemu_status -> run_test_status

[Thomas Huth]

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>

[kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Alexandru Elisei]

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

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Andre Przywara]

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;

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Alexandru Elisei]

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;

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Andrew Jones]

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;
> 

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Alexandru Elisei]

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;

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Andrew Jones]

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

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Alexandru Elisei]

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
>

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Andrew Jones]

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

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Alexandru Elisei]

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
>

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Andrew Jones]

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
> >
> 

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Alexandru Elisei]

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
>>>

Re: [kvm-unit-tests RFC PATCH 3/5] run_tests.sh: Add kvmtool support

[Andrew Jones]

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
> >>>
> 

[kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

[Alexandru Elisei]

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

Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

[Andrew Jones]

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 

Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

[Alexandru Elisei]

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 
>

Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

[Andrew Jones]

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 
> >
> 

Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

[Alexandru Elisei]

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 
>>>

Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

[Andrew Jones]

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 
> >>>
> 

Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

[Alexandru Elisei]

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 
>>>>>

Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

[Andrew Jones]

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 
> >>>>>
> 

Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

[Alexandru Elisei]

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 
>>>>>>>

[kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool

[Alexandru Elisei]

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

Re: [kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool

[Andrew Jones]

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 

Re: [kvm-unit-tests RFC PATCH 5/5] configure: Ignore --erratatxt when --target=kvmtool

[Alexandru Elisei]

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 
>