Cortex-A9 port: disable interrupts before writing to ICC_PMR

Kernel
1

Hi, this topic comes right after I discovered that interrupts where enabled within vTaskSwitchContext in a previous topic. The problem is present when configNUMBER_OF_CORES is higher than 1 and configCHECK_FOR_STACK_OVERFLOW is defined, either to 1 or 2. In this case, pxCurrentTCB is defined in tasks.c as xTaskGetCurrentTaskHandle() so, when it is used inside taskCHECK_FOR_STACK_OVERFLOW, it causes to enable the interrupts even though they where previously disabled. I also noticed this was already reported so I do not know if it should be considered a problem of the multi-core port I am using or not. At this point I could just check if the interrupts are already disabled before disabling them in ulPortSetInterruptMask and vPortClearInterruptMask but is it really necessary to disable them? I know ARM recommends to keep an interrupt disabled if its configuration is going to be changed but I cannot find anything about disabling interrupts before modifying the Priority Mask Register.

2

You are right. The stack macros should access TCB directly as the interrupts are already blocked. I’ll get back to you with a proposal.

1 Like
3

I tried both the previously proposed approaches:

  1. Check if the interrupt are already disabled and skip the execution of portCPU_IRQ_DISABLE() and portCPU_IRQ_ENABLE() before the modification of the PM Register.
  2. Remove portCPU_IRQ_DISABLE() and portCPU_IRQ_ENABLE() from vPortClearInterruptMask and ulPortSetInterruptMask.

They both seems to work even though the second one may require more extensive testing. In case the second approach were considered reliable, it could also be applied to the single-core Cortex-A9 port.

4

couldn’t your solution 1 be subject to race conditions where the condition changes in between its check and the follow up?

5

The code I am using is the following one:

#define portCPU_IRQ_DISABLE()                                       \
    __asm volatile ( "CPSID i" ::: "memory" );                      \
    __asm volatile ( "DSB" );                                       \
    __asm volatile ( "ISB" );

#define portCPU_IRQ_ENABLE()                                        \
    __asm volatile ( "CPSIE i" ::: "memory" );                      \
    __asm volatile ( "DSB" );                                       \
    __asm volatile ( "ISB" );

#define portCHECK_IF_INTERRUPTS_DISABLED()                          \
    ({                                                              \
        uint32_t ulInterruptsDisabled;                              \
        __asm volatile (                                            \
            "MRS    %[IntDisabled], cpsr                     \n\t"  \
            "AND    %[IntDisabled], %[IntDisabled], %[iMask]  \n\t" \
        :[IntDisabled] "+r" (ulInterruptsDisabled)                  \
        :[iMask] "I" (1 << 7)                                       \
        :                                                           \
        );                                                          \
        ulInterruptsDisabled;                                       \
    })

uint32_t ulPortSetInterruptMask( void )
{
    uint32_t ulSavedInterruptState;
    const uint32_t ulInterruptsEnabled = !portCHECK_IF_INTERRUPTS_DISABLED();
    
    /* Interrupts are disabled before the ICCPMR is updated */
    if( ulInterruptsEnabled )
    {
        portCPU_IRQ_DISABLE();
    }

    ulSavedInterruptState = portICCPMR_PRIORITY_MASK_REGISTER;
    portICCPMR_PRIORITY_MASK_REGISTER = ( uint32_t )
(configMAX_API_CALL_INTERRUPT_PRIORITY << portPRIORITY_SHIFT );
    __asm volatile (    "DSB        \n"               
                        "ISB        \n" ::: "memory" );
    
    if( ulInterruptsEnabled )
    {
        portCPU_IRQ_ENABLE();
    }

    return ulSavedInterruptState;        
}

void vPortClearInterruptMask( uint32_t ulSavedInterruptState )
{
    const uint32_t ulInterruptsEnabled = !portCHECK_IF_INTERRUPTS_DISABLED();

    /* Interrupts are disabled before the ICCPMR is updated */
    if( ulInterruptsEnabled )
    {
        portCPU_IRQ_DISABLE();
    }

    portICCPMR_PRIORITY_MASK_REGISTER = ulSavedInterruptState;
    __asm volatile (    "DSB        \n"        
                        "ISB        \n" ::: "memory" );      
    
    if( ulInterruptsEnabled )
    {
        portCPU_IRQ_ENABLE();
    }
}

If the interrupts are disabled properly, so placing the appropriate barrier to disable them exactly when they should be, I don’t see how the condition could change within ulPortSetInterruptMask and vPortClearInterruptMask.

6

Hi @aggarg, do you have any official news about this matter?

7

Hi @Matth9814, Apologies for the delay in response. Would you please try the following patch and see if it addresses your issue - stack_macros_smp_fix.patch (20.5 KB)?

8

Thank you for addressing this issue, the patched code works as expected.

9

Thank you for confirming! Would you like to raise a PR for this or would you like us to?

10

I am going to do it, thank you.

11

Thank you for raising the PR - FreeRTOS SMP: direct access to current TCB inside stack macros by Matth9814 · Pull Request #1270 · FreeRTOS/FreeRTOS-Kernel · GitHub.

1 Like
12

I want to report that the same problem is present in Percepio View 4.10.3 since the TCB retrieval on the traced core is performed by calling pxCurrentTCB (i.e. xTaskGetCurrentTaskHandle()). I am working with multiple cores so I have defined TRC_CFG_RECORDER_MODE as TRC_RECORDER_MODE_STREAMING inside trcKernelPortConfig.h. With this configuration, the FreeRTOS trace macros are defined inside trcKernelPort.h, starting at line 1952. I do not know the exact number of macros that can replicate the problem but traceTASK_SWITCHED_IN() is surely one of them. Using the code reported above prevents the issue.
I have contacted ARM about the original question of this post and they confirmed that there is no architectural constraint to disable interrupts before modifying the ICC_PMR when a Cortex-A9 is paired with a GICv1 (or GICv2), like in my case, so I could definitely solve the problem on my end by not disabling the interrupts. However, I know this is only one of the possible SoC configurations so it is not feasible to do so in general.

13

Is my understanding correct that the implementation of these macros need to be updated in the percepio library?

14

According to what has been done with FreeRTOS I suppose the macro in the Percepio View library should be updated. However, this is only a problem in multi-core and, according to my knowledge, it should not be a problem in any already contributed port that supports SMP. The issue is only present in the custom SMP Cortex-A9 port I am using, this is why I also suggested possible ways to fix it on my end.

15

You are right. IfportSET_INTERRUPT_MASK and portCLEAR_INTERRUPT_MASK are implemented to support recursive calls, then this problem won’t happen. So I guess fixing it in your port is a better idea. If we do that, we can also revert the last PR we merged?

16

Absolutely, I am already using this implementation so you can revert the merged PR.

17

Thank you for confirming @Matth9814 !

1 Like
18

Here is the PR to revert the change introduced in PR 1270 - Revert PR 1270: Direct access to current TCB inside stack macros by aggarg · Pull Request #1272 · FreeRTOS/FreeRTOS-Kernel · GitHub.

1 Like