| Message ID | 20220822152741.1617527-3-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement FEAT_HAFDBS | expand |
On Mon, 22 Aug 2022 at 16:28, Richard Henderson <richard.henderson@linaro.org> wrote: > > The starting security state comes with the translation regime, > not the current state of arm_is_secure_below_el3(). > > More use of the local variable, ipa_secure, which does not need > to be written back to result->attrs.secure -- we compute that > value later, after the S2 walk is complete. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/ptw.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/target/arm/ptw.c b/target/arm/ptw.c > index 8db2abac01..478ff74550 100644 > --- a/target/arm/ptw.c > +++ b/target/arm/ptw.c > @@ -2308,6 +2308,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, > GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > { > ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx); > + bool is_secure = regime_is_secure(env, mmu_idx); > > if (mmu_idx != s1_mmu_idx) { > /* > @@ -2332,19 +2333,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, > } > > ipa = result->phys; > - ipa_secure = result->attrs.secure; > - if (arm_is_secure_below_el3(env)) { > - if (ipa_secure) { > - result->attrs.secure = !(env->cp15.vstcr_el2 & VSTCR_SW); > - } else { > - result->attrs.secure = !(env->cp15.vtcr_el2 & VTCR_NSW); > - } > + if (is_secure) { > + /* Select TCR based on the NS bit from the S1 walk. */ > + ipa_secure = !(result->attrs.secure > + ? env->cp15.vstcr_el2 & VSTCR_SW > + : env->cp15.vtcr_el2 & VTCR_NSW); > } else { > - assert(!ipa_secure); > + ipa_secure = false; > } This is mixing two things up: (1) is the IPA from a first stage translation Secure or Non-secure ? -- this is what ipa_secure is in the current code (2) should the s2 translation table walk be Secure or Non-secure ? -- this is determined by either VSTCR_EL2.SW or .NSW, depending on what ipa_secure is This change ends up setting ipa_secure to the answer to q2. If we want to use it for "is the s2 tt walk secure?" we should give it a different name, eg s2walk_secure. We also seem to have lost the assertion that if this is a non-secure translation regime then we shouldn't have ended up claiming that the first-stage IPA was Secure. I agree we don't really want to silently borrow result->attrs.secure for the place we keep the answer to q2. > > - s2_mmu_idx = (result->attrs.secure > - ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2); > + s2_mmu_idx = (ipa_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2); > is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == ARMMMUIdx_SE10_0; > > /* > @@ -2388,7 +2386,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, > result->cacheattrs); > > /* Check if IPA translates to secure or non-secure PA space. */ > - if (arm_is_secure_below_el3(env)) { > + if (is_secure) { > if (ipa_secure) { > result->attrs.secure = > !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW)); In particular, looking at the pseudocode for AArch64.SS2OutputPASpace() I think this code is correctly looking at ipa_secure and the change above to what that variable is holding means it will now be wrong. (That is, ipa_secure == true in the old code corresponds to pseudocode's ipaspace == PAS_Secure.) thanks -- PMM
diff --git a/target/arm/ptw.c b/target/arm/ptw.c index 8db2abac01..478ff74550 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -2308,6 +2308,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, GetPhysAddrResult *result, ARMMMUFaultInfo *fi) { ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx); + bool is_secure = regime_is_secure(env, mmu_idx); if (mmu_idx != s1_mmu_idx) { /* @@ -2332,19 +2333,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, } ipa = result->phys; - ipa_secure = result->attrs.secure; - if (arm_is_secure_below_el3(env)) { - if (ipa_secure) { - result->attrs.secure = !(env->cp15.vstcr_el2 & VSTCR_SW); - } else { - result->attrs.secure = !(env->cp15.vtcr_el2 & VTCR_NSW); - } + if (is_secure) { + /* Select TCR based on the NS bit from the S1 walk. */ + ipa_secure = !(result->attrs.secure + ? env->cp15.vstcr_el2 & VSTCR_SW + : env->cp15.vtcr_el2 & VTCR_NSW); } else { - assert(!ipa_secure); + ipa_secure = false; } - s2_mmu_idx = (result->attrs.secure - ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2); + s2_mmu_idx = (ipa_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2); is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == ARMMMUIdx_SE10_0; /* @@ -2388,7 +2386,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, result->cacheattrs); /* Check if IPA translates to secure or non-secure PA space. */ - if (arm_is_secure_below_el3(env)) { + if (is_secure) { if (ipa_secure) { result->attrs.secure = !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW)); @@ -2412,7 +2410,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, * cannot upgrade an non-secure translation regime's attributes * to secure. */ - result->attrs.secure = regime_is_secure(env, mmu_idx); + result->attrs.secure = is_secure; result->attrs.user = regime_is_user(env, mmu_idx); /*
The starting security state comes with the translation regime, not the current state of arm_is_secure_below_el3(). More use of the local variable, ipa_secure, which does not need to be written back to result->attrs.secure -- we compute that value later, after the S2 walk is complete. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/ptw.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)