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

[Greg Bellows]

On 9 December 2014 at 13:46, 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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d1b856c..99a092c 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,18 @@ static void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 }

+static bool raw_accessors_valid(const ARMCPRegInfo *ri)
+{
+    /* Return true if a raw access on this register is OK (ie will not
+     * fall into the assert in raw_read() or raw_write())
+     */

I believe this comment is somewhat misleading as there are registers that would return true from this function yet still hit the aforementioned asserts.

 

+    if (ri->type & ARM_CP_CONST) {
+        return true;
+    }

I realize CONST registers would not likely go through the raw functions but these too make the above comment incorrect.

 

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

+}
+

Unless we are going to use this function elsewhere, wouldn't it be better to just inline the code?  The name is otherwise misleading and the comment may cause this to be incorrectly used elsewhere in the future.  Maybe instead just update the call location with something like:

if (!(r2->type & (ARM_CP_NO_RAW | ARM_CP_CONST)) {

    assert((ri->raw_writefn || ri->writefn || ri->fieldoffset) &&

           (ri->raw_readfn || ri->readfn || ri->fieldoffset);

}

 

 bool write_cpustate_to_list(ARMCPU *cpu)
 {
     /* Write the coprocessor state from cpu->env to the (index,value) list. */
@@ -3516,6 +3530,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_valid(r2));
+    }
+
     /* Overriding of an existing definition must be explicitly
      * requested.
      */
--
1.9.1