* [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target @ 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 0 siblings, 2 replies; 8+ messages in thread From: Alexandru Elisei @ 2021-04-20 16:13 UTC (permalink / raw) To: drjones, kvm, kvmarm; +Cc: pbonzini 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 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. 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". Alexandru Elisei (1): configure: arm: Replace --vmm with --target configure | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [kvm-unit-tests RFC PATCH 1/1] configure: arm: Replace --vmm with --target 2021-04-20 16:13 [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target Alexandru Elisei @ 2021-04-20 16:13 ` Alexandru Elisei 2021-04-20 16:51 ` [kvm-unit-tests RFC PATCH 0/1] " Andrew Jones 1 sibling, 0 replies; 8+ messages in thread From: Alexandru Elisei @ 2021-04-20 16:13 UTC (permalink / raw) To: drjones, kvm, kvmarm; +Cc: pbonzini The --vmm configure option was added to distinguish between the two virtual machine managers that kvm-unit-tests supports, qemu or kvmtool. There are plans to make kvm-unit-tests work as an EFI app, which will require changes to the way tests are compiled. Instead of adding a new configure option specifically for EFI and have it coexist with --vmm, or overloading the semantics of the existing --vmm option, let's replace --vmm with the more generic name --target. --vmm has been kept for now as to avoid breaking existing users, but it has been modified to shadow value of --target and a message will be displayed to notify users that it will be removed at some point in the future. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- configure | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/configure b/configure index 01a0b262a9f2..71d6dc9490df 100755 --- a/configure +++ b/configure @@ -21,7 +21,8 @@ pretty_print_stacks=yes environ_default=yes u32_long= wa_divide= -vmm="qemu" +vmm= +target="qemu" errata_force=0 erratatxt="$srcdir/errata.txt" host_key_document= @@ -35,8 +36,11 @@ usage() { Options include: --arch=ARCH architecture to compile for ($arch) --processor=PROCESSOR processor to compile for ($arch) - --vmm=VMM virtual machine monitor to compile for (qemu - or kvmtool, default is qemu) (arm/arm64 only) + --target=TARGET target platform that the tests will be running on (qemu or + kvmtool, default is qemu) (arm/arm64 only) + --vmm=VMM virtual machine monitor to compile for (qemu or kvmtool). + If specified, it must have the same value as the --target + option (arm/arm64 only) (deprecated) --cross-prefix=PREFIX cross compiler prefix --cc=CC c compiler to use ($cc) --ld=LD ld linker to use ($ld) @@ -58,7 +62,7 @@ usage() { --earlycon=EARLYCON Specify the UART name, type and address (optional, arm and arm64 only). The specified address will overwrite the UART - address set by the --vmm option. EARLYCON can be one of + address set by the --target option. EARLYCON can be one of (case sensitive): uart[8250],mmio,ADDR Specify an 8250 compatible UART at address ADDR. Supported @@ -91,6 +95,9 @@ while [[ "$1" = -* ]]; do --vmm) vmm="$arg" ;; + --target) + target="$arg" + ;; --cross-prefix) cross_prefix="$arg" ;; @@ -177,16 +184,24 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then testdir=x86 elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then testdir=arm - if [ "$vmm" = "qemu" ]; then + if [ "$target" = "qemu" ]; then arm_uart_early_addr=0x09000000 - elif [ "$vmm" = "kvmtool" ]; then + elif [ "$target" = "kvmtool" ]; then arm_uart_early_addr=0x3f8 errata_force=1 else - echo '--vmm must be one of "qemu" or "kvmtool"!' + echo '--target must be one of "qemu" or "kvmtool"!' usage fi + if [ -n "$vmm" ]; then + echo "INFO: --vmm is deprecated and will be removed in future versions" + if [ "$vmm" != "$target" ]; then + echo "--vmm must have the same value as --target ($target)" + usage + fi + fi + if [ "$earlycon" ]; then IFS=, read -r name type_addr addr <<<"$earlycon" if [ "$name" != "uart" ] && [ "$name" != "uart8250" ] && @@ -317,6 +332,7 @@ U32_LONG_FMT=$u32_long WA_DIVIDE=$wa_divide GENPROTIMG=${GENPROTIMG-genprotimg} HOST_KEY_DOCUMENT=$host_key_document +TARGET=$target EOF cat <<EOF > lib/config.h -- 2.31.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target 2021-04-20 16:13 [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target Alexandru Elisei 2021-04-20 16:13 ` [kvm-unit-tests RFC PATCH 1/1] " Alexandru Elisei @ 2021-04-20 16:51 ` Andrew Jones 2021-04-22 15:17 ` Alexandru Elisei 1 sibling, 1 reply; 8+ messages in thread From: Andrew Jones @ 2021-04-20 16:51 UTC (permalink / raw) To: Alexandru Elisei; +Cc: kvm, kvmarm, pbonzini 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 And it wouldn't bother me to have special logic for kvmtool's lack of migration put directly in scripts/runtime.bash 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. Thanks, drew ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target 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 0 siblings, 1 reply; 8+ messages in thread From: Alexandru Elisei @ 2021-04-22 15:17 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvmarm, pbonzini 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. > > 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. Thanks, Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target 2021-04-22 15:17 ` Alexandru Elisei @ 2021-04-22 15:57 ` Andrew Jones 2021-04-23 15:43 ` Alexandru Elisei 0 siblings, 1 reply; 8+ messages in thread From: Andrew Jones @ 2021-04-22 15:57 UTC (permalink / raw) To: Alexandru Elisei; +Cc: kvm, kvmarm, pbonzini 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. > > > > > 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. Thanks, drew ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target 2021-04-22 15:57 ` Andrew Jones @ 2021-04-23 15:43 ` Alexandru Elisei 2021-04-26 8:59 ` Andrew Jones 0 siblings, 1 reply; 8+ messages in thread From: Alexandru Elisei @ 2021-04-23 15:43 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvmarm, pbonzini 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 moment. 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 migration 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 --target=qemu 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 --target=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? Thanks, Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target 2021-04-23 15:43 ` Alexandru Elisei @ 2021-04-26 8:59 ` Andrew Jones 2021-04-26 14:11 ` Alexandru Elisei 0 siblings, 1 reply; 8+ messages in thread From: Andrew Jones @ 2021-04-26 8:59 UTC (permalink / raw) To: Alexandru Elisei; +Cc: kvm, kvmarm, pbonzini On Fri, Apr 23, 2021 at 04:43:14PM +0100, Alexandru Elisei wrote: > 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 > moment. > > 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 > migration > 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? I'll leave that up to you, since you're the one driving support for kvmtool and, hopefully soon, bare-metal AArch64. BTW, I think we're long overdue for adding kvmtool runner functionality, either by adapting what we have (possibly by applying a TARGET variable :-) or by simply adding new runner scripts. I personally would like to easily run kvmtool when I'm testing arm/queue, and I don't want to have my own personal kvmtool runner script to do that. > > > > >>> 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 > --target=qemu > 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 > --target=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? Deprecation requires you do two things at once; add a warning to the old and add the new. I'm saying we don't need to deprecate --vmm. Either just do the new (s/vmm/target/g) or always allow the old (s/vmm/target/g plus make --vmm an alias for --target without any warning). I'd prefer the first one, since I'm not too worried about a few users having to figure out how to change their muscle memory and CI scripts when they start getting unknown option errors at configure time. Thanks, drew ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target 2021-04-26 8:59 ` Andrew Jones @ 2021-04-26 14:11 ` Alexandru Elisei 0 siblings, 0 replies; 8+ messages in thread From: Alexandru Elisei @ 2021-04-26 14:11 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, kvmarm, pbonzini Hi Drew, On 4/26/21 9:59 AM, Andrew Jones wrote: > On Fri, Apr 23, 2021 at 04:43:14PM +0100, Alexandru Elisei wrote: >> 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 >> moment. >> >> 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 >> migration >> 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? > I'll leave that up to you, since you're the one driving support for > kvmtool and, hopefully soon, bare-metal AArch64. BTW, I think we're long If it's up to me, then I would prefer this gets added to the test definitions along with kvmtool (or baremetal) runscript support, when we have a well defined usecase for it. > overdue for adding kvmtool runner functionality, either by adapting what > we have (possibly by applying a TARGET variable :-) or by simply adding > new runner scripts. I personally would like to easily run kvmtool when > I'm testing arm/queue, and I don't want to have my own personal kvmtool > runner script to do that. I agree, this is sorely needed. There was someone from Arm that was interested in adding it, but that hasn't materialized yet. Regardless, I'll added it to my (rather long) list of todo's. > >>>>> 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 >> --target=qemu >> 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 >> --target=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? > Deprecation requires you do two things at once; add a warning to the old > and add the new. I'm saying we don't need to deprecate --vmm. Either just > do the new (s/vmm/target/g) or always allow the old (s/vmm/target/g plus > make --vmm an alias for --target without any warning). I'd prefer the > first one, since I'm not too worried about a few users having to figure > out how to change their muscle memory and CI scripts when they start > getting unknown option errors at configure time. Ok, I see now, I prefer the first approach, I'll remove --vmm entirely and replace it with --target. Thanks, Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-26 14:11 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-20 16:13 [kvm-unit-tests RFC PATCH 0/1] configure: arm: Replace --vmm with --target 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 2021-04-26 8:59 ` Andrew Jones 2021-04-26 14:11 ` Alexandru Elisei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).