From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46210) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvXvS-0007JG-6w for qemu-devel@nongnu.org; Fri, 13 Jun 2014 16:18:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvXvK-0001s0-6c for qemu-devel@nongnu.org; Fri, 13 Jun 2014 16:18:14 -0400 Received: from mail-qa0-f44.google.com ([209.85.216.44]:36362) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvXvK-0001rp-1w for qemu-devel@nongnu.org; Fri, 13 Jun 2014 16:18:06 -0400 Received: by mail-qa0-f44.google.com with SMTP id hw13so3176597qab.3 for ; Fri, 13 Jun 2014 13:18:05 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1402444514-19658-21-git-send-email-aggelerf@ethz.ch> References: <1402444514-19658-1-git-send-email-aggelerf@ethz.ch> <1402444514-19658-21-git-send-email-aggelerf@ethz.ch> Date: Fri, 13 Jun 2014 15:18:05 -0500 Message-ID: From: Greg Bellows Content-Type: multipart/alternative; boundary=047d7bdc82c20b9b5f04fbbd6087 Subject: Re: [Qemu-devel] [PATCH v3 20/32] target-arm: arrayfying fieldoffset for banking List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabian Aggeler Cc: Peter Maydell , Peter Crosthwaite , QEMU Developers , Sergey Fedorov , "Edgar E. Iglesias" , Christoffer Dall --047d7bdc82c20b9b5f04fbbd6087 Content-Type: text/plain; charset=UTF-8 On 10 June 2014 18:55, Fabian Aggeler wrote: > Prepare ARMCPRegInfo to support specifying two fieldoffsets per > register definition. This will allow us to keep one register > definition for banked registers (different offsets for secure/ > non-secure world). > > Signed-off-by: Fabian Aggeler > --- > target-arm/cpu.h | 16 +++++++++++++--- > target-arm/helper.c | 35 +++++++++++++++++++++++------------ > 2 files changed, 36 insertions(+), 15 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index d4eab39..7d7782e 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -1086,12 +1086,22 @@ struct ARMCPRegInfo { > * 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: > + /* Offsets of the fields (secure/non-secure) in CPUARMState for this > + * register. The array will be accessed by the ns bit which means the > + * secure instance has to be at [0] while the non-secure instance > must be > + * at [1]. If a register is not banked .fieldoffset can be used, > which maps > + * to the non-secure bank. > + * 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) */ > + union { /* offsetof(CPUARMState, field) */ > + struct { > + ptrdiff_t fieldoffset_padding; > + ptrdiff_t fieldoffset; > + }; > + ptrdiff_t bank_fieldoffsets[2]; > + }; > /* Function for making any access checks for this register in > addition to > * those specified by the 'access' permissions bits. If NULL, no extra > * checks required. The access check is performed at runtime, not at > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 610245d..dfaf636 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -2891,20 +2891,31 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, > const ARMCPRegInfo *r, > uint32_t *key = g_new(uint32_t, 1); > ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo)); > int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0; > - if (r->state == ARM_CP_STATE_BOTH && state == ARM_CP_STATE_AA32) { > - /* The AArch32 view of a shared register sees the lower 32 bits > - * of a 64 bit backing field. It is not migratable as the AArch64 > - * view handles that. AArch64 also handles reset. > - * We assume it is a cp15 register. > - */ > - r2->cp = 15; > - r2->type |= ARM_CP_NO_MIGRATE; > - r2->resetfn = arm_cp_reset_ignore; > -#ifdef HOST_WORDS_BIGENDIAN > - if (r2->fieldoffset) { > - r2->fieldoffset += sizeof(uint32_t); > + > + if (state == ARM_CP_STATE_AA32) { > + if (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) { > + /* Register is banked (using both entries in array). > + * Overwriting fieldoffset as the array was only used to > define > + * banked registers but later only fieldoffset is used. > + */ > + r2->fieldoffset = r->bank_fieldoffsets[nsbit]; > This is somewhat convoluted as sometimes we will be overwriting one of the bank offsets and other times not. Would it be better to not unionize the fieldoffset and the bank offsets and allow this update to be made while keeping the incoming bank values? > } > + > + if (r->state == ARM_CP_STATE_BOTH) { > + /* The AArch32 view of a shared register sees the lower 32 > bits > + * of a 64 bit backing field. It is not migratable as the > AArch64 > + * view handles that. AArch64 also handles reset. > + * We assume it is a cp15 register. > + */ > + r2->cp = 15; > + r2->type |= ARM_CP_NO_MIGRATE; > + r2->resetfn = arm_cp_reset_ignore; > +#ifdef HOST_WORDS_BIGENDIAN > + if (r2->fieldoffset) { > + r2->fieldoffset += sizeof(uint32_t); > + } > #endif > + } > } > if (state == ARM_CP_STATE_AA64) { > /* To allow abbreviation of ARMCPRegInfo > -- > 1.8.3.2 > > --047d7bdc82c20b9b5f04fbbd6087 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable



On 10 June 2014 18:55, Fabian Aggeler <aggelerf@ethz.ch> wrote:
Prepare ARMCPRegInfo to support specifying t= wo fieldoffsets per
register definition. This will allow us to keep one register
definition for banked registers (different offsets for secure/
non-secure world).

Signed-off-by: Fabian Aggeler <aggel= erf@ethz.ch>
---
=C2=A0target-arm/cpu.h =C2=A0 =C2=A0| 16 +++++++++++++---
=C2=A0target-arm/helper.c | 35 +++++++++++++++++++++++------------
=C2=A02 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index d4eab39..7d7782e 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1086,12 +1086,22 @@ struct ARMCPRegInfo {
=C2=A0 =C2=A0 =C2=A0 * fieldoffset is non-zero, the reset value of the regi= ster.
=C2=A0 =C2=A0 =C2=A0 */
=C2=A0 =C2=A0 =C2=A0uint64_t resetvalue;
- =C2=A0 =C2=A0/* Offset of the field in CPUARMState for this register. Thi= s is not
- =C2=A0 =C2=A0 * needed if either:
+ =C2=A0 =C2=A0/* Offsets of the fields (secure/non-secure) in CPUARMState = for this
+ =C2=A0 =C2=A0 * register. The array will be accessed by the ns bit which = means the
+ =C2=A0 =C2=A0 * secure instance has to be at [0] while the non-secure ins= tance must be
+ =C2=A0 =C2=A0 * at [1]. If a register is not banked .fieldoffset can be u= sed, which maps
+ =C2=A0 =C2=A0 * to the non-secure bank.
+ =C2=A0 =C2=A0 * This is not needed if either:
=C2=A0 =C2=A0 =C2=A0 * =C2=A01. type is ARM_CP_CONST or one of the ARM_CP_S= PECIALs
=C2=A0 =C2=A0 =C2=A0 * =C2=A02. both readfn and writefn are specified
=C2=A0 =C2=A0 =C2=A0 */
- =C2=A0 =C2=A0ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) */ + =C2=A0 =C2=A0union { /* offsetof(CPUARMState, field) */
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0struct {
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ptrdiff_t fieldoffset_padding; + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ptrdiff_t fieldoffset;
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0};
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0ptrdiff_t bank_fieldoffsets[2];
+ =C2=A0 =C2=A0};
=C2=A0 =C2=A0 =C2=A0/* Function for making any access checks for this regis= ter in addition to
=C2=A0 =C2=A0 =C2=A0 * those specified by the 'access' permissions = bits. If NULL, no extra
=C2=A0 =C2=A0 =C2=A0 * checks required. The access check is performed at ru= ntime, not at
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 610245d..dfaf636 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2891,20 +2891,31 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, con= st ARMCPRegInfo *r,
=C2=A0 =C2=A0 =C2=A0uint32_t *key =3D g_new(uint32_t, 1);
=C2=A0 =C2=A0 =C2=A0ARMCPRegInfo *r2 =3D g_memdup(r, sizeof(ARMCPRegInfo));=
=C2=A0 =C2=A0 =C2=A0int is64 =3D (r->type & ARM_CP_64BIT) ? 1 : 0; - =C2=A0 =C2=A0if (r->state =3D=3D ARM_CP_STATE_BOTH && state = =3D=3D ARM_CP_STATE_AA32) {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0/* The AArch32 view of a shared register sees = the lower 32 bits
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 * of a 64 bit backing field. It is not migrat= able as the AArch64
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 * view handles that. AArch64 also handles res= et.
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 * We assume it is a cp15 register.
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
- =C2=A0 =C2=A0 =C2=A0 =C2=A0r2->cp =3D 15;
- =C2=A0 =C2=A0 =C2=A0 =C2=A0r2->type |=3D ARM_CP_NO_MIGRATE;
- =C2=A0 =C2=A0 =C2=A0 =C2=A0r2->resetfn =3D arm_cp_reset_ignore;
-#ifdef HOST_WORDS_BIGENDIAN
- =C2=A0 =C2=A0 =C2=A0 =C2=A0if (r2->fieldoffset) {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r2->fieldoffset +=3D sizeof(u= int32_t);
+
+ =C2=A0 =C2=A0if (state =3D=3D ARM_CP_STATE_AA32) {
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0if (r->bank_fieldoffsets[0] && r-&g= t;bank_fieldoffsets[1]) {
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Register is banked (using bot= h entries in array).
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * Overwriting fieldoffset as th= e array was only used to define
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * banked registers but later on= ly fieldoffset is used.
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r2->fieldoffset =3D r->ban= k_fieldoffsets[nsbit];

This is somewhat= convoluted as sometimes we will be overwriting one of the bank offsets and= other times not. =C2=A0Would it be better to not unionize the fieldoffset = and the bank offsets and allow this update to be made while keeping the inc= oming bank values?
=C2=A0
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0if (r->state =3D=3D ARM_CP_STATE_BOTH) { + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* The AArch32 view of a shared = register sees the lower 32 bits
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * of a 64 bit backing field. It= is not migratable as the AArch64
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * view handles that. AArch64 al= so handles reset.
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * We assume it is a cp15 regist= er.
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r2->cp =3D 15;
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r2->type |=3D ARM_CP_NO_MIGRA= TE;
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r2->resetfn =3D arm_cp_reset_= ignore;
+#ifdef HOST_WORDS_BIGENDIAN
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (r2->fieldoffset) {
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r2->fieldoffset= +=3D sizeof(uint32_t);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
=C2=A0#endif
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0 =C2=A0if (state =3D=3D ARM_CP_STATE_AA64) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* To allow abbreviation of ARMCPRegInfo<= br> --
1.8.3.2


--047d7bdc82c20b9b5f04fbbd6087--