This patch fixes a bug in the memory translation for target/arm.
If in realm space, e.g., R-EL2 executing code from an executable 
memory page currently results in a level 3 permission fault. 
As we cannot access secure memory from an insecure space, 
QEMU checks on each memory translation if the in_space is secure va 
!ptw->in_secure. 
If this is the case we always set the NS bit in the memory attributes
to prevent ever reading secure memory from an insecure space.
This collides with FEAT_RME, since if the system is in realm space,
!ptw->in_secure also applies, and thus QEMU sets the NS bit, 
meaning that the memory will be translated into insecure space.
Fetching the instruction from this memory space leads to a fault, 
as you cannot execute NS instructions from a realm context.
To fix this we introduce the ptw->in_realm variable mirroring the
behavior for in_secure and only set the NS bit if both do not apply.

Signed-off-by: Matti Schulze <matti.schulze@fau.de>
---
 target/arm/cpu.h |  6 ++++++
 target/arm/ptw.c | 17 +++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 88e5accda6..ff7f8f511d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2436,6 +2436,12 @@ static inline bool arm_space_is_secure(ARMSecuritySpace space)
     return space == ARMSS_Secure || space == ARMSS_Root;
 }
 
+/* Return true if @space is Realm space */
+static inline bool arm_space_is_realm(ARMSecuritySpace space)
+{
+    return space == ARMSS_Realm;
+}
+
 /* Return the ARMSecuritySpace for @secure, assuming !RME or EL[0-2]. */
 static inline ARMSecuritySpace arm_secure_to_space(bool secure)
 {
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 8f94100c61..db1b5a7fbd 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -58,6 +58,13 @@ typedef struct S1Translate {
      * this field is updated accordingly.
      */
     bool in_secure;
+    /*
+     * in_realm: whether the translation regime is Realm
+     * This is always requal to arm_space_in_realm(in_space).
+     * If a Realm ptw is "downgraded" to a NonSecure by an NSTable bit
+     * this field is updated accordingly.
+     */
+    bool in_realm;
     /*
      * in_debug: is this a QEMU debug access (gdbstub, etc)? Debug
      * accesses will not update the guest page table access flags
@@ -535,6 +542,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
             .in_mmu_idx = s2_mmu_idx,
             .in_ptw_idx = ptw_idx_for_stage_2(env, s2_mmu_idx),
             .in_secure = arm_space_is_secure(s2_space),
+            .in_realm = arm_space_is_realm(s2_space),
             .in_space = s2_space,
             .in_debug = true,
         };
@@ -724,7 +732,7 @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t old_val,
             fi->s2addr = ptw->out_virt;
             fi->stage2 = true;
             fi->s1ptw = true;
-            fi->s1ns = !ptw->in_secure;
+            fi->s1ns = !ptw->in_secure && !ptw->in_realm;
             return 0;
         }
 
@@ -1767,6 +1775,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
         QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2_S + 1 != ARMMMUIdx_Stage2);
         ptw->in_ptw_idx += 1;
         ptw->in_secure = false;
+        ptw->in_realm = false;
         ptw->in_space = ARMSS_NonSecure;
     }
 
@@ -1872,7 +1881,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
      */
     attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50, 14));
     if (!regime_is_stage2(mmu_idx)) {
-        attrs |= !ptw->in_secure << 5; /* NS */
+        attrs |= (!ptw->in_secure && !ptw->in_realm) << 5; /* NS */
         if (!param.hpd) {
             attrs |= extract64(tableattrs, 0, 2) << 53;     /* XN, PXN */
             /*
@@ -3139,6 +3148,7 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
     ptw->in_mmu_idx = ipa_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
     ptw->in_secure = ipa_secure;
     ptw->in_space = ipa_space;
+    ptw->in_realm = arm_space_is_realm(ipa_space);
     ptw->in_ptw_idx = ptw_idx_for_stage_2(env, ptw->in_mmu_idx);
 
     /*
@@ -3371,6 +3381,7 @@ bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
     S1Translate ptw = {
         .in_mmu_idx = mmu_idx,
         .in_secure = is_secure,
+        .in_realm = false,
         .in_space = arm_secure_to_space(is_secure),
     };
     return get_phys_addr_gpc(env, &ptw, address, access_type, result, fi);
@@ -3443,6 +3454,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
 
     ptw.in_space = ss;
     ptw.in_secure = arm_space_is_secure(ss);
+    ptw.in_realm = arm_space_is_realm(ss);
     return get_phys_addr_gpc(env, &ptw, address, access_type, result, fi);
 }
 
@@ -3457,6 +3469,7 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
         .in_mmu_idx = mmu_idx,
         .in_space = ss,
         .in_secure = arm_space_is_secure(ss),
+        .in_realm = arm_space_is_realm(ss),
         .in_debug = true,
     };
     GetPhysAddrResult res = {};
-- 
2.41.0

On Tue, 22 Aug 2023 at 17:18, Matti Schulze <matti.schulze@fau.de> wrote:
>
> This patch fixes a bug in the memory translation for target/arm.
> If in realm space, e.g., R-EL2 executing code from an executable
> memory page currently results in a level 3 permission fault.
> As we cannot access secure memory from an insecure space,
> QEMU checks on each memory translation if the in_space is secure va
> !ptw->in_secure.
> If this is the case we always set the NS bit in the memory attributes
> to prevent ever reading secure memory from an insecure space.
> This collides with FEAT_RME, since if the system is in realm space,
> !ptw->in_secure also applies, and thus QEMU sets the NS bit,
> meaning that the memory will be translated into insecure space.

In the patch series
https://patchew.org/QEMU/20230714154648.327466-1-peter.maydell@linaro.org/
(which is just waiting for 8.1 to formally be released before
it gets applied) the ptw->in_secure field is removed entirely
in favour of only ever looking at ptw->in_space (among other
reasons, because it fixes this kind of bug).

There are also some other realm-related bugs which we noticed
but which again are pending 8.1 release before they go into
the tree. If you're playing with the realm support, I would
suggest you start with this branch for the moment:
 https://git.linaro.org/people/peter.maydell/qemu-arm.git/log/?h=target-arm-for-8.2
and see whether the bugs you're trying to fix are still
present there. (Don't use that branch indefinitely, though --
by next week I expect it to have merged into upstream and
then it will just be stale.)

thanks
-- PMM