armv7/8m: fix regresion from apache#14881

reason:
svc call may trigger hardfault

Background
    The origin of this issue is our desire to eliminate the function of storing
"regs" in g_current_regs and instead utilize (*running_task)->xcp.regs for storage.
The benefits of this approach include faster storage speed and
avoiding multiple accesses to g_current_regs during context switching,
thus ensuring that whether returning from an interrupt or an exception,
we consistently use this_task()->xcp.regs

Issue Encountered
    However, when storing registers, we must ensure that (running_task)->xcp.regs is invalid
so that it can be safely overwritten.
According to the existing logic, the only scenario where (running_task)->xcp.regs
is valid is during restore_context. We must accurately identify this scenario.
Initially, we used the condition (running_task)==NULL for this purpose, but we deemed
this approach unsatisfactory as it did not align well with the actual logic.
(running_task) should not be NULL. Consequently, we adopted other arch-specific methods for judgment,
but due to special logic in some arch, the judgment was not accurate, leading to this issue.

Solution:
    For armv6-m, we haven't found a more suitable solution, so we are sticking with (*running_task)==NULL.
    For armv7-m/armv8-m, by removing support for primask, we can achieve accurate judgment.

    PRIMASK is a bad design in armv6-m, that's why arm introduce BASEPRI from armv7-m.
It's wrong to provide this option for armv7-m/armv8-m arch.

Signed-off-by: hujun5 <[email protected]>

arch/Kconfig

@@ -1279,11 +1279,6 @@ config ARCH_HIPRI_INTERRUPT
 		is extended to any other family, then this discussion will have to
 		be generalized.
 
