KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, pbonzini@redhat.com
Subject: Re: [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target
Date: Fri, 23 Apr 2021 16:43:14 +0100
Message-ID: <854c2d33-0b20-b7e3-c522-b01a53fcbbb3@arm.com> (raw)
In-Reply-To: <20210422155757.t4pvv6blkvoyi2oy@gator>

Hi Drew,

On 4/22/21 4:57 PM, Andrew Jones wrote:
> On Thu, Apr 22, 2021 at 04:17:27PM +0100, Alexandru Elisei wrote:
>> Hi Drew,
>> On 4/20/21 5:51 PM, Andrew Jones wrote:
>>> Hi Alex,
>>> On Tue, Apr 20, 2021 at 05:13:37PM +0100, Alexandru Elisei wrote:
>>>> This is an RFC because it's not exactly clear to me that this is the best
>>>> approach. I'm also open to using a different name for the new option, maybe
>>>> something like --platform if it makes more sense.
>>> I like 'target'.
>>>> I see two use cases for the patch:
>>>> 1. Using different files when compiling kvm-unit-tests to run as an EFI app
>>>> as opposed to a KVM guest (described in the commit message).
>>>> 2. This is speculation on my part, but I can see extending
>>>> arm/unittests.cfg with a "target" test option which can be used to decide
>>>> which tests need to be run based on the configure --target value. For
>>>> example, migration tests don't make much sense on kvmtool, which doesn't
>>>> have migration support. Similarly, the micro-bench test doesn't make much
>>>> sense (to me, at least) as an EFI app. Of course, this is only useful if
>>>> there are automated scripts to run the tests under kvmtool or EFI, which
>>>> doesn't look likely at the moment, so I left it out of the commit message.
>>> Sounds like a good idea. unittests.cfg could get a new option 'targets'
>>> where a list of targets is given. If targets is not present, then the
>>> test assumes it's for all targets. Might be nice to also accept !<target>
>>> syntax. E.g.
>>> # builds/runs for all targets
>>> [mytest]
>>> file = mytest.flat
>>> # builds/runs for given targets
>>> [mytest2]
>>> file = mytest2.flat
>>> targets = qemu,kvmtool
>>> # builds/runs for all targets except disabled targets
>>> [mytest3]
>>> file = mytest3.flat
>>> targets = !kvmtool
>> That's sounds like a good idea, but to be honest, I would wait until someone
>> actually needs it before implementing it. That way we don't risk not taking a use
>> case into account and then having to rework it.
> Don't we have a usecase? Above you said that kvmtool should at least skip
> the migration tests.

Sorry for not making myself clear, when I was talking about adding a "targets"
parameter to a test, I was thinking that it will only be used by the run scripts.
All the tests can run under qemu, and run_tests.sh only knows about qemu, so, from
that point of view, that's why I think the "targets" argument is not useful at the

As for the migration test specifically, the VM migration is implemented in the run
scripts, not in the test itself; the test waits for the UART to signal that
migration is complete. That test runs just fine under kvmtool, but no migration is
taking place:

$ ./vm run --irqchip=gicv3-its -c6 -m128 -f arm/gic.flat --params its-migration
  # lkvm run --firmware arm/gic.flat -m 128 -c 6 --name guest-1440
  Info: Placing fdt at 0x80200000 - 0x80210000
chr_testdev_init: chr-testdev: can't find a virtio-console
ITS: MAPD devid=2 size = 0x8 itt=0x801e0000 valid=1
ITS: MAPD devid=7 size = 0x8 itt=0x801f0000 valid=1
MAPC col_id=3 target_addr = 0x30000 valid=1
MAPC col_id=2 target_addr = 0x20000 valid=1
INVALL col_id=2
INVALL col_id=3
MAPTI dev_id=2 event_id=20 -> phys_id=8195, col_id=3
MAPTI dev_id=7 event_id=255 -> phys_id=8196, col_id=2
Now migrate the VM, then press a key to continue...
INFO: gicv3: its-migration: Migration complete
INT dev_id=2 event_id=20
PASS: gicv3: its-migration: dev2/eventid=20 triggers LPI 8195 on PE #3 after migration
INT dev_id=7 event_id=255
PASS: gicv3: its-migration: dev7/eventid=255 triggers LPI 8196 on PE #2 after
SUMMARY: 2 tests

Even the pci-test works under kvmtool, even though it targets qemu's pci-testdev:

$ ./vm run --irqchip=gicv3-its -c6 -m128 -f arm/pci-test.flat
  # lkvm run --firmware arm/pci-test.flat -m 128 -c 6 --name guest-1468
  Info: Placing fdt at 0x80200000 - 0x80210000
chr_testdev_init: chr-testdev: can't find a virtio-console
No PCIe ECAM compatible controller found
PCI bus probing failed, skipping tests...
SUMMARY: 0 tests

The test is still useful for kvmtool, because it tests that the PCI node in the
DTB is generated as expected. And after kvmtool gets support for PCIE (work in
progress), it will test PCI device probing, which makes it even more useful than
it is today.

So I guess the question is, do what should "targets" represent, how should it be
used and do we need it now?

>>> And it wouldn't bother me to have special logic for kvmtool's lack of
>>> migration put directly in scripts/runtime.bash
>> Good to keep in mind when support is added.
>>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>>> index 132389c7dd59..0d5cb51df4f4 100644
>>> --- a/scripts/runtime.bash
>>> +++ b/scripts/runtime.bash
>>> @@ -132,7 +132,7 @@ function run()
>>>      }
>>>      cmdline=$(get_cmdline $kernel)
>>> -    if grep -qw "migration" <<<$groups ; then
>>> +    if grep -qw "migration" <<<$groups && [ "$TARGET" != "kvmtool" ]; then
>>>          cmdline="MIGRATION=yes $cmdline"
>>>      fi
>>>      if [ "$verbose" = "yes" ]; then
>>>> Using --vmm will trigger a warning. I was thinking about removing it entirely in
>>>> a about a year's time, but that's not set in stone. Note that qemu users
>>>> (probably the vast majority of people) will not be affected by this change as
>>>> long as they weren't setting --vmm explicitely to its default value of "qemu".
>>> While we'd risk automated configure+build tools, like git{hub,lab} CI,
>>> failing, I think the risk is pretty low right now that anybody is using
>>> the option. Also, we might as well make them change sooner than later by
>>> failing configure. IOW, I'd just do s/vmm/target/g to rename it now. If
>>> we are concerned about the disruption, then I'd just make vmm an alias
>>> for target and not bother deprecating it ever.
>> I also think it will not be too bad if we make the change now, but I'm not sure
>> what you mean by making vmm an alias of target. The patch ignores --vmm is it's
>> not specified, and if it is specified on the configure command line, then it must
>> match the value of --target, otherwise configure fails.
> The current patch does both things; it says don't use --vmm and it says
> the new --vmm is --target. I'm saying do one or the other. Either
> completely rename vmm to target, which will then error out when vmm is
> specified as an unknown option or allow the user to use either --vmm or
> --target with no error and where both mean to do the same thing, which is
> to set the TARGET variable.

