RISCV Add FPU context save #1250

cubidesj · 2025-02-21T08:09:47Z

Add FPU context switch support for the RISC-V port

Description

Current context switch in the RISC-V port only supports the integer (x) registers. Which means that having two tasks using FPU code will corrupt one another's work.

This PR

Creates a chip_specific_extension define (portasmSTORE_FPU_CONTEXT) to identify if the FPU context should be saved and does it when the FPU is present and the flag is set. To that have a platform that can tested I also added a RV32IF_CLINT under risc_v_chip_specific_extensions
Create portFPUCONTEXT_SIZE macro, that is defined as 0 when the FPU support is not present or is not enabled
Creates a couple of macros to save/restore the FPU context and uses it when portasmSTORE_FPU_CONTEXT is enabled

Test Steps

A Demo using QEMU was created. That demo also adds a couple of tasks modifying the FPU registers to check that they values are maintained across context switch. This is part of another PR in the FreeRTOS repo

Checklist:

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

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

cubidesj · 2025-02-21T08:10:53Z

Matching PR FreeRTOS/FreeRTOS#1329

jasonpcarroll

Hi @cubidesj.

Sorry for the late response.

Overall I think this looks fine. My main hang up is I am not sure if we should add directly to base RISC V implementation or use the already existing portasmSAVE_ADDITIONAL_REGISTERS and portasmADDITIONAL_CONTEXT_SIZE. I am not terribly familiar with the implementations of this port, but that would make sense to me. I would like to get the opinion of someone more familiar.

jasonpcarroll · 2025-03-03T22:31:04Z

portable/GCC/RISC-V/portContext.h

@@ -66,6 +97,97 @@
   .extern pxCriticalNesting
 /*-----------------------------------------------------------*/

+    .macro portcontexSAVE_FPU_CONTEXT_INTERNAL


Suggested change

.macro portcontexSAVE_FPU_CONTEXT_INTERNAL

.macro portcontextSAVE_FPU_CONTEXT_INTERNAL

jasonpcarroll · 2025-03-03T22:31:36Z

portable/GCC/RISC-V/portContext.h

+    .endm
+/*-----------------------------------------------------------*/
+
+    .macro portasmRESTORE_FPU_CONTEXT_INTERNAL


portasm macros should be in portASM.S

Suggested change

.macro portasmRESTORE_FPU_CONTEXT_INTERNAL

.macro portcontextRESTORE_FPU_CONTEXT_INTERNAL

for consistency / its declared in this file not the asm file.

jasonpcarroll · 2025-03-03T22:32:08Z

portable/GCC/RISC-V/portContext.h

@@ -105,6 +227,9 @@ store_x t0, portCRITICAL_NESTING_OFFSET * portWORD_SIZE( sp ) /* Store the criti

 csrr t0, mstatus /* Required for MPIE bit. */
 store_x t0, portMSTATUS_OFFSET * portWORD_SIZE( sp )
+#ifdef portasmSTORE_FPU_CONTEXT
+portcontexSAVE_FPU_CONTEXT_INTERNAL


Suggested change

portcontexSAVE_FPU_CONTEXT_INTERNAL

portcontextSAVE_FPU_CONTEXT_INTERNAL

jasonpcarroll · 2025-03-03T22:32:22Z

portable/GCC/RISC-V/portContext.h

@@ -145,6 +270,10 @@ csrw mepc, t0
 /* Defined in freertos_risc_v_chip_specific_extensions.h to restore any registers unique to the RISC-V implementation. */
 portasmRESTORE_ADDITIONAL_REGISTERS

+#ifdef portasmSTORE_FPU_CONTEXT
+    portasmRESTORE_FPU_CONTEXT_INTERNAL


Suggested change

portasmRESTORE_FPU_CONTEXT_INTERNAL

portcontextRESTORE_FPU_CONTEXT_INTERNAL

jasonpcarroll · 2025-03-03T22:33:15Z

portable/GCC/RISC-V/portASM.S

@@ -196,8 +197,9 @@ definitions. */
 * pxCode
 */
 pxPortInitialiseStack:
+    addi a0, a0, -portFPUCONTEXT_SIZE


Why are we adding it like this instead of just making use of portasmADDITIONAL_CONTEXT_SIZE?

This will probably be solved as part of #1250 (comment) but if we don't use the portasm*_ADDITIONAL_REGISTERS macros we cannot use portasmADDITIONAL_CONTEXT_SIZE as the "additional context" is not in the same place as the FPU registers.

jasonpcarroll · 2025-03-03T22:33:38Z

.../GCC/RISC-V/chip_specific_extensions/RV32IF_CLINT/freertos_risc_v_chip_specific_extensions.h

+#define portasmADDITIONAL_CONTEXT_SIZE    0 /* Must be even number on 32-bit cores. */
+#define portasmSTORE_FPU_CONTEXT          1
+
+.macro portasmSAVE_ADDITIONAL_REGISTERS


Should we just store the FPU registers using this macro?

Hi @jasonpcarroll, this is an interesting question.
We could call the portcontext*_FPU_CONTEXT_INTERNAL from the freertos_risc_v_chip_specific_extensions.h but I still think the definition of the macro should be available in the "main" part of the port so it can be used by multiple freertos_risc_v_chip_specific_extensions.h instead of having multiple copies of that context save/restore.
I chose to put it directly as part of the main context macros as the FPU is a standard extension in the CPU rather than for other parts of the chip but I also hesitated with this.
I agree that it is a judgment call so I will do whatever you recommend. Keep in mind that I plan to create a follow up adding Vector context save support that should follow a very similar structure to what we do for the FPU and that having FPU and having Vectors is orthogonal.

kstribrnAmzn · 2025-03-04T00:13:26Z

