Open danielappiagyei-bc opened 1 month ago
Hi @danielappiagyei-bc nice finding! Thank you very much?
I think the second option is a better approach, because will work for all cases without special cases or other inconveniences.
@anchao what do you think?
Hi @danielappiagyei-bc nice finding! Thank you very much?
I think the second option is a better approach, because will work for all cases without special cases or other inconveniences.
@anchao what do you think?
I also agree with the second option, could we add the uint64_t only for this function to handle the case of base + size
overflow?
@danielappiagyei-bc please submit a PR with your fix
for sure, will do
I also agree with the second option, could we add the uint64_t only for this function to handle the case of base + size overflow?
@anchao The other issue is that because the argument base
to this function is a uint32_t, you cannot specify a region size as 4GiB (because (uint32_t)(4 1024 1024 * 1024) overflows to 0
). You may want to set a rule for the entire memory region due to erratum ERR011573
for Cortex-M7 MPUs specified here. The workaround states:
Workaround: Instead of leaving memory unmapped, software should use MPU region 0 to cover all unmapped memory and make this region execute-never and inaccessible. That is, MPU_RASR0 should be programmed with: • MPU_RASR0.ENABLE = 1; MPU region 0 enable. • MPU_RASR0.SIZE = b11111; MPU region 0 size = 2^32 bytes to cover entire memory.
To fix this, I need to make base
uint64_t along with any function that base
is passed to. So, mpu_subregion()
and (maybe?) mpu_log2regionceil()
need uint64_t args. Is this okay?
I also agree with the second option, could we add the uint64_t only for this function to handle the case of base + size overflow?
@anchao The other issue is that because the argument
base
to this function is a uint32_t, you cannot specify a region size as 4GiB (because (uint32_t)(4 1024 1024 * 1024) overflows to0
). You may want to set a rule for the entire memory region due to erratumERR011573
for Cortex-M7 MPUs specified here. The workaround states:Workaround: Instead of leaving memory unmapped, software should use MPU region 0 to cover all unmapped memory and make this region execute-never and inaccessible. That is, MPU_RASR0 should be programmed with: • MPU_RASR0.ENABLE = 1; MPU region 0 enable. • MPU_RASR0.SIZE = b11111; MPU region 0 size = 2^32 bytes to cover entire memory.
To fix this, I need to make
base
uint64_t along with any function thatbase
is passed to. So,mpu_subregion()
and (maybe?)mpu_log2regionceil()
need uint64_t args. Is this okay?
This is a programming error and the compiler will report corresponding warning like Woverflow
. BTW similar data types are used in popular RTOS such as Zephyr:
https://github.com/zephyrproject-rtos/zephyr/blob/main/arch/arm/include/kernel_arch_data.h#L52-L56
I also agree with the second option, could we add the uint64_t only for this function to handle the case of base + size overflow?
@anchao The other issue is that because the argument
base
to this function is a uint32_t, you cannot specify a region size as 4GiB (because (uint32_t)(4 1024 1024 * 1024) overflows to0
). You may want to set a rule for the entire memory region due to erratumERR011573
for Cortex-M7 MPUs specified here. The workaround states:Workaround: Instead of leaving memory unmapped, software should use MPU region 0 to cover all
unmapped memory and make this region execute-never and inaccessible. That is,
MPU_RASR0 should be programmed with:
• MPU_RASR0.ENABLE = 1; MPU region 0 enable.
• MPU_RASR0.SIZE = b11111; MPU region 0 size = 2^32 bytes to cover entire memory.
To fix this, I need to make
base
uint64_t along with any function thatbase
is passed to. So,mpu_subregion()
and (maybe?)mpu_log2regionceil()
need uint64_t args. Is this okay?This is a programming error and the compiler will report corresponding warning like
Woverflow
. BTW similar data types are used in popular RTOS such as Zephyr:https://github.com/zephyrproject-rtos/zephyr/blob/main/arch/arm/include/kernel_arch_data.h#L52-L56
Fair enough, I guess my point is that the architecture supports configuring a 4GiB region whereas this API (and zephyr's apparently) does not. But I understand we can't handle every special case while keeping the API generic, consistent between architectures, and simple. I'll make the requested changes 🙂
Will get back to this in a few weeks, appreciate the patience, am busy with work and personal
Hi, minor bug to report regarding the MPU configuration in armv7-m:
The cortex-M7 MPU supports configuring regions up to 4GB in size (see ARMv7-M Arch. Reference Manual, System Address Map :: B3.5 Protected Memory System Architecture). (Download link here)
In arm_mpu.c, say you want to configure a 2 GiB sized region 2GiB offset from address 0:
When we make it to the
DEBUG_ASSERT
s in the function, we'll have:The first assert,
DEBUGASSERT(alignedbase + (1 << l2size) >= base + size);
, will expand toDEBUGASSERT( 0 >= 0)
(due to unsigned integer overflow) and pass. The second assert, however,will expand to
and fail because only the right-hand-side will overflow. The workaround for this would be:
base
andsize
>= 2GiB are handled differently, or