search

FreeRTOS / FreeRTOS-Kernel

FreeRTOS kernel files only, submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
https://www.FreeRTOS.org
MIT License
2.71k stars 1.11k forks source link

Update XMOS xcore.ai port to be compatible with v11.x #1096

Closed ACascarino closed 3 months ago

ACascarino commented 3 months ago

Update XMOS xcore.ai port

Description

The merge of the smp branch into main in version 11 broke an assumption that the xcore.ai port relied upon - the backwards-compatibility-preserving decision to conditionally define in tasks.c either pxCurrentTCB or pxCurrentTCBs depending solely on the value of configNUMBER_OF_CORES means that the pxCurrentTCBs symbol does not always exist. The assumption was made that this symbol would always exist, and for FreeRTOS instances with one core in use its 0th element would solely be populated. This change means that ports to SMP platforms need instead to be aware of the number of cores defined and change behaviour accordingly. This went unnoticed at XMOS at the time of release of v11, but we've now noticed the issue and I've (hopefully) created a fix.

This PR attempts to make the XMOS xcore.ai port agnostic to whether it is running on a single-core or SMP instance of FreeRTOS by simply introducing an additional layer of indirection to pxCurrentTCB(s) accesses. When the scheduler is started (which is the first time in the application that the port layer needs to interact with the TCB pointer(s)), we populate a global symbol with the address of the TCB pointer. It is this symbol, rather than pxCurrentTCB(s), which is then used in scheduler initialisation and on context switches.

This PR adds one instruction (plus compiler-defined register spill/restoration to protect the r5 clobber) to scheduler initialisation, and zero instructions to RTOS interrupt processing and context switching.

This PR also includes a fix to make exception behaviour entirely predictable; the previous implementation had assumed that the symbol _TrapHandler would always exist at 0x80080. This is not the case; for example, using the --first option in our toolchain to place arbitrary data at the top of memory shifts all other symbols, breaking this assumption and causing wildly undefined behaviour on exception. _DoException is a link-time visible symbol that presents the appropriate entry-point to the platform's exception handling routines, and its use here is preferable in all contexts.

Test Steps

As this is a community-supported port, there are no automated tests to highlight the issue, nor any to prove lack of regression. However, internal testing of FreeRTOS-based products has shown no regression in their functionality due to this change.

As a query - partner-maintained ports seem to have a series of automated tests that they use to show lack of regression, but these, from a cursory look, seem entirely single-core focussed. Are there any partner-maintained SMP ports for which the test suite would exercise the additional SMP-related functionality?

Checklist:

Related Issue

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

AniruddhaKanhere commented 3 months ago

Hello @ACascarino,

Thank you for taking the time to report and fix the issue. To me, in the first glance, it looks good. I'll pass it along to our experts in the team for review.

Thanks, Aniruddha

ACascarino commented 3 months ago

Apologies - it turns out adding a new commit dismissed stale reviews. Sorry to ask you to review again!

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.31%. Comparing base (0c79e74) to head (a0cca63). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1096 +/- ## ======================================= Coverage 92.31% 92.31% ======================================= Files 6 6 Lines 3226 3226 Branches 885 885 ======================================= Hits 2978 2978 Misses 132 132 Partials 116 116 ``` | [Flag](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/1096/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/1096/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | `92.31% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

chinglee-iot commented 3 months ago

@ACascarino

As a query - partner-maintained ports seem to have a series of automated tests that they use to show lack of regression, but these, from a cursory look, seem entirely single-core focussed. Are there any partner-maintained SMP ports for which the test suite would exercise the additional SMP-related functionality?

We have on target test cases which could exercise the SMP-related functionality. We are still in the progress to publish all the test cases. You could reference reference this link for more information.

ACascarino commented 3 months ago

@ACascarino

As a query - partner-maintained ports seem to have a series of automated tests that they use to show lack of regression, but these, from a cursory look, seem entirely single-core focussed. Are there any partner-maintained SMP ports for which the test suite would exercise the additional SMP-related functionality?

We have on target test cases which could exercise the SMP-related functionality. We are still in the progress to publish all the test cases. You could reference reference this link for more information.

Ah great thank you - I managed to completely miss these!