qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 14/23] target-arm: add banked coprocessor reg


From: Greg Bellows
Subject: Re: [Qemu-devel] [PATCH v2 14/23] target-arm: add banked coprocessor register type and macros
Date: Wed, 14 May 2014 11:42:00 -0500




On 13 May 2014 11:15, Fabian Aggeler <address@hidden> wrote:
Banked CP registers can be defined with a A32_BANKED_REG macro which defines
a non-secure instance of the register followed by an adjacent secure instance.
Using a union makes the code backwards-compatible since the non-secure
instance can normally be accessed by its existing name.

This comment indicates that the 0th entry or the default name is the non-secure bank, which differs from the code below.
 

When translating a banked CP register access instruction in monitor mode,
the SCR.NS bit determines which instance is going to be accessed.

If EL3 is operating in Aarch64 state coprocessor registers are not
banked anymore but in some cases have its own _EL3 instance.

Signed-off-by: Sergey Fedorov <address@hidden>
Signed-off-by: Fabian Aggeler <address@hidden>
---
 target-arm/cpu.h       | 121 +++++++++++++++++++++++++++++++++++++++++++++----
 target-arm/helper.c    |  64 ++++++++++++++++++++++++--
 target-arm/translate.c |  19 +++++---
 3 files changed, 184 insertions(+), 20 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index a970d55..9e325ac 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -80,6 +80,16 @@
 #define offsetofhigh32(S, M) (offsetof(S, M) + sizeof(uint32_t))
 #endif

+/* Define a banked coprocessor register state field. Use %name as the
+ * register field name for the secure instance. The non-secure instance
+ * has a "_nonsecure" suffix.

Where is the "_nonsecure" suffix?

The above comment appears to be incorrect as the code assumes that the 0th entry as the non-secure bank.

+ */
+#define A32_BANKED_REG(type, name) \
+    union { \
+        type name; \
+        type name##_banked[2]; \
+    }
 /* Meanings of the ARMCPU object's two inbound GPIO lines */
 #define ARM_CPU_IRQ 0
 #define ARM_CPU_FIQ 1
@@ -89,6 +99,7 @@ typedef void ARMWriteCPFunc(void *opaque, int cp_info,
 typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
                                int dstreg, int operand);

+
 struct arm_boot_info;

 #define NB_MMU_MODES 5
@@ -673,6 +684,78 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el)
     return arm_feature(env, ARM_FEATURE_AARCH64);
 }

