Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Armv8-M (except Cortex-M23) interrupt priority checking #673

Merged
merged 4 commits into from
May 11, 2023
Merged

Armv8-M (except Cortex-M23) interrupt priority checking #673

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1429841
Armv8-M: Formatting changes
urutva May 8, 2023
3824fae
Armv8-M: Add support for interrupt priority check
urutva Apr 26, 2023
3adb1e4
Use SHPR2 for calculating interrupt priority bits
aggarg May 11, 2023
9768c7c
Merge branch 'main' into armv8-m-interrupt-priority-checking
aggarg May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 187 additions & 0 deletions portable/ARMv8M/non_secure/port.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,17 @@
#define portSCB_MEM_FAULT_ENABLE_BIT ( 1UL << 16UL )
/*-----------------------------------------------------------*/

/* Constants required to check the validity of an interrupt priority. */
#define portFIRST_USER_INTERRUPT_NUMBER ( 16 )
#define portNVIC_IP_REGISTERS_OFFSET_16 ( 0xE000E3F0 )
#define portAIRCR_REG ( *( ( volatile uint32_t * ) 0xE000ED0C ) )
#define portMAX_8_BIT_VALUE ( ( uint8_t ) 0xff )
#define portTOP_BIT_OF_BYTE ( ( uint8_t ) 0x80 )
#define portMAX_PRIGROUP_BITS ( ( uint8_t ) 7 )
#define portPRIORITY_GROUP_MASK ( 0x07UL << 8UL )
#define portPRIGROUP_SHIFT ( 8UL )
/*-----------------------------------------------------------*/

/**
* @brief Constants required to manipulate the FPU.
*/
Expand Down Expand Up @@ -369,6 +380,17 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
PRIVILEGED_DATA portDONT_DISCARD volatile SecureContextHandle_t xSecureContext = portNO_SECURE_CONTEXT;
#endif /* configENABLE_TRUSTZONE */

/**
* @brief Used by the portASSERT_IF_INTERRUPT_PRIORITY_INVALID() macro to ensure
* FreeRTOS API functions are not called from interrupts that have been assigned
* a priority above configMAX_SYSCALL_INTERRUPT_PRIORITY.
*/
#if ( configASSERT_DEFINED == 1 && portARM_CORTEX_M23 != 1 )
static uint8_t ucMaxSysCallPriority = 0;
static uint32_t ulMaxPRIGROUPValue = 0;
static const volatile uint8_t * const pcInterruptPriorityRegisters = ( const volatile uint8_t * const ) portNVIC_IP_REGISTERS_OFFSET_16;
#endif /* configASSERT_DEFINED == 1 && portARM_CORTEX_M23 != 1 */

#if ( configUSE_TICKLESS_IDLE == 1 )

