[PATCH 0/3] target/arm: Fix handling of VSTCR_EL2.SW and VTCR_EL2.NSW

[PATCH 0/3] target/arm: Fix handling of VSTCR_EL2.SW and VTCR_EL2.NSW

[Peter Maydell]

When FEAT_SEL2 (secure EL2) is implemented, the bits
VSTCR_EL2.SW and VTCR_EL2.NSW allow the guest to set things up
so that the stage 2 walk for an IPA is done to the other
address space, eg
 * a stage 2 walk for an NS IPA done to secure physical memory
   (where the translation table base address and other parameters
   for the walk come from the NS control registers VTTBR_EL2
   and VTCR_EL2)
 * a stage 2 walk for an S IPA done to non-secure physical memory
   (where the parameters from the walk come from the S control
   registers VSTTBR_EL2 and VSTCR_EL2)

We tried to implement this, but didn't get it right -- in
get_phys_addr_twostage() we identify whether we need to do
the s2 walk in Secure or NonSecure, but then we fail to pay
attention to whether we were doing the walk for an NS or S IPA.
The fix for this is simple -- set ptw->in_mmu_idx and ptw->in_secure
based on ipa_secure, with only ptw->in_ptw_idx depending on
s2walk_secure. However to make this work we first need to fix
a couple of places in the ptw code that were incorrectly looking
at ptw->in_secure when they either should not be or should
be doing something based on ptw->in_ptw_idx.

This fixes https://gitlab.com/qemu-project/qemu/-/issues/1600 .
NB: I have tested that this fixes the test case in the bug, and
that it doesn't break 'make check-avocado', but I don't have a
huge supply of EL2-using guests to hand so the patchset hasn't
received exhaustive testing. Plus this area of the architecture
and this bit of QEMU's codebase are pretty hairy -- so careful
review would be a good idea :-)

thanks
-- PMM

Peter Maydell (3):
  target/arm: Don't allow stage 2 page table walks to downgrade to NS
  target/arm: Set ptw->out_secure correctly for stage 2 translations
  target/arm: handle ipa_secure vs s2walk_secure correctly

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

-- 
2.34.1

[PATCH 1/3] target/arm: Don't allow stage 2 page table walks to downgrade to NS

[Peter Maydell]

Bit 63 in a Table descriptor is only the NSTable bit for stage 1
translations; in stage 2 it is RES0.  We were incorrectly looking at
it all the time.

This causes problems if:
 * the stage 2 table descriptor was incorrectly setting the RES0 bit
 * we are doing a stage 2 translation in Secure address space for
   a NonSecure stage 1 regime -- in this case we would incorrectly
   do an immediate downgrade to NonSecure

A bug elsewhere in the code currently prevents us from getting
to the second situation, but when we fix that it will be possible.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/ptw.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 6d72950a795..06865227642 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1403,17 +1403,18 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
     descaddrmask &= ~indexmask_grainsize;
 
     /*
-     * Secure accesses start with the page table in secure memory and
+     * Secure stage 1 accesses start with the page table in secure memory and
      * can be downgraded to non-secure at any step. Non-secure accesses
      * remain non-secure. We implement this by just ORing in the NSTable/NS
      * bits at each step.
+     * Stage 2 never gets this kind of downgrade.
      */
     tableattrs = is_secure ? 0 : (1 << 4);
 
  next_level:
     descaddr |= (address >> (stride * (4 - level))) & indexmask;
     descaddr &= ~7ULL;
