Re: [Qemu-devel] [PATCH v4 1/4] target-arm: Add CPU property to disable AArch64

[Peter Maydell]

On 11 February 2015 at 04:27, Greg Bellows <address@hidden> wrote:
>
>
> On Tue, Feb 10, 2015 at 10:03 PM, Peter Maydell <address@hidden>
> wrote:
>>
>> On 10 February 2015 at 10:50, Greg Bellows <address@hidden>
>> wrote:
>> > +    if (!kvm_enabled()) {
>> > +        error_setg(errp,
>> > +                   "'aarch64' option only supported with
>> > '-enable-kvm'");
>> > +        return;
>>
>> This check needs to go in the "we're unsetting the feature bit"
>> code path, and we could make the error message a little clearer:
>> "'aarch64' feature cannot be disabled unless KVM is enabled"
>> (setting the feature to on is harmless and will work with TCG :-))
>>
> Although it may be benign, the user may be doing something they think may
> have an effect which is why I catch any case (not just off).  I see this as
> being cleaner.

OK; I don't feel strongly about this.

> As far as the message, the user does not really know about an "aarch64"
> feature, that is internal to QEMU.  Given this is a command line option
> error, I believe it makes more sense to keep it in that domain.

The point is that it's not a command line option. That would be
"qemu-system-aarch64 -aarch64", but what we actually have is
"-cpu whatever,-aarch64=off", which is a feature enable/disable.
You could say "Setting CPU property 'aarch64' to off is not
valid unless KVM is enabled" if you prefer that wording.

> The message
> as is describes the error in terms the command line options.  Maybe a
> compromise such as below would work:
>
> "aarch64 execution state can only be disabled when enabling KVM using the
> '-enable-kvm' option

This seems backwards, because it isn't precise about what
the user has done wrong on their command line (asked us to
disable the 'aarch64' cpu feature) but is precise about
something that's not very important (the option you can use
to enable KVM, and in fact you can enable KVM via other
command line syntax too).

Also, if you're referring to the CPU state, that needs capitals:
AArch64. Only use lowercase if you're talking about the
user-facing feature-switch name (in which case it should
go in quotes).

thanks
-- PMM