| Message ID | 20181106113133.51542781@doriath (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm-unit-tests: really use QEMU_ACCEL | expand |
On Tue, Nov 06, 2018 at 11:31:33AM -0500, Luiz Capitulino wrote: > > According to README.md, users can set QEMU_ACCEL to specify > kvm or tcg. However, get_qemu_acceletor() uses ACCEL instead, > which is wrong since ACCEL is an "internal" variable. The end > result is that tests will still run if the user wants to use > kvm but kvm is unavailable. Fix get_qemu_acceletor() to use > QEMU_ACCEL. Hi Luiz, I agree "ACCEL" isn't a very good name for an environment variable, which is why I reserved the name QEMU_ACCEL in the readme. However, ACCEL doesn't have to *only* be an internal variable, i.e. 'ACCEL=tcg ./run_tests.sh' currently works, and some users may be used to it now. Can you elaborate on the problem you're solving? Thanks, drew > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > scripts/arch-run.bash | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index d3ca19d..0997569 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -317,13 +317,13 @@ kvm_available () > > get_qemu_accelerator () > { > - if [ "$ACCEL" = "kvm" ] && ! kvm_available; then > + if [ "$QEMU_ACCEL" = "kvm" ] && ! kvm_available; then > echo "KVM is needed, but not available on this host" >&2 > return 2 > fi > > - if [ "$ACCEL" ]; then > - echo $ACCEL > + if [ "$QEMU_ACCEL" ]; then > + echo $QEMU_ACCEL > elif kvm_available; then > echo kvm > else > -- > 2.17.2 >
On Tue, 6 Nov 2018 17:48:52 +0100 Andrew Jones <drjones@redhat.com> wrote: > On Tue, Nov 06, 2018 at 11:31:33AM -0500, Luiz Capitulino wrote: > > > > According to README.md, users can set QEMU_ACCEL to specify > > kvm or tcg. However, get_qemu_acceletor() uses ACCEL instead, > > which is wrong since ACCEL is an "internal" variable. The end > > result is that tests will still run if the user wants to use > > kvm but kvm is unavailable. Fix get_qemu_acceletor() to use > > QEMU_ACCEL. > > Hi Luiz, > > I agree "ACCEL" isn't a very good name for an environment > variable, which is why I reserved the name QEMU_ACCEL in > the readme. However, ACCEL doesn't have to *only* be an > internal variable, i.e. 'ACCEL=tcg ./run_tests.sh' currently > works, and some users may be used to it now. For some reason I thought ACCEL=tcg ./run_tests.sh wouldn't work, but you're right it does. > Can you elaborate on the problem you're solving? Long story short, I just needed QEMU_ACCEL=kvm ./run_tests.sh to fail/skip tests if /dev/kvm is not available (instead of switching to tcg by default). Since ACCEL works, my problem is solved. IMO we need to update the README.md file or make QEMU_ACCEL work. > > Thanks, > drew > > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > scripts/arch-run.bash | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > > index d3ca19d..0997569 100644 > > --- a/scripts/arch-run.bash > > +++ b/scripts/arch-run.bash > > @@ -317,13 +317,13 @@ kvm_available () > > > > get_qemu_accelerator () > > { > > - if [ "$ACCEL" = "kvm" ] && ! kvm_available; then > > + if [ "$QEMU_ACCEL" = "kvm" ] && ! kvm_available; then > > echo "KVM is needed, but not available on this host" >&2 > > return 2 > > fi > > > > - if [ "$ACCEL" ]; then > > - echo $ACCEL > > + if [ "$QEMU_ACCEL" ]; then > > + echo $QEMU_ACCEL > > elif kvm_available; then > > echo kvm > > else > > -- > > 2.17.2 > > >
On Tue, Nov 06, 2018 at 12:05:05PM -0500, Luiz Capitulino wrote: > On Tue, 6 Nov 2018 17:48:52 +0100 > Andrew Jones <drjones@redhat.com> wrote: > > > On Tue, Nov 06, 2018 at 11:31:33AM -0500, Luiz Capitulino wrote: > > > > > > According to README.md, users can set QEMU_ACCEL to specify > > > kvm or tcg. However, get_qemu_acceletor() uses ACCEL instead, > > > which is wrong since ACCEL is an "internal" variable. The end > > > result is that tests will still run if the user wants to use > > > kvm but kvm is unavailable. Fix get_qemu_acceletor() to use > > > QEMU_ACCEL. > > > > Hi Luiz, > > > > I agree "ACCEL" isn't a very good name for an environment > > variable, which is why I reserved the name QEMU_ACCEL in > > the readme. However, ACCEL doesn't have to *only* be an > > internal variable, i.e. 'ACCEL=tcg ./run_tests.sh' currently > > works, and some users may be used to it now. > > For some reason I thought ACCEL=tcg ./run_tests.sh wouldn't > work, but you're right it does. > > > Can you elaborate on the problem you're solving? > > Long story short, I just needed QEMU_ACCEL=kvm ./run_tests.sh > to fail/skip tests if /dev/kvm is not available (instead of > switching to tcg by default). Since ACCEL works, my problem > is solved. > > IMO we need to update the README.md file or make QEMU_ACCEL > work. > We need to clarify the readme, and in general better document what variables do and how to use them. QEMU_ACCEL isn't wrong, it's just not very useful. It's purpose is for the guest code to determine if it's kvm or tcg, not the runtime bash scripts like ACCEL. You can use QEMU_ACCEL like this $ cat x86/output-accel.c #include <libcflat.h> void main(void) { printf("%s\n", getenv("QEMU_ACCEL")); } $ echo 'QEMU_ACCEL=ANY_STRING' > kvm_unit_tests_env $ ACCEL=tcg KVM_UNIT_TESTS_ENV=kvm_unit_tests_env x86/run x86/output-accel.flat /usr/bin/qemu-system-x86_64 -nodefaults -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device pci-testdev -machine accel=tcg -kernel x86/output-accel.flat -initrd kvm_unit_tests_env enabling apic ANY_STRING So using the same name for both won't necessarily avoid confusion, as they're actually two different things. Updating the readme and the wiki have been on my TODO for a long time. I'll really try to bump the priority. Thanks, drew
On Wed, 7 Nov 2018 10:33:21 +0100 Andrew Jones <drjones@redhat.com> wrote: > On Tue, Nov 06, 2018 at 12:05:05PM -0500, Luiz Capitulino wrote: > > On Tue, 6 Nov 2018 17:48:52 +0100 > > Andrew Jones <drjones@redhat.com> wrote: > > > > > On Tue, Nov 06, 2018 at 11:31:33AM -0500, Luiz Capitulino wrote: > > > > > > > > According to README.md, users can set QEMU_ACCEL to specify > > > > kvm or tcg. However, get_qemu_acceletor() uses ACCEL instead, > > > > which is wrong since ACCEL is an "internal" variable. The end > > > > result is that tests will still run if the user wants to use > > > > kvm but kvm is unavailable. Fix get_qemu_acceletor() to use > > > > QEMU_ACCEL. > > > > > > Hi Luiz, > > > > > > I agree "ACCEL" isn't a very good name for an environment > > > variable, which is why I reserved the name QEMU_ACCEL in > > > the readme. However, ACCEL doesn't have to *only* be an > > > internal variable, i.e. 'ACCEL=tcg ./run_tests.sh' currently > > > works, and some users may be used to it now. > > > > For some reason I thought ACCEL=tcg ./run_tests.sh wouldn't > > work, but you're right it does. > > > > > Can you elaborate on the problem you're solving? > > > > Long story short, I just needed QEMU_ACCEL=kvm ./run_tests.sh > > to fail/skip tests if /dev/kvm is not available (instead of > > switching to tcg by default). Since ACCEL works, my problem > > is solved. > > > > IMO we need to update the README.md file or make QEMU_ACCEL > > work. > > > > We need to clarify the readme, and in general better document what > variables do and how to use them. QEMU_ACCEL isn't wrong, it's just > not very useful. It's purpose is for the guest code to determine if > it's kvm or tcg, not the runtime bash scripts like ACCEL. You can > use QEMU_ACCEL like this > > $ cat x86/output-accel.c > #include <libcflat.h> > void main(void) > { > printf("%s\n", getenv("QEMU_ACCEL")); > } > > $ echo 'QEMU_ACCEL=ANY_STRING' > kvm_unit_tests_env > > $ ACCEL=tcg KVM_UNIT_TESTS_ENV=kvm_unit_tests_env x86/run x86/output-accel.flat > /usr/bin/qemu-system-x86_64 -nodefaults -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device pci-testdev -machine accel=tcg -kernel x86/output-accel.flat -initrd kvm_unit_tests_env > enabling apic > ANY_STRING > > So using the same name for both won't necessarily avoid confusion, > as they're actually two different things. Ohhh, that makes a lot of sense now. Thanks for taking the time to explain it! So, what happened in my case was: due to another bug, the kvm modules weren't loaded on my machine so I didn't have /dev/kvm. kvm-unit-tests then switched automatically to tcg, which caused some tests to fail. So, I went to the readme to see how I could force /dev/kvm usage and fail if it wasn't available. This led me to QEMU_ACCEL, not ACCEL. And I have a second question/request. Even when using ACCEL, if /dev/kvm is not available all tests will be skipped but I think it's useful to completely fail run_tests.sh instead right at the beginning. Can I add an option to run_tests.sh to do that? > Updating the readme and the wiki have been on my TODO for a long > time. I'll really try to bump the priority. I'd volunteer, but I have to learn more about kvm-unit-tests. But I think we probably want to separate documentation for those writing unit-tests and for those who just want to run them.
On Wed, Nov 07, 2018 at 09:13:58AM -0500, Luiz Capitulino wrote: > On Wed, 7 Nov 2018 10:33:21 +0100 > Andrew Jones <drjones@redhat.com> wrote: > > > On Tue, Nov 06, 2018 at 12:05:05PM -0500, Luiz Capitulino wrote: > > > On Tue, 6 Nov 2018 17:48:52 +0100 > > > Andrew Jones <drjones@redhat.com> wrote: > > > > > > > On Tue, Nov 06, 2018 at 11:31:33AM -0500, Luiz Capitulino wrote: > > > > > > > > > > According to README.md, users can set QEMU_ACCEL to specify > > > > > kvm or tcg. However, get_qemu_acceletor() uses ACCEL instead, > > > > > which is wrong since ACCEL is an "internal" variable. The end > > > > > result is that tests will still run if the user wants to use > > > > > kvm but kvm is unavailable. Fix get_qemu_acceletor() to use > > > > > QEMU_ACCEL. > > > > > > > > Hi Luiz, > > > > > > > > I agree "ACCEL" isn't a very good name for an environment > > > > variable, which is why I reserved the name QEMU_ACCEL in > > > > the readme. However, ACCEL doesn't have to *only* be an > > > > internal variable, i.e. 'ACCEL=tcg ./run_tests.sh' currently > > > > works, and some users may be used to it now. > > > > > > For some reason I thought ACCEL=tcg ./run_tests.sh wouldn't > > > work, but you're right it does. > > > > > > > Can you elaborate on the problem you're solving? > > > > > > Long story short, I just needed QEMU_ACCEL=kvm ./run_tests.sh > > > to fail/skip tests if /dev/kvm is not available (instead of > > > switching to tcg by default). Since ACCEL works, my problem > > > is solved. > > > > > > IMO we need to update the README.md file or make QEMU_ACCEL > > > work. > > > > > > > We need to clarify the readme, and in general better document what > > variables do and how to use them. QEMU_ACCEL isn't wrong, it's just > > not very useful. It's purpose is for the guest code to determine if > > it's kvm or tcg, not the runtime bash scripts like ACCEL. You can > > use QEMU_ACCEL like this > > > > $ cat x86/output-accel.c > > #include <libcflat.h> > > void main(void) > > { > > printf("%s\n", getenv("QEMU_ACCEL")); > > } > > > > $ echo 'QEMU_ACCEL=ANY_STRING' > kvm_unit_tests_env > > > > $ ACCEL=tcg KVM_UNIT_TESTS_ENV=kvm_unit_tests_env x86/run x86/output-accel.flat > > /usr/bin/qemu-system-x86_64 -nodefaults -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device pci-testdev -machine accel=tcg -kernel x86/output-accel.flat -initrd kvm_unit_tests_env > > enabling apic > > ANY_STRING > > > > So using the same name for both won't necessarily avoid confusion, > > as they're actually two different things. > > Ohhh, that makes a lot of sense now. Thanks for taking the time to > explain it! > > So, what happened in my case was: due to another bug, the kvm modules > weren't loaded on my machine so I didn't have /dev/kvm. kvm-unit-tests > then switched automatically to tcg, which caused some tests to fail. So, > I went to the readme to see how I could force /dev/kvm usage and fail > if it wasn't available. This led me to QEMU_ACCEL, not ACCEL. > > And I have a second question/request. Even when using ACCEL, if /dev/kvm > is not available all tests will be skipped but I think it's useful to > completely fail run_tests.sh instead right at the beginning. Can I add > an option to run_tests.sh to do that? Yes, as long as we can tell that the user requires kvm, then, if there isn't kvm, it makes sense to fail immediately. > > > Updating the readme and the wiki have been on my TODO for a long > > time. I'll really try to bump the priority. > > I'd volunteer, but I have to learn more about kvm-unit-tests. But I > think we probably want to separate documentation for those writing > unit-tests and for those who just want to run them. Good idea. Maybe while you're getting familiar with the test suite you can jot down some notes. Then, if you find time to turn those notes into documentation I'd be more than happy to review :-) I'd also be happy to take your notes in order to help me identify gaps to focus on while updating the docs myself. Thanks, drew
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index d3ca19d..0997569 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -317,13 +317,13 @@ kvm_available () get_qemu_accelerator () { - if [ "$ACCEL" = "kvm" ] && ! kvm_available; then + if [ "$QEMU_ACCEL" = "kvm" ] && ! kvm_available; then echo "KVM is needed, but not available on this host" >&2 return 2 fi - if [ "$ACCEL" ]; then - echo $ACCEL + if [ "$QEMU_ACCEL" ]; then + echo $QEMU_ACCEL elif kvm_available; then echo kvm else
According to README.md, users can set QEMU_ACCEL to specify kvm or tcg. However, get_qemu_acceletor() uses ACCEL instead, which is wrong since ACCEL is an "internal" variable. The end result is that tests will still run if the user wants to use kvm but kvm is unavailable. Fix get_qemu_acceletor() to use QEMU_ACCEL. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- scripts/arch-run.bash | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)