Re: [PATCH] arm/run: don't enable KVM if system can't do it

[Andrew Jones <drjones@redhat.com>]

On Thu, Jul 02, 2015 at 02:17:18PM +0100, Alex Bennée wrote:
> 
> Andrew Jones <drjones@redhat.com> writes:
> 
> > On Thu, Jul 02, 2015 at 12:05:31PM +0100, Alex Bennée wrote:
> >> As ARM (and no doubt other systems) can also run tests in pure TCG mode
> >> we might as well not bother enabling accel=kvm if we aren't on a real
> >> ARM based system. This prevents us seeing ugly warning messages when
> >> testing TCG.
> >
> > First,
> > YAY! We're getting contributions to kvm-unit-tests/arm!
> 
> :-) well so far I've been noodling about looking at it for KVM Guest
> Debug testing. I've a hideous branch on github that attempts to test
> exercise the debug register trapping code. However that falls down as I
> really need to find an easy way of attaching GDB to the qemu-gdb stub
> while the test is running.
> 
> However with the TCG multi-thread work coming up I certainly see the
> need to exercise QEMU in a way that the internal TCG test code might
> have trouble with.
> 
> >
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  arm/run | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/arm/run b/arm/run
> >> index 662a856..2bdb4be 100755
> >> --- a/arm/run
> >> +++ b/arm/run
> >> @@ -33,7 +33,13 @@ if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \
> >>  	exit 2
> >>  fi
> >>  
> >> -M='-machine virt,accel=kvm:tcg'
> >> +host=`uname -m | sed -e 's/arm.*/arm/'`
> >> +if [ "${host}" = "arm" ] || [ "${host}" = "aarch64" ]; then
> >> +    M='-machine virt,accel=kvm:tcg'
> >> +else
> >> +    M='-machine virt,accel=tcg'
> >> +fi
> >
> > I think this is a good idea, although I had actually left that warning
> > on purpose. Originally, the plan was for these unit tests to be kvm
> > specific. If they could be developed with the aid of tcg, and even used
> > to test tcg, then fine, but running them on tcg should always complain,
> > in order to make sure that the test output clearly showed that it had
> > not been running on kvm. Developing unit tests for tcg is also a good
> > idea though, and there's really no reason not to share this framework.
> >
> > So, for this patch I'd prefer we do a few things differently;
> >
> > 1) we should be able to integrate this new condition with the
> >    "arm64 must use '-cpu host' with kvm" condition that is lower down.
> >    And, let's just make this $HOST variable one that ./configure
> >    prepares, allowing that arm64 condition to s/$(arch)/$HOST/ and
> >    avoiding the need to duplicate the sed -e 's/arm.*/arm/'
> 
> Yeah makes sense.
> 
> >
> > 2) we might as well do something like
> >
> >    M='-machine virt'
> >    if using-kvm
> >      M+=',accel=kvm'
> >    else
> >      M+=',accel=tcg'
> >    fi
> >
> >    now, since we don't want to use the accel fallback feature anymore
> >
> > 3) outputting which one we're using might still be nice, otherwise
> >    one must inspect the qemu command line in the logs to find out
> >
> > 4) I recently mentioned[*] it might be nice to add a '-force-tcg' type
> >    of arm/run command line option, allowing tcg to be used even if
> >    it's possible to use kvm. Adding that at the same time would be
> >    nice.
> 
> Would it also be useful for other arches? Does run-tests.sh pass 

Maybe someday, so we might as well add it there. As long as it allows
current command lines to keep working as they have, then why not.

> >
> > 5) we use tabs for indentation in arm/run, and only bother with the
> >    variable's {}, if necessary
> 
> My shell quoting was rusty. I think $(host) was calling the host command
> for some reason.

Yes, $(cmd) executes cmd. ${var} is correct, but only necessary if you're
substituting a substring. For example

X=FOO
echo ${X}_BAR will echo FOO_BAR, but echo $X_BAR will echo whatever
the variable X_BAR is. It's not necessary to use the {} in most cases
though, space and some other characters, like /, automatically end
the variable name.

> 
> >
> > 6) we should post patches with [kvm-unit-tests PATCH] to avoid
> >    confusion with other kvm postings. (I screwed that up on my
> >    last two postings...).
> 
> /me ponders if he can just config git for that.

You can. Add

[format]
	subjectprefix = kvm-unit-tests PATCH

to your kvm-unit-tests/.git/config. I just hadn't bothered until now...

> 
> I'll patch the readme ;-)

Contributing code !AND! updating the readme! Double YAY!

Thanks,
drew