cpu/cortexm_common: add function to check address for validity by olegart · Pull Request #10051 · RIOT-OS/RIOT

olegart · 2018-09-26T15:31:39Z

Contribution description

Adds extremely useful function to check memory addresses for validity on Cortex-M3/M4/M7 MCUs.

Can be used to determine memory sizes, peripherals availability, etc. in runtime to build firmware that runs effectively on a differenent MCUs without recompiling.

Testing procedure

Build and run tests/cpu_cortexm_address_check for your board, then type 'check address' in the shell, where address is a memory address in hex notation (with '0x' prefix). Check your MCUs' datasheet for memory map.

E.g. on STM32L152RET6:

check 0x1FF02000
Address 0x1ff02000 is NOT valid. Accessing it will result in BusFault

check 0x1FF01999
Address 0x1ff01999 is valid. Feel free to access it

(0x1FF01999 is an upper address of the System Memory section)

Issues/PRs references

~~Cortex-M0 not supported, it needs completely different approach.~~

P.S. Will provide real-world usage example in the next PR.

olegart · 2018-09-26T15:59:27Z

See #10052 for example

PeterKietzmann · 2018-09-27T12:10:30Z

@olegart would you address the codays issue please?

jcarrano

Interesting functionality. Please see the issues regarding CPS and interrupts.

jcarrano · 2018-09-27T12:36:57Z

cpu/cortexm_common/cortexm_init.c

+        is_valid = false;
+    }
+
+    __asm volatile ("cpsie f;");


What if the mask was disabled when calling this function. They will be enabled. Maybe it's better to save/restore the flag.

I'm pretty sure people should not play with FAULTMASK without a very solid reason, so it should be enabled at all times except when doind some very specific things.

Also, not sure if it's a good idea to write FAULTMASK register directly - there's MSR/MRS instructions to do so, but I can't find if there are any undesired consequences compared to using CPS instructions. I've tried and MSR/MRS (__get_FAULTMASK / __set_FAULTMASK from CMSIS) work, but still not sure if it's ok to use it so.

Regular interrupts (controlled by irq_enable / irq_disable) won't change anyway, it's PRIMASK, not FAULTMASK.

P.S. Changed assember to inline CMSIS instructions.

I'm pretty sure people should not play with FAULTMASK without a very solid reason, so it should be enabled at all times except when doind some very specific things.

Agreed, but I wanted to prevent any surprising weird interaction.

Also, not sure if it's a good idea to write FAULTMASK register directly

If you look at https://github.com/RIOT-OS/RIOT/blob/master/cpu/cortexm_common/irq_arch.c it is doing that (it's even using cmsis builtins). I'm not sure why it is doing what it is doing in irq_enable (I just opened a bug report - #10076).

It may work to have a fault_disable/fault_restore.

Ok, as no one sees no harm either, I've changed CPS to direct access with MRS with FAULTMASK state preservation. Moved BFHFNMIGN after interrupts disables too.

P.S. Don't think we need separate fault_disable function in public API.

cpu/cortexm_common/cortexm_init.c

olegart · 2018-09-28T14:18:13Z

@PeterKietzmann Codacy is in disagreement with GCC here.

jcarrano · 2018-09-28T15:43:12Z

@PeterKietzmann Codacy is in disagreement with GCC here.

In any case, using PRIx32 for the scanf should get rid of the problem.

jcarrano · 2018-09-28T16:04:52Z

cpu/cortexm_common/cortexm_init.c

+    SCB->CFSR |= BFARVALID_MASK;
+
+    /* Ignore BusFault by enabling BFHFNMIGN */
+    SCB->CCR |= SCB_CCR_BFHFNMIGN_Msk;


Sorry for not raising this before. Why is BFHFNMIGN set before disabling interrupts?

olegart · 2018-09-28T16:17:12Z

In any case, using PRIx32 for the scanf should get rid of the problem.

Doesn't work in newlib-nano. It builds, but produces some garbage.

kaspar030 · 2018-09-28T16:34:37Z

cpu/cortexm_common/cortexm_init.c

+    (void) address;
+
+    printf("[warning] %s: Cortex-M0 doesn't have BusFault\n", __func__);
+    return true;


Is there a use-case for having this compiled in on Cortex-M0? If not, I suggest guarding the whole function.

(returning true seems wrong)

Or wouldn't it be possible to catch an "expected" hardfault and that way somehow replicate the functionality on M0?

@kaspar030 Yes, just wrote a catcher for M0.

olegart · 2018-09-29T23:08:18Z

Added address probing for Cortex-M0 cores too.

M0 doesn't have BusFault, so we need to catch intended HardFaults by looking for some magic numbers in registers, than set some other register to indicate HardFault has occured and return from HardFault to the next instruction.

Here R1 and R2 are set with magic numbers (two different numbers just to be sure) and R5 is modified inside HardFault.

Tested with STM32F042K6T6:

check 0x1FF02000
Address 0x1ff02000 is NOT valid. Accessing it will result in BusFault

check 0x40007999
Address 0x40007999 is valid. Feel free to access it

jcarrano

I ran the test in the SAMR21 (M0+) and KW22D (M4) . Seems to work. I wonder if there is some address that is invalid for most ARM chips, so that the test can be made automated (I prefer automated tests).

There's a couple of issues in the test that I missed in the previous reviews. Other than that I'm fine.

jcarrano · 2018-10-04T14:56:39Z

