diff mbox

[01/32] target-arm: initial coprocessor register framework

Message ID 1334497585-867-2-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show

Commit Message

Peter Maydell April 15, 2012, 1:45 p.m. UTC
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(-)

Comments

Andreas Färber May 13, 2012, 10:57 p.m. UTC | #1
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
Peter Maydell May 14, 2012, 10:34 a.m. UTC | #2
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
Peter Maydell May 14, 2012, 12:50 p.m. UTC | #3
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
diff mbox

Patch

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)
+{
+    /* 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)
 {
@@ -39,6 +70,7 @@  static void arm_cpu_reset(CPUState *s)
     acc->parent_reset(s);
 
     memset(env, 0, offsetof(CPUARMState, breakpoints));
+    g_hash_table_foreach(env->cp_regs, cp_reg_reset, env);
     env->cp15.c15_config_base_address = cpu->reset_cbar;
     env->cp15.c0_cpuid = cpu->midr;
     env->vfp.xregs[ARM_VFP_FPSID] = cpu->reset_fpsid;
@@ -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;
@@ -404,6 +407,217 @@  void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
                        ARMReadCPFunc *cp_read, ARMWriteCPFunc *cp_write,
                        void *opaque);
 
+/* Interface for defining coprocessor registers.
+ * Registers are defined in tables of arm_cp_reginfo structs
+ * which are passed to define_arm_cp_regs().
+ */
+
+/* When looking up a coprocessor register we look for it
+ * via an integer which encodes all of:
+ *  coprocessor number
+ *  Crn, Crm, opc1, opc2 fields
+ *  32 or 64 bit register (ie is it accessed via MRC/MCR
+ *    or via MRRC/MCRR?)
+ * We allow 4 bits for opc1 because MRRC/MCRR have a 4 bit field.
+ * (In this case crn and opc2 should be zero.)
+ */
+#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2)   \
+    (((cp) << 16) | ((is64) << 15) | ((crn) << 11) |    \
+     ((crm) << 7) | ((opc1) << 3) | (opc2))
+
+#define DECODE_CPREG_CRN(enc) (((enc) >> 7) & 0xf)
+
+/* ARMCPRegInfo type field bits. If the SPECIAL bit is set this is a
+ * special-behaviour cp reg and bits [15..8] indicate what behaviour
+ * it has. Otherwise it is a simple cp reg, where CONST indicates that
+ * TCG can assume the value to be constant (ie load at translate time)
+ * and 64BIT indicates a 64 bit wide coprocessor register. SUPPRESS_TB_END
+ * indicates that the TB should not be ended after a write to this register
+ * (the default is that the TB ends after cp writes).
+ */
+#define ARM_CP_SPECIAL 1
+#define ARM_CP_CONST 2
+#define ARM_CP_64BIT 4
+#define ARM_CP_SUPPRESS_TB_END 8
+#define ARM_CP_NOP (ARM_CP_SPECIAL | (1 << 8))
+#define ARM_CP_WFI (ARM_CP_SPECIAL | (2 << 8))
+/* Used only as a terminator for ARMCPRegInfo lists */
+#define ARM_CP_SENTINEL 0xffff
+/* Mask of only the flag bits in a type field */
+#define ARM_CP_FLAG_MASK 0xf
+
+/* Return true if cptype is a valid type field. This is used to try to
+ * catch errors where the sentinel has been accidentally left off the end
+ * of a list of registers.
+ */
+static inline int cptype_valid(int cptype)
+{
+    return cptype == ARM_CP_NOP
+        || cptype == ARM_CP_WFI
+        || ((cptype & ~ARM_CP_FLAG_MASK) == 0);
+}
+
+/* Access rights:
+ * We define bits for Read and Write access for what rev C of the v7-AR ARM ARM
+ * defines as PL0 (user), PL1 (fiq/irq/svc/abt/und/sys, ie privileged), and
+ * PL2 (hyp). The other level which has Read and Write bits is Secure PL1
+ * (ie any of the privileged modes in Secure state, or Monitor mode).
+ * If a register is accessible in one privilege level it's always accessible
+ * in higher privilege levels too. Since "Secure PL1" also follows this rule
+ * (ie anything visible in PL2 is visible in S-PL1, some things are only
+ * visible in S-PL1) but "Secure PL1" is a bit of a mouthful, we bend the
+ * terminology a little and call this PL3.
+ *
+ * If access permissions for a register are more complex than can be
+ * described with these bits, then use a laxer set of restrictions, and
+ * do the more restrictive/complex check inside a helper function.
+ */
+#define PL3_R 0x80
+#define PL3_W 0x40
+#define PL2_R (0x20 | PL3_R)
+#define PL2_W (0x10 | PL3_W)
+#define PL1_R (0x08 | PL2_R)
+#define PL1_W (0x04 | PL2_W)
+#define PL0_R (0x02 | PL1_R)
+#define PL0_W (0x01 | PL1_W)
+
+#define PL3_RW (PL3_R | PL3_W)
+#define PL2_RW (PL2_R | PL2_W)
+#define PL1_RW (PL1_R | PL1_W)
+#define PL0_RW (PL0_R | PL0_W)
+
+static inline int arm_current_pl(CPUARMState *env)
+{
+    if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
+        return 0;
+    }
+    /* We don't currently implement the Virtualization or TrustZone
+     * extensions, so PL2 and PL3 don't exist for us.
+     */
+    return 1;
+}
+
+/* Check that encoded cp and type agree about 64bitness of register */
+static inline int encoded_cp_matches_type(uint32_t encoded_cp, int type)
+{
+    if (type & ARM_CP_SPECIAL) {
+        return 1;
+    }
+    if (encoded_cp & (1 << 15)) {
+        return (type & ARM_CP_64BIT) ? 1 : 0;
+    } else {
+        return (type & ARM_CP_64BIT) ? 0 : 1;
+    }
+}
+
+typedef struct ARMCPRegInfo ARMCPRegInfo;
+
+/* Access functions for coprocessor registers. These should return
+ * 0 on success, or one of the EXCP_* constants if access should cause
+ * an exception (in which case *value is not written).
+ */
+typedef int CPReadFn(CPUARMState *env, const ARMCPRegInfo *opaque,
+                     uint64_t *value);
+typedef int CPWriteFn(CPUARMState *env, const ARMCPRegInfo *opaque,
+                      uint64_t value);
+/* Hook function for register reset */
+typedef void CPResetFn(CPUARMState *env, const ARMCPRegInfo *opaque);
+
+#define CP_ANY 0xff
+
+/* Definition of an ARM coprocessor register */
+struct ARMCPRegInfo {
+    /* Name of register (useful mainly for debugging, need not be unique) */
+    const char *name;
+    /* Location of register: coprocessor number and (crn,crm,opc1,opc2)
+     * tuple. Any of crm, opc1 and opc2 may be CP_ANY to indicate a
+     * 'wildcard' field -- any value of that field in the MRC/MCR insn
+     * will be decoded to this register. The register read and write
+     * callbacks will be passed an ARMCPRegInfo with the crn/crm/opc1/opc2
+     * used by the program, so it is possible to register a wildcard and
+     * then behave differently on read/write if necessary.
+     * For 64 bit registers, only crm and opc1 are relevant; crn and opc2
+     * must both be zero.
+     */
+    uint8_t cp;
+    uint8_t crn;
+    uint8_t crm;
+    uint8_t opc1;
+    uint8_t opc2;
+    /* Register type: ARM_CP_* bits/values */
+    int type;
+    /* Access rights: PL*_[RW] */
+    int access;
+    /* The opaque pointer passed to define_arm_cp_regs_with_opaque() when
+     * this register was defined: can be used to hand data through to the
+     * register read/write functions, since they are passed the ARMCPRegInfo*.
+     */
+    void *opaque;
+    /* Value of this register, if it is ARM_CP_CONST. Otherwise, if
+     * fieldoffset is non-zero, the reset value of the register.
+     */
+    uint64_t resetvalue;
+    /* Offset of the field in CPUARMState for this register. This is not
+     * needed if either:
+     *  1. type is ARM_CP_CONST or one of the ARM_CP_SPECIALs
+     *  2. both readfn and writefn are specified
+     */
+    ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) */
+    /* Function for handling reads of this register. If NULL, then reads
+     * will be done by loading from the offset into CPUARMState specified
+     * by fieldoffset.
+     */
+    CPReadFn *readfn;
+    /* Function for handling writes of this register. If NULL, then writes
+     * will be done by writing to the offset into CPUARMState specified
+     * by fieldoffset.
+     */
+    CPWriteFn *writefn;
+    /* Function for resetting the register. If NULL, then reset will be done
+     * by writing resetvalue to the field specified in fieldoffset. If
+     * fieldoffset is 0 then no reset will be done.
+     */
+    CPResetFn *resetfn;
+};
+
+/* Macros which are lvalues for the field in CPUARMState for the
+ * ARMCPRegInfo *ri.
+ */
+#define CPREG_FIELD32(env, ri) \
+    (*(uint32_t *)((char *)(env) + (ri)->fieldoffset))
+#define CPREG_FIELD64(env, ri) \
+    (*(uint64_t *)((char *)(env) + (ri)->fieldoffset))
+
+#define REGINFO_SENTINEL { .type = ARM_CP_SENTINEL }
+
+void define_arm_cp_regs_with_opaque(CPUARMState *env,
+                                    const ARMCPRegInfo *regs, void *opaque);
+void define_one_arm_cp_reg_with_opaque(CPUARMState *env,
+                                       const ARMCPRegInfo *regs, void *opaque);
+static inline void define_arm_cp_regs(CPUARMState *env,
+                                      const ARMCPRegInfo *regs)
+{
+    define_arm_cp_regs_with_opaque(env, regs, 0);
+}
+static inline void define_one_arm_cp_reg(CPUARMState *env,
+                                         const ARMCPRegInfo *regs)
+{
+    define_one_arm_cp_reg_with_opaque(env, regs, 0);
+}
+const ARMCPRegInfo *get_arm_cp_reginfo(CPUARMState *env, uint32_t encoded_cp);
+
+/* CPWriteFn that can be used to implement writes-ignored behaviour */
+int arm_cp_write_ignore(CPUARMState *env, const ARMCPRegInfo *ri,
+                        uint64_t value);
+/* CPReadFn that can be used for read-as-zero behaviour */
+int arm_cp_read_zero(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value);
+
+static inline int cp_access_ok(CPUARMState *env,
+                               const ARMCPRegInfo *ri, int isread)
+{
+    return (ri->access >> ((arm_current_pl(env) * 2) + isread)) & 1;
+}
+
 /* Does the core conform to the the "MicroController" profile. e.g. Cortex-M3.
    Note the M in older cores (eg. ARM7TDMI) stands for Multiply. These are
    conventional cores (ie. Application or Realtime profile).  */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 101031d..f054fca 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -142,6 +142,89 @@  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     g_slist_free(list);
 }
 
