[PATCH] target/arm: Make the final stage1+2 write to secure be unconditional

[PATCH] target/arm: Make the final stage1+2 write to secure be unconditional

[Richard Henderson]

While the stage2 call to get_phys_addr_lpae should never set
attrs.secure when given a non-secure input, it's just as easy
to make the final update to attrs.secure be unconditional and
false in the case of non-secure input.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

Hi Peter,

This is the promised patch 1.5 for v3 FEAT_HAFDBS.  It generates minor
conflicts down the line, which I have already fixed up locally.  I think
the first one you would encounter is beyond the proposed 20 that you
indicated that you intend to take into target-arm.next right now.


r~

---
 target/arm/ptw.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index b8c494ad9f..7d763a5847 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2365,17 +2365,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
             result->cacheattrs = combine_cacheattrs(env, cacheattrs1,
                                                     result->cacheattrs);
 
-            /* Check if IPA translates to secure or non-secure PA space. */
-            if (is_secure) {
-                if (ipa_secure) {
-                    result->attrs.secure =
-                        !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW));
-                } else {
-                    result->attrs.secure =
-                        !((env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW))
-                        || (env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW)));
-                }
-            }
+            /*
+             * Check if IPA translates to secure or non-secure PA space.
+             * Note that VSTCR overrides VTCR and {N}SW overrides {N}SA.
+             */
+            result->attrs.secure =
+                (is_secure
+                 && !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW))
+                 && (ipa_secure
+                     || !(env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW))));
+
             return 0;
         } else {
             /*
-- 
2.34.1

Re: [PATCH] target/arm: Make the final stage1+2 write to secure be unconditional

[Peter Maydell]

On Fri, 7 Oct 2022 at 16:22, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> While the stage2 call to get_phys_addr_lpae should never set
> attrs.secure when given a non-secure input, it's just as easy
> to make the final update to attrs.secure be unconditional and
> false in the case of non-secure input.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> Hi Peter,
>
> This is the promised patch 1.5 for v3 FEAT_HAFDBS.  It generates minor
> conflicts down the line, which I have already fixed up locally.  I think
> the first one you would encounter is beyond the proposed 20 that you
> indicated that you intend to take into target-arm.next right now.
>
>
> r~
>
> ---
>  target/arm/ptw.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index b8c494ad9f..7d763a5847 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2365,17 +2365,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>              result->cacheattrs = combine_cacheattrs(env, cacheattrs1,
>                                                      result->cacheattrs);
>
> -            /* Check if IPA translates to secure or non-secure PA space. */
> -            if (is_secure) {
> -                if (ipa_secure) {
> -                    result->attrs.secure =
> -                        !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW));
> -                } else {
> -                    result->attrs.secure =
> -                        !((env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW))
> -                        || (env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW)));
> -                }
> -            }

If:
 is_secure == true
 ipa_secure == false
 (env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW) is non-zero
 (env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW) is zero
then the old code sets attrs.secure to true...

> +            /*
> +             * Check if IPA translates to secure or non-secure PA space.
> +             * Note that VSTCR overrides VTCR and {N}SW overrides {N}SA.
> +             */
> +            result->attrs.secure =
> +                (is_secure
> +                 && !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW))
> +                 && (ipa_secure
> +                     || !(env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW))));

...but the new code will set it to false, I think ?

thanks
-- PMM

Re: [PATCH] target/arm: Make the final stage1+2 write to secure be unconditional

[Richard Henderson]

On 10/7/22 09:20, Peter Maydell wrote:
>> -            /* Check if IPA translates to secure or non-secure PA space. */
>> -            if (is_secure) {
>> -                if (ipa_secure) {
>> -                    result->attrs.secure =
>> -                        !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW));
>> -                } else {
>> -                    result->attrs.secure =
>> -                        !((env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW))
>> -                        || (env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW)));
>> -                }
>> -            }
> 
> If:
>   is_secure == true
>   ipa_secure == false
>   (env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW) is non-zero
>   (env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW) is zero
> then the old code sets attrs.secure to true...

No, I think the misalignment of the two lines wrt the !() may have been confusing:

   if (true) {
     if (false) {
     } else {
       secure = !((0) || (non-zero))
              = !(1)
              = 0
     }
   }


r~

> 
>> +            /*
>> +             * Check if IPA translates to secure or non-secure PA space.
>> +             * Note that VSTCR overrides VTCR and {N}SW overrides {N}SA.
>> +             */
>> +            result->attrs.secure =
>> +                (is_secure
>> +                 && !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW))
>> +                 && (ipa_secure
>> +                     || !(env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW))));
> 
> ...but the new code will set it to false, I think ?
> 
> thanks
> -- PMM

Re: [PATCH] target/arm: Make the final stage1+2 write to secure be unconditional

[Peter Maydell]

On Fri, 7 Oct 2022 at 17:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/7/22 09:20, Peter Maydell wrote:
> >> -            /* Check if IPA translates to secure or non-secure PA space. */
> >> -            if (is_secure) {
> >> -                if (ipa_secure) {
> >> -                    result->attrs.secure =
> >> -                        !(env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW));
> >> -                } else {
> >> -                    result->attrs.secure =
> >> -                        !((env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW))
> >> -                        || (env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW)));
> >> -                }
> >> -            }
> >
> > If:
> >   is_secure == true
> >   ipa_secure == false
> >   (env->cp15.vstcr_el2 & (VSTCR_SA | VSTCR_SW) is non-zero
> >   (env->cp15.vtcr_el2 & (VTCR_NSA | VTCR_NSW) is zero
> > then the old code sets attrs.secure to true...
>
> No, I think the misalignment of the two lines wrt the !() may have been confusing:

Ah, yes -- I was indeed confused by the misalignment.

thanks
-- PMM