tests/cpu_cortexm_address_check/Makefile

@@ -0,0 +1,8 @@
+include ../Makefile.tests_common
+
+BOARD = nucleo-l152re


Please use a ?= for the board. It took me some time and a reboot to figure out why I could not flash.

jcarrano · 2018-10-04T14:59:14Z

tests/cpu_cortexm_address_check/main.c

+    char *address;
+    uint32_t address_value;
+
+    sscanf(argv[1] + 2, "%lx", &address_value); /* ignore 0x prefix */


Sorry for missing this one too. If argv[1] is shorter than 3 this code is dangerous. It also causes weirdness when inputting a number not in 0x format.
I suggest strtol with base=0.

And some code to check if the parse is successful.

Fixed.

No way to check parsing, as strtol outputs 0 in case of error, and 0 is a valid address. Still, user will see it.

It can be done, but just leave it, it's a test after all. The important thing is that it does not blow up and that is already fixed.

BTW, here is how to do it:

In particular, if *nptr is not '\0' but **endptr is '\0' on return, the entire string is valid.

jcarrano

All issues addressed. Code tested in M0+ and M3.

I think @kaspar030's issues regarding M0 support were addressed too.

jcarrano · 2018-10-11T12:20:57Z

@olegart Squash, fix the whitespace issue and we're ready to go (I was waiting for @kaspar030 because I did not want to just dismiss his review.)

olegart · 2018-10-11T13:15:36Z

Done.

I wonder if there is some address that is invalid for most ARM chips

Addresses above 0xEFFFFFFF should be.

And 0xE0000000 should be valid on all Cortex-M as it's the beginning of a Cortex Private Peripheral Bus.

jcarrano · 2018-10-11T13:49:13Z

Seems like clang does not like the assembly code.

Also, what happened to the test? Are you splitting it up?

olegart · 2018-10-11T15:12:41Z

Yep, it seems GCC and CLANG have a bit different assembler understanding.

This should satisfy both.

jcarrano · 2018-10-11T17:11:10Z

The test is failing because the CI is trying to build it for non-ARM boards. Unfortunately we don't have a way to blacklist based on architecture. Two options:

Add all non-ARM boards to the blacklist.
Add some M0 and some M3/M4 to the whitelist (adding the samr21xpro would be appreciated) . I think this is the option that makes sense.

fjmolinas · 2019-03-14T16:56:10Z

@olegart @jcarrano Hi, I would like to use the PR to get low power for stm32l1, it seems this was ready to merge except for the white list part. @olegart do you think you can add the whitelist part and rebase it? if not, and if you don't mind I can use your branch and implement the changes needed to merge.

jcarrano · 2019-03-14T20:20:40Z

@fjmolinas I'm not the author, but I think it should be fine. I already took over one of @olegart's PRs dealing with AES. He will get credited anyway. I'll review it.

olegart mentioned this pull request Sep 26, 2018

cpu/stm32_common: minimize power consumption on STM32L1 #10052

Closed

PeterKietzmann requested review from haukepetersen and jnohlgard September 27, 2018 12:09

PeterKietzmann added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Sep 27, 2018

jcarrano suggested changes Sep 27, 2018

View reviewed changes

olegart force-pushed the riot-cortexm-address-check branch from 7d7dd69 to 86a6a6e Compare September 28, 2018 14:48

RIOT-OS deleted a comment Sep 28, 2018

jcarrano reviewed Sep 28, 2018

View reviewed changes

RIOT-OS deleted a comment Sep 28, 2018

kaspar030 requested changes Sep 28, 2018

View reviewed changes

RIOT-OS deleted a comment Sep 29, 2018

jcarrano suggested changes Oct 4, 2018

View reviewed changes

RIOT-OS deleted a comment Oct 5, 2018

jcarrano approved these changes Oct 5, 2018

View reviewed changes

jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 11, 2018

olegart force-pushed the riot-cortexm-address-check branch from e2f8ad9 to 066d508 Compare October 11, 2018 13:09

cpu/cortexm_common: function to check address validity

ecdc79e

olegart force-pushed the riot-cortexm-address-check branch from 066d508 to ecdc79e Compare October 11, 2018 15:10

This was referenced Apr 8, 2019

cpu/cortexm_common: function to check address validity #11358

Merged

cpu/stm32: optimize power consumption #11359

Closed

jcarrano closed this in #11358 May 13, 2019

		@@ -0,0 +1,8 @@
		include ../Makefile.tests_common

		BOARD = nucleo-l152re

Conversation

olegart commented Sep 26, 2018 • edited by jcarrano Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

olegart commented Sep 26, 2018

Uh oh!

PeterKietzmann commented Sep 27, 2018

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

olegart commented Sep 28, 2018

Uh oh!

jcarrano commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olegart commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olegart commented Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcarrano left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcarrano commented Oct 11, 2018

Uh oh!

olegart commented Oct 11, 2018

Uh oh!

jcarrano commented Oct 11, 2018

Uh oh!

olegart commented Oct 11, 2018

Uh oh!

jcarrano commented Oct 11, 2018

Uh oh!

fjmolinas commented Mar 14, 2019

Uh oh!

jcarrano commented Mar 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

olegart commented Sep 26, 2018 •

edited by jcarrano

Loading

olegart commented Sep 29, 2018 •

edited

Loading

jcarrano left a comment •

edited

Loading