Skip to content

Fix RISC-V configMTIMECMP_BASE_ADDRESS (64-bit) stored in 32-bit int #1176

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.

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

Merged

Conversation

vishwamartur
Copy link
Contributor

Related to #189

Update configMTIMECMP_BASE_ADDRESS to be stored in a 64-bit integer.

  • Change the type of ullMachineTimerCompareRegisterBase to uint64_t in portable/GCC/RISC-V/port.c.
  • Change the type of ullMachineTimerCompareRegisterBase to uint64_t in portable/IAR/RISC-V/port.c.
  • Update the initialization of ullMachineTimerCompareRegisterBase to use configMTIMECMP_BASE_ADDRESS in both files.

Related to FreeRTOS#189

Update `configMTIMECMP_BASE_ADDRESS` to be stored in a 64-bit integer.

* Change the type of `ullMachineTimerCompareRegisterBase` to `uint64_t` in `portable/GCC/RISC-V/port.c`.
* Change the type of `ullMachineTimerCompareRegisterBase` to `uint64_t` in `portable/IAR/RISC-V/port.c`.
* Update the initialization of `ullMachineTimerCompareRegisterBase` to use `configMTIMECMP_BASE_ADDRESS` in both files.
@vishwamartur vishwamartur requested a review from a team as a code owner November 1, 2024 07:06
@ActoryOu
Copy link
Member

ActoryOu commented Nov 2, 2024

Hello @vishwamartur,
Thanks for your contribution! Could you share your verification results on this PR? I'm curious what platform you're using to verify this.

Thank you!

@aggarg
Copy link
Member

aggarg commented Nov 4, 2024

@vishwamartur Would you please share how you tested these changes on 32-bit and 64-bit platforms as the same code is used on both the platforms.

@vishwamartur
Copy link
Contributor Author

Hi @ActoryOu and @aggarg,

Thank you for the feedback! I haven’t tested these changes yet, as I currently lack access to a compatible platform for both 32-bit and 64-bit verification. If you have any suggestions for alternative testing methods or an emulator that could support this setup, I’d be glad to give it a try.

Thanks again!

Copy link

sonarqubecloud bot commented Nov 4, 2024

@aggarg
Copy link
Member

aggarg commented Nov 4, 2024

I tested this change using Qemu and looks good to me.

On a side note, I'd suggest to use the PR template as it guides you what all information to provide when creating a PR (which includes testing).

@aggarg aggarg merged commit b4a9707 into FreeRTOS:main Nov 4, 2024
16 checks passed
@landretk
Copy link

This produces a compiler warning on 32-bit because it truncates a 64-bit integer into a pointer.

FreeRTOS doesn't appear to have a type that can hold an integer with the same width as a pointer. uintptr_t seems to be disallowed because it's C99, unsigned long because it's not a stdint or freertos type although it's correct for ilp32 and lp64, and none of the freertos types are defined as being able to hold an address. configMTIMECMP_BASE_ADDRESS might need to be used directly without being stored in an intermediate variable, unless UBaseType_t would be accepted

aggarg added a commit to aggarg/FreeRTOS-Kernel that referenced this pull request Mar 17, 2025
Use architecture-dependent UBaseType_t instead of fixed type for
ullMachineTimerCompareRegisterBase. This type is defined to uint32_t or
uint64_t based on XLEN, resolving warnings on 32-bit platforms.

Reported by landretk@ on the PR FreeRTOS#1176.

Signed-off-by: Gaurav Aggarwal <[email protected]>
@aggarg
Copy link
Member

aggarg commented Mar 17, 2025

unless UBaseType_t would be accepted

Thank you pointing this out. I agree that we should use UBaseType_t as it is defined correctly based on XLEN. Here is the PR to address this - #1258.

Thank you again for bringing this to our attention.

aggarg added a commit that referenced this pull request Mar 17, 2025
Use architecture-dependent UBaseType_t instead of fixed type for
ullMachineTimerCompareRegisterBase. This type is defined to uint32_t or
uint64_t based on XLEN, resolving warnings on 32-bit platforms.

Reported by landretk@ on the PR #1176.

Signed-off-by: Gaurav Aggarwal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants