| Message ID | 20221001162318.153420-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Implement FEAT_HAFDBS | expand |
On Sat, 1 Oct 2022 at 17:24, 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(). > > Create a new local variable, s2walk_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> > --- > v3: Do not modify ipa_secure, per review. > --- Reviewed-by: Peter Maydell <peter.maydell@linaro.org> I did find myself wondering if we should explicitly set result->attrs.secure = false; in an else-branch of the last "if (is_secure)", though. At the moment we rely on get_phys_addr_lpae() for the stage2 doing that for us, I think. Having the code here always set result->attrs.secure before the 'return 0' avoids having to think about that... thanks -- PMM
On 10/6/22 07:27, Peter Maydell wrote: > On Sat, 1 Oct 2022 at 17:24, 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(). >> >> Create a new local variable, s2walk_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> >> --- >> v3: Do not modify ipa_secure, per review. >> --- > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > I did find myself wondering if we should explicitly set > result->attrs.secure = false; > in an else-branch of the last "if (is_secure)", though. > At the moment we rely on get_phys_addr_lpae() for the stage2 > doing that for us, I think. Having the code here always set > result->attrs.secure before the 'return 0' avoids having to think > about that... Yes, we're currently (and predating this patch set) relying on the attrs structure being cleared to start. But I can certainly add the assignment if you like. r~
On Thu, 6 Oct 2022 at 16:10, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 10/6/22 07:27, Peter Maydell wrote: > > On Sat, 1 Oct 2022 at 17:24, 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(). > >> > >> Create a new local variable, s2walk_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> > >> --- > >> v3: Do not modify ipa_secure, per review. > >> --- > > > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > > > I did find myself wondering if we should explicitly set > > result->attrs.secure = false; > > in an else-branch of the last "if (is_secure)", though. > > At the moment we rely on get_phys_addr_lpae() for the stage2 > > doing that for us, I think. Having the code here always set > > result->attrs.secure before the 'return 0' avoids having to think > > about that... > > Yes, we're currently (and predating this patch set) relying on the attrs structure being > cleared to start. But I can certainly add the assignment if you like. Yeah, cleared-at-start is fine. But here we're also relying on the stage 2 call to get_phys_addr_lpae() not setting it to 1, because we pass that the same 'result' pointer, not a fresh one. -- PMM
On 10/6/22 08:22, Peter Maydell wrote: > Yeah, cleared-at-start is fine. But here we're also relying on > the stage 2 call to get_phys_addr_lpae() not setting it to 1, > because we pass that the same 'result' pointer, not a fresh one. I clear it first: that patch is already merged: memset(result, 0, sizeof(*result)); ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx, is_el0, result, fi); r~
On Thu, 6 Oct 2022 at 19:20, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 10/6/22 08:22, Peter Maydell wrote: > > Yeah, cleared-at-start is fine. But here we're also relying on > > the stage 2 call to get_phys_addr_lpae() not setting it to 1, > > because we pass that the same 'result' pointer, not a fresh one. > > I clear it first: that patch is already merged: > > memset(result, 0, sizeof(*result)); > > > > ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx, > > is_el0, result, fi); Yes, but that doesn't help if this ^^^ get_phys_addr_lpae() call sets result->attrs.secure = true. thanks -- PMM
On 10/6/22 11:55, Peter Maydell wrote: > On Thu, 6 Oct 2022 at 19:20, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 10/6/22 08:22, Peter Maydell wrote: >>> Yeah, cleared-at-start is fine. But here we're also relying on >>> the stage 2 call to get_phys_addr_lpae() not setting it to 1, >>> because we pass that the same 'result' pointer, not a fresh one. >> >> I clear it first: that patch is already merged: >> >> memset(result, 0, sizeof(*result)); >> ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx, >> is_el0, result, fi); > > Yes, but that doesn't help if this ^^^ get_phys_addr_lpae() > call sets result->attrs.secure = true. Ok, sure, let's make the write to .secure be unconditional. I've split this out into a new patch 2 for clarity. r~
On Thu, 6 Oct 2022 at 21:58, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 10/6/22 11:55, Peter Maydell wrote: > > On Thu, 6 Oct 2022 at 19:20, Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> On 10/6/22 08:22, Peter Maydell wrote: > >>> Yeah, cleared-at-start is fine. But here we're also relying on > >>> the stage 2 call to get_phys_addr_lpae() not setting it to 1, > >>> because we pass that the same 'result' pointer, not a fresh one. > >> > >> I clear it first: that patch is already merged: > >> > >> memset(result, 0, sizeof(*result)); > >> ret = get_phys_addr_lpae(env, ipa, access_type, s2_mmu_idx, > >> is_el0, result, fi); > > > > Yes, but that doesn't help if this ^^^ get_phys_addr_lpae() > > call sets result->attrs.secure = true. > > Ok, sure, let's make the write to .secure be unconditional. > I've split this out into a new patch 2 for clarity. If you can send that extra patch out, I can take it plus 1..20 from this series into target-arm.next, so your next revision of this series can be smaller. thanks -- PMM
diff --git a/target/arm/ptw.c b/target/arm/ptw.c index 2ddfc028ab..b8c494ad9f 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -2298,7 +2298,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, hwaddr ipa; int s1_prot; int ret; - bool ipa_secure; + bool ipa_secure, s2walk_secure; ARMCacheAttrs cacheattrs1; ARMMMUIdx s2_mmu_idx; bool is_el0; @@ -2313,17 +2313,17 @@ 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. */ + s2walk_secure = !(ipa_secure + ? env->cp15.vstcr_el2 & VSTCR_SW + : env->cp15.vtcr_el2 & VTCR_NSW); } else { assert(!ipa_secure); + s2walk_secure = false; } - s2_mmu_idx = (result->attrs.secure + s2_mmu_idx = (s2walk_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2); is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == ARMMMUIdx_SE10_0; @@ -2366,7 +2366,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));
The starting security state comes with the translation regime, not the current state of arm_is_secure_below_el3(). Create a new local variable, s2walk_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> --- v3: Do not modify ipa_secure, per review. --- target/arm/ptw.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)