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

[Peter Maydell <peter.maydell@linaro.org>]

On 8 May 2012 06:57, Rusty Russell <rusty.russell@linaro.org> wrote:
> (Accidentally made first reply to Peter only, fixed that now).
>
> On Mon, 7 May 2012 13:25:07 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 7 May 2012 08:23, Rusty Russell <rusty@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.

I'd just been bitten by forgetting the terminator on a qdev Property
list and having it work fine when compiled in debug mode and only
break with optimisation turned on, so I wanted to try to catch that
kind of error :-)

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

Yes, that's probably a good idea.

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

At some point properties like "does this core have $FEATURE?" and so on
will become user-settable QOM properties, which means that they will
apply per-instance of the object. So different instances of the same
object could have different sets of cp15 registers. Admittedly at
the moment QEMU's insistence that there can be only one CPU means there
aren't really any sane configs where the cores differ[*], but I think
it indicates that in principle the hashtable should be per-instance
rather than per-class.

[*] Apparently there have occasionally been FPGAs with stupid configs
like an A9x2 with only one core with Neon, and that is technically
a valid way to configure the RTL, but nobody does it in practice.

-- PMM