From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55826) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEf6b-0003f0-Tm for qemu-devel@nongnu.org; Mon, 13 Jul 2015 10:53:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEf6X-00087l-ID for qemu-devel@nongnu.org; Mon, 13 Jul 2015 10:53:17 -0400 Received: from mail-pa0-x22e.google.com ([2607:f8b0:400e:c03::22e]:34252) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEf6X-00086k-6G for qemu-devel@nongnu.org; Mon, 13 Jul 2015 10:53:13 -0400 Received: by pacan13 with SMTP id an13so17150463pac.1 for ; Mon, 13 Jul 2015 07:53:12 -0700 (PDT) Date: Tue, 14 Jul 2015 00:53:05 +1000 From: "Edgar E. Iglesias" Message-ID: <20150713145305.GC4658@toto> References: <1436797559-20835-1-git-send-email-peter.maydell@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436797559-20835-1-git-send-email-peter.maydell@linaro.org> Subject: Re: [Qemu-devel] [PATCH] target-arm: Add debug check for mismatched cpreg resets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "Edgar E. Iglesias" , qemu-devel@nongnu.org, patches@linaro.org On Mon, Jul 13, 2015 at 03:25:59PM +0100, Peter Maydell wrote: > It's easy to accidentally define two cpregs which both try > to reset the same underlying state field (for instance a > clash between an AArch64 EL3 definition and an AArch32 > banked register definition). if the two definitions disagree > about the reset value then the result is dependent on which > one happened to be reached last in the hashtable enumeration. > > Add a consistency check to detect and assert in these cases: > after reset, we run a second pass where we check that the > reset operation doesn't change the value of the register. > > Signed-off-by: Peter Maydell Good idea! Reviewed-by: Edgar E. Iglesias Cheers, Edgar > --- > This does correctly flag up the SCTLR_EL3 mismatch I've just posted > a patch for, and doesn't seem to complain about anything else. > However it seems prudent to not put this into 2.4... > > target-arm/cpu.c | 23 +++++++++++++++++++++++ > target-arm/cpu.h | 3 +++ > target-arm/helper.c | 2 +- > 3 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 8b4323d..9fb08ab 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -79,6 +79,27 @@ static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque) > } > } > > +static void cp_reg_check_reset(gpointer key, gpointer value, gpointer opaque) > +{ > + /* Purely an assertion check: we've already done reset once, > + * so now check that running the reset for the cpreg doesn't > + * change its value. This traps bugs where two different cpregs > + * both try to reset the same state field but to different values. > + */ > + ARMCPRegInfo *ri = value; > + ARMCPU *cpu = opaque; > + uint64_t oldvalue, newvalue; > + > + if (ri->type & (ARM_CP_SPECIAL | ARM_CP_ALIAS | ARM_CP_NO_RAW)) { > + return; > + } > + > + oldvalue = read_raw_cp_reg(&cpu->env, ri); > + cp_reg_reset(key, value, opaque); > + newvalue = read_raw_cp_reg(&cpu->env, ri); > + assert(oldvalue == newvalue); > +} > + > /* CPUClass::reset() */ > static void arm_cpu_reset(CPUState *s) > { > @@ -90,6 +111,8 @@ static void arm_cpu_reset(CPUState *s) > > memset(env, 0, offsetof(CPUARMState, features)); > g_hash_table_foreach(cpu->cp_regs, cp_reg_reset, cpu); > + g_hash_table_foreach(cpu->cp_regs, cp_reg_check_reset, cpu); > + > env->vfp.xregs[ARM_VFP_FPSID] = cpu->reset_fpsid; > env->vfp.xregs[ARM_VFP_MVFR0] = cpu->mvfr0; > env->vfp.xregs[ARM_VFP_MVFR1] = cpu->mvfr1; > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 7e89152..76a0a97 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -1445,6 +1445,9 @@ static inline bool cp_access_ok(int current_el, > return (ri->access >> ((current_el * 2) + isread)) & 1; > } > > +/* Raw read of a coprocessor register (as needed for migration, etc) */ > +uint64_t read_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri); > + > /** > * write_list_to_cpustate > * @cpu: ARMCPU > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 01f0d0d..fc2f61a 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -144,7 +144,7 @@ static void *raw_ptr(CPUARMState *env, const ARMCPRegInfo *ri) > return (char *)env + ri->fieldoffset; > } > > -static uint64_t read_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri) > +uint64_t read_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri) > { > /* Raw read of a coprocessor register (as needed for migration, etc). */ > if (ri->type & ARM_CP_CONST) { > -- > 1.9.1 > >