[v4,1/4] target-arm: Add CPU property to disable AArch64

Adds registration and get/set functions for enabling/disabling the AArch64
execution state on AArch64 CPUs.  By default AArch64 execution state is enabled
on AArch64 CPUs, setting the property to off, will disable the execution state.
The below QEMU invocation would have AArch64 execution state disabled.

    $ ./qemu-system-aarch64 -machine virt -cpu cortex-a57,aarch64=off

Also adds stripping of features from CPU model string in acquiring the ARM CPU
by name.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

---

v3 -> v4
- Switch from using strtok to g_strsplit
- Add disablement of aarch64 option if KVM is not enabled.

v1 -> v2
- Scrap the custom CPU feature parsing in favor of using the default CPU
  parsing.
- Add registration of CPU AArch64 property to disable/enable the AArch64
  feature.
---
 target-arm/cpu.c   |  5 ++++-
 target-arm/cpu64.c | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

On 10 February 2015 at 10:50, Greg Bellows <greg.bellows@linaro.org> wrote:
> Adds registration and get/set functions for enabling/disabling the AArch64
> execution state on AArch64 CPUs.  By default AArch64 execution state is enabled
> on AArch64 CPUs, setting the property to off, will disable the execution state.
> The below QEMU invocation would have AArch64 execution state disabled.
>
>     $ ./qemu-system-aarch64 -machine virt -cpu cortex-a57,aarch64=off
>
> Also adds stripping of features from CPU model string in acquiring the ARM CPU
> by name.
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>
> ---
>
> v3 -> v4
> - Switch from using strtok to g_strsplit
> - Add disablement of aarch64 option if KVM is not enabled.
>
> v1 -> v2
> - Scrap the custom CPU feature parsing in favor of using the default CPU
>   parsing.
> - Add registration of CPU AArch64 property to disable/enable the AArch64
>   feature.
> ---
>  target-arm/cpu.c   |  5 ++++-
>  target-arm/cpu64.c | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index d38af74..986f04c 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -544,13 +544,16 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
>  {
>      ObjectClass *oc;
>      char *typename;
> +    char **cpuname;
>
>      if (!cpu_model) {
>          return NULL;
>      }
>
> -    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
> +    cpuname = g_strsplit(cpu_model, ",", 1);
> +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname[0]);
>      oc = object_class_by_name(typename);
> +    g_strfreev(cpuname);
>      g_free(typename);
>      if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
>          object_class_is_abstract(oc)) {
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index bb778b3..d03f203 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -32,6 +32,11 @@ static inline void set_feature(CPUARMState *env, int feature)
>      env->features |= 1ULL << feature;
>  }
>
> +static inline void unset_feature(CPUARMState *env, int feature)
> +{
> +    env->features &= ~(1ULL << feature);
> +}
> +
>  #ifndef CONFIG_USER_ONLY
>  static uint64_t a57_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> @@ -170,8 +175,42 @@ static const ARMCPUInfo aarch64_cpus[] = {
>      { .name = NULL }
>  };
>
> +static bool aarch64_cpu_get_aarch64(Object *obj, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    return arm_feature(&cpu->env, ARM_FEATURE_AARCH64);
> +}
> +
> +static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    /* At this time, this property is only allowed if KVM is enabled.  This
> +     * restriction allows us to avoid fixing up functionality that assumes a
> +     * uniform execution state like do_interrupt.
> +     */
> +    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 :-))

