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

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

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

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

[Peter Maydell]

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

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

[Peter Maydell]

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.

So, I'm fixing cptype_valid to return bool, and encoded_cp_matches_type is
actually never used so I'm just deleting it. However arm_current_pl()
returns an integer 0..3 (although at the moment QEMU doesn't implement
PL2/PL3) and the access bits really are bits (cf how we implement
cp_access_ok()) so I think I'd rather not use an enum there.

cp_access_ok() also can return bool.

-- PMM

Re: [Qemu-devel] [PATCH 01/32] target-arm: initial coprocessor register framework

[Andreas Färber]

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

> +    /* Reset a single ARMCPRegInfo register */
> +    ARMCPRegInfo *ri = value;
> +    CPUARMState *env = udata;
> +
> +    if (ri->type & ARM_CP_SPECIAL) {
> +        return;
> +    }
> +
> +    if (ri->resetfn) {
> +        ri->resetfn(env, ri);
> +        return;
> +    }
> +
> +    /* A zero offset is never possible as it would be regs[0]
> +     * so we use it to indicate that reset is being handled elsewhere.
> +     * This is basically only used for fields in non-core coprocessors
> +     * (like the pxa2xx ones).
> +     */
> +    if (!ri->fieldoffset) {
> +        return;
> +    }
> +
> +    if (ri->type & ARM_CP_64BIT) {
> +        CPREG_FIELD64(env, ri) = ri->resetvalue;
> +    } else {
> +        CPREG_FIELD32(env, ri) = ri->resetvalue;
> +    }
> +}
> +
>  /* CPUClass::reset() */
>  static void arm_cpu_reset(CPUState *s)
>  {
[...]
> @@ -130,6 +162,8 @@ static void arm_cpu_initfn(Object *obj)
>      ARMCPU *cpu = ARM_CPU(obj);
>  
>      cpu_exec_init(&cpu->env);
> +    cpu->env.cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
> +                                             g_free, g_free);
>  }
>  
>  void arm_cpu_realize(ARMCPU *cpu)
> 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.

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?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 01/32] target-arm: initial coprocessor register framework

[Peter Maydell]

On 13 May 2012 23:57, Andreas Färber <afaerber@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.maydell@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

Re: [Qemu-devel] [PATCH 01/32] target-arm: initial coprocessor register framework

[Peter Maydell]

On 14 May 2012 11:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 May 2012 23:57, Andreas Färber <afaerber@suse.de> wrote:
>> 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...

So I've updated to put the hashtable in ARMCPU, and changed the APIs
for "define new register" so they take an ARMCPU* rather than the
CPUARMState*, which all looks good. Do you think the CPReadFn/CPWriteFn/
CPResetFn access function prototypes should also change to take an
ARMCPU*? I could do it, but it would mean that all the implementations
of these functions would just grow an extra "ARMCPUState *env = &cpu->env;"
or equivalent, since they have no need to get at the ARMCPU* but do need
the ARMCPUState*. If that's the thing that makes more long term sense
I'm happy to do it but I thought I'd check before I updated all
these patches :-)

thanks
-- PMM