[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/32] target-arm: initial coprocessor register
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH 01/32] target-arm: initial coprocessor register framework |
Date: |
Mon, 14 May 2012 11:34:13 +0100 |
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