Skip to content

Conversation

@vonhust
Copy link

@vonhust vonhust commented Aug 7, 2019

  • raise software triggerd exception unconditionally
  • fix the handling when exception raised in irq context
  • software-trigger exception only allow oops and stack check fail
  • remove the exception of arc in tests/mem_protection/userspace

Fixes #17899 #17590

@vonhust vonhust requested a review from ruuddw as a code owner August 7, 2019 09:48
@vonhust vonhust requested review from abrodkin and andrewboie August 7, 2019 09:49
@zephyrbot zephyrbot added the area: API Changes to public APIs label Aug 7, 2019
@ruuddw
Copy link
Member

ruuddw commented Aug 7, 2019

Not sure if it's relevant or not, but this seems to change the behavior for the kernel non-isr case: it doesn't trigger an exception anymore. Alternative would be to first test for user mode, and then test for interrupt context (I assume that is to avoid fatal double exception?)

@vonhust
Copy link
Author

vonhust commented Aug 7, 2019

Not sure if it's relevant or not, but this seems to change the behavior for the kernel non-isr case: it doesn't trigger an exception anymore. Alternative would be to first test for user mode, and then test for interrupt context (I assume that is to avoid fatal double exception?)

z_except_reason/Z_ARCH_EXCEPT is actively called by kernel like k_panic, k_oops, it's a kind of SW exception, different with HW exception. So if the processor in kernel mode, it has enough privilege to do things like an exception fix. Although it's also ok to raise a hardware exception by TRAP, it's not efficient because of exception prologue handling.

any thought on this? Does z_except_reason need to raise a real exception? @andrewboie

@vonhust
Copy link
Author

vonhust commented Aug 7, 2019

I did a further look of Z_ARCH_EXCEPT. It seems to it's required to raise an cpu exception. So i made some updates that remove the check of user_context/isr.

For the case that called in exception handler, it will raise double exception. But this case is unreasonable.

@andrewboie
Copy link
Contributor

any thought on this? Does z_except_reason need to raise a real exception? @andrewboie

Yes, this should trigger an exception.

@andrewboie
Copy link
Contributor

Why is this check even needed?
Can't you just trigger a trap unconditionally?

@vonhust
Copy link
Author

vonhust commented Aug 8, 2019

Now, raise exception unconditionally

@ioannisg ioannisg added the bug The issue is a bug, or the PR is fixing a bug label Aug 8, 2019
Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

It's now completely unconditional, right?

Looks ok to me, @ruuddw pls, take a look

@ioannisg
Copy link
Member

ioannisg commented Aug 8, 2019

@abrodkin could you also review?

@ruuddw
Copy link
Member

ruuddw commented Aug 8, 2019

This indeed fixes #17899. Not sure though if the ISR check isn't required to handle the case that Z_ARCH_EXCEPT is called from interrupt context. Sanity check looks clean, is there a test that calls Z_ARCH_EXCEPT from isr context? Would that be expected to work?

Also, I see a related 'FIXME' in userspace tests. ARC is excluded in test_oops ("FIXME #17590"). Can #17590 be closed now as well?

Copy link
Contributor

@abrodkin abrodkin left a comment

Choose a reason for hiding this comment

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

Looks good to me to.

use "trap_s 3" to simulate SW exception raised by kernel

Signed-off-by: Wayne Ren <[email protected]>
@vonhust
Copy link
Author

vonhust commented Aug 9, 2019

@ruuddw @andrewboie , I found why there is a condition to raise exception, i.e., whether in isr,
because there is no specific handling in exc handling code about the case where exception comes out in interrupt handling, one example is z_check_stack_sentinel will raise exception by Z_ARCH_EXCEPT.

I add another commit to fix this in this PR. Pls review, and do not merge before the sanitycheck test is clean.

exception, different with irq offload,  may be raised interrupt
handling, e.g.
  * z_check_stack_sentinel
  * wrong code

we need to add specific handling of this case in exception handling

Signed-off-by: Wayne Ren <[email protected]>
@zephyrbot zephyrbot added the area: ARC ARC Architecture label Aug 9, 2019
@andrewboie
Copy link
Contributor

This indeed fixes #17899. Not sure though if the ISR check isn't required to handle the case that Z_ARCH_EXCEPT is called from interrupt context. Sanity check looks clean, is there a test that calls Z_ARCH_EXCEPT from isr context? Would that be expected to work?

@ruuddw I'm not sure I understand.

If you trigger this while servicing an interrupt, doesn't this just induce an exception? That's what we would want.

The behavior should be:

  1. Someone calls k_panic() in an ISR, because something horrible happened while servicing that interrupt.
  2. A fatal error is generated with reason code K_ERR_KERNEL_PANIC

I don't know if there's a test for this scenario, it should be reasonable to implement using irq_offload().

@vonhust vonhust requested review from andyross and nashif as code owners August 9, 2019 13:10
Copy link
Member

@ruuddw ruuddw left a comment

Choose a reason for hiding this comment

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

Thanks Wayne, this fixes both #17899 and #17590, and addresses raising an exception from interrupt context.

@zephyrbot zephyrbot added area: Tests Issues related to a particular existing or missing test area: Kernel labels Aug 9, 2019
@vonhust vonhust added this to the v2.0.0 milestone Aug 9, 2019
Wayne Ren added 2 commits August 9, 2019 21:53
according to high-level design,in user mode software-triggered system
fatal exceptions only allow oops and stack check failure

Signed-off-by: Wayne Ren <[email protected]>
the tested feature is supported now, remove the exception

Signed-off-by: Wayne Ren <[email protected]>
@wentongwu
Copy link
Contributor

@ruuddw @vonhust @andrewboie @nashif can we backport this PR to v1.14 branch?

@andrewboie
Copy link
Contributor

@wentongwu agree this looks like it should be backported. I would recommend look at all other arch/arc patches I am not sure if all ARC userspace patches have been backported since 1.14. there were a lot of things fixed after 1.14

@wentongwu
Copy link
Contributor

@andrewboie yes, checked the commit history, lots of enhancement/fix for user space added, I don't think user space on arc will work well without them. It would be better if these patches can be backported to v1.14 branch.

@ruuddw is it possible to do that backport for arc?

@ruuddw
Copy link
Member

ruuddw commented Oct 28, 2020

We've done few backports, but there have been many enhancements (to support SMP) as well that we didn't backport.
Let me see if we missed some essential ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: ARC ARC Architecture area: Kernel area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tests/kernel/mem_protect/stackprot/kernel.memory_protection fails on nsim_em

7 participants