portable/GCC/RISC-V/portASM.S

@@ -196,8 +197,9 @@ definitions. */
 * pxCode
 */
 pxPortInitialiseStack:
+    addi a0, a0, -portFPUCONTEXT_SIZE
    csrr t0, mstatus                    /* Obtain current mstatus value. */


Not blocking but would it be more consistent to perform the add for the FPU context size before reading the mstatus registers? Looks like most stack sizing operations occur before this register access.

Hi @kstribrnAmzn, I am not sure that I fully understand the question but this instruction can be moved a bit later but no later than line 207 unless I move the FPU context to some other place (this includes using the portasmSAVE_ADDITIONAL_REGISTERS as in another discussion).
Assuming that it remains as is, this function calculates the value to store, stores in the context (using a0 as pointer) and decreases a0. In order to keep mstatus at the right offset I need to decrease a0 by the size of the FPU context before mstatus is stored (at line 208 store_x t0, 0(a0) /* mstatus onto the stack. */.

Did I understand your concern correctly?

aggarg · 2025-03-04T18:18:38Z

I have the following 2 suggestions:

Use a config macro configENABLE_FPU to enable FPU support. This would be consistent with other ports.
The current implementation reserves space for FPU registers in the task context regardless of whether or not the uses FPU. I think we can only reserve space for FPU registers if the task uses FPU.

Both my suggestions are in this patch. Note that I am yet to build and test this patch but I just wanted to share the idea with you.

aggarg · 2025-03-05T17:33:18Z

Here is the patch that I have tested to ensure that it correctly stores and restores FPU registers only for the tasks which use FPU - 0001-Code-review-suggestions.patch.

You'll need to apply the following patch to your demo Makefile - Makefile.patch.

Please apply these patches and refresh this branch from upstream main so that we can merge this PR.

cubidesj · 2025-03-06T07:41:18Z

Please apply these patches and refresh this branch from upstream main so that we can merge this PR.

OK, thanks. I will do that, test and push the changes back.

…t size

…extension

sonarqubecloud · 2025-03-06T09:54:02Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

cubidesj requested a review from a team as a code owner February 21, 2025 08:09

cubidesj mentioned this pull request Feb 21, 2025

Update RV32 qemu Demo to support RVA23 FreeRTOS/FreeRTOS#1329

Merged

jasonpcarroll reviewed Mar 3, 2025

View reviewed changes

kstribrnAmzn reviewed Mar 4, 2025

View reviewed changes

cubidesj added 9 commits March 6, 2025 10:52

port: riscv: Fix typo

d82b6d1

port: riscv: Split the number of registers and the size of the context

f38a54b

port: riscv: Create some macros for the FPU context

214b809

port: riscv: Add a couple of macros that store fpu context

0fc228d

port: riscv: Update the stack init function to include the fpu contex…

a864fe3

…t size

port: riscv: Add a chip_specific_extensions file that includes the F …

f40d89f

…extension

Update dictionary to include some risc-v instructions

621e8fb

port: riscv: Fix a few typos

1198dab

port: riscv: Apply @aggarg's sugestions

dec30a8

cubidesj force-pushed the s5/jcu/add_fpu_support branch from 5c824e5 to dec30a8 Compare March 6, 2025 09:53

aggarg approved these changes Mar 6, 2025

View reviewed changes

kstribrnAmzn approved these changes Mar 6, 2025

View reviewed changes

xuelix merged commit 4d9cd90 into FreeRTOS:main Mar 6, 2025
17 checks passed

cubidesj mentioned this pull request Mar 18, 2025

Add support for Vector context save support on RISC-V #1260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RISCV Add FPU context save #1250

RISCV Add FPU context save #1250

cubidesj commented Feb 21, 2025 •

edited by kstribrnAmzn

Loading

cubidesj commented Feb 21, 2025

jasonpcarroll left a comment

jasonpcarroll Mar 3, 2025

jasonpcarroll Mar 3, 2025

jasonpcarroll Mar 3, 2025

jasonpcarroll Mar 3, 2025

jasonpcarroll Mar 3, 2025

jasonpcarroll Mar 3, 2025

cubidesj Mar 4, 2025

jasonpcarroll Mar 3, 2025

cubidesj Mar 4, 2025

kstribrnAmzn Mar 4, 2025

cubidesj Mar 4, 2025

aggarg commented Mar 4, 2025

aggarg commented Mar 5, 2025 •

edited

Loading

cubidesj commented Mar 6, 2025

sonarqubecloud bot commented Mar 6, 2025

	.macro portcontexSAVE_FPU_CONTEXT_INTERNAL
	.macro portcontextSAVE_FPU_CONTEXT_INTERNAL

	.macro portasmRESTORE_FPU_CONTEXT_INTERNAL
	.macro portcontextRESTORE_FPU_CONTEXT_INTERNAL

	portcontexSAVE_FPU_CONTEXT_INTERNAL
	portcontextSAVE_FPU_CONTEXT_INTERNAL

	portasmRESTORE_FPU_CONTEXT_INTERNAL
	portcontextRESTORE_FPU_CONTEXT_INTERNAL

RISCV Add FPU context save #1250

RISCV Add FPU context save #1250

Conversation

cubidesj commented Feb 21, 2025 • edited by kstribrnAmzn Loading

Description

Test Steps

Checklist:

cubidesj commented Feb 21, 2025

jasonpcarroll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aggarg commented Mar 4, 2025

aggarg commented Mar 5, 2025 • edited Loading

cubidesj commented Mar 6, 2025

sonarqubecloud bot commented Mar 6, 2025

Quality Gate passed

cubidesj commented Feb 21, 2025 •

edited by kstribrnAmzn

Loading

aggarg commented Mar 5, 2025 •

edited

Loading