On 13 May 2012 23:57, Andreas Färber <afaer...@suse.de> 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 <peter.mayd...@linaro.org> >> --- >> 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