[RFC PATCH 1/5] gdb/arm: Introduce control_s and control_ns registers

[RFC PATCH 1/5] gdb/arm: Introduce control_s and control_ns registers

[Tomas Vanek]

The M-profile security extension registers will be used for stack
selection in FNC_RETURN and return from secure to non-secure state.

The presence of the registers is optional.
If one or both are missing the security extension profile is accepted.
The code using the registers must check m_profile_control_[n]?s_regnum
for -1.

Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
---
 gdb/arm-tdep.c | 20 ++++++++++++++++++++
 gdb/arm-tdep.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 3105543..564ee43 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -9996,6 +9996,8 @@ enum arm_vfp_cprc_base_type
   int m_profile_psp_ns_regnum = -1;
   int m_profile_msp_s_regnum = -1;
   int m_profile_psp_s_regnum = -1;
+  int m_profile_control_s_regnum = -1;
+  int m_profile_control_ns_regnum = -1;
   int tls_regnum = 0;
 
   /* If we have an object to base this architecture on, try to determine
@@ -10473,6 +10475,22 @@ enum arm_vfp_cprc_base_type
 		}
 	      m_profile_psp_s_regnum = register_count++;
 
+	      /* Regard the control_s and control_ns registers optional,
+	       * accept the security extension feature even without them */
+	      valid_p = tdesc_numbered_register (feature, tdesc_data.get (),
+						 register_count, "control_s");
+	      if (!valid_p)
+		warning (_("M-profile secext feature is missing required register control_s."));
+	      else
+		m_profile_control_s_regnum = register_count++;
+
+	      valid_p = tdesc_numbered_register (feature, tdesc_data.get (),
+						 register_count, "control_ns");
+	      if (!valid_p)
+		warning (_("M-profile secext feature is missing required register control_ns."));
+	      else
+		m_profile_control_ns_regnum = register_count++;
+
 	      have_sec_ext = true;
 	    }
 
@@ -10553,6 +10571,8 @@ enum arm_vfp_cprc_base_type
       tdep->m_profile_psp_ns_regnum = m_profile_psp_ns_regnum;
       tdep->m_profile_msp_s_regnum = m_profile_msp_s_regnum;
       tdep->m_profile_psp_s_regnum = m_profile_psp_s_regnum;
+      tdep->m_profile_control_s_regnum = m_profile_control_s_regnum;
+      tdep->m_profile_control_ns_regnum = m_profile_control_ns_regnum;
     }
 
   arm_register_g_packet_guesses (gdbarch);
diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
index bcd7e08..11670e9 100644
--- a/gdb/arm-tdep.h
+++ b/gdb/arm-tdep.h
@@ -133,6 +133,8 @@ struct arm_gdbarch_tdep : gdbarch_tdep_base
   int m_profile_psp_ns_regnum = ARM_SP_REGNUM;	/* M-profile PSP_NS register number.  */
   int m_profile_msp_s_regnum = ARM_SP_REGNUM;	/* M-profile MSP_S register number.  */
   int m_profile_psp_s_regnum = ARM_SP_REGNUM;	/* M-profile PSP_S register number.  */
+  int m_profile_control_s_regnum = -1;		/* M-profile CONTROL_S register number.  */
+  int m_profile_control_ns_regnum = -1;		/* M-profile CONTROL_NS register number.  */
 
   int tls_regnum = 0;		/* Number of the tpidruro register.  */
 
-- 
1.9.1

[RFC PATCH 2/5] gdb/arm: PR 29716 Fix FNC_RETURN stack selection in exception unwinder

[Tomas Vanek]

Unwinding of FNC_RETURN selected the process stack whenever zero IPSR
indicated thread mode.

This does not comply
Arm v8-M Architecture Reference Manual
B3.8 Stack pointer
IDMLS "In Thread mode, CONTROL.SPSEL determines whether the PE uses
the main or process stack"

Check SPSEL bit of CONTROL_S register.

For simplicity the CONTROL_S is not tracked for changes
in the inner frames, the CONTROL_S value is passed unchanged from
the innermost frame.

Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
---
 gdb/arm-tdep.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 564ee43..4180277 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3485,13 +3485,27 @@ struct frame_unwind arm_stub_unwind = {
 	  return cache;
 	}
 
-      ULONGEST xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
-      if ((xpsr & 0x1ff) != 0)
-	/* Handler mode: This is the mode that exceptions are handled in.  */
-	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
-      else
-	/* Thread mode: This is the normal mode that programs run in.  */
-	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
+      bool spsel = true;
+      if (tdep->m_profile_control_s_regnum >= 0)
+	{
+	  ULONGEST control_s
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_control_s_regnum);
+	  spsel = (control_s & (1 << 1)) != 0;
+	}
+
+      bool s_process_stack = false;
+      if (spsel)
+	{
+	  ULONGEST xpsr = get_frame_register_unsigned (this_frame,
+						       ARM_PS_REGNUM);
+	  s_process_stack = (xpsr & 0x1ff) == 0;
+	}
+
+      arm_cache_switch_prev_sp (cache, tdep,
+				s_process_stack ?
+				tdep->m_profile_psp_s_regnum :
+				tdep->m_profile_msp_s_regnum);
 
       CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
 
