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