Re: [Qemu-devel] [PATCH 1/5] target-arm: Add ARM CPU feature parsing

["Alex Bennée" <alex.bennee@linaro.org>]

Greg Bellows <greg.bellows@linaro.org> writes:

> Adds a CPU feature parsing function and assigns to the CPU class.  The only
> feature added was "-aarch64" which disabled the AArch64 execution state on a
> 64-bit ARM CPU.
>
> 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>
> ---
>  target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 285947f..f327dd7 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -514,13 +514,17 @@ 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_strdup(cpu_model);
> +    cpuname = strtok(cpuname, ",");
> +    typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname);
>      oc = object_class_by_name(typename);
> +    g_free(cpuname);
>      g_free(typename);

Aren't we leaking here? strtok returns the next token (or NULL) so don't
we loose the original ptr?

Also while using glib you might want to consider using glib's own
tokenising functions (e.g. g_strsplit). This has the advantage of having
helper functions like g_strfreev() which can clean-up everything in one go.

>      if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
>          object_class_is_abstract(oc)) {
> @@ -1163,6 +1167,44 @@ static Property arm_cpu_properties[] = {
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> +static void arm_cpu_parse_features(CPUState *cs, char *features,
> +                                   Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    char *featurestr;
> +
> +    featurestr = features ? strtok(features, ",") : NULL;
> +    while (featurestr) {
> +        if (featurestr[0] == '-') {
> +            if (!strcmp(featurestr+1, "aarch64")) {
> +                /* If AArch64 is disabled then we need to unset the feature */
> +                unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> +            } else {
> +                /* Everyting else is unsupported */
> +                error_setg(errp, "unsupported CPU property '%s'",
> +                           &featurestr[1]);
> +                return;
> +            }
> +        } else if (featurestr[0] == '+') {
> +            /* No '+' properties supported yet */
> +            error_setg(errp, "unsupported CPU property '%s'",
> +                       &featurestr[1]);
> +            return;
> +        } else if (g_strstr_len(featurestr, -1, "=")) {
> +            /* No '=' properties supported yet */
> +            char *prop = strtok(featurestr, "=");
> +            error_setg(errp, "unsupported CPU property '%s'", prop);
> +            return;
> +        } else {
> +            /* Everything else is a bad format */
> +            error_setg(errp, "CPU property string '%s' not in format "
> +                             "(+feature|-feature|feature=xyz)", featurestr);
> +            return;
> +        }
> +        featurestr = strtok(NULL, ",");
> +    }
> +}

I only point to this for reference to a "gliby" approach to the parsing,
relative beauty being in the eye of the beholder ;-)

https://github.com/stsquad/qemu/commit/86bc88f661141b93cbe5b107c4d5b4322b563241#diff-286aa0f2c1f0d862c4197781280a92efR116

It does make me think boilerplate but I wonder if this is a generic
enough problem to be solved more generally in QEMU?

> +
>  static void arm_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> @@ -1183,6 +1225,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->set_pc = arm_cpu_set_pc;
>      cc->gdb_read_register = arm_cpu_gdb_read_register;
>      cc->gdb_write_register = arm_cpu_gdb_write_register;
> +    cc->parse_features = arm_cpu_parse_features;
>  #ifdef CONFIG_USER_ONLY
>      cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
>  #else

-- 
Alex Bennée