+/* When EL3 is operating in Aarch32 state, the NS-bit determines
+ * whether the secure instance of a cp-register should be used. */
+#define USE_SECURE_REG(env) ( \
+                        arm_feature(env, ARM_FEATURE_SECURITY_EXTENSIONS) && \
+                        !arm_el_is_aa64(env, 3) && \
+                        !((env)->cp15.c1_scr & 1/*NS*/))
+
+#define NONSECURE_BANK 0
+#define SECURE_BANK 1
+
+#define A32_BANKED_REG_GET(env, regname) \
+    ((USE_SECURE_REG(env)) ? \
+            (env)->cp15.regname##_banked[SECURE_BANK] : \
+            (env)->cp15.regname)
+
+#define A32_MAPPED_EL3_REG_GET(env, regname) \
+    (((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
+      (USE_SECURE_REG(env))) ? \
+            (env)->cp15.regname##_el3 : \
+            (env)->cp15.regname##_el1)
+
+#define A32_BANKED_REG_SET(env, regname, val) \
+        do { \
+            if (USE_SECURE_REG(env)) { \
+                (env)->cp15.regname##_banked[SECURE_BANK] = (val); \
+            } else { \
+                (env)->cp15.regname = (val); \
+            } \
+        } while (0)
+
+#define A32_MAPPED_EL3_REG_SET(env, regname, val) \
+        do { \
+            if ((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
+                    (USE_SECURE_REG(env))) { \
+                (env)->cp15.regname##_el3 = (val); \
+            } else { \
+                (env)->cp15.regname##_el1 = (val); \
+            } \
+        } while (0)
+
+
+#define A32_BANKED_CURRENT_REG_GET(env, regname) \
+    ((!arm_el_is_aa64(env, 3) && arm_is_secure(env)) ? \
+            (env)->cp15.regname##_banked[SECURE_BANK] : \
+            (env)->cp15.regname)
+
+#define A32_MAPPED_EL3_CURRENT_REG_GET(env, regname) \
+    (((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
+      (!arm_el_is_aa64(env, 3) && arm_is_secure(env))) ? \
+            (env)->cp15.regname##_el3 : \
+            (env)->cp15.regname##_el1)
+
+#define A32_BANKED_CURRENT_REG_SET(env, regname, val) \
+        do { \
+            if (!arm_el_is_aa64(env, 3) && arm_is_secure(env)) { \
+                (env)->cp15.regname##_banked[SECURE_BANK] = (val); \
+            } else { \
+                (env)->cp15.regname = (val); \
+            } \
+        } while (0)
+
+#define A32_MAPPED_EL3_CURRENT_REG_SET(env, regname, val) \
+        do { \
+            if ((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
+                    (!arm_el_is_aa64(env, 3) && arm_is_secure(env))) { \
+                (env)->cp15.regname##_el3 = (val); \
+            } else { \
+                (env)->cp15.regname##_el1 = (val); \
+            } \
+        } while (0)
+
+
 void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);

 /* Interface between CPU and Interrupt controller.  */
@@ -691,6 +774,7 @@ void armv7m_nvic_complete_irq(void *opaque, int irq);
  *  Crn, Crm, opc1, opc2 fields
  *  32 or 64 bit register (ie is it accessed via MRC/MCR
  *    or via MRRC/MCRR?)
+ *  nonsecure/secure bank (Aarch32 only)
  * We allow 4 bits for opc1 because MRRC/MCRR have a 4 bit field.
  * (In this case crn and opc2 should be zero.)
  * For AArch64, there is no 32/64 bit size distinction;
@@ -708,9 +792,16 @@ void armv7m_nvic_complete_irq(void *opaque, int irq);
 #define CP_REG_AA64_SHIFT 28
 #define CP_REG_AA64_MASK (1 << CP_REG_AA64_SHIFT)

-#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2)   \
-    (((cp) << 16) | ((is64) << 15) | ((crn) << 11) |    \
-     ((crm) << 7) | ((opc1) << 3) | (opc2))
+/* To enable banking of coprocessor registers depending on ns-bit we
+ * add a bit to distinguish between secure and non-secure cpregs in the
+ * hashtable.
+ */
+#define CP_REG_NS_SHIFT 27
+#define CP_REG_NS_MASK(nsbit) (nsbit << CP_REG_NS_SHIFT)
+
+#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2, ns)   \
+    (CP_REG_NS_MASK(ns) | ((cp) << 16) | ((is64) << 15) |   \
+     ((crn) << 11) | ((crm) << 7) | ((opc1) << 3) | (opc2))

 #define ENCODE_AA64_CP_REG(cp, crn, crm, op0, op1, op2) \
     (CP_REG_AA64_MASK |                                 \
@@ -771,6 +862,14 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
  * IO indicates that this register does I/O and therefore its accesses
  * need to be surrounded by gen_io_start()/gen_io_end(). In particular,
  * registers which implement clocks or timers require this.
+ * In an implementation with Security Extensions supporting Aarch32 cp regs can
+ * be banked or common. If a register is common it references the same variable
+ * from both worlds (non-secure and secure). For cp regs which neither set
+ * ARM_CP_SECURE nor ARM_CP_NONSECURE (default) we assume it's common and it
+ * will be inserted twice into the hashtable. If a register has
+ * ARM_CP_BANKED/ARM_CP_BANKED_64BIT set, it will be inserted twice but with
+ * different offset respectively. This way Aarch32 registers which can be
+ * mapped to Aarch64 PL3 registers can be inserted individually.
  */
 #define ARM_CP_SPECIAL 1
 #define ARM_CP_CONST 2
@@ -779,16 +878,20 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
 #define ARM_CP_OVERRIDE 16
 #define ARM_CP_NO_MIGRATE 32
 #define ARM_CP_IO 64
-#define ARM_CP_NOP (ARM_CP_SPECIAL | (1 << 8))
-#define ARM_CP_WFI (ARM_CP_SPECIAL | (2 << 8))
-#define ARM_CP_NZCV (ARM_CP_SPECIAL | (3 << 8))
-#define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8))
-#define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8))
+#define ARM_CP_SECURE (1 << 7)
+#define ARM_CP_NONSECURE (1 << 8)
+#define ARM_CP_BANKED (ARM_CP_NONSECURE | ARM_CP_SECURE)
+#define ARM_CP_BANKED_64BIT ((1 << 9) | ARM_CP_BANKED)
+#define ARM_CP_NOP (ARM_CP_SPECIAL | (1 << 10))
+#define ARM_CP_WFI (ARM_CP_SPECIAL | (2 << 10))
+#define ARM_CP_NZCV (ARM_CP_SPECIAL | (3 << 10))
+#define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 10))
+#define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 10))
 #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
 /* 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 0x7f
+#define ARM_CP_FLAG_MASK 0x3ff

 /* Valid values for ARMCPRegInfo state field, indicating which of
  * the AArch32 and AArch64 execution states this register is visible in.
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 9326ef8..98c3dc9 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2703,7 +2703,7 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)

 static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
                                    void *opaque, int state,
-                                   int crm, int opc1, int opc2)
+                                   int crm, int opc1, int opc2, int nsbit)
 {
     /* Private utility function for define_one_arm_cp_reg_with_opaque():
      * add a single reginfo struct to the hash table.
@@ -2726,6 +2726,34 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
         }
 #endif
     }
+
+    if ((r->type & ARM_CP_BANKED) == ARM_CP_BANKED && !nsbit) {
+        if (r2->fieldoffset) {
+            /* We simplify register definitions by providing a type
+             * ARM_CP_BANKED, for which the fieldoffset of the secure instance
+             * will be increased to point at the second entry of the array.
+             *
+             * We cannot use is64 or the type ARM_CP_STATE_BOTH to know how
+             * wide the banked register is because some registers are 64bit
+             * wide but the register is not defined as 64bit because it is
+             * mapped to the lower 32 bit.
+             * Therefore two separate types for 64bit banked registers and
+             * 32bit registers are used (ARM_CP_BANKED/ARM_CP_BANKED_64BIT).
+             */
+            r2->fieldoffset +=
+                    ((r->type & ARM_CP_BANKED_64BIT) == ARM_CP_BANKED_64BIT) ?
+                            sizeof(uint64_t) : sizeof(uint32_t);

Do we want the register info descriptors of banked registers to point to the same storage if the security extension is not enabled?
 
+        }
+    }
+    /* For A32 we want to be able to know whether the secure or non-secure
+     * instance wants to be accessed. A64 does not know this banking scheme
+     * anymore, but it might use the same readfn/writefn as A32 which might
+     * rely on it (e.g. in the case of ARM_CP_STATE_BOTH).
+     * Reset the type according to ns-bit passed as argument.
+     */
+    r2->type &= ~(ARM_CP_NONSECURE | ARM_CP_SECURE);
+    r2->type |= nsbit ? ARM_CP_NONSECURE : ARM_CP_SECURE;
+
     if (state == ARM_CP_STATE_AA64) {
         /* To allow abbreviation of ARMCPRegInfo
          * definitions, we treat cp == 0 as equivalent to
@@ -2737,7 +2765,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
         *key = ENCODE_AA64_CP_REG(r2->cp, r2->crn, crm,
                                   r2->opc0, opc1, opc2);
     } else {
-        *key = ENCODE_CP_REG(r2->cp, is64, r2->crn, crm, opc1, opc2);
+        *key = ENCODE_CP_REG(r2->cp, is64, r2->crn, crm, opc1, opc2, nsbit);
     }
     if (opaque) {
         r2->opaque = opaque;
@@ -2773,9 +2801,10 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
         oldreg = g_hash_table_lookup(cpu->cp_regs, key);
         if (oldreg && !(oldreg->type & ARM_CP_OVERRIDE)) {
             fprintf(stderr, "Register redefined: cp=%d %d bit "
-                    "crn=%d crm=%d opc1=%d opc2=%d, "
+                    "crn=%d crm=%d opc1=%d opc2=%d ns=%d, "
                     "was %s, now %s\n", r2->cp, 32 + 32 * is64,
                     r2->crn, r2->crm, r2->opc1, r2->opc2,
+                    (r2->type & ARM_CP_NONSECURE),
                     oldreg->name, r2->name);
             g_assert_not_reached();
         }
@@ -2886,8 +2915,33 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
                     if (r->state != state && r->state != ARM_CP_STATE_BOTH) {
                         continue;
                     }
-                    add_cpreg_to_hashtable(cpu, r, opaque, state,
-                                           crm, opc1, opc2);
+
+                    if (state == ARM_CP_STATE_AA32) {
+                        if ((r->type & ARM_CP_BANKED) == ARM_CP_BANKED ||
+                                (r->type & ARM_CP_BANKED) == 0) {
+                            /* Under Aarch32 CP registers can be common
+                             * (same for secure and non-secure world) or banked.
+                             * Register definitions with neither secure nor
+                             * non-secure type set (common) or with both bits
+                             * set (banked) will be inserted twice into the
+                             * hashtable.
+                             */
+                            add_cpreg_to_hashtable(cpu, r, opaque, state,
+                                    crm, opc1, opc2, 0);
+                            add_cpreg_to_hashtable(cpu, r, opaque, state,
+                                    crm, opc1, opc2, 1);
+                        } else {
+                            /* Only one of both bank types were specified */
+                            add_cpreg_to_hashtable(cpu, r, opaque, state,
+                                    crm, opc1, opc2,
+                                    (r->type & ARM_CP_NONSECURE) ? 1 : 0);
+                        }
+                    } else {
+                        /* Aarch64 registers get mapped to non-secure instance
+                         * of Aarch32 */
+                        add_cpreg_to_hashtable(cpu, r, opaque, state,
+                                crm, opc1, opc2, 1);
+                    }
                 }
             }
         }
