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

[Greg Bellows]

Thanks Alex comments inline....

On Tue, Jan 20, 2015 at 8:19 AM, Alex Bennée <address@hidden> wrote:

Greg Bellows <address@hidden> 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 <address@hidden>
> ---
>  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?

​As I understand it, strtok uses static pointers to track the location within an existing string rather than allocating storage that the user must free.  This is apparently what makes the version I used non-reentrant.​  In which case, there should not be an leak due to its use.  

 

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.

​I certainly can use the glib version, but in this case I did not see the advantage. In fact, it actually may be less performant​ to use the glib version given it needs to allocate/free memory.  I am fine either way if anyone feels strongly.

 

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

If we were to go with a boilerplate approach then it would make sense to follow the mechanism used for machine properties.  However, this was how other arch were doing the CPU props, so we stuck to this approach.  It does look very similar to the code you pointed at, but I think that the conditional piece looking for {+-=} would bt the only common piece once you add unique options and handling.

 

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