From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43732) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGqs5-0000tE-Jf for qemu-devel@nongnu.org; Thu, 29 Jan 2015 10:19:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGqs2-00042B-NB for qemu-devel@nongnu.org; Thu, 29 Jan 2015 10:19:05 -0500 Received: from mail-qg0-f49.google.com ([209.85.192.49]:50487) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGqs2-00041r-Fp for qemu-devel@nongnu.org; Thu, 29 Jan 2015 10:19:02 -0500 Received: by mail-qg0-f49.google.com with SMTP id i50so29414427qgf.8 for ; Thu, 29 Jan 2015 07:19:01 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1422037228-5363-1-git-send-email-peter.maydell@linaro.org> <1422037228-5363-10-git-send-email-peter.maydell@linaro.org> Date: Thu, 29 Jan 2015 09:19:01 -0600 Message-ID: From: Greg Bellows Content-Type: multipart/alternative; boundary=001a1133c3fe00cfd8050dcc0235 Subject: Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "Edgar E. Iglesias" , Andrew Jones , =?UTF-8?B?QWxleCBCZW5uw6ll?= , QEMU Developers , Patch Tracking --001a1133c3fe00cfd8050dcc0235 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Wed, Jan 28, 2015 at 4:30 PM, Peter Maydell wrote: > On 28 January 2015 at 21:37, Greg Bellows wrote= : > > > >> +/* Return true if the translation regime is using LPAE format page > tables > >> */ > >> +static inline bool regime_using_lpae_format(CPUARMState *env, > >> + ARMMMUIdx mmu_idx) > >> +{ > >> + int el =3D regime_el(env, mmu_idx); > >> + if (el =3D=3D 2 || arm_el_is_aa64(env, el)) { > > > > > > For the life of me, I can not figure out why EL2 is wired to always use > > LPAE. Is this stated in the spec somewhere? I found places where EL2 > > registers can vary depending on TTBCR.EAE bit settings which implies it > is > > not always true. > > The only translation regimes controlled by EL2 are: > (1) EL2's own stage 1 translations > (2) the stage 2 translations > > These must both be LPAE format: see the v8 ARM ARM section G4.4: > "the translation tables for the Non-secure PL2 stage 1 translations, > and for the Non-secure PL1&0 stage 2 translations, must use the > Long-descriptor translation table format." v7 ARM ARM B3.3 has > similar text. > =E2=80=8BI see that now, thanks for pointing this out=E2=80=8B > > (Basically, short-descriptors are obsolete and are only supported in > the pre-Virtualization translation regimes, ie AArch32 EL1/3.) > > > I realize EL2 is not supported yet, but wouldn't this break AArch32 if = it > > were and the LPAE feature was not enabled? > > Implementations with Virtualization must include LPAE. > > >> -static bool get_level1_table_address(CPUARMState *env, uint32_t *tabl= e, > >> - uint32_t address) > >> +static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx > mmu_idx, > >> + uint32_t *table, uint32_t addres= s) > >> { > >> - /* Get the TCR bank based on our security state */ > >> - TCR *tcr =3D &env->cp15.tcr_el[arm_is_secure(env) ? 3 : 1]; > >> + /* Note that we can only get here for an AArch32 PL0/PL1 lookup *= / > >> + int el =3D regime_el(env, mmu_idx); > >> + TCR *tcr =3D regime_tcr(env, mmu_idx); > >> > >> - /* We only get here if EL1 is running in AArch32. If EL3 is runni= ng > >> in > >> - * AArch32 there is a secure and non-secure instance of the > >> translation > >> - * table registers. > >> - */ > >> if (address & tcr->mask) { > >> if (tcr->raw_tcr & TTBCR_PD1) { > >> /* Translation table walk disabled for TTBR1 */ > >> return false; > >> } > >> - *table =3D A32_BANKED_CURRENT_REG_GET(env, ttbr1) & 0xffffc00= 0; > >> + *table =3D env->cp15.ttbr1_el[el] & 0xffffc000; > > > > > > Perhaps you plan to address this in a separate patch, but I believe > TTBR1 is > > only applicable to EL1 and EL0 in AArch64. > > It's true that TTBR1 is only for EL0/EL1, but see the comment at the > start of the function -- we can't get here except for EL0 and EL1, > because this function is only used for some kinds of short-descriptor > tables. > =E2=80=8BI saw that comment, but was not sure whether it was stale given we= were adding EL3. I now see how AArch64 is routed away from this function. > > >> @@ -4663,7 +4736,12 @@ static int get_phys_addr_v5(CPUARMState *env, > >> uint32_t address, int access_type, > >> desc =3D ldl_phys(cs->as, table); > >> type =3D (desc & 3); > >> domain =3D (desc >> 5) & 0x0f; > >> - domain_prot =3D (A32_BANKED_CURRENT_REG_GET(env, dacr) >> (domain= * > 2)) > >> & 3; > >> + if (regime_el(env, mmu_idx) =3D=3D 1) { > >> + dacr =3D env->cp15.dacr_ns; > >> + } else { > >> + dacr =3D env->cp15.dacr_s; > >> + } > >> + domain_prot =3D (dacr >> (domain * 2)) & 3; > > > > > > Is there a reason that you did not add a regime_dacr() here like you di= d > for > > SCTLR and TCR? > > Didn't seem necessary, since we know we only need to deal with S vs NS, > and the concept isn't generally applicable to most regimes. If the > dacr in env->cp15 was stored as dacr[4] I'd have used dacr[el] as > I do above for the ttbr1_el[], but it isn't, hence the conditional. > > =E2=80=8BFair enough, was more a question of consistency since you do the= same thing in both the v5 and v6 code.=E2=80=8B > The TCR and SCTLR are used in LPAE format page tables so they apply > for the whole set of translation regimes. > > > Also, the ARMv8 ARM makes it sound like DACR has no value in AArch64. > > Well, the DACR is only relevant to short-descriptor format page tables, > so it's only consulted for AArch32 translations, and there's no > equivalent register in AArch64. (There is a DACR32_EL2 64 bit register, > but that is only there so a hypervisor can save and restore the state > of a 32 bit VM (at EL1) that is using short-descriptor page tables.) > > > However, if it did have meaning in AArch64, then for S1SE1 would we be > > accessing the wrong bank as regime_el returns 1? This working off the > > understanding that an address reference from an instruction executed in > > S/EL1 and AArch64 would generate such an index. > =E2=80=8BSo this comment is moot given my misunderstanding that we would no= t be in this code if AArch64. =E2=80=8B > > We can only get here for regime S1SE1 if: > * EL3 is AArch64 > * EL1 is AArch32 > > Since EL3 is 64 bit, there is no banking of registers and regardless > of whether EL1 is Secure or NonSecure we want the one and only > register (which is in dacr_ns). (Compare the way we use > ttbr1_el[regime_el(env, mmu_idx)] and so get TTBR1_EL1 whether > this is Secure EL1 or NonSecure EL1.) > > If EL3 is 32 bit then there is banking of registers, but it's > not possible to get here for S1SE1 in that case (only for S EL3 > and NS EL1). > =E2=80=8BRight, that all makes sense. now. > > >> + /* TODO: > >> + * This code assumes we're either a 64-bit EL1 or a 32-bit PL1; > >> + * it doesn't handle the different format TCR for TCR_EL2, TCR_EL= 3, > >> + * and VTCR_EL2, or the fact that those regimes don't have a spli= t > >> + * TTBR0/TTBR1. Attribute and permission bit handling should also > >> + * be checked when adding support for those page table walks. > >> + */ > > > > > > Maybe copy this comment up above in get_level1_table_address(). > > This is the correct location; see remarks above. > > >> @@ -5171,39 +5269,43 @@ static inline int get_phys_addr(CPUARMState > *env, > >> target_ulong address, > >> hwaddr *phys_ptr, int *prot, > >> target_ulong *page_size) > >> { > >> - /* This is not entirely correct as get_phys_addr() can also be > called > >> - * from ats_write() for an address translation of a specific > regime. > >> - */ > >> - uint32_t sctlr =3D A32_BANKED_CURRENT_REG_GET(env, sctlr); > >> - > >> - /* This will go away when we handle mmu_idx properly here */ > >> - int is_user =3D (mmu_idx =3D=3D ARMMMUIdx_S12NSE0 || > >> - mmu_idx =3D=3D ARMMMUIdx_S1SE0 || > >> - mmu_idx =3D=3D ARMMMUIdx_S1NSE0); > >> + if (mmu_idx =3D=3D ARMMMUIdx_S12NSE0 || mmu_idx =3D=3D ARMMMUIdx_= S12NSE1) { > >> + /* TODO: when we support EL2 we should here call ourselves > >> recursively > >> + * to do the stage 1 and then stage 2 translations. The > ldl_phys > >> + * calls for stage 1 will also need changing. > >> + * For non-EL2 CPUs a stage1+stage2 translation is just stage > 1. > >> + */ > >> + assert(!arm_feature(env, ARM_FEATURE_EL2)); > >> + mmu_idx +=3D ARMMMUIdx_S1NSE0; > >> + } > >> > >> /* Fast Context Switch Extension. */ > >> - if (address < 0x02000000) { > >> + if (address < 0x02000000 && mmu_idx !=3D ARMMMUIdx_S2NS) { > >> address +=3D A32_BANKED_CURRENT_REG_GET(env, fcseidr); > > > > > > Now that the MMU index includes security state info we use in in certai= n > > circumstances to determine the security state. However, we don't seem = to > > consistently use it. For example, earlier changes used the mmu_index t= o > > choose certain register banks but here we still rely on the BANKED > macro. I > > see this inconsistency being prone to errors. Maybe we have just not > gotten > > to change all of the cases over, but I thought I'd highlight it. > > Yes, you're right: if the Secure world does an NS ATS operation > then we should be using the NS copy of the register. I think > I mentally skipped over this requirement because the whole bit > of code is irrelevant for v8 CPUs (where FSCEIDR is mandatorily > RAZ/WI and so it doesn't matter which one we use). > > It would also I think be safer to explicitly guard this with > a not-if-v8 check, because we don't actually implement that > RAZ/WI behaviour. So something like: > > if (address < 0x02000000 && mmu_idx !=3D ARMMMUIdx_S2NS > && !arm_feature(env, ARM_FEATURE_V8)) { > uint32_t fcseidr; > if (regime_el(env, mmu_idx) =3D=3D 3) { > fcseidr =3D env->cp15.fcseidr_s; > } else { > fcseidr =3D env->cp15.fcseidr_ns; > } > address +=3D fcseidr; > } > > (note that stage 1 PL2 lookups need to use the NS FCSEIDR.) > > =E2=80=8BYeah, this approach makes sense.=E2=80=8B > thanks > -- PMM > --001a1133c3fe00cfd8050dcc0235 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Wed, Jan 28, 2015 at 4:30 PM, Peter Maydell <peter.= maydell@linaro.org> wrote:
= On 28 January 2015 at 21:37, Greg Bellows <greg.bellows@linaro.org> wrote= :
>
>> +/* Return true if the translation regime is using LPAE format pag= e tables
>> */
>> +static inline bool regime_using_lpae_format(CPUARMState *env,
>> +=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 ARMMMUIdx mmu_idx)
>> +{
>> +=C2=A0 =C2=A0 int el =3D regime_el(env, mmu_idx);
>> +=C2=A0 =C2=A0 if (el =3D=3D 2 || arm_el_is_aa64(env, el)) {
>
>
> For the life of me, I can not figure out why EL2 is wired to always us= e
> LPAE.=C2=A0 =C2=A0Is this stated in the spec somewhere?=C2=A0 I found = places where EL2
> registers can vary depending on TTBCR.EAE bit settings which implies i= t is
> not always true.

The only translation regimes controlled by EL2 are:
=C2=A0(1) EL2's own stage 1 translations
=C2=A0(2) the stage 2 translations

These must both be LPAE format: see the v8 ARM ARM section G4.4:
"the translation tables for the Non-secure PL2 stage 1 translations, and for the Non-secure PL1&0 stage 2 translations, must use the
Long-descriptor translation table format." v7 ARM ARM B3.3 has
similar text.

=E2=80=8BI see that no= w, thanks for pointing this out=E2=80=8B
=C2=A0

(Basically, short-descriptors are obsolete and are only supported in
the pre-Virtualization translation regimes, ie AArch32 EL1/3.)

> I realize EL2 is not supported yet, but wouldn't this break AArch3= 2 if it
> were and the LPAE feature was not enabled?

Implementations with Virtualization must include LPAE.

>> -static bool get_level1_table_address(CPUARMState *env, uint32_t *= table,
>> -=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=A0uint32_t address)
>> +static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx = mmu_idx,
>> +=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=A0uint32= _t *table, uint32_t address)
>>=C2=A0 {
>> -=C2=A0 =C2=A0 /* Get the TCR bank based on our security state */<= br> >> -=C2=A0 =C2=A0 TCR *tcr =3D &env->cp15.tcr_el[arm_is_secure= (env) ? 3 : 1];
>> +=C2=A0 =C2=A0 /* Note that we can only get here for an AArch32 PL= 0/PL1 lookup */
>> +=C2=A0 =C2=A0 int el =3D regime_el(env, mmu_idx);
>> +=C2=A0 =C2=A0 TCR *tcr =3D regime_tcr(env, mmu_idx);
>>
>> -=C2=A0 =C2=A0 /* We only get here if EL1 is running in AArch32. I= f EL3 is running
>> in
>> -=C2=A0 =C2=A0 =C2=A0* AArch32 there is a secure and non-secure in= stance of the
>> translation
>> -=C2=A0 =C2=A0 =C2=A0* table registers.
>> -=C2=A0 =C2=A0 =C2=A0*/
>>=C2=A0 =C2=A0 =C2=A0 if (address & tcr->mask) {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (tcr->raw_tcr & TTBCR_= PD1) {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Translation tab= le walk disabled for TTBR1 */
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return false;
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 *table =3D A32_BANKED_CURRENT_REG_GET= (env, ttbr1) & 0xffffc000;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 *table =3D env->cp15.ttbr1_el[el] = & 0xffffc000;
>
>
> Perhaps you plan to address this in a separate patch, but I believe TT= BR1 is
> only applicable to EL1 and EL0 in AArch64.

It's true that TTBR1 is only for EL0/EL1, but see the comment at= the
start of the function -- we can't get here except for EL0 and EL1,
because this function is only used for some kinds of short-descriptor
tables.

=E2=80=8BI saw that comment,= but was not sure whether it was stale given we were adding EL3.=C2=A0 I no= w see how AArch64 is routed away from this function.
=C2=A0=

>> @@ -4663,7 +4736,12 @@ static int get_phys_addr_v5(CPUARMState *en= v,
>> uint32_t address, int access_type,
>>=C2=A0 =C2=A0 =C2=A0 desc =3D ldl_phys(cs->as, table);
>>=C2=A0 =C2=A0 =C2=A0 type =3D (desc & 3);
>>=C2=A0 =C2=A0 =C2=A0 domain =3D (desc >> 5) & 0x0f;
>> -=C2=A0 =C2=A0 domain_prot =3D (A32_BANKED_CURRENT_REG_GET(env, da= cr) >> (domain * 2))
>> & 3;
>> +=C2=A0 =C2=A0 if (regime_el(env, mmu_idx) =3D=3D 1) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 dacr =3D env->cp15.dacr_ns;
>> +=C2=A0 =C2=A0 } else {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 dacr =3D env->cp15.dacr_s;
>> +=C2=A0 =C2=A0 }
>> +=C2=A0 =C2=A0 domain_prot =3D (dacr >> (domain * 2)) & = 3;
>
>
> Is there a reason that you did not add a regime_dacr() here like you d= id for
> SCTLR and TCR?

Didn't seem necessary, since we know we only need to deal with S= vs NS,
and the concept isn't generally applicable to most regimes. If the
dacr in env->cp15 was stored as dacr[4] I'd have used dacr[el] as I do above for the ttbr1_el[], but it isn't, hence the conditional.

=E2=80=8BFair enough, was more a question of consistency since you do the= same thing in both the v5 and v6 code.=E2=80=8B
=C2= =A0
The TCR and SCTLR are used in LPAE format page tables so they apply
for the whole set of translation regimes.

> Also, the ARMv8 ARM makes it sound like DACR has no value in AArch64.<= br>
Well, the DACR is only relevant to short-descriptor format page tabl= es,
so it's only consulted for AArch32 translations, and there's no
equivalent register in AArch64. (There is a DACR32_EL2 64 bit register,
but that is only there so a hypervisor can save and restore the state
of a 32 bit VM (at EL1) that is using short-descriptor page tables.)

> However, if it did have meaning in AArch64, then for S1SE1 would we be=
> accessing the wrong bank as regime_el returns 1?=C2=A0 This working of= f the
> understanding that an address reference from an instruction executed i= n
> S/EL1 and AArch64 would generate such an index.

=E2=80=8BSo this comment is moot giv= en my misunderstanding that we would not be in this code if AArch64. =C2=A0=
=E2=80=8B
=C2=A0

We can only get here for regime S1SE1 if:
=C2=A0* EL3 is AArch64
=C2=A0* EL1 is AArch32

Since EL3 is 64 bit, there is no banking of registers and regardless
of whether EL1 is Secure or NonSecure we want the one and only
register (which is in dacr_ns). (Compare the way we use
ttbr1_el[regime_el(env, mmu_idx)] and so get TTBR1_EL1 whether
this is Secure EL1 or NonSecure EL1.)

If EL3 is 32 bit then there is banking of registers, but it's
not possible to get here for S1SE1 in that case (only for S EL3
and NS EL1).

=E2=80=8BRight, that al= l makes sense. now.
=C2=A0

>> +=C2=A0 =C2=A0 /* TODO:
>> +=C2=A0 =C2=A0 =C2=A0* This code assumes we're either a 64-bit= EL1 or a 32-bit PL1;
>> +=C2=A0 =C2=A0 =C2=A0* it doesn't handle the different format = TCR for TCR_EL2, TCR_EL3,
>> +=C2=A0 =C2=A0 =C2=A0* and VTCR_EL2, or the fact that those regime= s don't have a split
>> +=C2=A0 =C2=A0 =C2=A0* TTBR0/TTBR1. Attribute and permission bit h= andling should also
>> +=C2=A0 =C2=A0 =C2=A0* be checked when adding support for those pa= ge table walks.
>> +=C2=A0 =C2=A0 =C2=A0*/
>
>
> Maybe copy this comment up above in get_level1_table_address().

This is the correct location; see remarks above.

>> @@ -5171,39 +5269,43 @@ static inline int get_phys_addr(CPUARMStat= e *env,
>> target_ulong address,
>>=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 hwaddr *phys_ptr, int = *prot,
>>=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 target_ulong *page_siz= e)
>>=C2=A0 {
>> -=C2=A0 =C2=A0 /* This is not entirely correct as get_phys_addr() = can also be called
>> -=C2=A0 =C2=A0 =C2=A0* from ats_write() for an address translation= of a specific regime.
>> -=C2=A0 =C2=A0 =C2=A0*/
>> -=C2=A0 =C2=A0 uint32_t sctlr =3D A32_BANKED_CURRENT_REG_GET(env, = sctlr);
>> -
>> -=C2=A0 =C2=A0 /* This will go away when we handle mmu_idx properl= y here */
>> -=C2=A0 =C2=A0 int is_user =3D (mmu_idx =3D=3D ARMMMUIdx_S12NSE0 |= |
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0mmu_idx =3D=3D ARMMMUIdx_S1SE0 ||
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0mmu_idx =3D=3D ARMMMUIdx_S1NSE0);
>> +=C2=A0 =C2=A0 if (mmu_idx =3D=3D ARMMMUIdx_S12NSE0 || mmu_idx =3D= =3D ARMMMUIdx_S12NSE1) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* TODO: when we support EL2 we shoul= d here call ourselves
>> recursively
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* to do the stage 1 and then st= age 2 translations. The ldl_phys
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* calls for stage 1 will also n= eed changing.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* For non-EL2 CPUs a stage1+sta= ge2 translation is just stage 1.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 assert(!arm_feature(env, ARM_FEATURE_= EL2));
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 mmu_idx +=3D ARMMMUIdx_S1NSE0;
>> +=C2=A0 =C2=A0 }
>>
>>=C2=A0 =C2=A0 =C2=A0 /* Fast Context Switch Extension.=C2=A0 */
>> -=C2=A0 =C2=A0 if (address < 0x02000000) {
>> +=C2=A0 =C2=A0 if (address < 0x02000000 && mmu_idx !=3D= ARMMMUIdx_S2NS) {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 address +=3D A32_BANKED_CURRENT_= REG_GET(env, fcseidr);
>
>
> Now that the MMU index includes security state info we use in in certa= in
> circumstances to determine the security state.=C2=A0 However, we don&#= 39;t seem to
> consistently use it.=C2=A0 For example, earlier changes used the mmu_i= ndex to
> choose certain register banks but here we still rely on the BANKED mac= ro. I
> see this inconsistency being prone to errors.=C2=A0 Maybe we have just= not gotten
> to change all of the cases over, but I thought I'd highlight it.
Yes, you're right: if the Secure world does an NS ATS opera= tion
then we should be using the NS copy of the register. I think
I mentally skipped over this requirement because the whole bit
of code is irrelevant for v8 CPUs (where FSCEIDR is mandatorily
RAZ/WI and so it doesn't matter which one we use).

It would also I think be safer to explicitly guard this with
a not-if-v8 check, because we don't actually implement that
RAZ/WI behaviour. So something like:

=C2=A0 if (address < 0x02000000 && mmu_idx !=3D ARMMMUIdx_S2NS =C2=A0 =C2=A0 =C2=A0 && !arm_feature(env, ARM_FEATURE_V8)) {=
=C2=A0 =C2=A0 =C2=A0 uint32_t fcseidr;
=C2=A0 =C2=A0 =C2=A0 if (regime_el(env, mmu_idx) =3D=3D 3) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 fcseidr =3D env->cp15.fcseidr_s;
=C2=A0 =C2=A0 =C2=A0 } else {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 fcseidr =3D env->cp15.fcseidr_ns;
=C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 address +=3D fcseidr;
=C2=A0 }

(note that stage 1 PL2 lookups need to use the NS FCSEIDR.)


=E2=80=8BYeah, this approach makes = sense.=E2=80=8B
=C2=A0
thanks
-- PMM

--001a1133c3fe00cfd8050dcc0235--