/**
Expand Down Expand Up @@ -1069,6 +1091,113 @@ void vPortSVCHandler_C( uint32_t * pulCallerStackAddress ) /* PRIVILEGED_FUNCTIO

BaseType_t xPortStartScheduler( void ) /* PRIVILEGED_FUNCTION */
{
#if ( configASSERT_DEFINED == 1 && portARM_CORTEX_M23 != 1 )
{
volatile uint8_t ucOriginalPriority;
volatile uint32_t ulImplementedPrioBits = 0;
volatile uint8_t * const pucFirstUserPriorityRegister = ( volatile uint8_t * const ) ( portNVIC_IP_REGISTERS_OFFSET_16 + portFIRST_USER_INTERRUPT_NUMBER );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We constrain register access to be aligned in most cases. It's possible that a user has CCR_UNALIGN_TRP enabled, which could be triggered by the use of uint8_t here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uint8_t here looks right to me. The NVIC_IPRn registers are byte accessible by design (on all ARMv8-M except CM23).

There is a possible issue here when running FreeRTOS on the non-secure side because NVIC_IPRn is RAZ/WI (reads as zero, writes ignored) from non-secure state when the corresponding interrupt targets the secure state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@urutva The fix I would recommend here is to use the SVC priority register instead of the priority register for the first user interrupt. In ARMv8-M, it looks like the system handler priority registers are required to reflect only the number of implemented PRIO bits. So the strategy should be the same as you have now. And there are no issues related to secure/non-secure sides.

image

Plus, the ARMv8-M ports require the SVC interrupt priority to be zero, so you can destroy the contents during your PRIO bit counting, and then the subsequent (existing) code that sets for SysTick and PendSV can also include setting SHPR2 to zero. Be careful though because CM23 doesn't require the PRI_11 field to be byte accessible. Luckily PRI_10 PRI_9 and PRI_8 are all RES0, so you can safely write zeros with no side effects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we were testing this, we realized this issue that using NVIC_IPR registers does not work if the secure software does not mark the interrupt as non-secure using the corresponding NVIC_ITNS register. This suggestion looks really good. Let me try that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and it works perfectly. Thank you for the suggestion @jefftenney !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 One minor nit though, and it could wait for future work. Now using SHPR2, we shouldn't preserve its value. The ARMv8-M ports expect SVCall to use priority zero (the highest priority). The existing code that sets the PendSV and SysTick interrupt priorities should also set the SVCall priority to zero so all the kernel interrupt priorities are set there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jefftenney Can you please elaborate bit more on the requirement to set SVCall to priority zero (highest priority) for Armv8-M port? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@urutva Here's a recent forum thread for ARMv7-M. I think all the same reasoning applies to ARMv8-M.

The SVC instruction induces an exception that must be handled immediately. Compare to PendSV which is willing to pend for later execution. If the SVC instruction executes while the current execution priority prevents the SVCall handler, then a fault occurs (usage fault I think). The forum post describes how the ARMv7-M port executes the SVC instruction with BASEPRI masking some interrupts. The same happens in ARMv8-M. I am happy to find some references in ARM docs if it helps.

volatile uint8_t ucMaxPriorityValue;

/* Determine the maximum priority from which ISR safe FreeRTOS API
* functions can be called. ISR safe functions are those that end in
* "FromISR". FreeRTOS maintains separate thread and ISR API functions to
* ensure interrupt entry is as fast and simple as possible.
*
* Save the interrupt priority value that is about to be clobbered. */
ucOriginalPriority = *pucFirstUserPriorityRegister;

/* Determine the number of priority bits available. First write to all
* possible bits. */
*pucFirstUserPriorityRegister = portMAX_8_BIT_VALUE;

/* Read the value back to see how many bits stuck. */
ucMaxPriorityValue = *pucFirstUserPriorityRegister;

/* Use the same mask on the maximum system call priority. */
ucMaxSysCallPriority = configMAX_SYSCALL_INTERRUPT_PRIORITY & ucMaxPriorityValue;

/* Check that the maximum system call priority is nonzero after
* accounting for the number of priority bits supported by the
* hardware. A priority of 0 is invalid because setting the BASEPRI
* register to 0 unmasks all interrupts, and interrupts with priority 0
* cannot be masked using BASEPRI.
* See https://www.FreeRTOS.org/RTOS-Cortex-M3-M4.html */
configASSERT( ucMaxSysCallPriority );

/* Calculate the maximum acceptable priority group value for the number
* of bits read back. */

while( ( ucMaxPriorityValue & portTOP_BIT_OF_BYTE ) == portTOP_BIT_OF_BYTE )
{
ulImplementedPrioBits++;
ucMaxPriorityValue <<= ( uint8_t ) 0x01;
}

if( ulImplementedPrioBits == 8 )
{
/* When the hardware implements 8 priority bits, there is no way for
* the software to configure PRIGROUP to not have sub-priorities. As
* a result, the least significant bit is always used for sub-priority
* and there are 128 preemption priorities and 2 sub-priorities.
*
* This may cause some confusion in some cases - for example, if
* configMAX_SYSCALL_INTERRUPT_PRIORITY is set to 5, both 5 and 4
* priority interrupts will be masked in Critical Sections as those
* are at the same preemption priority. This may appear confusing as
* 4 is higher (numerically lower) priority than
* configMAX_SYSCALL_INTERRUPT_PRIORITY and therefore, should not
* have been masked. Instead, if we set configMAX_SYSCALL_INTERRUPT_PRIORITY
* to 4, this confusion does not happen and the behaviour remains the same.
*
* The following assert ensures that the sub-priority bit in the
* configMAX_SYSCALL_INTERRUPT_PRIORITY is clear to avoid the above mentioned
* confusion. */
configASSERT( ( configMAX_SYSCALL_INTERRUPT_PRIORITY & 0x1U ) == 0U );
ulMaxPRIGROUPValue = 0;
}
else
{
ulMaxPRIGROUPValue = portMAX_PRIGROUP_BITS - ulImplementedPrioBits;
}

/* The interrupt priority bits are not modelled in QEMU and the assert that
* checks the number of implemented bits and __NVIC_PRIO_BITS will always fail.
* Therefore, this assert is not adding any value for QEMU targets. The config
* option `configQEMU_DISABLE_INTERRUPT_PRIO_BITS_CHECK` should be defined in
* the `FreeRTOSConfig.h` for QEMU targets. */
#ifndef configQEMU_DISABLE_INTERRUPT_PRIO_BITS_CHECK
#ifdef __NVIC_PRIO_BITS
{
/*
* Check that the number of implemented priority bits queried from
* hardware is equal to the CMSIS __NVIC_PRIO_BITS configuration macro.
*/
configASSERT( ulImplementedPrioBits == __NVIC_PRIO_BITS );
}
#endif /* __NVIC_PRIO_BITS */

#ifdef configPRIO_BITS
{
/*
* Check that the number of implemented priority bits queried from
* hardware is equal to the FreeRTOS configPRIO_BITS configuration macro.
*/
configASSERT( ulImplementedPrioBits == configPRIO_BITS );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly prefer that this check be >= rather than == for compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a developer knowingly building for fewer PRIO bits than are actually implemented should disable these checks. That's an argument for keeping == here and for dropping 'QEMU' from configQEMU_DISABLE_INTERRUPT_PRIO_BITS_CHECK. Maybe call it configPORT_DISABLE_INTERRUPT_PRIO_BITS_CHECK.

}
#endif /* configPRIO_BITS */
#endif /* ifndef configQEMU_DISABLE_INTERRUPT_PRIO_BITS_CHECK */

/* Shift the priority group value back to its position within the AIRCR
* register. */
ulMaxPRIGROUPValue <<= portPRIGROUP_SHIFT;
ulMaxPRIGROUPValue &= portPRIORITY_GROUP_MASK;

/* Restore the clobbered interrupt priority register to its original
* value. */
*pucFirstUserPriorityRegister = ucOriginalPriority;
}
#endif /* configASSERT_DEFINED == 1 && portARM_CORTEX_M23 != 1 */

/* Make PendSV, CallSV and SysTick the same priority as the kernel. */
portNVIC_SHPR3_REG |= portNVIC_PENDSV_PRI;
portNVIC_SHPR3_REG |= portNVIC_SYSTICK_PRI;
Expand Down Expand Up @@ -1259,3 +1388,61 @@ BaseType_t xPortIsInsideInterrupt( void )
return xReturn;
}
/*-----------------------------------------------------------*/

