| Message ID | 20221007152159.1414065-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Make the final stage1+2 write to secure be unconditional | expand |
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
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
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
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 { /*
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(-)