Re: [Qemu-devel] [PATCH 01/32] target-arm: initial coprocessor register framework

[Andreas Färber]

Am 15.04.2012 15:45, schrieb Peter Maydell:
> Initial infrastructure for data-driven registration of
> coprocessor register implementations.
> 
> We still fall back to the old-style switch statements pending
> complete conversion of all existing registers.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target-arm/cpu.c       |   34 ++++++++
>  target-arm/cpu.h       |  214 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/helper.c    |   83 +++++++++++++++++++
>  target-arm/helper.h    |    5 +
>  target-arm/op_helper.c |   42 +++++++++-
>  target-arm/translate.c |  155 ++++++++++++++++++++++++++++++++++-
>  6 files changed, 530 insertions(+), 3 deletions(-)
> 
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 8f5e309..ae55cd0 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -24,6 +24,37 @@
>  #include "hw/loader.h"
>  #endif
>  
> +static void cp_reg_reset(void *key, void *value, void *udata)
> +{

This ugliness is thanks to GLib, it seems. In that case please stick to
their signature to make that clear, i.e. 3x gpointer.

http://developer.gnome.org/glib/stable/glib-Hash-Tables.html#GHFunc

user_data / data / opaque would also seem nicer than udata.

> +    /* Reset a single ARMCPRegInfo register */
> +    ARMCPRegInfo *ri = value;
> +    CPUARMState *env = udata;
> +
> +    if (ri->type & ARM_CP_SPECIAL) {
> +        return;
> +    }
> +
> +    if (ri->resetfn) {
> +        ri->resetfn(env, ri);
> +        return;
> +    }
> +
> +    /* A zero offset is never possible as it would be regs[0]
> +     * so we use it to indicate that reset is being handled elsewhere.
> +     * This is basically only used for fields in non-core coprocessors
> +     * (like the pxa2xx ones).
> +     */
> +    if (!ri->fieldoffset) {
> +        return;
> +    }
> +
> +    if (ri->type & ARM_CP_64BIT) {
> +        CPREG_FIELD64(env, ri) = ri->resetvalue;
> +    } else {
> +        CPREG_FIELD32(env, ri) = ri->resetvalue;
> +    }
> +}
> +
>  /* CPUClass::reset() */
>  static void arm_cpu_reset(CPUState *s)
>  {
[...]
> @@ -130,6 +162,8 @@ static void arm_cpu_initfn(Object *obj)
>      ARMCPU *cpu = ARM_CPU(obj);
>  
>      cpu_exec_init(&cpu->env);
> +    cpu->env.cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
> +                                             g_free, g_free);
>  }
>  
>  void arm_cpu_realize(ARMCPU *cpu)
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 12f5854..f35d24f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -228,6 +228,9 @@ typedef struct CPUARMState {
>      /* Internal CPU feature flags.  */
>      uint32_t features;
>  
> +    /* Coprocessor information */
> +    GHashTable *cp_regs;
> +
>      /* Coprocessor IO used by peripherals */
>      struct {
>          ARMReadCPFunc *cp_read;
[snip]

I'm aware this series predates the QOM era, but I'm not really happy how
this aligns with my CPUState overhaul. Independent of what needs to be
fixed for cpu_copy(), I would like to see new non-TCG fields such as
this hashtable added to ARMCPU after the env field, not to the old
CPUARMState (using fieldoffset +/- from env is correct though). By
consequence the API should be changed to take ARMCPU *cpu rather than
CPUARMState *env to avoid the QOM casts that you so loathe and to avoid
us refactoring this new API in a few weeks again.

The ARM cp14/cp15 specific bits I do not feel qualified to review and am
counting on Rusty and Paul to review. Any chance to further split up
this patch?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg