[PATCH 0/2] target/arm: Fix gdbstub for m-profile (#1421)

[PATCH 0/2] target/arm: Fix gdbstub for m-profile (#1421)

[Richard Henderson]

This will conflict with FEAT_RME patches; I'll fix that up later.


r~


Richard Henderson (2):
  Revert "target/arm: Merge regime_is_secure into get_phys_addr"
  target/arm: Fix arm_cpu_get_phys_page_attrs_debug for m-profile

 target/arm/ptw.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH 1/2] Revert "target/arm: Merge regime_is_secure into get_phys_addr"

[Richard Henderson]

This reverts commit 03bea66e7fa3af42976ceafb20512c59abf2e699,
but restore into ptw.c instead of internals.h.

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

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 2b125fff44..cb073ac477 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2926,12 +2926,9 @@ bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
                                      result, fi);
 }
 
-bool get_phys_addr(CPUARMState *env, target_ulong address,
-                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
-                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
+/* Return true if this address translation regime is secure */
+static bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
 {
-    bool is_secure;
-
     switch (mmu_idx) {
     case ARMMMUIdx_E10_0:
     case ARMMMUIdx_E10_1:
@@ -2943,16 +2940,14 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
     case ARMMMUIdx_Stage1_E1:
     case ARMMMUIdx_Stage1_E1_PAN:
     case ARMMMUIdx_E2:
-        is_secure = arm_is_secure_below_el3(env);
-        break;
+        return arm_is_secure_below_el3(env);
     case ARMMMUIdx_Stage2:
     case ARMMMUIdx_Phys_NS:
     case ARMMMUIdx_MPrivNegPri:
     case ARMMMUIdx_MUserNegPri:
     case ARMMMUIdx_MPriv:
     case ARMMMUIdx_MUser:
-        is_secure = false;
-        break;
+        return false;
     case ARMMMUIdx_E3:
     case ARMMMUIdx_Stage2_S:
     case ARMMMUIdx_Phys_S:
@@ -2960,13 +2955,18 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
     case ARMMMUIdx_MSUserNegPri:
     case ARMMMUIdx_MSPriv:
     case ARMMMUIdx_MSUser:
-        is_secure = true;
-        break;
-    default:
-        g_assert_not_reached();
+        return true;
     }
+    g_assert_not_reached();
+}
+
+bool get_phys_addr(CPUARMState *env, target_ulong address,
+                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
+                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
+{
     return get_phys_addr_with_secure(env, address, access_type, mmu_idx,
-                                     is_secure, result, fi);
+                                     regime_is_secure(env, mmu_idx),
+                                     result, fi);
 }
 
 hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
-- 
2.34.1

Re: [PATCH 1/2] Revert "target/arm: Merge regime_is_secure into get_phys_addr"

[Philippe Mathieu-Daudé]

On 21/2/23 04:41, Richard Henderson wrote:
> This reverts commit 03bea66e7fa3af42976ceafb20512c59abf2e699,
> but restore into ptw.c instead of internals.h.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/ptw.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)

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

[PATCH 2/2] target/arm: Fix arm_cpu_get_phys_page_attrs_debug for m-profile

[Richard Henderson]

M-profile is not supported by arm_is_secure, so using it as
a replacement when bypassing get_phys_addr was incorrect.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1421
Fixes: 4a35855682ce ("target/arm: Plumb debug into S1Translate")
Signed-off-by: Richard Henderson <richard.henderson@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 cb073ac477..057cc9f641 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2974,9 +2974,10 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
+    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
     S1Translate ptw = {
-        .in_mmu_idx = arm_mmu_idx(env),
-        .in_secure = arm_is_secure(env),
+        .in_mmu_idx = mmu_idx,
+        .in_secure = regime_is_secure(env, mmu_idx),
         .in_debug = true,
     };
     GetPhysAddrResult res = {};
-- 
2.34.1

Re: [PATCH 2/2] target/arm: Fix arm_cpu_get_phys_page_attrs_debug for m-profile

[Philippe Mathieu-Daudé]

On 21/2/23 04:41, Richard Henderson wrote:
> M-profile is not supported by arm_is_secure, so using it as
> a replacement when bypassing get_phys_addr was incorrect.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1421
> Fixes: 4a35855682ce ("target/arm: Plumb debug into S1Translate")
> Signed-off-by: Richard Henderson <richard.henderson@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 cb073ac477..057cc9f641 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2974,9 +2974,10 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> +    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
>       S1Translate ptw = {
> -        .in_mmu_idx = arm_mmu_idx(env),
> -        .in_secure = arm_is_secure(env),
> +        .in_mmu_idx = mmu_idx,
> +        .in_secure = regime_is_secure(env, mmu_idx),
>           .in_debug = true,
>       };
>       GetPhysAddrResult res = {};

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

Re: [PATCH 2/2] target/arm: Fix arm_cpu_get_phys_page_attrs_debug for m-profile

[Peter Maydell]

On Tue, 21 Feb 2023 at 03:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> M-profile is not supported by arm_is_secure, so using it as
> a replacement when bypassing get_phys_addr was incorrect.

That's pretty non-obvious. I think we should either
make arm_is_secure() handle M-profile[*], or else have
it assert if you try to call it for an M-profile CPU.

[*] i.e.
  if (arm_feature(env, ARM_FEATURE_M)) {
      return env->v7m.secure;
  }
at the top of the function.

If we do the latter we wouldn't need the revert in patch 1,
right? Or do you think regime_is_secure() is a better
choice of function here anyway?

thanks
-- PMM

Re: [PATCH 2/2] target/arm: Fix arm_cpu_get_phys_page_attrs_debug for m-profile

[Richard Henderson]

On 2/21/23 06:56, Peter Maydell wrote:
> On Tue, 21 Feb 2023 at 03:42, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> M-profile is not supported by arm_is_secure, so using it as
>> a replacement when bypassing get_phys_addr was incorrect.
> 
> That's pretty non-obvious. I think we should either
> make arm_is_secure() handle M-profile[*], or else have
> it assert if you try to call it for an M-profile CPU.
> 
> [*] i.e.
>    if (arm_feature(env, ARM_FEATURE_M)) {
>        return env->v7m.secure;
>    }
> at the top of the function.
> 
> If we do the latter we wouldn't need the revert in patch 1,
> right? Or do you think regime_is_secure() is a better
> choice of function here anyway?

You're absolutely right that it's surprising, as it surprised me.
I'll re-spin.

r~