-- 
1.9.1

[RFC PATCH 3/5] gdb/dwarf2: Add dwarf2_frame_reg_rule for GDB register number

[Tomas Vanek]

Arm M-profile security extension requires the dwarf2 unwinder to
copy a value from a mode specific stack pointer to the general sp.

We can achive this using DWARF2_FRAME_REG_SAVED_REG but
it is complicated as the DWARF base register number has to be filled
to reg->loc.reg (and dwarf2_frame_prev_register() in turn converts
it back to the GDB register number).

To avoid the useless conversion forth and back introduce a new item
in enum dwarf2_frame_reg_rule for GDB internal use.

DWARF2_FRAME_REG_SAVED_GDB_REG copies the value from a register
indexed by GDB number.

Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
---
 gdb/dwarf2/frame.c | 4 ++++
 gdb/dwarf2/frame.h | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
index 3f884ab..a0e554f 100644
--- a/gdb/dwarf2/frame.c
+++ b/gdb/dwarf2/frame.c
@@ -1166,6 +1166,10 @@ struct dwarf2_frame_cache
 	(gdbarch, cache->reg[regnum].loc.reg);
       return frame_unwind_got_register (this_frame, regnum, realnum);
 
+    case DWARF2_FRAME_REG_SAVED_GDB_REG:
+      realnum = cache->reg[regnum].loc.reg;
+      return frame_unwind_got_register (this_frame, regnum, realnum);
+
     case DWARF2_FRAME_REG_SAVED_EXP:
       addr = execute_stack_op (cache->reg[regnum].loc.exp.start,
 			       cache->reg[regnum].loc.exp.len,
diff --git a/gdb/dwarf2/frame.h b/gdb/dwarf2/frame.h
index 06c8a10..848e924 100644
--- a/gdb/dwarf2/frame.h
+++ b/gdb/dwarf2/frame.h
@@ -61,7 +61,10 @@ enum dwarf2_frame_reg_rule
   DWARF2_FRAME_REG_RA,		/* Return Address.  */
   DWARF2_FRAME_REG_RA_OFFSET,	/* Return Address with offset.  */
   DWARF2_FRAME_REG_CFA,		/* Call Frame Address.  */
-  DWARF2_FRAME_REG_CFA_OFFSET	/* Call Frame Address with offset.  */
+  DWARF2_FRAME_REG_CFA_OFFSET,	/* Call Frame Address with offset.  */
+  DWARF2_FRAME_REG_SAVED_GDB_REG
+    /* As DWARF2_FRAME_REG_SAVED_REG, loc.reg contains GDB register number,
+       not DWARF register number.  */
 };
 
 /* Register state.  */
-- 
1.9.1

[RFC PATCH 4/5] gdb/arm: Unwinding of secure procedure with cmse_nonsecure_entry attribute

[Tomas Vanek]

This patch depends on pending:
"gdb/arm: PR 29738 Cache value for stack pointers for dwarf2 frames"

A secure procedure with cmse_nonsecure_entry attribute is compiled with
an epilogue ending by a return to the non-secure mode:
 bxns lr

When a non-secure context called such procedure, the dwarf2 unwinder
did not know about cmse_nonsecure_entry attribute, did not see 'bxns'
at the return and therefore assumed a normal return keeping the security
state unchanged. This caused incorrect unwinding of the frames following
this one as the secure stack was used instead of non-secure.

Detect a procedure with cmse_nonsecure_entry attribute when unwinding
a secure frame. Change the security state to non-secure and use
the proper stack if the cmse_nonsecure_entry was detected.

The detection of the cmse_nonsecure_entry attribute is based on the split
secure gateway veneer and the rest of procedure with the name prefixed
by '__acle_se_'. This is documented in
https://developer.arm.com/documentation/100748/0619/Security-features-supported-in-Arm-Compiler-for-Embedded/Overview-of-building-Secure-and-Non-secure-images-with-the-Armv8-M-Security-Extension
and GCC conforms this model too.

