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

[Peter Maydell]

On 10 February 2015 at 10:50, Greg Bellows <address@hidden> 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 <address@hidden>
>
> ---
>
> 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 <address@hidden>

-- PMM