ARMv7M: Adjust implemented priority bit assertions

[paulbartell]

Description

Adjust assertions related to the CMSIS __NVIC_PRIO_BITS and FreeRTOS configPRIO_BITS configuration macros such that these macros specify the minimum number of implemented priority bits supported by a config build rather than the exact number of implemented priority bits.

Test Steps

Run the kernel in qemu with the mps2-an385 target with __NVIC_PRIO_BITS and configPRIO_BITS set to all valid values: 3,4,5,6,7,8.

Checklist:

• I have tested my changes. No regression in existing tests.
• [ N/A ] I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

Related to Qemu issue #1122. See related discussion on the qemu mailing list.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (1b8a424) 94.46% compared to head (8f19fa0) 94.46%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #665   +/-   ##
=======================================
  Coverage   94.46%   94.46%           
=======================================
  Files           6        6           
  Lines        2422     2422           
  Branches      594      594           
=======================================
  Hits         2288     2288           
  Misses         85       85           
  Partials       49       49           

Flag	Coverage Δ
unittests	94.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jefftenney]

Does this PR eliminate the requirement that __NVIC_PRIO_BITS == configPRIO_BITS when both are defined?

[aggarg]

Does this PR eliminate the requirement that __NVIC_PRIO_BITS == configPRIO_BITS when both are defined?

Yes, seems so.

[jefftenney]

OK. It seems that part was unintentional, and probably not a good thing. Otherwise this is a great change for qemu.

[urutva]

I second the opinion of @jefftenney. First hit on the search for configPRIO_BITS on FreeRTOS repo resulted in https://github.com/FreeRTOS/FreeRTOS/blob/391c79958f635ee5476dcf2774dab59e2b151eff/FreeRTOS/Demo/CORTEX_STM32L152_IAR/FreeRTOSConfig.h#L84 where configPRIO_BITS is used to set configKERNEL_INTERRUPT_PRIORITY and configMAX_SYSCALL_INTERRUPT_PRIORITY and is set to 4 when __NVIC_PRIO_BITS isn't defined. Imagine a scenario where the number of implemented priority bits is higher than the configPRIO_BITS then configKERNEL_INTERRUPT_PRIORITY and configMAX_SYSCALL_INTERRUPT_PRIORITY would be set incorrectly, but the configASSERT wouldn't catch it.

I was thinking, would it make sense to create something like portqemu.h, similar to portmacro.h to handle qemu related changes.

[jefftenney]

@urutva Sorry I think was too brief in my comment. My concern is a little different than the one you mention. I think we should restore the requirement configPRIO_BITS == __NVIC_PRIO_BITS (when both are defined). That requirement was in place prior to this PR, and I think that requirement should remain in place. It prevents confusion.

Consider a developer trying to use fewer bits than are implemented in the hardware. They set configPRIO_BITS to 3, but their CMSIS implementation sets __NVIC_PRIO_BITS to 4 because that's how many bits are actually implemented. Their calls to NVIC_SetPriority() will set "wrong" priorities because that function takes unshifted priorities and then shifts them based on __NVIC_PRIO_BITS (not based on configPRIO_BITS). The configASSERT() statements in port.c would still catch any violations of configMAX_SYSCALL_INTERRUPT_PRIORITY, but the developer might struggle to understand why the assert is failing.

[RichardBarry]

The configPRIO_BITS == __NVIC_PRIO_BITS assert provides immediate feedback to users that have an incorrect configuration, maybe because they re-used a FreeRTOSConfig.h without realising the consequence. These asserts get into the code in response to the most common errors we observe users making.

[paulbartell]

I will open a PR with an additional assertion as such.

Really though, configPRIO_BITS should probably be something like configMINIMUM_PRIO_BITS.
The whole configuration for interrupt priority is needlessly confusing to users. I wish we just had some a sane default like in the ARMv8M ports.

[paulbartell]

@jefftenney
On second though, perhaps the assertion should be that

configPRIO_BITS <= __NVIC_PRIO_BITS 

rather than

configPRIO_BITS == __NVIC_PRIO_BITS

Does that seem reasonable to you?

To do this, I would add the following to each portmacro.h:

#if ( defined( configPRIO_BITS ) && defined( __NVIC_PRIO_BITS ) )
    #if ( configPRIO_BITS > __NVIC_PRIO_BITS )
        #error configPRIO_BITS must be <= __NVIC_PRIO_BITS when both are declared. \
            Refer to https://www.freertos.org/RTOS-Cortex-M3-M4.html for more details.
    #endif
