Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Arch arm mpu move internal api #16136

Merged
merged 3 commits into from
May 30, 2019
Merged

Arch arm mpu move internal api #16136

merged 3 commits into from
May 30, 2019

Conversation

ioannisg
Copy link
Member

A clean-up patch; moving an internal API header where it belongs (IMHO) :)

@ioannisg ioannisg added area: ARM ARM (32-bit) Architecture Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. labels May 14, 2019
@ioannisg ioannisg added this to the v2.0.0 milestone May 14, 2019
@ioannisg ioannisg mentioned this pull request May 14, 2019
Closed
26 tasks
agansari
agansari approved these changes May 14, 2019
View reviewed changes
Copy link
Collaborator

@agansari agansari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ioannisg looks okay

@ioannisg ioannisg force-pushed the arch_arm_mpu_move_internal_api branch from 7beee9b to f8782da Compare May 15, 2019 10:31
galak
galak reviewed May 15, 2019
View reviewed changes
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks fine, but curious about why arm_core_mpu_disable has to get exported and what actually the test case is doing in disable_mmu_mpu().

Feels odd to me that we are "Testing" something that doesnt have an exposed API.

tests/kernel/mem_protect/userspace/src/main.c
@@ -23,7 +23,7 @@
#endif

#if defined(CONFIG_ARM)
#include <arch/arm/cortex_m/mpu/arm_core_mpu_dev.h>
extern void arm_core_mpu_disable(void);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this made me look at the actual test, why do we have a disable_mmu_mpu() if we don't have an exposed API related to it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewboie FYI

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@galak I get the feeling that this test is there to show that we can't disable the MPU from user mode, right @andrewboie ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't object if we deleted that test. There's some other dubious cases in there as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that it "tests" that user-mode can't play with the mpu :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we trying to prove whether the CPU works or the software?
Proving that the CPU works is a waste of time IMO

Copy link
Member Author

@ioannisg ioannisg May 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap, that's in fact, correct. So do you confirm that this test was all about verifying hardware, in the first place? If so, I'll just add a commit, to delete it :)

galak
galak previously requested changes May 15, 2019
View reviewed changes
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove arm_core_mpu_disable() from arm_core_mpu_dev.h since its effectively static and the only non-static use is the explicit extern in the testcase.

@ioannisg
Copy link
Member Author

ioannisg commented May 15, 2019 •
edited
Loading

Can we remove arm_core_mpu_disable() from arm_core_mpu_dev.h since its effectively static and the only non-static use is the explicit extern in the testcase.

Hm, I think yes - we used to invoke the enable/disable functions in arm_core_mpu.c, but perhaps not anymore. Will add a commit that does it.

@ioannisg
Copy link
Member Author

Can we remove arm_core_mpu_disable() from arm_core_mpu_dev.h since its effectively static and the only non-static use is the explicit extern in the testcase.

Done now, thanks.

@ioannisg ioannisg requested a review from galak May 15, 2019 12:24
@galak
Copy link
Collaborator

galak commented May 15, 2019

LGTM, will wait to see if any discussion from @andrewboie on what the test is trying to do.

ioannisg added 3 commits May 29, 2019 08:58
@ioannisg
arch: arm: mpu: move internal API inside arch/arm/cortex_m/mpu
595cf90 
arm_core_mpu_dev.h is an internal API, and is not supposed to
be directly called by kernel / application functions, therefore,
we can move it inside arch/arm/core/cortex_m/mpu directory.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
@ioannisg
tests: kernel: userspace: explicitly declare function for the test
fc06809 
In the wake of moving the internal API header arm_core_mpu_dev.h
into arch/arm/cortex_m/mpu, we need to explicitly declare the
arm_core_mpu_disable() function in the userspace test. Note that
arm_core_mpu_disable() (as any other function in this internal
API) is not supposed to be called directly by kernel/application
functions; an exception is allowed in this test suite, so we are
able to test the MPU disabling functionality.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
@ioannisg
arch: arm: mpu: remove arm_core_mpu_enable/disable internal API
7720d45 
arm_core_mpu_enable() and arm_core_mpu_disable() functions are
effectively static functions, only used in the drivers for ARM
and NXP MPU, therefore, we do not need to expose them in the
arm_core_mpu_dev.h header.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
@ioannisg ioannisg force-pushed the arch_arm_mpu_move_internal_api branch from 720d6a4 to 7720d45 Compare May 29, 2019 07:06
@ioannisg ioannisg added the Enhancement Changes/Updates/Additions to existing features label May 29, 2019
@MaureenHelm MaureenHelm merged commit 18e80ae into zephyrproject-rtos:master May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewboie andrewboie andrewboie approved these changes

@agansari agansari agansari approved these changes

@MaureenHelm MaureenHelm MaureenHelm approved these changes

@andyross andyross Awaiting requested review from andyross

@nashif nashif Awaiting requested review from nashif

@galak galak Awaiting requested review from galak

Assignees
No one assigned
Labels
area: ARM ARM (32-bit) Architecture Enhancement Changes/Updates/Additions to existing features Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc.
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ioannisg @galak @andrewboie @MaureenHelm @agansari