I'm sorry, but it's still not clear to me what you are trying to say.

The current behaviour:

$ ./configure --arch=arm64 --cross-prefix=aarch64-linux-gnu- --vmm=qemu
INFO: --vmm is deprecated and will be removed in future versions
$ ./configure --arch=arm64 --cross-prefix=aarch64-linux-gnu- --vmm=qemu --target=qemu
INFO: --vmm is deprecated and will be removed in future versions
$ ./configure --arch=arm64 --cross-prefix=aarch64-linux-gnu- --vmm=kvmtool
INFO: --vmm is deprecated and will be removed in future versions
--vmm must have the same value as --target (qemu)
Usage: ./configure [options]
$ ./configure --arch=arm64 --cross-prefix=aarch64-linux-gnu- --vmm=kvmtool
INFO: --vmm is deprecated and will be removed in future versions

Can you point out what makes you think that the patch tries to do two things at once?



  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-20 16:13 Alexandru Elisei
2021-04-20 16:13 ` [kvm-unit-tests RFC PATCH 1/1] " Alexandru Elisei
2021-04-20 16:51 ` [kvm-unit-tests RFC PATCH 0/1] " Andrew Jones
2021-04-22 15:17   ` Alexandru Elisei
2021-04-22 15:57     ` Andrew Jones
2021-04-23 15:43       ` Alexandru Elisei [this message]
2021-04-26  8:59         ` Andrew Jones
2021-04-26 14:11           ` Alexandru Elisei

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=854c2d33-0b20-b7e3-c522-b01a53fcbbb3@arm.com \
    --to=alexandru.elisei@arm.com \
    --cc=drjones@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=pbonzini@redhat.com \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git