#endif /* configPRIO_BITS && __NVIC_PRIO_BITS */

Alternatively,

#if ( defined( configPRIO_BITS ) && defined( __NVIC_PRIO_BITS ) )
    #if ( configPRIO_BITS != __NVIC_PRIO_BITS )
        #error configPRIO_BITS must equal __NVIC_PRIO_BITS when both are declared. \
            Refer to https://www.freertos.org/RTOS-Cortex-M3-M4.html for more details.
    #endif
#endif /* configPRIO_BITS && __NVIC_PRIO_BITS */

[RichardBarry]

Why are we changing this? When does it makes sense for configPRIOR_BITS to not be equal to __NVIC_PRIO_BITS? Experience shows the kernel code should never have a dependency on a third party library. Originally there was only configPRIOR_BITS, the assert with __NVIC_PRIO_BITS is a way of checking that setting is correct IF __NVIC_PRIO_BITS is defined. Currently it's possible to have only __NVIC_PRIO_BITS, which, in a way, doesn't comply with the "no dependency on third party code" rule - although in this case it is ok as there is a work around that doesn't involve changing the code if it isn't defined.

(for a long time a vendor, who shall remain nameless, shipped a header file with __NVIC_PRIO_BITS set incorrectly, which was found by an assert in FreeRTOS that checked the actual number of bits defined. As it's possible to programmatically check the number of bits we could create code that doesn't define configPRIOR_BIT or __NVIC_PRIO_BITS - but we don't do that because it would mean defining the max syscall interrupt priority as a non-const variable rather than a constant, which has an execution time impact).

[paulbartell]

@RichardBarry : Qemu models of the ARM MPS boards implement more priority bits than the actual ARM MPS boards.

It's quite common for a single codebase might target multiple SoCs. In this case, it would make sure to share a FreeRTOSConfig.h with the same configPRIO_BITS value even if one of the target SoCs supports more priority bits in hardware (and thus __NVIC_PRIO_BITS would differ).

[jefftenney]

@paulbartell Prior this PR, a codebase that targets multiple SoCs would recompile for the different targets' values of __NVIC_PRIO_BITS. And configPRIO_BITS would always match __NVIC_PRIO_BITS. And those values would all match the hardware for that build. That codebase would be written for the minimum number of implemented priority bits among all target SoCs, so it would work properly on all systems. All of that is prior this PR. Accomplished in part by this typical code in FreeRTOSConfig.h:

/* Cortex-M specific definitions. */
#ifdef __NVIC_PRIO_BITS
   /* __NVIC_PRIO_BITS will be specified when CMSIS is being used; its value is MCU-specific. */
   #define configPRIO_BITS             __NVIC_PRIO_BITS

If the minimum SoC has only 3 implemented bits, then the codebase uses only priorities 0 through 7. If another of the SoCs has 4 implemented bits, the codebase still uses priorities 0 through 7, leaving priorities 8 through 15 unused (except for FreeRTOS automatically using priority 15 for the kernel).

The above is also true when QEMU is one of the targets, as long as __NVIC_PRIO_BITS is set to 8 -- the correct value for QEMU. Only a small concession might be required related to the developer's choice of configMAX_SYSCALL_INTERRUPT_PRIORITY. With QEMU, priorities 8 through 255 would be unused (except for FreeRTOS automatically using priority 255 for the kernel). And again, all of this is true prior to this PR.

So, I don't clearly see the motivation for this PR. Maybe it makes all targets use the same (upper) bits in the priority registers? For QEMU I see that it avoids the awkward bit 0 in the priority registers which cannot be configured as a preemption-priority bit. Are there other benefits?

[jefftenney]

@paulbartell On the question of configPRIO_BITS < __NVIC_PRIO_BITS. If someone is purposely using fewer priority bits than are actually implemented, and if they set configPRIO_BITS to the number of bits they want to actually use, they still would not want configPRIO_BITS < __NVIC_PRIO_BITS. That's because CMSIS functions like NVIC_SetPriority() treat interrupt priorities as unshifted values, and internally those functions shift the values based on __NVIC_PRIO_BITS, not based on configPRIO_BITS. The developer would need to reduce the value of __NVIC_PRIO_BITS in the CMSIS implementation provided by the silicon vendor to match configPRIO_BITS. Then calls to NVIC_SetPriority() would configure priorities as expected.

But again, maybe the preferred approach would be to leave them both set to the number actually implemented in the hardware, and limit the range of priority values used (as described in the previous post).

[RichardBarry]

I just tried V10.4.6 in QEMU without __NVIC_PRIORITY_BITS or configPRIO_BITS defined and, as far as I can determine in a few minutes, is working as expected. configASSERT() is defined.

V10.4.6 has neither of the last two changes to this part of the code applied. https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/V10.4.6/portable/GCC/ARM_CM3/port.c#L303

ulMaxPRIORGroup underflows here https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/V10.4.6/portable/GCC/ARM_CM3/port.c#L299 because the loop doesn't expect all the priority bits defined (invalid on real hardware), but is ANDed with a mask here https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/V10.4.6/portable/GCC/ARM_CM3/port.c#L324

[paulbartell]

@jefftenney and @urutva Given that cmsis priorities are unshifted, I see the reasoning to check that the values are the same.

Thus I'll open a PR with the following in each portmacro.h:

#if ( defined( configPRIO_BITS ) && defined( __NVIC_PRIO_BITS ) )
    #if ( configPRIO_BITS != __NVIC_PRIO_BITS )
        #error configPRIO_BITS must equal __NVIC_PRIO_BITS when both are declared. \
            Refer to https://www.freertos.org/RTOS-Cortex-M3-M4.html for more details.
    #endif
#endif /* configPRIO_BITS && __NVIC_PRIO_BITS */

Does that address your concerns?

[paulbartell]

@jefftenney

So, I don't clearly see the motivation for this PR. Maybe it makes all targets use the same (upper) bits in the priority registers? For QEMU I see that it avoids the awkward bit 0 in the priority registers which cannot be configured as a preemption-priority bit. Are there other benefits?

The objective of this PR was to be able to run binaries compiled for a particular mps2 board including the official cmsis headers on it's corresponding qemu target without an assertion failure each and every time.

[jefftenney]

Does that address your concerns?

Yes.

he objective of this PR was to be able to run binaries compiled for a particular mps2 board including the official cmsis headers on it's corresponding qemu target

Then this is perfect.

[RichardBarry]

I just downloaded FreeRTOSv202212.00 and ran this MPS2 QEMU demo https://github.com/FreeRTOS/FreeRTOS/tree/202212.00/FreeRTOS/Demo/CORTEX_MPS2_QEMU_IAR_GCC, which executed as expected (not sure if it matches the actual MPS2 FPGA model). There is a second (incorrectly named) MPS2 project that I didn't try. That second project includes the CMSIS library, so __NVIC_PRIO_BITS may be defined, and that may cause an assert. I recommend deleting the second demo as it isn't referenced in the QEMU documentation on FreeRTOS.org.

The asserts in 202212.00 are as they were prior to PRs updating that code sequence, are simpler than after the updates, and work as intended outside of QEMU. After the PRs the asserts don't work as intended.

[jefftenney]

After the PRs the asserts don't work as intended.

Specifically, the new asserts have lost the ability to catch a vendor's setting __NVIC_PRIO_BITS lower than the number actually implemented in the hardware. Note that the application still works correctly with __NVIC_PRIO_BITS set too low. The developer ends up using fewer bits than are available, but no harm done. The application works as designed. Still, losing the ability to catch that specific vendor mistake deserves some consideration.

ran this MPS2 QEMU ... which executed as expected

In v202212.00, this assert didn't work in QEMU. It couldn't catch configuration errors because the port code set ulMaxPRIGROUPValue incorrectly. That issue was fixed in a recent commit.

[jefftenney]

Note that the application still works correctly with __NVIC_PRIO_BITS set too low. The developer ends up using fewer bits than are available, but no harm done.

That is only true if the developer configures interrupt priorities based on the definition of __NVIC_PRIO_BITS. But consider this example:

The developer sets all the project's interrupt priorities based on the documentation for the hardware (Reference Manual, data sheet, etc) which says 4 bits are implemented. So the developer uses priorities 0 through 15 in CMSIS calls. The developer doesn't notice the vendor's mistaken setting of __NVIC_PRIO_BITS to 3. The CMSIS calls would shift priorities by 5 (not by 4 as the developer expects), quietly dropping the most-significant bit of the developer's 4-bit values. I don't see any compile-time warnings or runtime asserts (or even return codes) that would indicate the problem. But the interrupts would have wrong priorities and even the wrong order of priorities. Prior to this PR, there would have been a runtime assert about __NVIC_PRIO_BITS.

I have not tested this but it deserves some consideration.