To choose main or process non-secure stack we need xPSR and SPSEL
bit of CONTROL_NS. For simplicity CONTROL_NS is not tracked for changes
in the inner frames, the CONTROL_NS value is passed unchanged from
the innermost frame.

Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
---
 gdb/arm-tdep.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 71 insertions(+), 8 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 4180277..4fac09b 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -5125,6 +5125,7 @@ enum arm_vfp_cprc_base_type
 			   frame_info_ptr this_frame)
 {
   arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
+  bool is_sp = (regnum == ARM_SP_REGNUM);
 
   if (is_pacbti_pseudo (gdbarch, regnum))
     {
@@ -5140,12 +5141,10 @@ enum arm_vfp_cprc_base_type
       reg->how = DWARF2_FRAME_REG_FN;
       reg->loc.fn = arm_dwarf2_prev_register;
     }
-  else if (regnum == ARM_SP_REGNUM)
-    reg->how = DWARF2_FRAME_REG_CFA;
-  else if (arm_is_alternative_sp_register (tdep, regnum))
+  else if (is_sp || arm_is_alternative_sp_register (tdep, regnum))
     {
       /* Identify what stack pointers that are synced with sp.  */
-      bool override_with_sp_value = false;
+      bool override_with_sp_value = is_sp;
 
       if (tdep->have_sec_ext)
 	{
@@ -5165,24 +5164,88 @@ enum arm_vfp_cprc_base_type
 	    = get_frame_register_unsigned (this_frame,
 					   tdep->m_profile_psp_ns_regnum);
 
+	  bool is_secure = (sp == msp_s || sp == psp_s);
+	  bool return_to_ns = false;
+	  if (is_secure)
+	    {
+	      CORE_ADDR func = get_frame_func (this_frame);
+	      struct bound_minimal_symbol sym
+		= lookup_minimal_symbol_by_pc (func);
+	      if (sym.minsym)
+		{
+		  const char *name = sym.minsym->natural_name ();
+		  arm_debug_printf ("ret to ns check minsym %s", name);
+		  return_to_ns = strncmp (name, "__acle_se_", 10) == 0;
+		}
+	    }
+
+	  bool ns_process_stack = false;
+	  if (return_to_ns &&
+	      (is_sp ||
+	       regnum == tdep->m_profile_msp_s_regnum ||
+	       regnum == tdep->m_profile_psp_s_regnum))
+	    {
+	      bool spsel = true;
+
+	      if (tdep->m_profile_control_ns_regnum >= 0)
+		{
+		  ULONGEST control_ns
+		    = get_frame_register_unsigned (this_frame,
+				tdep->m_profile_control_ns_regnum);
+		  spsel = (control_ns & (1 << 1)) != 0;
+		}
+
+	      if (spsel)
+		{
+		  ULONGEST xpsr = get_frame_register_unsigned (this_frame,
+							       ARM_PS_REGNUM);
+		  ns_process_stack = (xpsr & 0x1ff) == 0;
+		}
+
+	      if (is_sp)
+		{
+		  reg->how = DWARF2_FRAME_REG_SAVED_GDB_REG;
+		  reg->loc.reg = ns_process_stack ?
+				 tdep->m_profile_psp_ns_regnum :
+				 tdep->m_profile_msp_ns_regnum;
+		  return;
+		}
+	    }
+
+	  if (return_to_ns)
+	    {
+	      if (regnum == tdep->m_profile_msp_regnum)
+		{
+		  reg->how = DWARF2_FRAME_REG_SAVED_GDB_REG;
+		  reg->loc.reg = tdep->m_profile_msp_ns_regnum;
+		  return;
+		}
+	      else if (regnum == tdep->m_profile_psp_regnum)
+		{
+		  reg->how = DWARF2_FRAME_REG_SAVED_GDB_REG;
+		  reg->loc.reg = tdep->m_profile_psp_ns_regnum;
+		  return;
+		}
+	    }
+
 	  bool is_msp = (regnum == tdep->m_profile_msp_regnum)
 	    && (msp_s == sp || msp_ns == sp);
 	  bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
-	    && (msp_s == sp);
+	    && (msp_s == sp || (return_to_ns && !ns_process_stack));
 	  bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
 	    && (msp_ns == sp);
 	  bool is_psp = (regnum == tdep->m_profile_psp_regnum)
 	    && (psp_s == sp || psp_ns == sp);
 	  bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
-	    && (psp_s == sp);
+	    && (psp_s == sp || (return_to_ns && ns_process_stack));
 	  bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
 	    && (psp_ns == sp);
 
-	  override_with_sp_value = is_msp || is_msp_s || is_msp_ns
+	  override_with_sp_value = is_sp || is_msp || is_msp_s || is_msp_ns
 	    || is_psp || is_psp_s || is_psp_ns;
 
 	}
-      else if (tdep->is_m)
+      else if (tdep->is_m && !is_sp)
 	{
 	  CORE_ADDR sp
 	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
-- 
1.9.1

[RFC PATCH 5/5] HACK frame inner than comparison for Arm M-profile sec ext

[Tomas Vanek]

!!This change is not intended to be merged to the GDB code!!

Arm M-profile can use two stacks or four stack with the security
extension. core_addr_lessthan() used in set_gdbarch_inner_than()
can break stack unwinding with a false warning
"previous frame inner to this frame (corrupt stack?)"
when the active stack is switched to another one located below
the first one.

Unfortunately the settable comparison function used
in set_gdbarch_inner_than() takes just lhs and rhs addresses as arguments.
Therefore the possibility to avoid the comparison of addresses
from two different stack is very limited or impossible.

This hack abuses the internal RAM mapping to non-secure and secure/callable
areas used in STM32L5 device to prevent false unwinding fails
when switching secure/non-secure mode.

How to solve the problem correctly? Please advise...

Add a settable value
"set arm unwind-inner-check 0"
to switch off the inner frame check on user request?

Rework frame_id_inner() and gdbarch to allow a smarter comparator
with access to frame details?

Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
---
 gdb/arm-tdep.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 4fac09b..1c683ca 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -876,6 +876,16 @@ class target_arm_instruction_reader : public arm_instruction_reader
     }
 }
 
+static int
+arm_m_sec_ext_inner_than (CORE_ADDR lhs, CORE_ADDR rhs)
+{
+  if ((lhs & 0xfff00000) == 0x20000000
+      && (rhs & 0xfff00000) == 0x30000000)
+    return false;
+
+  return (lhs < rhs);
+}
+
 /* Remove useless bits from addresses in a running program.  */
 static CORE_ADDR
 arm_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR val)
