Re: [PATCH v2 02/66] target/arm: Fix ipa_secure in get_phys_addr

[Peter Maydell]

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