[PATCH] target/arm: Two fixes for secure ptw

[PATCH] target/arm: Two fixes for secure ptw

[Richard Henderson]

Reversed the sense of non-secure in get_phys_addr_lpae,
and failed to initialize attrs.secure for ARMMMUIdx_Phys_S.

Fixes: 48da29e4 ("target/arm: Add ptw_idx to S1Translate")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1293
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/ptw.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 58a7bbda50..df3573f150 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1357,7 +1357,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
     descaddr |= (address >> (stride * (4 - level))) & indexmask;
     descaddr &= ~7ULL;
     nstable = extract32(tableattrs, 4, 1);
-    if (!nstable) {
+    if (nstable) {
         /*
          * Stage2_S -> Stage2 or Phys_S -> Phys_NS
          * Assert that the non-secure idx are even, and relative order.
@@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
     bool is_secure = ptw->in_secure;
     ARMMMUIdx s1_mmu_idx;
 
+    /*
+     * The page table entries may downgrade secure to non-secure, but
+     * cannot upgrade an non-secure translation regime's attributes
+     * to secure.
+     */
+    result->f.attrs.secure = is_secure;
+
     switch (mmu_idx) {
     case ARMMMUIdx_Phys_S:
     case ARMMMUIdx_Phys_NS:
@@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
         break;
     }
 
-    /*
-     * The page table entries may downgrade secure to non-secure, but
-     * cannot upgrade an non-secure translation regime's attributes
-     * to secure.
-     */
-    result->f.attrs.secure = is_secure;
     result->f.attrs.user = regime_is_user(env, mmu_idx);
 
     /*
-- 
2.34.1

Re: [PATCH] target/arm: Two fixes for secure ptw

[Philippe Mathieu-Daudé]

On 2/11/22 06:47, Richard Henderson wrote:
> Reversed the sense of non-secure in get_phys_addr_lpae,
> and failed to initialize attrs.secure for ARMMMUIdx_Phys_S.
> 
> Fixes: 48da29e4 ("target/arm: Add ptw_idx to S1Translate")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1293

Thanks!

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/ptw.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)

Re: [PATCH] target/arm: Two fixes for secure ptw

[Peter Maydell]

On Wed, 2 Nov 2022 at 05:47, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Reversed the sense of non-secure in get_phys_addr_lpae,
> and failed to initialize attrs.secure for ARMMMUIdx_Phys_S.
>
> Fixes: 48da29e4 ("target/arm: Add ptw_idx to S1Translate")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1293
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/ptw.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 58a7bbda50..df3573f150 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1357,7 +1357,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
>      descaddr |= (address >> (stride * (4 - level))) & indexmask;
>      descaddr &= ~7ULL;
>      nstable = extract32(tableattrs, 4, 1);
> -    if (!nstable) {
> +    if (nstable) {
>          /*
>           * Stage2_S -> Stage2 or Phys_S -> Phys_NS
>           * Assert that the non-secure idx are even, and relative order.
> @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
>      bool is_secure = ptw->in_secure;
>      ARMMMUIdx s1_mmu_idx;
>
> +    /*
> +     * The page table entries may downgrade secure to non-secure, but
> +     * cannot upgrade an non-secure translation regime's attributes
> +     * to secure.
> +     */
> +    result->f.attrs.secure = is_secure;
> +
>      switch (mmu_idx) {
>      case ARMMMUIdx_Phys_S:
>      case ARMMMUIdx_Phys_NS:
> @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
>          break;
>      }
>
> -    /*
> -     * The page table entries may downgrade secure to non-secure, but
> -     * cannot upgrade an non-secure translation regime's attributes
> -     * to secure.
> -     */
> -    result->f.attrs.secure = is_secure;
>      result->f.attrs.user = regime_is_user(env, mmu_idx);

Do we also need to move this setting of attrs.user ?
get_phys_addr_disabled() doesn't set that either.

thanks
-- PMM

Re: [PATCH] target/arm: Two fixes for secure ptw

[Peter Maydell]

On Thu, 3 Nov 2022 at 13:19, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 2 Nov 2022 at 05:47, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > Reversed the sense of non-secure in get_phys_addr_lpae,
> > and failed to initialize attrs.secure for ARMMMUIdx_Phys_S.
> >
> > Fixes: 48da29e4 ("target/arm: Add ptw_idx to S1Translate")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1293
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >  target/arm/ptw.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> > index 58a7bbda50..df3573f150 100644
> > --- a/target/arm/ptw.c
> > +++ b/target/arm/ptw.c
> > @@ -1357,7 +1357,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
> >      descaddr |= (address >> (stride * (4 - level))) & indexmask;
> >      descaddr &= ~7ULL;
> >      nstable = extract32(tableattrs, 4, 1);
> > -    if (!nstable) {
> > +    if (nstable) {
> >          /*
> >           * Stage2_S -> Stage2 or Phys_S -> Phys_NS
> >           * Assert that the non-secure idx are even, and relative order.
> > @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
> >      bool is_secure = ptw->in_secure;
> >      ARMMMUIdx s1_mmu_idx;
> >
> > +    /*
> > +     * The page table entries may downgrade secure to non-secure, but
> > +     * cannot upgrade an non-secure translation regime's attributes
> > +     * to secure.
> > +     */
> > +    result->f.attrs.secure = is_secure;
> > +
> >      switch (mmu_idx) {
> >      case ARMMMUIdx_Phys_S:
> >      case ARMMMUIdx_Phys_NS:
> > @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
> >          break;
> >      }
> >
> > -    /*
> > -     * The page table entries may downgrade secure to non-secure, but
> > -     * cannot upgrade an non-secure translation regime's attributes
> > -     * to secure.
> > -     */
> > -    result->f.attrs.secure = is_secure;
> >      result->f.attrs.user = regime_is_user(env, mmu_idx);
>
> Do we also need to move this setting of attrs.user ?
> get_phys_addr_disabled() doesn't set that either.

I've applied this to target-arm.next for the moment anyway, since
it is definitely fixing an attrs.secure related bug. I can replace
that with a v2 or we can do a follow-on patch, depending whether
you get to this before or after I send out a pullreq.

thanks
-- PMM

Re: [PATCH] target/arm: Two fixes for secure ptw

[Richard Henderson]

On 11/4/22 00:19, Peter Maydell wrote:
>> @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
>>       bool is_secure = ptw->in_secure;
>>       ARMMMUIdx s1_mmu_idx;
>>
>> +    /*
>> +     * The page table entries may downgrade secure to non-secure, but
>> +     * cannot upgrade an non-secure translation regime's attributes
>> +     * to secure.
>> +     */
>> +    result->f.attrs.secure = is_secure;
>> +
>>       switch (mmu_idx) {
>>       case ARMMMUIdx_Phys_S:
>>       case ARMMMUIdx_Phys_NS:
>> @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
>>           break;
>>       }
>>
>> -    /*
>> -     * The page table entries may downgrade secure to non-secure, but
>> -     * cannot upgrade an non-secure translation regime's attributes
>> -     * to secure.
>> -     */
>> -    result->f.attrs.secure = is_secure;
>>       result->f.attrs.user = regime_is_user(env, mmu_idx);
> 
> Do we also need to move this setting of attrs.user ?
> get_phys_addr_disabled() doesn't set that either.

I don't think so.  The only cases which don't pass through the setting of attrs.user are 
the two Phys mmu_idx.  Which was by design per the comment up there about artificially 
deciding which EL regime they belong to.


r~

Re: [PATCH] target/arm: Two fixes for secure ptw

[Peter Maydell]

On Thu, 3 Nov 2022 at 20:30, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/4/22 00:19, Peter Maydell wrote:
> >> @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
> >>       bool is_secure = ptw->in_secure;
> >>       ARMMMUIdx s1_mmu_idx;
> >>
> >> +    /*
> >> +     * The page table entries may downgrade secure to non-secure, but
> >> +     * cannot upgrade an non-secure translation regime's attributes
> >> +     * to secure.
> >> +     */
> >> +    result->f.attrs.secure = is_secure;
> >> +
> >>       switch (mmu_idx) {
> >>       case ARMMMUIdx_Phys_S:
> >>       case ARMMMUIdx_Phys_NS:
> >> @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
> >>           break;
> >>       }
> >>
> >> -    /*
> >> -     * The page table entries may downgrade secure to non-secure, but
> >> -     * cannot upgrade an non-secure translation regime's attributes
> >> -     * to secure.
> >> -     */
> >> -    result->f.attrs.secure = is_secure;
> >>       result->f.attrs.user = regime_is_user(env, mmu_idx);
> >
> > Do we also need to move this setting of attrs.user ?
> > get_phys_addr_disabled() doesn't set that either.
>
> I don't think so.  The only cases which don't pass through the setting of attrs.user are
> the two Phys mmu_idx.  Which was by design per the comment up there about artificially
> deciding which EL regime they belong to.

OK. Do we ever do anything with the attrs for a phys tlb lookup
except use them internally for details of the stage 2 tlb walk ?
I guess they get used for the memory transaction to do the walk.
That matches the old code that just had a local MemTxAttrs in
arm_ldq_ptw() to do the walk which therefore implicitly got
user == false.

-- PMM

Re: [PATCH] target/arm: Two fixes for secure ptw

[Richard Henderson]

On 11/4/22 21:58, Peter Maydell wrote:
> OK. Do we ever do anything with the attrs for a phys tlb lookup
> except use them internally for details of the stage 2 tlb walk ?
> I guess they get used for the memory transaction to do the walk.
> That matches the old code that just had a local MemTxAttrs in
> arm_ldq_ptw() to do the walk which therefore implicitly got
> user == false.

Exactly.

I can't think of any reason .user would be apply outside of the stage1 lookup, which is 
what records the setting for any future mmio.


r~