+void define_one_arm_cp_reg_with_opaque(CPUARMState *env,
+                                       const ARMCPRegInfo *r, void *opaque)
+{
+    /* Define implementations of coprocessor registers.
+     * We store these in a hashtable because typically
+     * there are less than 150 registers in a space which
+     * is 16*16*16*8*8 = 262144 in size.
+     * Wildcarding is supported for the crm, opc1 and opc2 fields.
+     * If a register is defined twice then the second definition is
+     * used, so this can be used to define some generic registers and
+     * then override them with implementation specific variations.
+     */
+    int crm, opc1, opc2;
+    int crmmin = (r->crm == CP_ANY) ? 0 : r->crm;
+    int crmmax = (r->crm == CP_ANY) ? 15 : r->crm;
+    int opc1min = (r->opc1 == CP_ANY) ? 0 : r->opc1;
+    int opc1max = (r->opc1 == CP_ANY) ? 7 : r->opc1;
+    int opc2min = (r->opc2 == CP_ANY) ? 0 : r->opc2;
+    int opc2max = (r->opc2 == CP_ANY) ? 7 : r->opc2;
+    /* 64 bit registers have only CRm and Opc1 fields */
+    assert(!((r->type & ARM_CP_64BIT) && (r->opc2 || r->crn)));
+    /* Check that the register definition has enough info to handle
+     * reads and writes if they are permitted.
+     */
+    if (!(r->type & (ARM_CP_SPECIAL|ARM_CP_CONST))) {
+        if (r->access & PL3_R) {
+            assert(r->fieldoffset || r->readfn);
+        }
+        if (r->access & PL3_W) {
+            assert(r->fieldoffset || r->writefn);
+        }
+    }
+    /* Bad type field probably means missing sentinel at end of reg list */
+    assert(cptype_valid(r->type));
+    for (crm = crmmin; crm <= crmmax; crm++) {
+        for (opc1 = opc1min; opc1 <= opc1max; opc1++) {
+            for (opc2 = opc2min; opc2 <= opc2max; opc2++) {
+                uint32_t *key = g_new(uint32_t, 1);
+                ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo));
+                int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
+                *key = ENCODE_CP_REG(r->cp, is64, r->crn, crm, opc1, opc2);
+                r2->opaque = opaque;
+                /* Make sure reginfo passed to helpers for wildcarded regs
+                 * has the correct crm/opc1/opc2 for this reg, not CP_ANY:
+                 */
+                r2->crm = crm;
+                r2->opc1 = opc1;
+                r2->opc2 = opc2;
+                g_hash_table_insert(env->cp_regs, key, r2);
+            }
+        }
+    }
+}
+
+void define_arm_cp_regs_with_opaque(CPUARMState *env,
+                                    const ARMCPRegInfo *regs, void *opaque)
+{
+    /* Define a whole list of registers */
+    const ARMCPRegInfo *r;
+    for (r = regs; r->type != ARM_CP_SENTINEL; r++) {
+        define_one_arm_cp_reg_with_opaque(env, r, opaque);
+    }
+}
+
+const ARMCPRegInfo *get_arm_cp_reginfo(CPUARMState *env, uint32_t encoded_cp)
+{
+    return g_hash_table_lookup(env->cp_regs, &encoded_cp);
+}
+
+int arm_cp_write_ignore(CPUARMState *env, const ARMCPRegInfo *ri,
+                        uint64_t value)
+{
+    /* Helper coprocessor write function for write-ignore registers */
+    return 0;
+}
+
+int arm_cp_read_zero(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value)
+{
+    /* Helper coprocessor write function for read-as-zero registers */
+    *value = 0;
+    return 0;
+}
+
 static int bad_mode_switch(CPUARMState *env, int mode)
 {
     /* Return true if it is not valid for us to switch to
diff --git a/target-arm/helper.h b/target-arm/helper.h
index 16dd5fc..b6cefed 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -65,6 +65,11 @@  DEF_HELPER_2(get_cp15, i32, env, i32)
 DEF_HELPER_3(set_cp, void, env, i32, i32)
 DEF_HELPER_2(get_cp, i32, env, i32)
 
+DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
+DEF_HELPER_2(get_cp_reg, i32, env, ptr)
+DEF_HELPER_3(set_cp_reg64, void, env, ptr, i64)
+DEF_HELPER_2(get_cp_reg64, i64, env, ptr)
+
 DEF_HELPER_2(get_r13_banked, i32, env, i32)
 DEF_HELPER_3(set_r13_banked, void, env, i32, i32)
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index b53369d..490111c 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -23,13 +23,11 @@ 
 #define SIGNBIT (uint32_t)0x80000000
 #define SIGNBIT64 ((uint64_t)1 << 63)
 
-#if !defined(CONFIG_USER_ONLY)
 static void raise_exception(int tt)
 {
     env->exception_index = tt;
     cpu_loop_exit(env);
 }
-#endif
 
 uint32_t HELPER(neon_tbl)(uint32_t ireg, uint32_t def,
                           uint32_t rn, uint32_t maxindex)
@@ -287,6 +285,46 @@  void HELPER(set_user_reg)(uint32_t regno, uint32_t val)
     }
 }
 
+void HELPER(set_cp_reg)(CPUARMState *env, void *rip, uint32_t value)
+{
+    const ARMCPRegInfo *ri = rip;
+    int excp = ri->writefn(env, ri, value);
+    if (excp) {
+        raise_exception(excp);
+    }
+}
+
+uint32_t HELPER(get_cp_reg)(CPUARMState *env, void *rip)
+{
+    const ARMCPRegInfo *ri = rip;
+    uint64_t value;
+    int excp = ri->readfn(env, ri, &value);
+    if (excp) {
+        raise_exception(excp);
+    }
+    return value;
+}
+
+void HELPER(set_cp_reg64)(CPUARMState *env, void *rip, uint64_t value)
+{
+    const ARMCPRegInfo *ri = rip;
+    int excp = ri->writefn(env, ri, value);
+    if (excp) {
+        raise_exception(excp);
+    }
+}
+
+uint64_t HELPER(get_cp_reg64)(CPUARMState *env, void *rip)
+{
+    const ARMCPRegInfo *ri = rip;
+    uint64_t value;
+    int excp = ri->readfn(env, ri, &value);
+    if (excp) {
+        raise_exception(excp);
+    }
+    return value;
+}
+
 /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
    The only way to do that in TCG is a conditional branch, which clobbers
    all our temporaries.  For now implement these as helper functions.  */
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 7a3c7d6..ebb547e 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6479,13 +6479,15 @@  static int disas_cp14_write(CPUARMState * env, DisasContext *s, uint32_t insn)
 
 static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
 {
-    int cpnum;
+    int cpnum, is64, crn, crm, opc1, opc2, isread, rt, rt2;
+    const ARMCPRegInfo *ri;
 
     cpnum = (insn >> 8) & 0xf;
     if (arm_feature(env, ARM_FEATURE_XSCALE)
 	    && ((env->cp15.c15_cpar ^ 0x3fff) & (1 << cpnum)))
 	return 1;
 
+    /* First check for coprocessor space used for actual instructions */
     switch (cpnum) {
       case 0:
       case 1:
@@ -6498,6 +6500,157 @@  static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
     case 10:
     case 11:
 	return disas_vfp_insn (env, s, insn);
+    default:
+        break;
+    }
+
+    /* Otherwise treat as a generic register access */
+    is64 = (insn & (1 << 25)) == 0;
+    if (!is64 && ((insn & (1 << 4)) == 0)) {
+        /* cdp */
+        return 1;
+    }
+
+    crm = insn & 0xf;
+    if (is64) {
+        crn = 0;
+        opc1 = (insn >> 4) & 0xf;
+        opc2 = 0;
+        rt2 = (insn >> 16) & 0xf;
+    } else {
+        crn = (insn >> 16) & 0xf;
+        opc1 = (insn >> 21) & 7;
+        opc2 = (insn >> 5) & 7;
+        rt2 = 0;
+    }
+    isread = (insn >> 20) & 1;
+    rt = (insn >> 12) & 0xf;
+
+    ri = get_arm_cp_reginfo(env,
+                            ENCODE_CP_REG(cpnum, is64, crn, crm, opc1, opc2));
+    if (ri) {
+        /* Check access permissions */
+        if (!cp_access_ok(env, ri, isread)) {
+            return 1;
+        }
+
+        /* Handle special cases first */
+        switch (ri->type) {
+        case ARM_CP_NOP:
+            return 0;
+        case ARM_CP_WFI:
+            if (isread) {
+                return 1;
+            }
+            gen_set_pc_im(s->pc);
+            s->is_jmp = DISAS_WFI;
+            break;
+        default:
+            break;
+        }
+
+        if (isread) {
+            /* Read */
+            if (is64) {
+                TCGv_i64 tmp64;
+                TCGv_i32 tmp;
+                if (ri->type & ARM_CP_CONST) {
+                    tmp64 = tcg_const_i64(ri->resetvalue);
+                } else if (ri->readfn) {
+                    TCGv_ptr tmpptr;
+                    gen_set_pc_im(s->pc);
+                    tmp64 = tcg_temp_new_i64();
+                    tmpptr = tcg_const_ptr(ri);
+                    gen_helper_get_cp_reg64(tmp64, cpu_env, tmpptr);
+                    tcg_temp_free_ptr(tmpptr);
+                } else {
+                    tmp64 = tcg_temp_new_i64();
+                    tcg_gen_ld_i64(tmp64, cpu_env, ri->fieldoffset);
+                }
+                tmp = tcg_temp_new_i32();
+                tcg_gen_trunc_i64_i32(tmp, tmp64);
+                store_reg(s, rt, tmp);
+                tcg_gen_shri_i64(tmp64, tmp64, 32);
+                tcg_gen_trunc_i64_i32(tmp, tmp64);
+                store_reg(s, rt2, tmp);
+            } else {
+                TCGv tmp;
+                if (ri->type & ARM_CP_CONST) {
+                    tmp = tcg_const_i32(ri->resetvalue);
+                } else if (ri->readfn) {
+                    TCGv_ptr tmpptr;
+                    gen_set_pc_im(s->pc);
+                    tmp = tcg_temp_new_i32();
+                    tmpptr = tcg_const_ptr(ri);
+                    gen_helper_get_cp_reg(tmp, cpu_env, tmpptr);
+                    tcg_temp_free_ptr(tmpptr);
+                } else {
+                    tmp = load_cpu_offset(ri->fieldoffset);
+                }
+                if (rt == 15) {
+                    /* Destination register of r15 for 32 bit loads sets
+                     * the condition codes from the high 4 bits of the value
+                     */
+                    gen_set_nzcv(tmp);
+                    tcg_temp_free_i32(tmp);
+                } else {
+                    store_reg(s, rt, tmp);
+                }
+            }
+        } else {
+            /* Write */
+            if (ri->type & ARM_CP_CONST) {
+                /* If not forbidden by access permissions, treat as WI */
+                return 0;
+            }
+
+            if (is64) {
+                TCGv tmplo, tmphi;
+                TCGv_i64 tmp64 = tcg_temp_new_i64();
+                tmplo = load_reg(s, rt);
+                tmphi = load_reg(s, rt2);
+                tcg_gen_concat_i32_i64(tmp64, tmplo, tmphi);
+                tcg_temp_free_i32(tmplo);
+                tcg_temp_free_i32(tmphi);
+                if (ri->writefn) {
+                    TCGv_ptr tmpptr = tcg_const_ptr(ri);
+                    gen_set_pc_im(s->pc);
+                    gen_helper_set_cp_reg64(cpu_env, tmpptr, tmp64);
+                    tcg_temp_free_ptr(tmpptr);
+                } else {
+                    tcg_gen_st_i64(tmp64, cpu_env, ri->fieldoffset);
+                }
+                tcg_temp_free_i64(tmp64);
+            } else {
+                if (ri->writefn) {
+                    TCGv tmp;
+                    TCGv_ptr tmpptr;
+                    gen_set_pc_im(s->pc);
+                    tmp = load_reg(s, rt);
+                    tmpptr = tcg_const_ptr(ri);
+                    gen_helper_set_cp_reg(cpu_env, tmpptr, tmp);
+                    tcg_temp_free_ptr(tmpptr);
+                    tcg_temp_free_i32(tmp);
+                } else {
+                    TCGv tmp = load_reg(s, rt);
+                    store_cpu_offset(tmp, ri->fieldoffset);
+                }
+            }
+            /* We default to ending the TB on a coprocessor register write,
+             * but allow this to be suppressed by the register definition
+             * (usually only necessary to work around guest bugs).
+             */
+            if (!(ri->type & ARM_CP_SUPPRESS_TB_END)) {
+                gen_lookup_tb(s);
+            }
+        }
+        return 0;
+    }
+
+    /* Fallback code: handle coprocessor registers not yet converted
+     * to ARMCPRegInfo.
+     */
+    switch (cpnum) {
     case 14:
         /* Coprocessors 7-15 are architecturally reserved by ARM.
            Unfortunately Intel decided to ignore this.  */