@@ -10725,7 +10735,9 @@ enum arm_vfp_cprc_base_type
   set_gdbarch_skip_trampoline_code (gdbarch, arm_skip_stub);
 
   /* The stack grows downward.  */
-  set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
+  set_gdbarch_inner_than (gdbarch, have_sec_ext ?
+				   arm_m_sec_ext_inner_than :
+				   core_addr_lessthan);
 
   /* Breakpoint manipulation.  */
   set_gdbarch_breakpoint_kind_from_pc (gdbarch, arm_breakpoint_kind_from_pc);
-- 
1.9.1

Re: [RFC PATCH 1/5] gdb/arm: Introduce control_s and control_ns registers

[Luis Machado]

Hi Tomas,

On 11/5/22 09:44, Tomas Vanek wrote:
> The M-profile security extension registers will be used for stack
> selection in FNC_RETURN and return from secure to non-secure state.
> 
> The presence of the registers is optional.
> If one or both are missing the security extension profile is accepted.
> The code using the registers must check m_profile_control_[n]?s_regnum
> for -1.
> 
> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
> ---
>   gdb/arm-tdep.c | 20 ++++++++++++++++++++
>   gdb/arm-tdep.h |  2 ++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 3105543..564ee43 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -9996,6 +9996,8 @@ enum arm_vfp_cprc_base_type
>     int m_profile_psp_ns_regnum = -1;
>     int m_profile_msp_s_regnum = -1;
>     int m_profile_psp_s_regnum = -1;
> +  int m_profile_control_s_regnum = -1;
> +  int m_profile_control_ns_regnum = -1;
>     int tls_regnum = 0;
>   
>     /* If we have an object to base this architecture on, try to determine
> @@ -10473,6 +10475,22 @@ enum arm_vfp_cprc_base_type
>   		}
>   	      m_profile_psp_s_regnum = register_count++;
>   
> +	      /* Regard the control_s and control_ns registers optional,
> +	       * accept the security extension feature even without them */
> +	      valid_p = tdesc_numbered_register (feature, tdesc_data.get (),
> +						 register_count, "control_s");

What tools are advertising control_s and control_ns? We need to document those properly if we're planning on using them
as part of decision-making in GDB.


> +	      if (!valid_p)
> +		warning (_("M-profile secext feature is missing required register control_s."));
> +	      else
> +		m_profile_control_s_regnum = register_count++;
> +
> +	      valid_p = tdesc_numbered_register (feature, tdesc_data.get (),
> +						 register_count, "control_ns");
> +	      if (!valid_p)
> +		warning (_("M-profile secext feature is missing required register control_ns."));
> +	      else
> +		m_profile_control_ns_regnum = register_count++;
> +
>   	      have_sec_ext = true;
>   	    }
>   
> @@ -10553,6 +10571,8 @@ enum arm_vfp_cprc_base_type
>         tdep->m_profile_psp_ns_regnum = m_profile_psp_ns_regnum;
>         tdep->m_profile_msp_s_regnum = m_profile_msp_s_regnum;
>         tdep->m_profile_psp_s_regnum = m_profile_psp_s_regnum;
> +      tdep->m_profile_control_s_regnum = m_profile_control_s_regnum;
> +      tdep->m_profile_control_ns_regnum = m_profile_control_ns_regnum;
>       }
>   
>     arm_register_g_packet_guesses (gdbarch);
> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
> index bcd7e08..11670e9 100644
> --- a/gdb/arm-tdep.h
> +++ b/gdb/arm-tdep.h
> @@ -133,6 +133,8 @@ struct arm_gdbarch_tdep : gdbarch_tdep_base
>     int m_profile_psp_ns_regnum = ARM_SP_REGNUM;	/* M-profile PSP_NS register number.  */
>     int m_profile_msp_s_regnum = ARM_SP_REGNUM;	/* M-profile MSP_S register number.  */
>     int m_profile_psp_s_regnum = ARM_SP_REGNUM;	/* M-profile PSP_S register number.  */
> +  int m_profile_control_s_regnum = -1;		/* M-profile CONTROL_S register number.  */
> +  int m_profile_control_ns_regnum = -1;		/* M-profile CONTROL_NS register number.  */
>   
>     int tls_regnum = 0;		/* Number of the tpidruro register.  */
>   

Re: [RFC PATCH 1/5] gdb/arm: Introduce control_s and control_ns registers

[Tomas Vanek]

Hi Luis,

