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

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

[Alex Bennée]

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.

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
+
 chr_testdev='-device virtio-serial-device'
 chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
 
-- 
2.4.5

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

[Andrew Jones]

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!

> 
> 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/'

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.

5) we use tabs for indentation in arm/run, and only bother with the
   variable's {}, if necessary

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

Thanks!
drew

[*] https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg07514.html

> +
>  chr_testdev='-device virtio-serial-device'
>  chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
>  
> -- 
> 2.4.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alex Bennée]

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

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

I'll patch the readme ;-)

>
> Thanks!
> drew
>
> [*] https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg07514.html
>
>> +
>>  chr_testdev='-device virtio-serial-device'
>>  chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
>>  
>> -- 
>> 2.4.5
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Alex Bennée

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

[Andrew Jones]

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

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

[Paolo Bonzini]

On 02/07/2015 13:51, Andrew Jones wrote:
> 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.

Can you just use --no-kvm?  It is equivalent to "-machine accel=tcg",
and it overrides previous "-machine accel=foo" options.

Paolo

ps: I also share the "yay" feeling, of course!

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

[Andrew Jones]

On Thu, Jul 02, 2015 at 03:45:17PM +0200, Paolo Bonzini wrote:
> 
> 
> On 02/07/2015 13:51, Andrew Jones wrote:
> > 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.
> 
> Can you just use --no-kvm?  It is equivalent to "-machine accel=tcg",

Sounds perfect. Thanks!

> and it overrides previous "-machine accel=foo" options.
> 
> Paolo
> 
> ps: I also share the "yay" feeling, of course!
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alex Bennée]

Andrew Jones <drjones@redhat.com> writes:

> On Thu, Jul 02, 2015 at 03:45:17PM +0200, Paolo Bonzini wrote:
>> 
>> 
>> On 02/07/2015 13:51, Andrew Jones wrote:
>> > 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.
>> 
>> Can you just use --no-kvm?  It is equivalent to "-machine accel=tcg",
>
> Sounds perfect. Thanks!
>
>> and it overrides previous "-machine accel=foo" options.

For a TCG only build it complains:

"Option no-kvm not supported for this target" :-/

>> 
>> Paolo
>> 
>> ps: I also share the "yay" feeling, of course!
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Alex Bennée

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

[Paolo Bonzini]

On 03/07/2015 14:24, Alex Bennée wrote:
> 
> Andrew Jones <drjones@redhat.com> writes:
> 
>> On Thu, Jul 02, 2015 at 03:45:17PM +0200, Paolo Bonzini wrote:
>>>
>>>
>>> On 02/07/2015 13:51, Andrew Jones wrote:
>>>> 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.
>>>
>>> Can you just use --no-kvm?  It is equivalent to "-machine accel=tcg",
>>
>> Sounds perfect. Thanks!
>>
>>> and it overrides previous "-machine accel=foo" options.
> 
> For a TCG only build it complains:
> 
> "Option no-kvm not supported for this target" :-/

Should be easy to fix though (or just use "-machine accel=tcg", it's
longer but it works).

Paolo