armv7-m/arm_mpu.c: MPU regions >= 2GB cannot be configured

[danielappiagyei-bc]

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:

const size_t SIZE_2_GiB = 2 * 1024 * 1024 *1024; /* 0x80000000 */
mpu_configure_region(/*base = */ SIZE_2_GiB, 
                                   /*size = */ SIZE_2_GiB,
                                  /*flags =*/ <whatever>);

When we make it to the DEBUG_ASSERTs in the function, we'll have:

l2size = 31;
alignedbase = SIZE_2_GiB;

The first assert, DEBUGASSERT(alignedbase + (1 << l2size) >= base + size);,
will expand to DEBUGASSERT( 0 >= 0) (due to unsigned integer overflow) and pass. The second assert, however,

DEBUGASSERT(l2size == 5 ||
              alignedbase + (1 << (l2size - 1)) < base + size);

will expand to

DEBUGASSERT(false ||
            SIZE_2_GiB + (1 << (30)) < 0);

and fail because only the right-hand-side will overflow. The workaround for this would be:

• saying we only allow regions of 1GiB max size to be configured,
• using uint64_t for our arguments and arithmetic in this entire file,
• just have a special case where values for base and size >= 2GiB are handled differently, or
• just telling users to program the MPU registers directly themselves rather than relying on this convenience function

[acassis]

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?

[anchao]

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?

[acassis]

@danielappiagyei-bc please submit a PR with your fix

[danielappiagyei-bc]

for sure, will do

[danielappiagyei-bc]

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?

[anchao]

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?

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

[danielappiagyei-bc]

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?

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 🙂

[danielappiagyei-bc]

Will get back to this in a few weeks, appreciate the patience, am busy with work and personal