-    nstable = extract32(tableattrs, 4, 1);
+    nstable = !regime_is_stage2(mmu_idx) && extract32(tableattrs, 4, 1);
     if (nstable) {
         /*
          * Stage2_S -> Stage2 or Phys_S -> Phys_NS
-- 
2.34.1

[PATCH 2/3] target/arm: Set ptw->out_secure correctly for stage 2 translations

[Peter Maydell]

In S1_ptw_translate(), we are in one of two situations:
 (1) translating an S1 page table walk through S2
 (2) translating an S2 page table walk through a physical regime

In case (1), ptw->in_secure indicates whether S1 is a Secure or
NonSecure translation regime.  In case (2), ptw->in_secure indicates
whether this stage 2 translation is for a Secure IPA or a NonSecure
IPA.  In particular, because of the VTCR_EL2.NSW and VSTCR_EL2.SW
bits, we can be doing a translation of a NonSecure IPA where the page
tables are in the Secure space.

Correct the setting of the ptw->out_secure value for the
regime-is-physical case:
 * it depends on whether s2_mmu_idx is Phys_S or Phys, not
   on the value of is_secure
 * we don't need to look at the VTCR_EL2.NSW and VSTCR.SW bits
   again here, as we already did that in get_phys_addr_twostage()
   to set the correct in_ptw_idx

This error doesn't currently cause a problem in itself because
we are incorrectly setting ptw->in_secure to s2walk_secure in
get_phys_addr_twostage(), but we need to fix this before we can
correct that problem.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/ptw.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 06865227642..c1e124df495 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -249,7 +249,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
             /* Regime is physical. */
             ptw->out_phys = addr;
             pte_attrs = 0;
-            pte_secure = is_secure;
+            pte_secure = s2_mmu_idx == ARMMMUIdx_Phys_S;
         }
         ptw->out_host = NULL;
         ptw->out_rw = false;
@@ -291,13 +291,15 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
             fi->s1ns = !is_secure;
             return false;
         }
+        /* Check if page table walk is to secure or non-secure PA space. */
+        ptw->out_secure = (is_secure
+                           && !(pte_secure
+                                ? env->cp15.vstcr_el2 & VSTCR_SW
+                                : env->cp15.vtcr_el2 & VTCR_NSW));
+    } else {
+        /* Regime is physical */
+        ptw->out_secure = pte_secure;
     }
-
-    /* Check if page table walk is to secure or non-secure PA space. */
-    ptw->out_secure = (is_secure
-                       && !(pte_secure
-                            ? env->cp15.vstcr_el2 & VSTCR_SW
-                            : env->cp15.vtcr_el2 & VTCR_NSW));
     ptw->out_be = regime_translation_big_endian(env, mmu_idx);
     return true;
 
-- 
2.34.1

[PATCH 3/3] target/arm: handle ipa_secure vs s2walk_secure correctly

[Peter Maydell]

In get_phys_addr_twostage() when we set up the stage 2 translation,
we currently incorrectly set all of in_mmu_idx, in_ptw_idx and
in_secure based on s2walk_secure.

Here s2walk_secure is true if we should be doing this stage 2
walk to physical memory. ipa_secure is true if the intermediate
physical address we got from stage 1 is secure. The VSTCR_EL2.SW
and VTCR_EL2.NSW bits allow the guest to configure secure EL2
so that these two things are different, eg:
 * a stage 2 walk for an NS IPA done to secure physical memory
   (where the translation table base address and other parameters
   for the walk come from the NS control registers VTTBR_EL2
   and VTCR_EL2)
 * a stage 2 walk for an S IPA done to non-secure physical memory
   (where the parameters from the walk come from the S control
   registers VSTTBR_EL2 and VSTCR_EL2)

To tell get_phys_addr_lpae() to do this, we need to pass in an
in_mmu_idx based on ipa_secure, and an in_ptw_idx based on
s2walk_secure.  The in_secure parameter follows in_mmu_idx, as usual.

Note that this change relies on fixes in the previous two commits
("Don't allow stage 2 page table walks to downgrade to NS" and
"Set ptw->out_secure correctly for stage 2 translations").

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1600
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This change also means that v8M, which also uses
get_phys_addr_twostage(), is no longer using a ptw->in_mmu_idx
calculated based on s2walk_secure, which was always a little
strange given that v8M doesn't do any kind of s2 walk, even
though it didn't produce incorrect behaviour.
---
 target/arm/ptw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index c1e124df495..2ee2b0b9241 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2741,9 +2741,9 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
     }
 
     is_el0 = ptw->in_mmu_idx == ARMMMUIdx_Stage1_E0;
-    ptw->in_mmu_idx = s2walk_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
+    ptw->in_mmu_idx = ipa_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
     ptw->in_ptw_idx = s2walk_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
