The computation of s1ns was simply wrong. For Stage 2 faults, it
should indicate whether the faulting IPA is in the Non-Secure IPA
space. Correct the logic to check for ARMSS_NonSecure and drop the
extraneous s2_mmu_idx test. The actual writing of HPFAR_EL2.NS is
already gated by arm_is_secure_below_el3(env).

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/2568
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---
v2
  - simplified commit message
  - added r-b
---
 target/arm/ptw.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index d5b2ed6c99c..57a413b468e 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -631,12 +631,11 @@ static ARMSecuritySpace S2_security_space(ARMSecuritySpace s1_space,
 static bool fault_s1ns(ARMSecuritySpace space, ARMMMUIdx s2_mmu_idx)
 {
     /*
-     * For stage 2 faults in Secure EL22, S1NS indicates
-     * whether the faulting IPA is in the Secure or NonSecure
-     * IPA space. For all other kinds of fault, it is false.
+     * For stage 2 faults, S1NS indicates whether the faulting IPA is
+     * in the Non-Secure (true) or Secure (false) IPA space. For all
+     * other kinds of fault, it is false.
      */
-    return space == ARMSS_Secure && regime_is_stage2(s2_mmu_idx)
-        && s2_mmu_idx == ARMMMUIdx_Stage2_S;
+    return space == ARMSS_NonSecure && regime_is_stage2(s2_mmu_idx);
 }
 
 /* Translate a S1 pagetable walk through S2 if needed.  */
-- 
2.47.3

On Sun, 5 Apr 2026 at 12:24, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The computation of s1ns was simply wrong. For Stage 2 faults, it
> should indicate whether the faulting IPA is in the Non-Secure IPA
> space. Correct the logic to check for ARMSS_NonSecure and drop the
> extraneous s2_mmu_idx test. The actual writing of HPFAR_EL2.NS is
> already gated by arm_is_secure_below_el3(env).
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/2568
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

This seems worth Cc: stable.

This is correct code-wise but we also need to update some
comments elsewhere. I'm going to squash this in:

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 8ec2750847..9684506592 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -740,7 +740,10 @@ typedef enum ARMGPCF {
  * @paddr_space: physical address space that caused a fault for gpc
  * @stage2: True if we faulted at stage 2
  * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
- * @s1ns: True if we faulted on a non-secure IPA while in secure state
+ * @s1ns: True if we faulted on a non-secure IPA. Note that (unlike the
+ * HPFAR_EL2.NS bit) this is set for any stage 2 fault for an NS IPA, so
+ * code must check that this is for a fault taken to Secure EL2 before
+ * propagating s1ns to HPFAR_EL2.NS.
  * @ea: True if we should set the EA (external abort type) bit in syndrome
  */
 typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index bbbfc34f6c..7b993bb5b3 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -613,7 +613,10 @@ static bool fault_s1ns(ARMSecuritySpace space,
ARMMMUIdx s2_mmu_idx)
     /*
      * For stage 2 faults, S1NS indicates whether the faulting IPA is
      * in the Non-Secure (true) or Secure (false) IPA space. For all
-     * other kinds of fault, it is false.
+     * other kinds of fault, it is false. Note that we do not
+     * distinguish "s2 fault on NS IPA taken to Secure EL2" from
+     * "s2 fault on NS IPA taken to NS EL2 or Realm EL2" here, but
+     * instead do that when setting HPFAR_EL2.NS.
      */
     return space == ARMSS_NonSecure && regime_is_stage2(s2_mmu_idx);
 }

thanks
-- PMM