-		If ARMV7M_USEBASEPRI is selected, then interrupts will be disabled
-		by setting the BASEPRI register to NVIC_SYSH_DISABLE_PRIORITY so
-		that most interrupts will not have execution priority.  SVCall must
-		have execution priority in all cases.
-
 		In the normal cases, interrupts are not nest-able and all interrupts
 		run at an execution priority between NVIC_SYSH_PRIORITY_MIN and
 		NVIC_SYSH_PRIORITY_MAX (with NVIC_SYSH_PRIORITY_MAX reserved for

arch/arm/include/armv7-m/irq.h

@@ -65,11 +65,7 @@
  */
 
 #define REG_R13             (0)  /* R13 = SP at time of interrupt */
-#ifdef CONFIG_ARMV7M_USEBASEPRI
-#  define REG_BASEPRI       (1)  /* BASEPRI */
-#else
-#  define REG_PRIMASK       (1)  /* PRIMASK */
-#endif
+#define REG_BASEPRI         (1)  /* BASEPRI */
 #define REG_R4              (2)  /* R4 */
 #define REG_R5              (3)  /* R5 */
 #define REG_R6              (4)  /* R6 */
@@ -385,13 +381,9 @@ static inline void raisebasepri(uint32_t basepri)
 static inline void up_irq_disable(void) always_inline_function;
 static inline void up_irq_disable(void)
 {
-#ifdef CONFIG_ARMV7M_USEBASEPRI
   /* Probably raising priority */
 
   raisebasepri(NVIC_SYSH_DISABLE_PRIORITY);
-#else
-  __asm__ __volatile__ ("\tcpsid  i\n");
-#endif
 }
 
 /* Save the current primask state & disable IRQs */
@@ -400,31 +392,11 @@ static inline irqstate_t up_irq_save(void)
 always_inline_function noinstrument_function;
 static inline irqstate_t up_irq_save(void)
 {
-#ifdef CONFIG_ARMV7M_USEBASEPRI
   /* Probably raising priority */
 
   uint8_t basepri = getbasepri();
   raisebasepri(NVIC_SYSH_DISABLE_PRIORITY);
   return (irqstate_t)basepri;
-
-#else
-
-  unsigned short primask;
-
-  /* Return the current value of primask register and set
-   * bit 0 of the primask register to disable interrupts
-   */
-
-  __asm__ __volatile__
-    (
-     "\tmrs    %0, primask\n"
-     "\tcpsid  i\n"
-     : "=r" (primask)
-     :
-     : "memory");
-
-  return primask;
-#endif
 }
 
 /* Enable IRQs */
@@ -444,27 +416,9 @@ static inline void up_irq_restore(irqstate_t flags)
 always_inline_function noinstrument_function;
 static inline void up_irq_restore(irqstate_t flags)
 {
-#ifdef CONFIG_ARMV7M_USEBASEPRI
   /* In this case, we are always retaining or lowering the priority value */
 
   setbasepri((uint32_t)flags);
-
-#else
-  /* If bit 0 of the primask is 0, then we need to restore
-   * interrupts.
-   */
-
-  __asm__ __volatile__
-    (
-      "\ttst    %0, #1\n"
-      "\tbne.n  1f\n"
-      "\tcpsie  i\n"
-      "1:\n"
-      :
-      : "r" (flags)
-      : "cc", "memory");
-
-#endif
 }
 
 /* Get/set IPSR */

arch/arm/include/armv7-m/nvicpri.h

@@ -33,12 +33,7 @@
  * Pre-processor Prototypes
  ****************************************************************************/
 
-/* If CONFIG_ARMV7M_USEBASEPRI is selected, then interrupts will be disabled
- * by setting the BASEPRI register to NVIC_SYSH_DISABLE_PRIORITY so that most
- * interrupts will not have execution priority.  SVCall must have execution
- * priority in all cases.
- *
- * In the normal cases, interrupts are not nest-able and all interrupts run
+/* In the normal cases, interrupts are not nest-able and all interrupts run
  * at an execution priority between NVIC_SYSH_PRIORITY_MIN and
  * NVIC_SYSH_PRIORITY_MAX (with NVIC_SYSH_PRIORITY_MAX reserved for SVCall).
  *

arch/arm/include/armv8-m/irq.h

@@ -65,11 +65,7 @@
  */
 
 #define REG_R13             (0)  /* R13 = SP at time of interrupt */
-#ifdef CONFIG_ARMV8M_USEBASEPRI
-#  define REG_BASEPRI       (1)  /* BASEPRI */
-#else
-#  define REG_PRIMASK       (1)  /* PRIMASK */
-#endif
+#define REG_BASEPRI         (1)  /* BASEPRI */
 #define REG_R4              (2)  /* R4 */
 #define REG_R5              (3)  /* R5 */
 #define REG_R6              (4)  /* R6 */
@@ -358,13 +354,9 @@ static inline void setbasepri(uint32_t basepri)
 static inline void up_irq_disable(void) always_inline_function;
 static inline void up_irq_disable(void)
 {
-#ifdef CONFIG_ARMV8M_USEBASEPRI
   /* Probably raising priority */
 
   raisebasepri(NVIC_SYSH_DISABLE_PRIORITY);
-#else
-  __asm__ __volatile__ ("\tcpsid  i\n");
-#endif
 }
 
 /* Save the current primask state & disable IRQs */
@@ -373,31 +365,11 @@ static inline irqstate_t up_irq_save(void)
 always_inline_function noinstrument_function;
 static inline irqstate_t up_irq_save(void)
 {
-#ifdef CONFIG_ARMV8M_USEBASEPRI
   /* Probably raising priority */
 
   uint8_t basepri = getbasepri();
   raisebasepri(NVIC_SYSH_DISABLE_PRIORITY);
   return (irqstate_t)basepri;
-
-#else
-
-  unsigned short primask;
-
-  /* Return the current value of primask register and set
-   * bit 0 of the primask register to disable interrupts
-   */
-
-  __asm__ __volatile__
-    (
-     "\tmrs    %0, primask\n"
-     "\tcpsid  i\n"
-     : "=r" (primask)
-     :
-     : "memory");
-
-  return primask;
-#endif
 }
 
 /* Enable IRQs */
@@ -417,27 +389,9 @@ static inline void up_irq_restore(irqstate_t flags)
 always_inline_function noinstrument_function;
 static inline void up_irq_restore(irqstate_t flags)
 {
-#ifdef CONFIG_ARMV8M_USEBASEPRI
   /* In this case, we are always retaining or lowering the priority value */
 
   setbasepri((uint32_t)flags);
-
-#else
-  /* If bit 0 of the primask is 0, then we need to restore
-   * interrupts.
-   */
-
-  __asm__ __volatile__
-    (
-      "\ttst    %0, #1\n"
-      "\tbne.n  1f\n"
-      "\tcpsie  i\n"
-      "1:\n"
-      :
-      : "r" (flags)
-      : "cc", "memory");
-
-#endif
 }
 
 /* Get/set IPSR */

arch/arm/include/armv8-m/nvicpri.h

@@ -33,12 +33,7 @@
  * Pre-processor Prototypes
  ****************************************************************************/
 
-/* If CONFIG_ARMV8M_USEBASEPRI is selected, then interrupts will be disabled
- * by setting the BASEPRI register to NVIC_SYSH_DISABLE_PRIORITY so that most
- * interrupts will not have execution priority.  SVCall must have execution
- * priority in all cases.
- *
- * In the normal cases, interrupts are not nest-able and all interrupts run
+/* In the normal cases, interrupts are not nest-able and all interrupts run
  * at an execution priority between NVIC_SYSH_PRIORITY_MIN and
  * NVIC_SYSH_PRIORITY_MAX (with NVIC_SYSH_PRIORITY_MAX reserved for SVCall).
  *

arch/arm/include/types.h

@@ -112,7 +112,7 @@ typedef unsigned int            _size_t;
  */
 
 #ifdef __thumb2__
-#if defined(CONFIG_ARMV7M_USEBASEPRI) || defined(CONFIG_ARCH_ARMV6M) || defined(CONFIG_ARMV8M_USEBASEPRI)
+#if defined(CONFIG_ARCH_ARMV6M) || defined(CONFIG_ARCH_ARMV7M) || defined(CONFIG_ARCH_ARMV8M)
 typedef unsigned char           irqstate_t;
 #else
 typedef unsigned short          irqstate_t;

arch/arm/src/armv7-m/Kconfig

@@ -13,30 +13,10 @@ config ARMV7M_HAVE_DCACHE
 	bool
 	default n
 
-config ARMV7M_USEBASEPRI
-	bool "Use BASEPRI Register"
-	default ARCH_HIPRI_INTERRUPT
-	depends on ARCH_CORTEXM3 || ARCH_CORTEXM4 || ARCH_CORTEXM7
-	---help---
-		Use the BASEPRI register to enable and disable interrupts. By
-		default, the PRIMASK register is used for this purpose. This
-		usually results in hardfaults when supervisor calls are made.
-		Though, these hardfaults are properly handled by the RTOS, the
-		hardfaults can confuse some debuggers. With the BASEPRI
-		register, these hardfaults, will be avoided. For more details see
-		https://cwiki.apache.org/confluence/display/NUTTX/ARMv7-M+Hardfaults%2C+SVCALL%2C+and+Debuggers
-
-		WARNING:  If CONFIG_ARCH_HIPRI_INTERRUPT is selected, then you
-		MUST select CONFIG_ARMV7M_USEBASEPRI.  The Kconfig dependencies
-		here will permit to select an invalid configuration because it
-		cannot enforce that requirement.  If you create this invalid
-		configuration, you will encounter some problems that may be
-		very difficult to debug.
-
 config ARMV7M_BASEPRI_WAR
 	bool "Cortex-M7 r0p1 Errata 837070 Workaround"
 	default n
-	depends on ARMV7M_USEBASEPRI && ARCH_CORTEXM7
+	depends on ARCH_CORTEXM7
 	---help---
 		Enable workaround for r0p1 Errata 837070: Increasing priority using
 		write to BASEPRI does not take effect immediately.

arch/arm/src/armv7-m/arm_exception.S

@@ -71,14 +71,6 @@
 
 #  if defined(CONFIG_BUILD_PROTECTED) && CONFIG_ARCH_INTERRUPTSTACK < 8
 #    error Interrupt stack must be used with high priority interrupts in protected mode
-#  endif
-
-  /* Use the BASEPRI to control interrupts is required if nested, high
-   * priority interrupts are supported.
-   */
-
-#  ifndef CONFIG_ARMV7M_USEBASEPRI
-#    error CONFIG_ARMV7M_USEBASEPRI must be used with CONFIG_ARCH_HIPRI_INTERRUPT
 #  endif
 #endif
 
@@ -151,11 +143,8 @@ exception_common:
 	mov		r2, r1					/* R2=Copy of the main/process stack pointer */
 	add		r2, #HW_XCPT_SIZE			/* R2=MSP/PSP before the interrupt was taken */
 								/* (ignoring the xPSR[9] alignment bit) */
-#ifdef CONFIG_ARMV7M_USEBASEPRI
+
 	mrs		r3, basepri				/* R3=Current BASEPRI setting */
-#else
-	mrs		r3, primask				/* R3=Current PRIMASK setting */
-#endif
 
 #ifdef CONFIG_ARCH_FPU
 
@@ -237,11 +226,7 @@ exception_common:
 
 	/* Restore the interrupt state */
 
-#ifdef CONFIG_ARMV7M_USEBASEPRI
 	msr		basepri, r3				/* Restore interrupts priority masking */
-#else
-	msr		primask, r3				/* Restore interrupts */
-#endif
 
 	msr		control, r12				/* Restore control */
 

arch/arm/src/armv7-m/arm_hardfault.c

@@ -42,10 +42,6 @@
  * Pre-processor Definitions
  ****************************************************************************/
 
-/* If CONFIG_ARMV7M_USEBASEPRI=n, then debug output from this file may
- * interfere with context switching!
- */
-
 #ifdef CONFIG_DEBUG_HARDFAULT_ALERT
 #  define hfalert(format, ...) _alert(format, ##__VA_ARGS__)
 #else
@@ -79,50 +75,7 @@ int arm_hardfault(int irq, void *context, void *arg)
   uint32_t cfsr = getreg32(NVIC_CFAULTS);
 
   UNUSED(cfsr);
-
-  /* Get the value of the program counter where the fault occurred */
-
-#ifndef CONFIG_ARMV7M_USEBASEPRI
-  uint32_t *regs = (uint32_t *)context;
-  uint16_t *pc = (uint16_t *)regs[REG_PC] - 1;
-
-  /* Check if the pc lies in known FLASH memory.
-   * REVISIT:  What if the PC lies in "unknown" external memory?  Best
-   * use the BASEPRI register if you have external memory.
-   */
-
-#ifdef CONFIG_BUILD_PROTECTED
-  /* In the kernel build, SVCalls are expected in either the base, kernel
-   * FLASH region or in the user FLASH region.
-   */
-
-  if (((uintptr_t)pc >= (uintptr_t)_START_TEXT &&
-       (uintptr_t)pc <  (uintptr_t)_END_TEXT) ||
-      ((uintptr_t)pc >= (uintptr_t)USERSPACE->us_textstart &&
-       (uintptr_t)pc <  (uintptr_t)USERSPACE->us_textend))
-#else
-  /* SVCalls are expected only from the base, kernel FLASH region */
-
-  if ((uintptr_t)pc >= (uintptr_t)_START_TEXT &&
-      (uintptr_t)pc <  (uintptr_t)_END_TEXT)
-#endif
-    {
-      /* Fetch the instruction that caused the Hard fault */
-
-      uint16_t insn = *pc;
-      hfinfo("  PC: %p INSN: %04x\n", pc, insn);
-
-      /* If this was the instruction 'svc 0', then forward processing
-       * to the SVCall handler
-       */
-
-      if (insn == INSN_SVC0)
-        {
-          hfinfo("Forward SVCall\n");
-          return arm_svcall(irq, context, arg);
-        }
-    }
-#endif
+  UNUSED(hfsr);
 
   if (hfsr & NVIC_HFAULTS_FORCED)
     {

arch/arm/src/armv7-m/arm_initialstate.c

@@ -161,19 +161,9 @@ void up_initial_state(struct tcb_s *tcb)
   /* Enable or disable interrupts, based on user configuration */
 
 #ifdef CONFIG_SUPPRESS_INTERRUPTS
-
-#ifdef CONFIG_ARMV7M_USEBASEPRI
   xcp->regs[REG_BASEPRI] = NVIC_SYSH_DISABLE_PRIORITY;
-#else
-  xcp->regs[REG_PRIMASK] = 1;
-#endif
-
 #else /* CONFIG_SUPPRESS_INTERRUPTS */
-
-#ifdef CONFIG_ARMV7M_USEBASEPRI
   xcp->regs[REG_BASEPRI] = 0;
-#endif
-
 #endif /* CONFIG_SUPPRESS_INTERRUPTS */
 }
 

arch/arm/src/armv7-m/arm_saveusercontext.S

@@ -91,11 +91,7 @@ up_saveusercontext:
 	/* Save r13, primask, r4~r11 */
 
 	mov		r2, sp
-#ifdef CONFIG_ARMV7M_USEBASEPRI
 	mrs		r3, basepri
-#else
-	mrs		r3, primask
-#endif
 	stmia		r0!, {r2-r11}
 
 	/* Save EXC_RETURN to 0xffffffff */