armv8-m: making full SRAM run-time partitioning (MPU gap filling) optional

[ioannisg]

Addresses #19067 for ARM.

It should be working atm.
I would like a review on the Kconfig symbol name I've used, and the default behavior.
I am also going to add a test of the feature,

[andrewboie]

I think it should be off by default -- an extra feature that can be enabled if users want it. It's not required for user mode at any rate.

As far as the naming of the Kconfig, MPU_FULL_BACKGROUND_SRAM_PARTITIONING is rather long winded, but sure, no complaints if you want to call it that.

[zephyrbot]

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[ioannisg]

I want to have this in for Zephyr v2.1 release; is it possible to have additional reviewing (besides @andrewboie ) on the introduced Kconfig option at least?

[ulfalizer]

Note that the direct (depends on) dependencies of symbols that are defined in multiple locations get ORed together, so there's no way to add extra dependencies by defining a symbol more than once.

The dependencies of MPU_SRAM_GAP_FILLING end up as this (copied from the menuconfig):

Direct dependencies (=n):
     USERSPACE(=n) && ARM_MPU(=n) && CPU_HAS_MPU(=y) && ARM(=y)  (=n)
  || MPU_REQUIRES_NON_OVERLAPPING_REGIONS(=n)

If MPU_REQUIRES_NON_OVERLAPPING_REGIONS is y, it will always be possible to enable MPU_SRAM_GAP_FILLING. Is that okay here?

A tip is to go into the menuconfig and jump to each of the definition locations of the symbol. That might help with figuring out how it works. Basically, the definition in arch/Kconfig will always be visible as long as MPU_REQUIRES_NON_OVERLAPPING_REGIONS is y, allowing the symbol to be toggled there.

[ioannisg]

Note that the direct (depends on) dependencies of symbols that are defined in multiple locations get ORed together, so there's no way to add extra dependencies by defining a symbol more than once.

The dependencies of MPU_SRAM_GAP_FILLING end up as this (copied frrom the menuconfig):

Direct dependencies (=n):
     USERSPACE(=n) && ARM_MPU(=n) && CPU_HAS_MPU(=y) && ARM(=y)  (=n)
  || MPU_REQUIRES_NON_OVERLAPPING_REGIONS(=n)

If MPU_REQUIRES_NON_OVERLAPPING_REGIONS is y, it will always be possible to enable MPU_SRAM_GAP_FILLING. Is that okay here?

A tip is to go into the menuconfig and jumping to each of the definition locations of the symbol. That might help with figuring out how it works. Basically, the definition in arch/Kconfig will always be visible as long as MPU_REQUIRES_NON_OVERLAPPING_REGIONS is y, allowing the symbol to be toggled there.

Thanks so much @ulfalizer for this feedback.

If MPU_REQUIRES_NON_OVERLAPPING_REGIONS is y, it will always be possible to enable MPU_SRAM_GAP_FILLING. Is that okay here?

Not exactly. I need MPU_SRAM_GAP_FILLING to have a direct dependency on MPU_REQUIRES_NON_OVERLAPPING_REGIONS, but I want it to be defined only if USERSPACE is also defined. I had the impression that dependencies are ANDed instead of ORed. So what's your proposal?

[ioannisg]

@ulfalizer can we put the definition into if USERSPACE block, to make it defined only if USERSPACE is also defined? Then, what will happen with the dependency on MPU_REQUIRES_NON_OVERLAPPING_REGIONS?

[ulfalizer]

(Note that if FOO is just a shorthand for adding depends on FOO to the symbol(s) within it. if and depends on are exactly the same thing. I think this is something that confuses people re. Kconfig, where people assume if does more than that. :)

If a symbol is defined in multiple locations and should depend on USERSPACE, then all definition locations need to have depends on USERSPACE (or be within an if USERSPACE -- same thing), since the dependencies get ORed together.

Personally, I'd try to refactor it so that there's just a single definition in arch/Kconfig, maybe along with some helper symbols.

[ioannisg]

Personally, I'd try to refactor it so that there's just a single definition in arch/Kconfig, maybe along with some helper symbols.

That's hard; I raised the definition up in arch/Kconfig, as it is essentially a cross-arch option, but I want to keep re-defining it in arch/arm/cortex-m/mpu/Kconfig , so I can have the additional help text that is ARMv8-M specific. But it is becoming clear from your comments that dependencies are always ORed so I cannot add additional dependencies here (so I need to move depends on USERSPACE up in the top-level definition, correct?)