#if ( configASSERT_DEFINED == 1 && portARM_CORTEX_M23 != 1 )
void vPortValidateInterruptPriority( void )
{
uint32_t ulCurrentInterrupt;
uint8_t ucCurrentPriority;

/* Obtain the number of the currently executing interrupt. */
__asm volatile ( "mrs %0, ipsr" : "=r" ( ulCurrentInterrupt )::"memory" );
Copy link
Contributor

@jefftenney jefftenney May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may also be an issue here for ISRs executing in the non-secure state. It seems IPSR won't indicate which interrupt is active when read from non-secure side.

Edit -- seems this would only be an issue if this function executes on non-secure side and is called from an ISR on secure side. That scenario doesn't seem likely.


/* Is the interrupt number a user defined interrupt? */
if( ulCurrentInterrupt >= portFIRST_USER_INTERRUPT_NUMBER )
{
/* Look up the interrupt's priority. */
ucCurrentPriority = pcInterruptPriorityRegisters[ ulCurrentInterrupt ];

/* The following assertion will fail if a service routine (ISR) for
* an interrupt that has been assigned a priority above
* configMAX_SYSCALL_INTERRUPT_PRIORITY calls an ISR safe FreeRTOS API
* function. ISR safe FreeRTOS API functions must *only* be called
* from interrupts that have been assigned a priority at or below
* configMAX_SYSCALL_INTERRUPT_PRIORITY.
*
* Numerically low interrupt priority numbers represent logically high
* interrupt priorities, therefore the priority of the interrupt must
* be set to a value equal to or numerically *higher* than
* configMAX_SYSCALL_INTERRUPT_PRIORITY.
*
* Interrupts that use the FreeRTOS API must not be left at their
* default priority of zero as that is the highest possible priority,
* which is guaranteed to be above configMAX_SYSCALL_INTERRUPT_PRIORITY,
* and therefore also guaranteed to be invalid.
*
* FreeRTOS maintains separate thread and ISR API functions to ensure
* interrupt entry is as fast and simple as possible.
*
* The following links provide detailed information:
* https://www.FreeRTOS.org/RTOS-Cortex-M3-M4.html
* https://www.FreeRTOS.org/FAQHelp.html */
configASSERT( ucCurrentPriority >= ucMaxSysCallPriority );
}

/* Priority grouping: The interrupt controller (NVIC) allows the bits
* that define each interrupt's priority to be split between bits that
* define the interrupt's pre-emption priority bits and bits that define
* the interrupt's sub-priority. For simplicity all bits must be defined
* to be pre-emption priority bits. The following assertion will fail if
* this is not the case (if some bits represent a sub-priority).
*
* If the application only uses CMSIS libraries for interrupt
* configuration then the correct setting can be achieved on all Cortex-M
* devices by calling NVIC_SetPriorityGrouping( 0 ); before starting the
* scheduler. Note however that some vendor specific peripheral libraries
* assume a non-zero priority group setting, in which cases using a value
* of zero will result in unpredictable behaviour. */
configASSERT( ( portAIRCR_REG & portPRIORITY_GROUP_MASK ) <= ulMaxPRIGROUPValue );
}
#endif /* configASSERT_DEFINED == 1 && portARM_CORTEX_M23 != 1 */
17 changes: 9 additions & 8 deletions portable/ARMv8M/non_secure/portable/GCC/ARM_CM23/portmacro.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@
#endif
/* *INDENT-ON* */

