From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57827) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlmSN-0003pT-I4 for qemu-devel@nongnu.org; Tue, 04 Nov 2014 17:20:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XlmSJ-0000sn-1J for qemu-devel@nongnu.org; Tue, 04 Nov 2014 17:20:07 -0500 Received: from mail-qa0-f45.google.com ([209.85.216.45]:39970) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlmSI-0000pu-Qj for qemu-devel@nongnu.org; Tue, 04 Nov 2014 17:20:02 -0500 Received: by mail-qa0-f45.google.com with SMTP id dc16so10479894qab.4 for ; Tue, 04 Nov 2014 14:20:02 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1414704538-17103-1-git-send-email-greg.bellows@linaro.org> <1414704538-17103-8-git-send-email-greg.bellows@linaro.org> Date: Tue, 4 Nov 2014 16:20:02 -0600 Message-ID: From: Greg Bellows Content-Type: multipart/alternative; boundary=001a11c350364e21a805070fdd45 Subject: Re: [Qemu-devel] [PATCH v8 07/27] target-arm: insert AArch32 cpregs twice into hashtable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Sergey Fedorov , QEMU Developers , Fabian Aggeler , "Edgar E. Iglesias" --001a11c350364e21a805070fdd45 Content-Type: text/plain; charset=UTF-8 I have fixed the code to properly handle the CONTEXTIDR/FCSEIDR registers. This is done in two parts: 1) I broke the FCSEIDR and CONTEXTIDR into separate secure/non-secure definitions. 2) I updated the check that filters the secure duplicate instance caused by registering unbanked register twice. On 31 October 2014 14:01, Greg Bellows wrote: > > > On 31 October 2014 07:44, Peter Maydell wrote: > >> On 30 October 2014 21:28, Greg Bellows wrote: >> > From: Fabian Aggeler >> > >> > Prepare for cp register banking by inserting every cp register twice, >> > once for secure world and once for non-secure world. >> > >> > Signed-off-by: Fabian Aggeler >> > Signed-off-by: Greg Bellows >> > >> > --- >> > >> > v7 -> v8 >> > - Updated define registers asserts to allow either a non-zero >> fieldoffset or >> > non-zero bank_fieldoffsets. >> > - Updated CP register hashing to always set the register fieldoffset >> when >> > banked register offsets are specified. >> > >> > v5 -> v6 >> > - Fixed NS-bit number in the CPREG hash lookup from 27 to 29. >> > - Switched to dedicated CPREG secure flags. >> > - Fixed disablement of reset and migration of common 32/64-bit >> registers. >> > - Globally replace Aarch# with AArch# >> > >> > v4 -> v5 >> > - Added use of ARM CP secure/non-secure bank flags during register >> processing >> > in define_one_arm_cp_reg_with_opaque(). We now only register the >> specified >> > bank if only one flag is specified, otherwise we register both a >> secure and >> > non-secure instance. >> > --- >> > target-arm/helper.c | 98 >> ++++++++++++++++++++++++++++++++++++++++++++--------- >> > 1 file changed, 82 insertions(+), 16 deletions(-) >> > >> > diff --git a/target-arm/helper.c b/target-arm/helper.c >> > index 959a46e..c1c6303 100644 >> > --- a/target-arm/helper.c >> > +++ b/target-arm/helper.c >> > @@ -3296,22 +3296,62 @@ 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 if the .cp field is left >> unset. >> > + >> > + if (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) { >> > + /* Register is banked (using both entries in array). >> > + * Overwriting fieldoffset as the array is only used to define >> > + * banked registers but later only fieldoffset is used. >> > */ >> > - if (r2->cp == 0) { >> > - r2->cp = 15; >> > + r2->fieldoffset = r->bank_fieldoffsets[nsbit]; >> > + } >> > + >> > + if (state == ARM_CP_STATE_AA32) { >> > + /* Clear the secure state flags and set based on incoming >> nsbit */ >> > + r2->secure &= ~(ARM_CP_SECSTATE_S | ARM_CP_SECSTATE_NS); >> > + r2->secure |= ARM_CP_SECSTATE_S << nsbit; >> >> This bitmanipulation looks like leftover from when these were in 'state'; >> r2->secure = secstate; >> should be sufficient (and you might as well put this down below the >> 'r2->state = state' assignment, since it's harmless to do it for all >> regdefs including 64 bit ones). >> >> > It was in the previous code, but it is still necessary for marking whether > the given register is secure or not. > > >> > + >> > + if (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) { >> > + /* If the register is banked and V8 is enabled then we >> don't need >> > + * to migrate or reset the AArch32 version of the banked >> > + * registers as this will be handled through the AArch64 >> view. >> > + * If v7 then we don't need to migrate or reset the AArch32 >> > + * non-secure bank as this will be handled through the >> AArch64 >> > + * view. In this case the secure bank is not mirrored, so >> we must >> > + * preserve it's reset criteria and allow it to be >> migrated. >> > + * >> > + * The exception to the above is cpregs with a crn of 13 >> > + * (specifically FCSEIDR and CONTEXTIDR) in which case >> there may >> > + * not be an AArch64 equivalent for one or either bank so >> migration >> > + * and reset must be preserved. >> > + */ >> >> I'm not sure what this paragraph is trying to say. The AArch64 equivalent >> of CONTEXTIDR(NS) is CONTEXTIDR_EL1. In v8 FCSEIDR is a constant RAZ/WI >> register, so migration and reset aren't relevant anyway. >> >> In any case, if we only have a couple of special case registers where >> this bank handling doesn't work, I suggest that we should handle them >> by having two separate reginfo structs for the S and NS versions, >> rather than special casing a specific crn value here. >> > > It does not sound like the comment was clear. The point of this code was > to disable migration and reset of one or both banks. If we know there is > an aarch64 version (BOTH) then we know we can disable the ns bank > instance. If we are ARMv8 then we know that we can also disable the sec > bank instance. However, there was an exception in that neither CONTEXTIDR > nor FCSEIDR actually have an ARMv8/AArch64 secure counterparts, so we still > have to allow migration and reset even if ARMv8 is supported. > > You are correct that FCSEIDR is RAZ/WI in ARMv8, which is the exact issue > as this is not the case in ARMv7. I'll work through it to see if adding > separate entries alleviates the need for the ugly conditional. BTW, I > didn't like this either, but at the time I hadn't found a more elegant > approach. > > >> >> > + if (r->state == ARM_CP_STATE_BOTH) { >> > + if ((arm_feature(&cpu->env, ARM_FEATURE_V8) && r->crn >> != 13) || >> > + nsbit) { >> > + r2->type |= ARM_CP_NO_MIGRATE; >> > + r2->resetfn = arm_cp_reset_ignore; >> > + } >> > + } >> > + } else if (!nsbit) { >> > + /* The register is not banked so we only want to allow >> migration of >> > + * the non-secure instance. >> > + */ >> > + r2->type |= ARM_CP_NO_MIGRATE; >> > + r2->resetfn = arm_cp_reset_ignore; >> > } >> > - r2->type |= ARM_CP_NO_MIGRATE; >> > - r2->resetfn = arm_cp_reset_ignore; >> > + >> > + if (r->state == ARM_CP_STATE_BOTH) { >> > + /* We assume it is a cp15 register if the .cp field is >> left unset. >> > + */ >> > + if (r2->cp == 0) { >> > + r2->cp = 15; >> > + } >> > + >> > #ifdef HOST_WORDS_BIGENDIAN >> > - if (r2->fieldoffset) { >> > - r2->fieldoffset += sizeof(uint32_t); >> > - } >> > + if (r2->fieldoffset) { >> > + r2->fieldoffset += sizeof(uint32_t); >> > + } >> > #endif >> > + } >> > } >> > if (state == ARM_CP_STATE_AA64) { >> > /* To allow abbreviation of ARMCPRegInfo >> > @@ -3460,10 +3500,14 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU >> *cpu, >> > */ >> > if (!(r->type & (ARM_CP_SPECIAL|ARM_CP_CONST))) { >> > if (r->access & PL3_R) { >> > - assert(r->fieldoffset || r->readfn); >> > + assert((r->fieldoffset || >> > + (r->bank_fieldoffsets[0] && >> r->bank_fieldoffsets[1])) || >> > + r->readfn); >> > } >> > if (r->access & PL3_W) { >> > - assert(r->fieldoffset || r->writefn); >> > + assert((r->fieldoffset || >> > + (r->bank_fieldoffsets[0] && >> r->bank_fieldoffsets[1])) || >> > + r->writefn); >> > } >> > } >> > /* Bad type field probably means missing sentinel at end of reg >> list */ >> > @@ -3476,8 +3520,30 @@ 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, SCR_NS); >> > + if (state == ARM_CP_STATE_AA32) { >> > + /* Under AArch32 CP registers can be common >> > + * (same for secure and non-secure world) or >> banked. >> > + */ >> > + uint32_t s = >> > + r->secure & (ARM_CP_SECSTATE_S | >> ARM_CP_SECSTATE_NS); >> > + if (ARM_CP_SECSTATE_S == s) { >> >> As a general remark, don't use this sort of "yoda conditional" with the >> constant on the LHS of the ==, please. >> > > Fixed in v9. > > >> >> > + add_cpreg_to_hashtable(cpu, r, opaque, >> state, >> > + crm, opc1, opc2, !SCR_NS); >> > + } else if (ARM_CP_SECSTATE_NS == s) { >> > + add_cpreg_to_hashtable(cpu, r, opaque, >> state, >> > + crm, opc1, opc2, SCR_NS); >> > + } else { >> > + add_cpreg_to_hashtable(cpu, r, opaque, >> state, >> > + crm, opc1, opc2, !SCR_NS); >> > + add_cpreg_to_hashtable(cpu, r, opaque, >> state, >> > + crm, opc1, opc2, SCR_NS); >> > + } >> >> Given the change to make add_cpreg_to_hashtable() take an ARM_CP_SECSTATE* >> constant that I suggested in the previous patch, you can simplify this to >> >> switch (r->secure) { >> case ARM_CP_SECSTATE_S: >> case ARM_CP_SECSTATE_NS: >> add_cpreg_to_hashtable(cpu, r, opaque, state, r->secure, >> crm, opc1, opc2); >> break; >> default: >> add_cpreg_to_hashtable(cpu, r, opaque, state, >> ARM_CP_SECSTATE_S, crm, opc1, opc2); >> add_cpreg_to_hashtable(cpu, r, opaque, state, >> ARM_CP_SECSTATE_NS, crm, opc1, opc2); >> break; >> } >> > >> > + } else { >> > + /* AArch64 registers get mapped to non-secure >> instance >> > + * of AArch32 */ >> > + add_cpreg_to_hashtable(cpu, r, opaque, state, >> > + crm, opc1, opc2, SCR_NS); >> > + } >> > } >> > } >> > } >> > -- >> > 1.8.3.2 >> >> thanks >> -- PMM >> > > --001a11c350364e21a805070fdd45 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
I have fixed the code to properly handle the=C2=A0CON= TEXTIDR/FCSEIDR registers.=C2=A0 This is done in two parts:=C2=A0
=
1) I broke the FCSEIDR and CONTEXTIDR into separate secure/non-secure = definitions.
2) I updated the check that filters the secure dupli= cate instance caused by registering unbanked register twice.