-    ptw->in_secure = s2walk_secure;
+    ptw->in_secure = ipa_secure;
 
     /*
      * S1 is done, now do S2 translation.
-- 
2.34.1

Re: [PATCH 1/3] target/arm: Don't allow stage 2 page table walks to downgrade to NS

[Philippe Mathieu-Daudé]

On 14/4/23 18:04, Peter Maydell wrote:
> Bit 63 in a Table descriptor is only the NSTable bit for stage 1
> translations; in stage 2 it is RES0.  We were incorrectly looking at
> it all the time.
> 
> This causes problems if:
>   * the stage 2 table descriptor was incorrectly setting the RES0 bit
>   * we are doing a stage 2 translation in Secure address space for
>     a NonSecure stage 1 regime -- in this case we would incorrectly
>     do an immediate downgrade to NonSecure
> 
> A bug elsewhere in the code currently prevents us from getting
> to the second situation, but when we fix that it will be possible.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

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

Re: [PATCH 1/3] target/arm: Don't allow stage 2 page table walks to downgrade to NS

[Richard Henderson]

On 4/14/23 18:04, Peter Maydell wrote:
> Bit 63 in a Table descriptor is only the NSTable bit for stage 1
> translations; in stage 2 it is RES0.  We were incorrectly looking at
> it all the time.
> 
> This causes problems if:
>   * the stage 2 table descriptor was incorrectly setting the RES0 bit
>   * we are doing a stage 2 translation in Secure address space for
>     a NonSecure stage 1 regime -- in this case we would incorrectly
>     do an immediate downgrade to NonSecure
> 
> A bug elsewhere in the code currently prevents us from getting
> to the second situation, but when we fix that it will be possible.
> 
> Cc:qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/3] target/arm: Set ptw->out_secure correctly for stage 2 translations

[Richard Henderson]

On 4/14/23 18:04, Peter Maydell wrote:
> +        /* Check if page table walk is to secure or non-secure PA space. */
> +        ptw->out_secure = (is_secure
> +                           && !(pte_secure
> +                                ? env->cp15.vstcr_el2 & VSTCR_SW
> +                                : env->cp15.vtcr_el2 & VTCR_NSW));
> +    } else {
> +        /* Regime is physical */
> +        ptw->out_secure = pte_secure;

Is that last comment really correct?  I think it could still be stage1 of 2.
That said, the actual code looks correct.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 3/3] target/arm: handle ipa_secure vs s2walk_secure correctly

[Richard Henderson]

On 4/14/23 18:04, Peter Maydell wrote:
> In get_phys_addr_twostage() when we set up the stage 2 translation,
> we currently incorrectly set all of in_mmu_idx, in_ptw_idx and
> in_secure based on s2walk_secure.
> 
> Here s2walk_secure is true if we should be doing this stage 2
> walk to physical memory. ipa_secure is true if the intermediate
> physical address we got from stage 1 is secure. The VSTCR_EL2.SW
> and VTCR_EL2.NSW bits allow the guest to configure secure EL2
> so that these two things are different, eg:
>   * a stage 2 walk for an NS IPA done to secure physical memory
>     (where the translation table base address and other parameters
>     for the walk come from the NS control registers VTTBR_EL2
>     and VTCR_EL2)
>   * a stage 2 walk for an S IPA done to non-secure physical memory
>     (where the parameters from the walk come from the S control
>     registers VSTTBR_EL2 and VSTCR_EL2)
> 
> To tell get_phys_addr_lpae() to do this, we need to pass in an
> in_mmu_idx based on ipa_secure, and an in_ptw_idx based on
> s2walk_secure.  The in_secure parameter follows in_mmu_idx, as usual.
> 
> Note that this change relies on fixes in the previous two commits
> ("Don't allow stage 2 page table walks to downgrade to NS" and
> "Set ptw->out_secure correctly for stage 2 translations").
> 
> Cc:qemu-stable@nongnu.org
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1600
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> This change also means that v8M, which also uses
> get_phys_addr_twostage(), is no longer using a ptw->in_mmu_idx
> calculated based on s2walk_secure, which was always a little
> strange given that v8M doesn't do any kind of s2 walk, even
> though it didn't produce incorrect behaviour.
> ---
>   target/arm/ptw.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/3] target/arm: Set ptw->out_secure correctly for stage 2 translations

