(Accidentally made first reply to Peter only, fixed that now). On Mon, 7 May 2012 13:25:07 +0100, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 7 May 2012 08:23, Rusty Russell <ru...@rustcorp.com.au> wrote: > > OK, I reviewed the infrastructure, and it looks excellent. A few of > > minor quibbles, which I only mention to show that I read it :) > > > > 1) cptype_valid, arm_current_pl and encoded_cp_matches_type could return > > bool. > > > > 2) the access bits could be an enum type, which could be used in the > > few places they're handled, for a bit more explicitness. > > Mmm. This kind of thing is my old-school-C heritage showing through > :-)
Maybe :) I was delighted by your use of a non-zero terminal value though, so I hardly noticed. > > 3) Perhaps an "assert(!g_hash_table_lookup(env->cp_regs, key));" before > > the g_hash_table_insert, to avoid overlapping entries. > > Overlapping entries are deliberately permitted (and used in some > cases). The idea is that last entry wins, so you can put in a > "whole region behaves like this" wildcard case and override it > with a few special cases. I feel nervous without flag on either the overridden or overriding one, to show it's deliberate. > > I then skimmed your epic refactoring, and wherever I stopped it looked > > completely sane. > > > > I wondered about an ARM_CP_DEPRECATED flag, which would print out a > > nasty "email the list" message if hit. Maybe it still won't provide > > enough confidence to tighten emulation, though. > > Mmm. Really I would like qemu to have a better logging infrastructure > so we could classify things into "debug info/qemu bug/guest has done > something dubious" and let the user turn the logging level up or down. > In the absence of that I tend to just not put in tracing :-( Seems like YA infrastructure adventure we could embark upon. I'll add it to the list :) > The other thing on my todo list is that I don't think we're correctly > handling the hashtable on cpu_delete/cpu_copy. I don't pretend to understand the QEMU Object Model, but it seems like you're missing a level of indirection by putting the cp_regs hash into each CPUARMState directly. More logically each ARMCPU would have a pointer to its ARMCPUType, which would contain the cp_regs hash (and maybe other things). Cheers, Rusty.