#include "portmacrocommon.h"

/*------------------------------------------------------------------------------
* Port specific definitions.
*
Expand All @@ -50,21 +48,24 @@
/**
* Architecture specifics.
*/
#define portARCH_NAME "Cortex-M23"
#define portDONT_DISCARD __attribute__( ( used ) )
#define portNORETURN __attribute__( ( noreturn ) )
#define portARCH_NAME "Cortex-M23"
#define portARM_CORTEX_M23 1
#define portDONT_DISCARD __attribute__( ( used ) )
#define portNORETURN __attribute__( ( noreturn ) )
/*-----------------------------------------------------------*/

#if( configTOTAL_MPU_REGIONS == 16 )
#include "portmacrocommon.h"

#if ( configTOTAL_MPU_REGIONS == 16 )
#error 16 MPU regions are not yet supported for this port.
#endif
/*-----------------------------------------------------------*/

/**
* @brief Critical section management.
*/
#define portDISABLE_INTERRUPTS() __asm volatile ( " cpsid i " ::: "memory" )
#define portENABLE_INTERRUPTS() __asm volatile ( " cpsie i " ::: "memory" )
#define portDISABLE_INTERRUPTS() __asm volatile ( " cpsid i " ::: "memory" )
#define portENABLE_INTERRUPTS() __asm volatile ( " cpsie i " ::: "memory" )
/*-----------------------------------------------------------*/

/* *INDENT-OFF* */
Expand Down
17 changes: 9 additions & 8 deletions portable/ARMv8M/non_secure/portable/GCC/ARM_CM23_NTZ/portmacro.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@
#endif
/* *INDENT-ON* */

#include "portmacrocommon.h"