[Peter Maydell]

On Tue, 18 Apr 2023 at 12:01, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/14/23 18:04, Peter Maydell wrote:
> > +        /* Check if page table walk is to secure or non-secure PA space. */
> > +        ptw->out_secure = (is_secure
> > +                           && !(pte_secure
> > +                                ? env->cp15.vstcr_el2 & VSTCR_SW
> > +                                : env->cp15.vtcr_el2 & VTCR_NSW));
> > +    } else {
> > +        /* Regime is physical */
> > +        ptw->out_secure = pte_secure;
>
> Is that last comment really correct?  I think it could still be stage1 of 2.

I borrowed the comment from earlier in the function, in the ptw->in_debug
branch of the code, which has the same

   if (regime_is_stage2(s2_mmu_idx)) {
      ...stuff...
   } else {
      /* Regime is physical */
   }

structure as this one does after this patch. If s2_mmu_idx isn't
a stage 2 index and it's not one of the Phys indexes, what is it ?

-- PMM

Re: [PATCH 3/3] target/arm: handle ipa_secure vs s2walk_secure correctly

[Peter Maydell]

On Fri, 14 Apr 2023 at 17:04, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In get_phys_addr_twostage() when we set up the stage 2 translation,
> we currently incorrectly set all of in_mmu_idx, in_ptw_idx and
> in_secure based on s2walk_secure.
>
> Here s2walk_secure is true if we should be doing this stage 2
> walk to physical memory. ipa_secure is true if the intermediate
> physical address we got from stage 1 is secure. The VSTCR_EL2.SW
> and VTCR_EL2.NSW bits allow the guest to configure secure EL2
> so that these two things are different, eg:
>  * a stage 2 walk for an NS IPA done to secure physical memory
>    (where the translation table base address and other parameters
>    for the walk come from the NS control registers VTTBR_EL2
>    and VTCR_EL2)
>  * a stage 2 walk for an S IPA done to non-secure physical memory
>    (where the parameters from the walk come from the S control
>    registers VSTTBR_EL2 and VSTCR_EL2)
>
> To tell get_phys_addr_lpae() to do this, we need to pass in an
> in_mmu_idx based on ipa_secure, and an in_ptw_idx based on
> s2walk_secure.  The in_secure parameter follows in_mmu_idx, as usual.

Looking again at this patchset, I think these changes are right,
but we might still be missing one -- in get_phys_addr_with_struct()
when we set up the ptw struct for the stage 1 walk, don't we need
to look at NSW there also to correctly set the ptw->in_ptw_idx ?
At the moment we do that based only on is_secure. Otherwise the
S2 page table walks we do for the S1 page table walks won't
honour NSW/SW correctly, I think. (At the moment we sort of
do something with that in S1_ptw_translate(), but it looks like
only by saying "once we've done the s2 walk coerce the result
into the right address space", so we won't actually do the s2
walk itself in the right address space.)

-- PMM

Re: [PATCH 2/3] target/arm: Set ptw->out_secure correctly for stage 2 translations

[Richard Henderson]

On 4/18/23 13:30, Peter Maydell wrote:
> On Tue, 18 Apr 2023 at 12:01, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 4/14/23 18:04, Peter Maydell wrote:
>>> +        /* Check if page table walk is to secure or non-secure PA space. */
>>> +        ptw->out_secure = (is_secure
>>> +                           && !(pte_secure
>>> +                                ? env->cp15.vstcr_el2 & VSTCR_SW
>>> +                                : env->cp15.vtcr_el2 & VTCR_NSW));
>>> +    } else {
>>> +        /* Regime is physical */
>>> +        ptw->out_secure = pte_secure;
>>
>> Is that last comment really correct?  I think it could still be stage1 of 2.
> 
> I borrowed the comment from earlier in the function, in the ptw->in_debug
> branch of the code, which has the same
> 
>     if (regime_is_stage2(s2_mmu_idx)) {
>        ...stuff...
>     } else {
>        /* Regime is physical */
>     }
> 
> structure as this one does after this patch. If s2_mmu_idx isn't
> a stage 2 index and it's not one of the Phys indexes, what is it ?

Oh, right.  Nevermind.

r~