=





<= br>



On 31 October 2014 14:01, Greg Bell= ows <greg.bellows@linaro.org> wrote:


On 31 October 2014 07:44, Peter= Maydell <peter.maydell@linaro.org> wrote:
On 30 October 2014 21:28, Greg Bellows &l= t;greg.bellows= @linaro.org> wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
>
> Prepare for cp register banking by inserting every cp register twice,<= br> > once for secure world and once for non-secure world.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>
> ---
>
> v7 -> v8
> - Updated define registers asserts to allow either a non-zero fieldoff= set or
>=C2=A0 =C2=A0non-zero bank_fieldoffsets.
> - Updated CP register hashing to always set the register fieldoffset w= hen
>=C2=A0 =C2=A0banked register offsets are specified.
>
> v5 -> v6
> - Fixed NS-bit number in the CPREG hash lookup from 27 to 29.
> - Switched to dedicated CPREG secure flags.
> - Fixed disablement of reset and migration of common 32/64-bit registe= rs.
> - Globally replace Aarch# with AArch#
>
> v4 -> v5
> - Added use of ARM CP secure/non-secure bank flags during register pro= cessing
>=C2=A0 =C2=A0in define_one_arm_cp_reg_with_opaque().=C2=A0 We now only = register the specified
>=C2=A0 =C2=A0bank if only one flag is specified, otherwise we register = both a secure and
>=C2=A0 =C2=A0non-secure instance.
> ---
>=C2=A0 target-arm/helper.c | 98 +++++++++++++++++++++++++++++++++++++++= +++++---------
>=C2=A0 1 file changed, 82 insertions(+), 16 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 959a46e..c1c6303 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3296,22 +3296,62 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu= , const ARMCPRegInfo *r,
>=C2=A0 =C2=A0 =C2=A0 uint32_t *key =3D g_new(uint32_t, 1);
>=C2=A0 =C2=A0 =C2=A0 ARMCPRegInfo *r2 =3D g_memdup(r, sizeof(ARMCPRegIn= fo));
>=C2=A0 =C2=A0 =C2=A0 int is64 =3D (r->type & ARM_CP_64BIT) ? 1 := 0;
> -=C2=A0 =C2=A0 if (r->state =3D=3D ARM_CP_STATE_BOTH && sta= te =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 =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* view handles that. AArch64 also h= andles reset.
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* We assume it is a cp15 register i= f the .cp field is left unset.
> +
> +=C2=A0 =C2=A0 if (r->bank_fieldoffsets[0] && r->bank_fi= eldoffsets[1]) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Register is banked (using both entries= in array).
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Overwriting fieldoffset as the ar= ray is only used to define
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* banked registers but later only f= ieldoffset is used.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r2->cp =3D=3D 0) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->cp =3D 15;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->fieldoffset =3D r->bank_fieldof= fsets[nsbit];
> +=C2=A0 =C2=A0 }
> +
> +=C2=A0 =C2=A0 if (state =3D=3D ARM_CP_STATE_AA32) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Clear the secure state flags and set b= ased on incoming nsbit */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->secure &=3D ~(ARM_CP_SECSTATE_= S | ARM_CP_SECSTATE_NS);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->secure |=3D ARM_CP_SECSTATE_S <= < nsbit;