> +    }
> +
> +    if (value == false) {
> +        unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> +    } else {
> +        set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> +    }
> +}
> +
>  static void aarch64_cpu_initfn(Object *obj)
>  {
> +    object_property_add_bool(obj, "aarch64", aarch64_cpu_get_aarch64,
> +                             aarch64_cpu_set_aarch64, NULL);
> +    object_property_set_description(obj, "aarch64",
> +                                    "Set on/off to enable/disable aarch64 "
> +                                    "execution state ",
> +                                    NULL);

"Set on/off to enable/disable support for AArch64 execution
state. Disable this to boot 32-bit guests in AArch32 state."

(Is that space at the end of your description deliberate or
accidental?)

Otherwise, Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

On Tue, Feb 10, 2015 at 10:03 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 10 February 2015 at 10:50, Greg Bellows <greg.bellows@linaro.org>
> wrote:
> > Adds registration and get/set functions for enabling/disabling the
> AArch64
> > execution state on AArch64 CPUs.  By default AArch64 execution state is
> enabled
> > on AArch64 CPUs, setting the property to off, will disable the execution
> state.
> > The below QEMU invocation would have AArch64 execution state disabled.
> >
> >     $ ./qemu-system-aarch64 -machine virt -cpu cortex-a57,aarch64=off
> >
> > Also adds stripping of features from CPU model string in acquiring the
> ARM CPU
> > by name.
> >
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> >
> > ---
> >
> > v3 -> v4
> > - Switch from using strtok to g_strsplit
> > - Add disablement of aarch64 option if KVM is not enabled.
> >
> > v1 -> v2
> > - Scrap the custom CPU feature parsing in favor of using the default CPU
> >   parsing.
> > - Add registration of CPU AArch64 property to disable/enable the AArch64
> >   feature.
> > ---
> >  target-arm/cpu.c   |  5 ++++-
> >  target-arm/cpu64.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index d38af74..986f04c 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -544,13 +544,16 @@ static ObjectClass *arm_cpu_class_by_name(const
> char *cpu_model)
> >  {
> >      ObjectClass *oc;
> >      char *typename;
> > +    char **cpuname;
> >
> >      if (!cpu_model) {
> >          return NULL;
> >      }
> >
> > -    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model);
> > +    cpuname = g_strsplit(cpu_model, ",", 1);
> > +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname[0]);
> >      oc = object_class_by_name(typename);
> > +    g_strfreev(cpuname);
> >      g_free(typename);
> >      if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
> >          object_class_is_abstract(oc)) {
> > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> > index bb778b3..d03f203 100644
> > --- a/target-arm/cpu64.c
> > +++ b/target-arm/cpu64.c
> > @@ -32,6 +32,11 @@ static inline void set_feature(CPUARMState *env, int
> feature)
> >      env->features |= 1ULL << feature;
> >  }
> >
> > +static inline void unset_feature(CPUARMState *env, int feature)
> > +{
> > +    env->features &= ~(1ULL << feature);
> > +}
> > +
> >  #ifndef CONFIG_USER_ONLY
> >  static uint64_t a57_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo
> *ri)
> >  {
> > @@ -170,8 +175,42 @@ static const ARMCPUInfo aarch64_cpus[] = {
> >      { .name = NULL }
> >  };
> >
> > +static bool aarch64_cpu_get_aarch64(Object *obj, Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +
> > +    return arm_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > +}
> > +
> > +static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error
> **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +
> > +    /* At this time, this property is only allowed if KVM is enabled.
> This
> > +     * restriction allows us to avoid fixing up functionality that
> assumes a
> > +     * uniform execution state like do_interrupt.
> > +     */
> > +    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.​

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


> > +    }
> > +
> > +    if (value == false) {
> > +        unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > +    } else {
> > +        set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > +    }
> > +}
> > +
> >  static void aarch64_cpu_initfn(Object *obj)
> >  {
> > +    object_property_add_bool(obj, "aarch64", aarch64_cpu_get_aarch64,
> > +                             aarch64_cpu_set_aarch64, NULL);
> > +    object_property_set_description(obj, "aarch64",
> > +                                    "Set on/off to enable/disable
> aarch64 "
> > +                                    "execution state ",
> > +                                    NULL);
>
> "Set on/off to enable/disable support for AArch64 execution
> state. Disable this to boot 32-bit guests in AArch32 state."
>

​I'll add suggested wording.​


>
> (Is that space at the end of your description deliberate or
> accidental?)
>

​Space is inadvertant, I'll remove.​


>
> Otherwise, Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> -- PMM
>

On 11 February 2015 at 04:27, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On Tue, Feb 10, 2015 at 10:03 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 10 February 2015 at 10:50, Greg Bellows <greg.bellows@linaro.org>
>> 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