| Message ID | 20220327093427.1548629-4-idan.horowitz@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bug fixes related to secure 2 stage translation | expand |
On 3/27/22 03:34, Idan Horowitz wrote: > As per the AArch64.S2Walk() psuedo-code in the ARMv8 ARM, the final > decision as to the output address's PA space based on the SA/SW/NSA/NSA > bits needs to take the input IPA's PA space into account, and not the > PA space of the result of the stage 2 walk itself. > > Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com> I believe I follow: because the walk uses walkstate.address.paddress.paspace, the ipa input parameter is unchanged, and it is ipa that is passed to AArch64.S2NextWalkStateLast() to form the output address. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Tue, 29 Mar 2022 at 22:09, Richard Henderson <richard.henderson@linaro.org> wrote: > > I believe I follow: because the walk uses walkstate.address.paddress.paspace, the ipa > input parameter is unchanged, and it is ipa that is passed to > AArch64.S2NextWalkStateLast() to form the output address. > Indeed, I initially found the issue when a test case passed on arm IP but raised a data abort in QEMU. Since fixing this issue solved the inconsistency, I believe this is the intended behaviour and not a spec bug. > > r~ Idan Horowitz
On Tue, 29 Mar 2022 at 20:09, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 3/27/22 03:34, Idan Horowitz wrote: > > As per the AArch64.S2Walk() psuedo-code in the ARMv8 ARM, the final > > decision as to the output address's PA space based on the SA/SW/NSA/NSA > > bits needs to take the input IPA's PA space into account, and not the > > PA space of the result of the stage 2 walk itself. > > > > Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com> > > I believe I follow: because the walk uses walkstate.address.paddress.paspace, the ipa > input parameter is unchanged, and it is ipa that is passed to > AArch64.S2NextWalkStateLast() to form the output address. Textually, this is described on page D5-4802 of DDI 0487H.a; the security of the output address of the memory access isn't affected by the security of the output address of the translation table walk. -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index e2695e846a..16c2628f8f 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -12644,6 +12644,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, hwaddr ipa; int s2_prot; int ret; + bool ipa_secure; ARMCacheAttrs cacheattrs2 = {}; ARMMMUIdx s2_mmu_idx; bool is_el0; @@ -12657,14 +12658,15 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, return ret; } + ipa_secure = attrs->secure; if (arm_is_secure_below_el3(env)) { - if (attrs->secure) { + if (ipa_secure) { attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & VSTCR_SW); } else { attrs->secure = !(env->cp15.vtcr_el2.raw_tcr & VTCR_NSW); } } else { - assert(!attrs->secure); + assert(!ipa_secure); } s2_mmu_idx = attrs->secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2; @@ -12701,7 +12703,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, /* Check if IPA translates to secure or non-secure PA space. */ if (arm_is_secure_below_el3(env)) { - if (attrs->secure) { + if (ipa_secure) { attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & (VSTCR_SA | VSTCR_SW)); } else {
As per the AArch64.S2Walk() psuedo-code in the ARMv8 ARM, the final decision as to the output address's PA space based on the SA/SW/NSA/NSA bits needs to take the input IPA's PA space into account, and not the PA space of the result of the stage 2 walk itself. Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com> --- target/arm/helper.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)