/*------------------------------------------------------------------------------
* Port specific definitions.
*
Expand All @@ -50,21 +48,24 @@
/**
* Architecture specifics.
*/
#define portARCH_NAME "Cortex-M23"
#define portDONT_DISCARD __attribute__( ( used ) )
#define portNORETURN __attribute__( ( noreturn ) )
#define portARCH_NAME "Cortex-M23"
#define portARM_CORTEX_M23 1
#define portDONT_DISCARD __attribute__( ( used ) )
#define portNORETURN __attribute__( ( noreturn ) )
/*-----------------------------------------------------------*/

#if( configTOTAL_MPU_REGIONS == 16 )
#include "portmacrocommon.h"

#if ( configTOTAL_MPU_REGIONS == 16 )
#error 16 MPU regions are not yet supported for this port.
#endif
/*-----------------------------------------------------------*/

/**
* @brief Critical section management.
*/
#define portDISABLE_INTERRUPTS() __asm volatile ( " cpsid i " ::: "memory" )
#define portENABLE_INTERRUPTS() __asm volatile ( " cpsie i " ::: "memory" )
#define portDISABLE_INTERRUPTS() __asm volatile ( " cpsid i " ::: "memory" )
#define portENABLE_INTERRUPTS() __asm volatile ( " cpsie i " ::: "memory" )
/*-----------------------------------------------------------*/

/* *INDENT-OFF* */
Expand Down
15 changes: 8 additions & 7 deletions portable/ARMv8M/non_secure/portable/IAR/ARM_CM23/portmacro.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@
#endif
/* *INDENT-ON* */

#include "portmacrocommon.h"

/*------------------------------------------------------------------------------
* Port specific definitions.
*
Expand All @@ -50,20 +48,23 @@
/**
* Architecture specifics.
*/
#define portARCH_NAME "Cortex-M23"
#define portDONT_DISCARD __root
#define portARCH_NAME "Cortex-M23"
#define portARM_CORTEX_M23 1
#define portDONT_DISCARD __root
/*-----------------------------------------------------------*/

#if( configTOTAL_MPU_REGIONS == 16 )
#include "portmacrocommon.h"

#if ( configTOTAL_MPU_REGIONS == 16 )
#error 16 MPU regions are not yet supported for this port.
#endif
/*-----------------------------------------------------------*/

/**
* @brief Critical section management.
*/
#define portDISABLE_INTERRUPTS() __asm volatile ( " cpsid i " ::: "memory" )
#define portENABLE_INTERRUPTS() __asm volatile ( " cpsie i " ::: "memory" )
#define portDISABLE_INTERRUPTS() __asm volatile ( " cpsid i " ::: "memory" )
#define portENABLE_INTERRUPTS() __asm volatile ( " cpsie i " ::: "memory" )
/*-----------------------------------------------------------*/

/* Suppress warnings that are generated by the IAR tools, but cannot be fixed in
Expand Down
15 changes: 8 additions & 7 deletions portable/ARMv8M/non_secure/portable/IAR/ARM_CM23_NTZ/portmacro.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@
#endif
/* *INDENT-ON* */

#include "portmacrocommon.h"

/*------------------------------------------------------------------------------
* Port specific definitions.
*
Expand All @@ -50,20 +48,23 @@
/**
* Architecture specifics.
*/
#define portARCH_NAME "Cortex-M23"
#define portDONT_DISCARD __root
#define portARCH_NAME "Cortex-M23"
#define portARM_CORTEX_M23 1
#define portDONT_DISCARD __root
/*-----------------------------------------------------------*/

#if( configTOTAL_MPU_REGIONS == 16 )
#include "portmacrocommon.h"

#if ( configTOTAL_MPU_REGIONS == 16 )
#error 16 MPU regions are not yet supported for this port.
#endif
/*-----------------------------------------------------------*/

/**
* @brief Critical section management.
*/
#define portDISABLE_INTERRUPTS() __asm volatile ( " cpsid i " ::: "memory" )
#define portENABLE_INTERRUPTS() __asm volatile ( " cpsie i " ::: "memory" )
#define portDISABLE_INTERRUPTS() __asm volatile ( " cpsid i " ::: "memory" )
#define portENABLE_INTERRUPTS() __asm volatile ( " cpsie i " ::: "memory" )
/*-----------------------------------------------------------*/

/* Suppress warnings that are generated by the IAR tools, but cannot be fixed in
Expand Down
Loading