On 08/11/2022 12:23, Luis Machado wrote:
> Hi Tomas,
>
> On 11/5/22 09:44, Tomas Vanek wrote:
>> The M-profile security extension registers will be used for stack
>> selection in FNC_RETURN and return from secure to non-secure state.
>>
>> The presence of the registers is optional.
>> If one or both are missing the security extension profile is accepted.
>> The code using the registers must check m_profile_control_[n]?s_regnum
>> for -1.
>>
>> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
>> ---
>>   gdb/arm-tdep.c | 20 ++++++++++++++++++++
>>   gdb/arm-tdep.h |  2 ++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index 3105543..564ee43 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -9996,6 +9996,8 @@ enum arm_vfp_cprc_base_type
>>     int m_profile_psp_ns_regnum = -1;
>>     int m_profile_msp_s_regnum = -1;
>>     int m_profile_psp_s_regnum = -1;
>> +  int m_profile_control_s_regnum = -1;
>> +  int m_profile_control_ns_regnum = -1;
>>     int tls_regnum = 0;
>>       /* If we have an object to base this architecture on, try to 
>> determine
>> @@ -10473,6 +10475,22 @@ enum arm_vfp_cprc_base_type
>>           }
>>             m_profile_psp_s_regnum = register_count++;
>>   +          /* Regard the control_s and control_ns registers optional,
>> +           * accept the security extension feature even without them */
>> +          valid_p = tdesc_numbered_register (feature, tdesc_data.get 
>> (),
>> +                         register_count, "control_s");
>
> What tools are advertising control_s and control_ns? We need to 
> document those properly if we're planning on using them
> as part of decision-making in GDB.

Good question.

OpenOCD exposes them, like other M-profile security extension registers 
they have the feature name "v8-m.sp".
There is a pending patch to change the feature name to 
"org.gnu.gdb.arm.secext"
7265: target/armv7m: fix feature name of ARMv8M security extension regs 
| https://review.openocd.org/c/openocd/+/7265

PyOCD exposes them, like other M-profile security extension registers 
they have the feature name "v8-m.sp".

ST-LINK GDB server. Version 5.6.0 (I assume not the latest version) 
exposes them (and other M-profile security extension registers too)
in the feature "org.gnu.gdb.arm.vfp"?? Obviously a bug!

I have no access to Keil tools.

Please be aware that control_s and control_ns are optional, the most of 
security extension support is functional
even in the case they are not accessible in the connected gdbserver.

