Skip to content

Handle armv8m unpriv_access with SAU #486

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Handle armv8m unpriv_access with SAU #486

wants to merge 1 commit into from

Conversation

@orenc17
Copy link

@orenc17 orenc17 commented Aug 14, 2017

No description provided.

Patater
Patater reviewed Aug 14, 2017
View reviewed changes
core/vmpu/src/mpu_armv8m/vmpu_armv8m_unpriv_access.c Outdated
if ((vmpu_unpriv_test_range(addr, UVISOR_UNPRIV_ACCESS_SIZE(size)) & (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) == (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) {
//if ((vmpu_unpriv_test_range(addr, UVISOR_UNPRIV_ACCESS_SIZE(size)) & (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) == (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) {
//this should be slower substantualy
if (vmpu_buffer_access_is_ok(g_active_box, addr, UVISOR_UNPRIV_ACCESS_SIZE(size))){
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to use the loop anymore, nor try to recover.

@orenc17
Copy link
Author

orenc17 commented Aug 14, 2017

works well on v8

alzix
alzix suggested changes Aug 14, 2017
View reviewed changes
Copy link
Contributor

@alzix alzix left a comment

Choose a reason for hiding this comment

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

squash the commits and write appropriate commit message describing the problem and the fix

core/vmpu/src/mpu_armv8m/vmpu_armv8m_unpriv_access.c Outdated
}
if (++tries > 1 || !vmpu_fault_recovery_mpu(0, 0, addr, 0)) {
break;
//if ((vmpu_unpriv_test_range(addr, UVISOR_UNPRIV_ACCESS_SIZE(size)) & (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) == (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove legacy code

Patater
Patater suggested changes Aug 14, 2017
View reviewed changes
core/vmpu/src/mpu_armv8m/vmpu_armv8m_unpriv_access.c Outdated
if (++tries > 1 || !vmpu_fault_recovery_mpu(0, 0, addr, 0)) {
break;
//if ((vmpu_unpriv_test_range(addr, UVISOR_UNPRIV_ACCESS_SIZE(size)) & (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) == (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) {
//this should be slower substantualy
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain how and why this is slower in this comment.

core/vmpu/src/mpu_armv8m/vmpu_armv8m_unpriv_access.c Outdated
break;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Make whitespace changes in a separate commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't add trailing whitespace.

core/vmpu/src/mpu_armv8m/vmpu_armv8m_unpriv_access.c
//if ((vmpu_unpriv_test_range(addr, UVISOR_UNPRIV_ACCESS_SIZE(size)) & (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) == (TT_RESP_NSRW_Msk | TT_RESP_SRVALID_Msk)) {
//this should be slower substantualy
if (vmpu_buffer_access_is_ok(g_active_box, (const void *) addr, UVISOR_UNPRIV_ACCESS_SIZE(size))){
switch(size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space after keywords like switch, before the (. It was wrong before. Please make style changes in a separate commit.

@Patater
Copy link
Contributor

Patater commented Aug 14, 2017

What testing has been done with v8-M? All v8-M tests pass?

@orenc17 orenc17 force-pushed the v8_upriv_access branch 2 times, most recently from c9f3515 to c0c6db1 Compare August 15, 2017 08:26
@orenc17
Copy link
Author

orenc17 commented Aug 15, 2017

@niklas-arm could you review the changes and compare it to your work?

@orenc17 orenc17 force-pushed the v8_upriv_access branch 2 times, most recently from 0f7c8ac to df08917 Compare August 15, 2017 08:56
@Patater Patater changed the title change behavior of vmpu_unpriv_access in v8 Handle armv8m unpriv_access with SAU Aug 15, 2017
@Patater
Copy link
Contributor

Patater commented Aug 15, 2017

Looks like this is suffering from the same problem on the page_heap_one_page_test_invalid_access_0 test as Niklas's implementation. Please debug.

@niklarm
Copy link
Contributor

niklarm commented Aug 15, 2017

This is pretty similar to what I implemented. The issue is related to the SAU not being updated with the region containing the accessed memory anymore. For some reason this now leads to a hardfault.

Patater
Patater reviewed Aug 22, 2017
View reviewed changes
core/vmpu/src/mpu_armv8m/vmpu_armv8m_unpriv_access.c Outdated
break;
}

if (++tries > 1 || !vmpu_fault_recovery_mpu(0, 0, addr, 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have to recover. vmpu_buffer_access_is_ok should tell you what access you would have if you did recover. The access can happen from secure mode without the recovery happening.

Patater
Patater reviewed Aug 22, 2017
View reviewed changes
core/vmpu/src/mpu_armv8m/vmpu_armv8m_unpriv_access.c Outdated
uint32_t vmpu_unpriv_access(uint32_t addr, uint32_t size, uint32_t data)
{
unsigned int tries = 0;
while(1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't have to loop here either.

orenc17
orenc17 commented Aug 23, 2017
View reviewed changes
core/debug/src/core_armv8m/debug_box_armv8m.c Outdated
/* Upon execution return debug_handler will be executed. Upon return from
* debug_handler, return_handler will be executed. */

//uint32_t caller = UVISOR_GET_NS_ALIAS(UVISOR_GET_NS_ADDRESS((uint32_t) debug_handler));
Copy link
Author

Choose a reason for hiding this comment

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

@Patater @niklas-arm
my problem is how can i proceed from here

@orenc17 orenc17 force-pushed the v8_upriv_access branch 2 times, most recently from af7667e to 249bea0 Compare August 24, 2017 13:24
Handle armv8m unpriv_access with SAU
eb32d63 
Previously when we got to unpriv_access we verfied the operation with the TT
assembly command, we now switch to using the SAU with uVisor vmpu functions.

This could slow down the operation since we switch an assembly command
with a C function which scans the virtual regions.
@Patater
Copy link
Contributor

Patater commented Sep 6, 2017

This is waiting for #497 to merge first, so that the debug box has a good context in which to run.

@orenc17
Copy link
Author

orenc17 commented Sep 7, 2017

@Patater this PR is independent from #497
the PR that should wait for #497 is PR #496

@Patater
Copy link
Contributor

Patater commented Sep 7, 2017

This needs to wait, as not all the v8-M tests are passing if I merge this.

@theamirocohen theamirocohen mentioned this pull request Oct 15, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Patater Patater Patater requested changes

@alzix alzix alzix requested changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

needs: ci needs: preceding PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@orenc17 @Patater @niklarm @alzix