So how can I do it in a way it satisfies you @ulfalizer ? :)

[ulfalizer]

These two are 100% identical:

if FOO
if BAR

config FOO
	bool "foo"
	depends on BAZ

endif
endif

config FOO
	bool "foo"
	depends on FOO && BAR && BAZ

if does not "cancel" the definition, but only modified the dependencies, like depends on does. When a symbol is defined more than once, the dependencies get ||'d together.

[ulfalizer]

A tip is to play around with this in menuconfig btw, because it'll make it clear how multiple definitions work:

config DEP1
	bool "dep1"

config DEP2
	bool "dep2"

config MULTIDEF
	bool "first multidef prompt"
	depends on DEP1

config MULTIDEF
	bool "second multidef prompt"
	depends on DEP2

[ioannisg]

The point is that : only the ARCH-specific option will be used in the code. the top-arch option will only be used in Kconfig, to enable the ARM-specific option.

[ulfalizer]

Was thinking from an interface design perspective. There'll be a button you toggle that says "MPU SRAM gap filling" or the like, but it won't do anything unless you also toggle "User mode threads".

Feels like it gets lost sometimes that Kconfig defines a configuration interface. :)

[ulfalizer]

As long as that wouldn't be weird to users, it'll work at least.

[ulfalizer]

My preferred solution would be either

1. A single definition in arch/Kconfig, with some arch-specific notes in the help text, and maybe some helper symbols to adjust it for different arch capabilities

2. Have multiple arch-specific definitions (wouldn't necessarily need to have the same name either), and skip the definition in arch/Kconfig

I won't impose anything though. Just keep the multiple-definition dependency gotchas in mind.

[ioannisg]

@ulfalizer could you come back to this, briefy?
FYI, I integrated your suggestion of _not_duplicating the definition of the Kconfig option - it is really cleaner. So, I introduced an internal hidden ARM-only option that takes the configuration enforced by the user. The internal option is used in source code.

[ulfalizer]

@ulfalizer could you come back to this, briefy?
FYI, I integrated your suggestion of _not_duplicating the definition of the Kconfig option - it is really cleaner. So, I introduced an internal hidden ARM-only option that takes the configuration enforced by the user. The internal option is used in source code.

Personally, I think I'd either have a single shared symbol in arch/Kconfig with some platform-specific notes in the help text, or separate symbols per arch/<platform>/mpu/Kconfig (maybe along with factoring out common ones into some Kconfig.*_mpu files or the like).

The main motivation is to avoid complicated multiple definition stuff where not many people know how it works (and that's easy to get wrong re. dependencies).

I won't block anything here though, so go with whatever you prefer. :)

Please also check how things look in the menuconfig. With the current approach, I suspect the help text for ARM_MPU_SRAM_GAP_FILLING might be hard to notice, because it's not what you get when you press ? on the configurable symbol in the menuconfig.

[aescolar]

@ioannisg note that you still have in this branch the sanitycheck hotfix which is already in master (I do not understand why github is not complaining about the conflict and let's one press the button..) Please disregard this review on your own when you fix it (no need to ping me back)

[ioannisg]

@ioannisg note that you still have in this branch the sanitycheck hotfix which is already in master (I do not understand why github is not complaining about the conflict and let's one press the button..) Please disregard this review on your own when you fix it (no need to ping me back)

The commit is present because I did not rebase.
There is no conflict because the commit in this PR is identical to what was merged (If I understand GH properly)

not applicable :)

[ioannisg]

@ulfalizer could you come back to this, briefy?
FYI, I integrated your suggestion of _not_duplicating the definition of the Kconfig option - it is really cleaner. So, I introduced an internal hidden ARM-only option that takes the configuration enforced by the user. The internal option is used in source code.

Personally, I think I'd either have a single shared symbol in arch/Kconfig with some platform-specific notes in the help text, or separate symbols per arch/<platform>/mpu/Kconfig (maybe along with factoring out common ones into some Kconfig.*_mpu files or the like).

The main motivation is to avoid complicated multiple definition stuff where not many people know how it works (and that's easy to get wrong re. dependencies).

I won't block anything here though, so go with whatever you prefer. :)

Please also check how things look in the menuconfig. With the current approach, I suspect the help text for ARM_MPU_SRAM_GAP_FILLING might be hard to notice, because it's not what you get when you press ? on the configurable symbol in the menuconfig.

@ulfalizer I finally went along with your preference here, i.e. removed the internal option, and added the ARMv8-M specific explanatory text in the config ARM_MPU option.