diff --git a/target-arm/translate.c b/target-arm/translate.c
index bbd4c77..3a429ac 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6866,7 +6866,7 @@ static int disas_neon_data_insn(CPUARMState * env, DisasContext *s, uint32_t ins

 static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
 {
-    int cpnum, is64, crn, crm, opc1, opc2, isread, rt, rt2;
+    int cpnum, is64, crn, crm, opc1, opc2, isread, rt, rt2, ns;
     const ARMCPRegInfo *ri;

     cpnum = (insn >> 8) & 0xf;
@@ -6937,8 +6937,11 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
     isread = (insn >> 20) & 1;
     rt = (insn >> 12) & 0xf;

+    /* Monitor mode is always treated as secure but cp register reads/writes
+     * can access secure and non-secure instances using SCR.NS bit*/
+    ns = IS_NS(s) ? 1 : !USE_SECURE_REG(env);

While monitor mode is always considered secure, which system register accessed is still based on the NS bit, so unless I am missing something, shouldn't the ns setting be purely based on USE_SECURE_REG?  

Also, doesn't IS_NS() simply indicate the the TB was generated for secure state and not necessarily monitor mode?  Plus, shouldn't this code still be allowed to access the non-secure bank?
 
     ri = get_arm_cp_reginfo(s->cp_regs,
-                            ENCODE_CP_REG(cpnum, is64, crn, crm, opc1, opc2));
+            ENCODE_CP_REG(cpnum, is64, crn, crm, opc1, opc2, ns));
     if (ri) {
         /* Check access permissions */
         if (!cp_access_ok(s->current_pl, ri, isread)) {
@@ -7125,12 +7128,16 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
      */
     if (is64) {
         qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
-                      "64 bit system register cp:%d opc1: %d crm:%d\n",
-                      isread ? "read" : "write", cpnum, opc1, crm);
+                      "64 bit system register cp:%d opc1: %d crm:%d "
+                      "(%s)\n",
+                      isread ? "read" : "write", cpnum, opc1, crm,
+                      ns ? "non-secure" : "secure");
     } else {
         qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
-                      "system register cp:%d opc1:%d crn:%d crm:%d opc2:%d\n",
-                      isread ? "read" : "write", cpnum, opc1, crn, crm, opc2);
+                      "system register cp:%d opc1:%d crn:%d crm:%d opc2:%d "
+                      "(%s)\n",
+                      isread ? "read" : "write", cpnum, opc1, crn, crm, opc2,
+                      ns ? "non-secure" : "secure");
     }

     return 1;
--
1.8.3.2




reply via email to

[Prev in Thread] Current Thread [Next in Thread]