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

[Peter Maydell]

On 13 May 2012 23:57, Andreas Färber <address@hidden> wrote:
> 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.

Sure.

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

Yes, makes sense; as you say I wrote most of this before the ARMCPU
changes went in. I think that would allow us to sidestep the cpu_copy()
issues and only leave us needing to free the hashtable on cpu deletion...

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

You mean this 01/32 patch specifically? I can't see an obvious split
to make but I'm open to the idea if you have one in mind.

-- PMM