Re: [Qemu-devel] [PATCH 2/2] target-arm: Add checks that cpreg raw accesses are handled

[Greg Bellows]

On Mon, Jan 19, 2015 at 9:17 AM, Peter Maydell <address@hidden> wrote:

Add assertion checking when cpreg structures are registered that they
either forbid raw-access attempts or at least make an attempt at
handling them. Also add an assert in the raw-accessor-of-last-resort,
to avoid silently doing a read or write from offset zero, which is
actually AArch32 CPU register r0.

Signed-off-by: Peter Maydell <address@hidden>
---
 target-arm/helper.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 18f04b2..39c0ad9 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -119,6 +119,7 @@ static int aarch64_fpu_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)

 static uint64_t raw_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
+    assert(ri->fieldoffset);
     if (cpreg_field_is_64bit(ri)) {
         return CPREG_FIELD64(env, ri);
     } else {
@@ -129,6 +130,7 @@ static uint64_t raw_read(CPUARMState *env, const ARMCPRegInfo *ri)
 static void raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
                       uint64_t value)
 {
+    assert(ri->fieldoffset);
     if (cpreg_field_is_64bit(ri)) {
         CPREG_FIELD64(env, ri) = value;
     } else {
@@ -174,6 +176,26 @@ static void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 }

+static bool raw_accessors_invalid(const ARMCPRegInfo *ri)
+{
+   /* Return true if the regdef would cause an assertion if you called
+    *

​​

read_raw_cp_reg() or write_raw_cp_reg() on it (ie if it is a
+    * program bug for it not to have the NO_RAW flag).
+    * NB that returning false here doesn't necessarily mean that calling
+    * read/write_raw_cp_reg() is safe, because we can't distinguish "has
+    * read/write access functions which are safe for raw use" from "has
+    * read/write access functions which have side effects but has forgotten
+    * to provide raw access functions".
+    * The tests here line up with the conditions in read/write_raw_cp_reg()
+    * and assertions in raw_read()/raw_write().
+    */
+    if (ri->type & ARM_CP_CONST) {
+        return true;
+    }

​Had to refresh my memory on this.  It appears we changed the name (polarity) of the function based on our previous discussion.  However, according to the above description, we should return 'true' if read/write would cause an assertion. but in the case of CONST we would not assert, but still return 'true'?

 

+    return (ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
+        (ri->raw_readfn || ri->readfn || ri->fieldoffset);

​This case appears to need inverting as it will resolve to 'true' but should be valid.

NIT: It would be cleaner to pull the ri->fieldoffset check above this check to simplify the conditional.

 

+}
+
 bool write_cpustate_to_list(ARMCPU *cpu)
 {
     /* Write the coprocessor state from cpu->env to the (index,value) list. */
@@ -3509,6 +3531,14 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
         r2->type |= ARM_CP_ALIAS;
     }

+    /* Check that raw accesses are either forbidden or handled. Note that
+     * we can't assert this earlier because the setup of fieldoffset for
+     * banked registers has to be done first.
+     */
+    if (!(r2->type & ARM_CP_NO_RAW)) {
+        assert(!raw_accessors_invalid(r2));
+    }
+
     /* Overriding of an existing definition must be explicitly
      * requested.
      */
--
1.9.1