This bitmanipulation looks like leftover from when these were i= n 'state';
=C2=A0 =C2=A0r2->secure =3D secstate;
should be sufficient (and you might as well put this down below the
'r2->state =3D state' assignment, since it's harmless to do = it for all
regdefs including 64 bit ones).


It was in the= previous code, but it is still necessary for marking whether the given reg= ister is secure or not.
=C2=A0
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r->bank_fieldoffsets[0] &&= r->bank_fieldoffsets[1]) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* If the register is banke= d and V8 is enabled then we don't need
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* to migrate or reset= the AArch32 version of the banked
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* registers as this w= ill be handled through the AArch64 view.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* If v7 then we don&#= 39;t need to migrate or reset the AArch32
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* non-secure bank as = this will be handled through the AArch64
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* view.=C2=A0 In this= case the secure bank is not mirrored, so we must
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* preserve it's r= eset criteria and allow it to be migrated.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* The exception to th= e above is cpregs with a crn of 13
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* (specifically FCSEI= DR and CONTEXTIDR) in which case there may
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* not be an AArch64 e= quivalent for one or either bank so migration
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* and reset must be p= reserved.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/

I'm not sure what this paragraph is trying to say. The AArch64 e= quivalent
of CONTEXTIDR(NS) is CONTEXTIDR_EL1. In v8 FCSEIDR is a constant RAZ/WI
register, so migration and reset aren't relevant anyway.

