Re: [Qemu-devel] [PATCH 00/32] target-arm: refactor copro register implementation

[Rusty Russell]

(Accidentally made first reply to Peter only, fixed that now).

On Mon, 7 May 2012 13:25:07 +0100, Peter Maydell <address@hidden> wrote:
> On 7 May 2012 08:23, Rusty Russell <address@hidden> 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.