>
>
>> +          if (!valid_p)
>> +        warning (_("M-profile secext feature is missing required 
>> register control_s."));
>> +          else
>> +        m_profile_control_s_regnum = register_count++;
>> +
>> +          valid_p = tdesc_numbered_register (feature, tdesc_data.get 
>> (),
>> +                         register_count, "control_ns");
>> +          if (!valid_p)
>> +        warning (_("M-profile secext feature is missing required 
>> register control_ns."));
>> +          else
>> +        m_profile_control_ns_regnum = register_count++;
>> +
>>             have_sec_ext = true;
>>           }
>>   @@ -10553,6 +10571,8 @@ enum arm_vfp_cprc_base_type
>>         tdep->m_profile_psp_ns_regnum = m_profile_psp_ns_regnum;
>>         tdep->m_profile_msp_s_regnum = m_profile_msp_s_regnum;
>>         tdep->m_profile_psp_s_regnum = m_profile_psp_s_regnum;
>> +      tdep->m_profile_control_s_regnum = m_profile_control_s_regnum;
>> +      tdep->m_profile_control_ns_regnum = m_profile_control_ns_regnum;
>>       }
>>       arm_register_g_packet_guesses (gdbarch);
>> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
>> index bcd7e08..11670e9 100644
>> --- a/gdb/arm-tdep.h
>> +++ b/gdb/arm-tdep.h
>> @@ -133,6 +133,8 @@ struct arm_gdbarch_tdep : gdbarch_tdep_base
>>     int m_profile_psp_ns_regnum = ARM_SP_REGNUM;    /* M-profile 
>> PSP_NS register number.  */
>>     int m_profile_msp_s_regnum = ARM_SP_REGNUM;    /* M-profile MSP_S 
>> register number.  */
>>     int m_profile_psp_s_regnum = ARM_SP_REGNUM;    /* M-profile PSP_S 
>> register number.  */
>> +  int m_profile_control_s_regnum = -1;        /* M-profile CONTROL_S 
>> register number.  */
>> +  int m_profile_control_ns_regnum = -1;        /* M-profile 
>> CONTROL_NS register number.  */
>>       int tls_regnum = 0;        /* Number of the tpidruro register.  */
>

Re: [RFC PATCH 1/5] gdb/arm: Introduce control_s and control_ns registers

[Tomas Vanek]

On 08/11/2022 16:52, Tomas Vanek wrote:
> Hi Luis,
>
> On 08/11/2022 12:23, Luis Machado wrote:
>> Hi Tomas,
>>
>> On 11/5/22 09:44, Tomas Vanek wrote:
>>> The M-profile security extension registers will be used for stack
>>> selection in FNC_RETURN and return from secure to non-secure state.
>>>
>>> The presence of the registers is optional.
>>> If one or both are missing the security extension profile is accepted.
>>> The code using the registers must check m_profile_control_[n]?s_regnum
>>> for -1.
>>>
>>> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
>>> ---
>>>   gdb/arm-tdep.c | 20 ++++++++++++++++++++
>>>   gdb/arm-tdep.h |  2 ++
>>>   2 files changed, 22 insertions(+)
>>>
>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>>> index 3105543..564ee43 100644
>>> --- a/gdb/arm-tdep.c
>>> +++ b/gdb/arm-tdep.c
>>> @@ -9996,6 +9996,8 @@ enum arm_vfp_cprc_base_type
>>>     int m_profile_psp_ns_regnum = -1;
>>>     int m_profile_msp_s_regnum = -1;
>>>     int m_profile_psp_s_regnum = -1;
>>> +  int m_profile_control_s_regnum = -1;
>>> +  int m_profile_control_ns_regnum = -1;
>>>     int tls_regnum = 0;
>>>       /* If we have an object to base this architecture on, try to 
>>> determine
>>> @@ -10473,6 +10475,22 @@ enum arm_vfp_cprc_base_type
>>>           }
>>>             m_profile_psp_s_regnum = register_count++;
>>>   +          /* Regard the control_s and control_ns registers optional,
>>> +           * accept the security extension feature even without 
>>> them */
>>> +          valid_p = tdesc_numbered_register (feature, 
>>> tdesc_data.get (),
>>> +                         register_count, "control_s");
>>
>> What tools are advertising control_s and control_ns? We need to 
>> document those properly if we're planning on using them
>> as part of decision-making in GDB.
>
> Good question.
>
> OpenOCD exposes them, like other M-profile security extension 
> registers they have the feature name "v8-m.sp".
> There is a pending patch to change the feature name to 
> "org.gnu.gdb.arm.secext"
> 7265: target/armv7m: fix feature name of ARMv8M security extension 
> regs | https://review.openocd.org/c/openocd/+/7265
>
> PyOCD exposes them, like other M-profile security extension registers 
> they have the feature name "v8-m.sp".

Oops, PyOCD exposes just control, no control_s and control_ns variants.

>
> ST-LINK GDB server. Version 5.6.0 (I assume not the latest version) 
> exposes them (and other M-profile security extension registers too)
> in the feature "org.gnu.gdb.arm.vfp"?? Obviously a bug!
>
> I have no access to Keil tools.
>
> Please be aware that control_s and control_ns are optional, the most 
> of security extension support is functional
> even in the case they are not accessible in the connected gdbserver.
>
>>
>>
>>> +          if (!valid_p)
>>> +        warning (_("M-profile secext feature is missing required 
>>> register control_s."));
>>> +          else
>>> +        m_profile_control_s_regnum = register_count++;
>>> +
>>> +          valid_p = tdesc_numbered_register (feature, 
>>> tdesc_data.get (),
>>> +                         register_count, "control_ns");
>>> +          if (!valid_p)
>>> +        warning (_("M-profile secext feature is missing required 
>>> register control_ns."));
>>> +          else
>>> +        m_profile_control_ns_regnum = register_count++;
>>> +
>>>             have_sec_ext = true;
>>>           }
>>>   @@ -10553,6 +10571,8 @@ enum arm_vfp_cprc_base_type
>>>         tdep->m_profile_psp_ns_regnum = m_profile_psp_ns_regnum;
>>>         tdep->m_profile_msp_s_regnum = m_profile_msp_s_regnum;
>>>         tdep->m_profile_psp_s_regnum = m_profile_psp_s_regnum;
>>> +      tdep->m_profile_control_s_regnum = m_profile_control_s_regnum;
>>> +      tdep->m_profile_control_ns_regnum = m_profile_control_ns_regnum;
>>>       }
>>>       arm_register_g_packet_guesses (gdbarch);
>>> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
>>> index bcd7e08..11670e9 100644
>>> --- a/gdb/arm-tdep.h
>>> +++ b/gdb/arm-tdep.h
>>> @@ -133,6 +133,8 @@ struct arm_gdbarch_tdep : gdbarch_tdep_base
>>>     int m_profile_psp_ns_regnum = ARM_SP_REGNUM;    /* M-profile 
>>> PSP_NS register number.  */
>>>     int m_profile_msp_s_regnum = ARM_SP_REGNUM;    /* M-profile 
>>> MSP_S register number.  */
>>>     int m_profile_psp_s_regnum = ARM_SP_REGNUM;    /* M-profile 
>>> PSP_S register number.  */
>>> +  int m_profile_control_s_regnum = -1;        /* M-profile 
>>> CONTROL_S register number.  */
>>> +  int m_profile_control_ns_regnum = -1;        /* M-profile 
>>> CONTROL_NS register number.  */
>>>       int tls_regnum = 0;        /* Number of the tpidruro 
>>> register.  */
>>
>

Re: [RFC PATCH 1/5] gdb/arm: Introduce control_s and control_ns registers

[Torbjorn SVENSSON]

On 2022-11-08 16:52, Tomas Vanek wrote:
> Hi Luis,
> 
> On 08/11/2022 12:23, Luis Machado wrote:
>> Hi Tomas,
>>
>> On 11/5/22 09:44, Tomas Vanek wrote:
>>> The M-profile security extension registers will be used for stack
>>> selection in FNC_RETURN and return from secure to non-secure state.
>>>
>>> The presence of the registers is optional.
>>> If one or both are missing the security extension profile is accepted.
>>> The code using the registers must check m_profile_control_[n]?s_regnum
>>> for -1.
>>>
>>> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
>>> ---
>>>   gdb/arm-tdep.c | 20 ++++++++++++++++++++
>>>   gdb/arm-tdep.h |  2 ++
>>>   2 files changed, 22 insertions(+)
>>>
>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>>> index 3105543..564ee43 100644
>>> --- a/gdb/arm-tdep.c
>>> +++ b/gdb/arm-tdep.c
>>> @@ -9996,6 +9996,8 @@ enum arm_vfp_cprc_base_type
>>>     int m_profile_psp_ns_regnum = -1;
>>>     int m_profile_msp_s_regnum = -1;
>>>     int m_profile_psp_s_regnum = -1;
>>> +  int m_profile_control_s_regnum = -1;
>>> +  int m_profile_control_ns_regnum = -1;
>>>     int tls_regnum = 0;
>>>       /* If we have an object to base this architecture on, try to 
>>> determine
>>> @@ -10473,6 +10475,22 @@ enum arm_vfp_cprc_base_type
>>>           }
>>>             m_profile_psp_s_regnum = register_count++;
>>>   +          /* Regard the control_s and control_ns registers optional,
>>> +           * accept the security extension feature even without them */
>>> +          valid_p = tdesc_numbered_register (feature, tdesc_data.get 
>>> (),
>>> +                         register_count, "control_s");
>>
>> What tools are advertising control_s and control_ns? We need to 
>> document those properly if we're planning on using them
>> as part of decision-making in GDB.
> 
> Good question.
> 
> OpenOCD exposes them, like other M-profile security extension registers 
> they have the feature name "v8-m.sp".
> There is a pending patch to change the feature name to 
> "org.gnu.gdb.arm.secext"
> 7265: target/armv7m: fix feature name of ARMv8M security extension regs 
> | https://review.openocd.org/c/openocd/+/7265
> 
> PyOCD exposes them, like other M-profile security extension registers 
> they have the feature name "v8-m.sp".
> 
> ST-LINK GDB server. Version 5.6.0 (I assume not the latest version) 
> exposes them (and other M-profile security extension registers too)
> in the feature "org.gnu.gdb.arm.vfp"?? Obviously a bug!

This was fixed in 5.9.0 and the latest released version i 7.0.0.

IIRC, SEGGER JLink exposes both CONTROL registers too, but I can't 
confirm that right now.

Kind regards,
Torbjörn

Re: [RFC PATCH 1/5] gdb/arm: Introduce control_s and control_ns registers

[Luis Machado]

Hi,

On 11/8/22 15:58, Tomas Vanek wrote:
> On 08/11/2022 16:52, Tomas Vanek wrote:
>> Hi Luis,
>>
>> On 08/11/2022 12:23, Luis Machado wrote:
>>> Hi Tomas,
>>>
>>> On 11/5/22 09:44, Tomas Vanek wrote:
>>>> The M-profile security extension registers will be used for stack
>>>> selection in FNC_RETURN and return from secure to non-secure state.
>>>>
>>>> The presence of the registers is optional.
>>>> If one or both are missing the security extension profile is accepted.
>>>> The code using the registers must check m_profile_control_[n]?s_regnum
>>>> for -1.
>>>>
>>>> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
>>>> ---
>>>>   gdb/arm-tdep.c | 20 ++++++++++++++++++++
>>>>   gdb/arm-tdep.h |  2 ++
>>>>   2 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>>>> index 3105543..564ee43 100644
>>>> --- a/gdb/arm-tdep.c
>>>> +++ b/gdb/arm-tdep.c
>>>> @@ -9996,6 +9996,8 @@ enum arm_vfp_cprc_base_type
>>>>     int m_profile_psp_ns_regnum = -1;
>>>>     int m_profile_msp_s_regnum = -1;
>>>>     int m_profile_psp_s_regnum = -1;
>>>> +  int m_profile_control_s_regnum = -1;
>>>> +  int m_profile_control_ns_regnum = -1;
>>>>     int tls_regnum = 0;
>>>>       /* If we have an object to base this architecture on, try to determine
>>>> @@ -10473,6 +10475,22 @@ enum arm_vfp_cprc_base_type
>>>>           }
>>>>             m_profile_psp_s_regnum = register_count++;
>>>>   +          /* Regard the control_s and control_ns registers optional,
>>>> +           * accept the security extension feature even without them */
>>>> +          valid_p = tdesc_numbered_register (feature, tdesc_data.get (),
>>>> +                         register_count, "control_s");
>>>
>>> What tools are advertising control_s and control_ns? We need to document those properly if we're planning on using them
>>> as part of decision-making in GDB.
>>
>> Good question.
>>
>> OpenOCD exposes them, like other M-profile security extension registers they have the feature name "v8-m.sp".
>> There is a pending patch to change the feature name to "org.gnu.gdb.arm.secext"
>> 7265: target/armv7m: fix feature name of ARMv8M security extension regs | https://review.openocd.org/c/openocd/+/7265
>>
>> PyOCD exposes them, like other M-profile security extension registers they have the feature name "v8-m.sp".
> 
> Oops, PyOCD exposes just control, no control_s and control_ns variants.
> 

Should we coordinate this a bit better between PyOCD and ST-LINK to expose the same set of registers. That would make it easier for
gdb to deal with it.

>>
>> ST-LINK GDB server. Version 5.6.0 (I assume not the latest version) exposes them (and other M-profile security extension registers too)
>> in the feature "org.gnu.gdb.arm.vfp"?? Obviously a bug!
>>
>> I have no access to Keil tools.
>>
>> Please be aware that control_s and control_ns are optional, the most of security extension support is functional
>> even in the case they are not accessible in the connected gdbserver.
>>
>>>
>>>
>>>> +          if (!valid_p)
>>>> +        warning (_("M-profile secext feature is missing required register control_s."));
>>>> +          else
>>>> +        m_profile_control_s_regnum = register_count++;
>>>> +
>>>> +          valid_p = tdesc_numbered_register (feature, tdesc_data.get (),
>>>> +                         register_count, "control_ns");
>>>> +          if (!valid_p)
>>>> +        warning (_("M-profile secext feature is missing required register control_ns."));
>>>> +          else
>>>> +        m_profile_control_ns_regnum = register_count++;
>>>> +
>>>>             have_sec_ext = true;
>>>>           }
>>>>   @@ -10553,6 +10571,8 @@ enum arm_vfp_cprc_base_type
>>>>         tdep->m_profile_psp_ns_regnum = m_profile_psp_ns_regnum;
>>>>         tdep->m_profile_msp_s_regnum = m_profile_msp_s_regnum;
>>>>         tdep->m_profile_psp_s_regnum = m_profile_psp_s_regnum;
>>>> +      tdep->m_profile_control_s_regnum = m_profile_control_s_regnum;
>>>> +      tdep->m_profile_control_ns_regnum = m_profile_control_ns_regnum;
>>>>       }
>>>>       arm_register_g_packet_guesses (gdbarch);
>>>> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
>>>> index bcd7e08..11670e9 100644
>>>> --- a/gdb/arm-tdep.h
>>>> +++ b/gdb/arm-tdep.h
>>>> @@ -133,6 +133,8 @@ struct arm_gdbarch_tdep : gdbarch_tdep_base
>>>>     int m_profile_psp_ns_regnum = ARM_SP_REGNUM;    /* M-profile PSP_NS register number.  */
>>>>     int m_profile_msp_s_regnum = ARM_SP_REGNUM;    /* M-profile MSP_S register number.  */
>>>>     int m_profile_psp_s_regnum = ARM_SP_REGNUM;    /* M-profile PSP_S register number.  */
>>>> +  int m_profile_control_s_regnum = -1;        /* M-profile CONTROL_S register number.  */
>>>> +  int m_profile_control_ns_regnum = -1;        /* M-profile CONTROL_NS register number.  */
>>>>       int tls_regnum = 0;        /* Number of the tpidruro register.  */
>>>
>>
> 

Re: [RFC PATCH 2/5] gdb/arm: PR 29716 Fix FNC_RETURN stack selection in exception unwinder

[Luis Machado]

Hi Tomas,

I'd like to address the performance regression first before we get to these additional improvements.

I've been discussing PR 29738 with Torbjörn and have provided some feedback.

On 11/5/22 09:44, Tomas Vanek wrote:
> Unwinding of FNC_RETURN selected the process stack whenever zero IPSR
> indicated thread mode.
> 
> This does not comply
> Arm v8-M Architecture Reference Manual
> B3.8 Stack pointer
> IDMLS "In Thread mode, CONTROL.SPSEL determines whether the PE uses
> the main or process stack"
> 
> Check SPSEL bit of CONTROL_S register.
> 
> For simplicity the CONTROL_S is not tracked for changes
> in the inner frames, the CONTROL_S value is passed unchanged from
> the innermost frame.
> 
> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
> ---
>   gdb/arm-tdep.c | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 564ee43..4180277 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3485,13 +3485,27 @@ struct frame_unwind arm_stub_unwind = {
>   	  return cache;
>   	}
>   
> -      ULONGEST xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
> -      if ((xpsr & 0x1ff) != 0)
> -	/* Handler mode: This is the mode that exceptions are handled in.  */
> -	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
> -      else
> -	/* Thread mode: This is the normal mode that programs run in.  */
> -	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
> +      bool spsel = true;
> +      if (tdep->m_profile_control_s_regnum >= 0)
> +	{
> +	  ULONGEST control_s
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_control_s_regnum);
> +	  spsel = (control_s & (1 << 1)) != 0;
> +	}
> +
> +      bool s_process_stack = false;
> +      if (spsel)
> +	{
> +	  ULONGEST xpsr = get_frame_register_unsigned (this_frame,
> +						       ARM_PS_REGNUM);
> +	  s_process_stack = (xpsr & 0x1ff) == 0;
> +	}
> +
> +      arm_cache_switch_prev_sp (cache, tdep,
> +				s_process_stack ?
> +				tdep->m_profile_psp_s_regnum :
> +				tdep->m_profile_msp_s_regnum);
>   
>         CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>