In any case, if we only have a couple of special case registers where
this bank handling doesn't work, I suggest that we should handle them by having two separate reginfo structs for the S and NS versions,
rather than special casing a specific crn value here.
=
It does not sound like the comment was clear.=C2=A0 T= he point of this code was to disable migration and reset of one or both ban= ks.=C2=A0 If we know there is an aarch64 version (BOTH) then we know we can= disable the ns bank instance.=C2=A0 If we are ARMv8 then we know that we c= an also disable the sec bank instance.=C2=A0 However, there was an exceptio= n in that neither CONTEXTIDR nor FCSEIDR actually have an ARMv8/AArch64 sec= ure counterparts, so we still have to allow migration and reset even if ARM= v8 is supported. =C2=A0

You are correct that FCSEI= DR is RAZ/WI in ARMv8, which is the exact issue as this is not the case in = ARMv7.=C2=A0 I'll work through it to see if adding separate entries all= eviates the need for the ugly conditional.=C2=A0 BTW, I didn't like thi= s either, but at the time I hadn't found a more elegant approach.
=
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r->state =3D=3D ARM_= CP_STATE_BOTH) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((arm_feat= ure(&cpu->env, ARM_FEATURE_V8) && r->crn !=3D 13) ||
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= nsbit) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= r2->type |=3D ARM_CP_NO_MIGRATE;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= r2->resetfn =3D arm_cp_reset_ignore;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 } else if (!nsbit) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* The register is not bank= ed so we only want to allow migration of
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* the non-secure inst= ance.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->type |=3D ARM_CP_NO_= MIGRATE;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->resetfn =3D arm_cp_r= eset_ignore;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->type |=3D ARM_CP_NO_MIGRATE;
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->resetfn =3D arm_cp_reset_ignore; > +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r->state =3D=3D ARM_CP_STATE_BOTH)= {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* We assume it is a cp15 r= egister if the .cp field is left unset.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r2->cp =3D=3D 0) { > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->cp =3D= 15;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +
>=C2=A0 #ifdef HOST_WORDS_BIGENDIAN
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r2->fieldoffset) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->fieldoffset +=3D siz= eof(uint32_t);
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r2->fieldoffset) { > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r2->fieldo= ffset +=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=A0 if (state =3D=3D ARM_CP_STATE_AA64) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* To allow abbreviation of ARMCPReg= Info
> @@ -3460,10 +3500,14 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU = *cpu,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>=C2=A0 =C2=A0 =C2=A0 if (!(r->type & (ARM_CP_SPECIAL|ARM_CP_CONS= T))) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r->access & PL3_R) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 assert(r->fieldoffset ||= r->readfn);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 assert((r->fieldoffset |= |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1])) ||
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= r->readfn);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r->access & PL3_W) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 assert(r->fieldoffset ||= r->writefn);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 assert((r->fieldoffset |= |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1])) ||
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= r->writefn);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 /* Bad type field probably means missing sentinel = at end of reg list */
> @@ -3476,8 +3520,30 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *= cpu,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 if (r->state !=3D state && r->state !=3D ARM_CP_STATE_= BOTH) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 continue;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 }
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= add_cpreg_to_hashtable(cpu, r, opaque, state,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0crm, opc1, opc2, SCR_NS);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= if (state =3D=3D ARM_CP_STATE_AA32) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 /* Under AArch32 CP registers can be common
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0* (same for secure and non-secure world) or banked. > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0*/
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 uint32_t s =3D
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 r->secure & (ARM_CP_SECSTATE_S | ARM_CP_SECSTA= TE_NS);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 if (ARM_CP_SECSTATE_S =3D=3D s) {

As a general remark, don't use this sort of "yoda cond= itional" with the
constant on the LHS of the =3D=3D, please.

<= /div>
Fixed in v9.
=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 add_cpreg_to_hashtable(cpu, r, opaque, state,<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 crm, opc1, opc2, != SCR_NS);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 } else if (ARM_CP_SECSTATE_NS =3D=3D s) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 add_cpreg_to_hashtable(cpu, r, opaque, state,<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 crm, opc1, opc2, S= CR_NS);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 } else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 add_cpreg_to_hashtable(cpu, r, opaque, state,<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 crm, opc1, opc2, != SCR_NS);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 add_cpreg_to_hashtable(cpu, r, opaque, state,<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 crm, opc1, opc2, S= CR_NS);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 }

Given the change to make add_cpreg_to_hashtable() take an ARM_CP_SEC= STATE*
constant that I suggested in the previous patch, you can simplify this to
=C2=A0 =C2=A0 switch (r->secure) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 case ARM_CP_SECSTATE_S:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 case ARM_CP_SECSTATE_NS:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 add_cpreg_to_hashtable(cpu, r, op= aque, state, r->secure,
crm, opc1, opc2);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 default:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 add_cpreg_to_hashtable(cpu, r, op= aque, state,
ARM_CP_SECSTATE_S, crm, opc1, opc2);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 add_cpreg_to_hashtable(cpu, r, op= aque, state,
ARM_CP_SECSTATE_NS, crm, opc1, opc2);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
=C2=A0 =C2=A0 }=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= } else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 /* AArch64 registers get mapped to non-secure instance
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0* of AArch32 */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 add_cpreg_to_hashtable(cpu, r, opaque, state,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 crm, opc1, opc2, SCR_NS);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> --
> 1.8.3.2

thanks
-- PMM


--001a11c350364e21a805070fdd45--