[BUG] [RISC-V] configMTIMECMP_BASE_ADDRESS (64-bit) stored in 32-bit int

[xerpi]

Describe the bug
configMTIMECMP_BASE_ADDRESS, which can be a 64-bit address is currently being stored in a 32-bit integer:

uint32_t const ullMachineTimerCompareRegisterBase = configMTIMECMP_BASE_ADDRESS;

GCC port: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/master/portable/GCC/RISC-V/port.c#L94
IAR port: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/master/portable/IAR/RISC-V/port.c#L94

Target
RISC-V port.

[RichardBarry]

Hmm, judging by the variable name, and where it is used, it was intended to be 64-bit. Suspect this has just not been tested on a device where that register appears beyond a 32-bit address.

Interested to know which device you are using to see if it can be part of our test setup.

[xerpi]

I was trying to compile for a 64-bit machine (with actually configMTIMECMP_BASE_ADDRESS being lower than 2³²-1) and the compiler failed given the address constant had ull as a suffix (included from a device HAL, so I can't change it). Maybe the data type should be uintptr_t instead.

Another related problem (I should probably file another bug): while during timer re-arming the XLEN is checked (32 or 64) and the accesses to the mtimeand mtimecmp are done using lw or ld accordingly, in the timer initialization function mtimeis always accessed as (two) 32-bit numbers, which is a problem in our platform (I can't tell which) because the timer IP only supports 64-bit accesses. And yet another problem: our platform has an isolated Hart with a mhartid much bigger than 0 and it has its own timer, so in this platform, assuming that each hart has its own mtimecmp at: ullMachineTimerCompareRegisterBase + ( ulHartId * sizeof( uint64_t ) ) is wrong. Maybe we could introduce a boolean macro like configMTIMECMP_ADDRESS_DEPENDS_HARTID to enable/disable + mhartid * 8 when accessing mtimecmp.

[xerpi]

Another small issue with the RISC-V 64-bit port: https://www.freertos.org/Using-FreeRTOS-on-RISC-V.html#GCC_COMMAND_LINE_OPTIONS

void external_interrupt_handler( uint32_t cause );

cause is read from the mcause CSR, which is MXLENin length, and it is probably 64-bits on most 64-bit platforms.

[RichardBarry]

Really appreciate you taking the time to report these!

[xerpi]

I see that when there's an unknown synchronous exception (the only ones currently handled by FreeRTOS are ECALLs), the code spins forever in a loop:

is_exception:
	csrr t0, mcause						/* For viewing in the debugger only. */
	csrr t1, mepc						/* For viewing in the debugger only */
	csrr t2, mstatus
	j is_exception						/* No other exceptions handled yet. */

I think it would be nice to be able to jump to a platform-specific routine to handle things like emulated instructions, unaligned memory accesses, etc, and to also pass the saved context as a parameter to that C-function handler (custom_exception_handler(mcause, mepc, mstatus, regs)?) so that one can modify the registers with something like: regs[x] = y;
Something similar to portasmHANDLE_INTERRUPT for external interrupts, but for exceptions in this case.
And the same capability (custom platform-specific handler) for Software Interrupts (IPIs. (custom_software_interrupt_handler).