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