Add SCB methods to enable/disable exceptions

[hug-dev]

Some exceptions might be disabled by default which means that the
HardFault handler will be called instead of the exception handler. This
commit adds methods on the SCB peripheral that use the SHCSR register to
enable/disable exceptions.

Signed-off-by: Hugues de Valon hugues.devalon@arm.com

[rust-highfive]

r? @ithinuel

(rust_highfive has picked a reviewer for you, use r? to override)

[thejpster]

One comment, otherwise I'm happy with this. Thank you for the PR!

[hug-dev]

This was ready to be merged, any updates?

[adamgreig]

Sorry for the wait!

On the whole I think this is good. I had a few thoughts as I looked over it...

• Should it be unsafe to disable the exception handlers? I think that since disabled exceptions propagate into HardFaults it's OK to allow safely disabling them. One reason we require that (for example) enabling interrupts be unsafe is to help enforce our invariant that interrupts won't fire within a CriticalSection closure, which could otherwise break our memory model. I don't think the same reasoning applies here since the exceptions would always cause a jump to the HardFault handler.

• Since the methods already contain a critical section, should they be static methods on SCB, instead of requiring an &mut? I think they probably could be without loss of safety, but haven't given it enough thought to really be sure. If possible, static methods are easier to use without requiring the user obtain the SCB instance.

• State checks in each method. Is there a requirement to check if the exception handler is enabled or not before setting/clearing it? I didn't see any requirement in the ARM. If so, perhaps we could remove the check and just execute the register modify, which also ensures the state doesn't change between us checking and us modifying the register.

[hug-dev]

Thanks for the review 😃

Should it be unsafe to disable the exception handlers? I think that since disabled exceptions propagate into HardFaults it's OK to allow safely disabling them. One reason we require that (for example) enabling interrupts be unsafe is to help enforce our invariant that interrupts won't fire within a CriticalSection closure, which could otherwise break our memory model. I don't think the same reasoning applies here since the exceptions would always cause a jump to the HardFault handler.

I would think so too because these methods do not enable/disable interrupts per say, but only modify their routing to the specific handler or HardFault. That is ensured (I think), because the read-modify-write sequence is done during a critical section (modifying other bits of the same register would set interrupts as active or pending).

Since the methods already contain a critical section, should they be static methods on SCB, instead of requiring an &mut? I think they probably could be without loss of safety, but haven't given it enough thought to really be sure. If possible, static methods are easier to use without requiring the user obtain the SCB instance.

I was not sure of what was the idiomatic thing to do! It is indeed easier to use static methods. I now realise that I used &mut for is_enabled that only read the SCB and could only take a reference to self.
I don't think I am qualified enough to say if it is safe to directly access the register by doing something like *Self::ptr(). I am worried if static methods might break the ones using ownership, if for example the SCB peripheral is instantiated as immutable to be used only with methods taking &self but there are also static methods that can modify its state. Not sure at all!

State checks in each method. Is there a requirement to check if the exception handler is enabled or not before setting/clearing it? I didn't see any requirement in the ARM. If so, perhaps we could remove the check and just execute the register modify, which also ensures the state doesn't change between us checking and us modifying the register.

I don't think there are any requirements ; it was done for performance to avoid going through a critical-section set-up if it is actually not needed! If you think this is not needed, I can remove it indeed.

[hug-dev]

Out of your three points, I think we agreed on the first one that unsafe is not needed. What about the other two?

• using static SCB instead of using a reference -> I am happy to do that change in my PR if you think this is safe
• state checks -> I am happy to remove them if the performance cost is minimal

[adamgreig]

Sorry for the wait (again!). We had a chat about this at today's meeting (logs are here). I'm cautious about merging this without enough consideration because how interrupts are handled is such an important part of our safety story and one of the hardest bits to get right.

Safety

This still seems like a point of concern, especially in relation to critical sections/mutexes. It might be related #196 as well. However I think this argument offers a way out:

• MemManage, BusFault, UsageFault are configurable priority exceptions, so they are not taken if they occur during one of our critical sections (where PRIMASK is 1).
• Instead they are promoted to HardFaults and the HardFault handler is taken
• This is the same as the default behaviour when the exception handlers are not enabled
• So, entering a critical section (which is safe) has the same effect as disabling these handlers
• Consequently other user code cannot rely on these handlers being active: a hardfault might always occur instead (the default behaviour), which hopefully prevents anyone's safety invariants relying on the exception handler being called
• Likewise, enabling one of these handlers does not change the exclusivity of a critical section: they won't run in it, we don't get interrupt::free does not disable all exceptions, which breaks critical sections #196-esque contention, but instead a HardFault occurs

I think this means we can keep them safe.

Static SCB and state checks

If we use &mut to ensure we have exclusive access to SCB, I don't think we need the critical section inside this code, because nothing else can have written to the register anyway, and software cannot clear the pending/active bits in the register. So I think the best course of action would be to keep the &mut self for modify (and swap to &self for reads), remove the critical sections, and remove the state checks. What do you think?

cc @japaric who's probably thought about this stuff the most...

[hug-dev]

Thanks a lot for putting that issue as a topic during last meeting 💯

I think this means we can keep them safe.

Agree with your points and the conclusion.

So I think the best course of action would be to keep the &mut self for modify (and swap to &self for reads), remove the critical sections, and remove the state checks. What do you think?

That seems like a good option and I agree that the ownership rules are better suited/more efficient to prevent data races than disabling exceptions.

[hug-dev]

Removed the critical sections and the state checks.

I am all-in about using references to peripherals in their methods to enforce the ownership rules but one thing which is weird is that in the following code:

    #[inline]
    #[cfg(not(any(armv6m, armv8m_base)))]
    pub fn enable(&mut self, exception: Exception) {
        if let Some(shift) = SCB::shcsr_enable_shift(exception) {
            // The mutable reference to SCB makes sure that only this code is currently modifying
            // the register.
            unsafe { self.shcsr.modify(|value| value | (1 << shift)) }
        }
    }

changing &mut self to &self will still compile, even if the method modify self. One need to be careful to not rely on the compiler to check for those!

[adamgreig]

Thanks!

changing &mut self to &self will still compile

The reason for this is we don't require &mut self to call modify on registers; they're treated as a sort of interior mutability. Since &self isn't Send or Sync, we know it will only be used from one thread of execution, so even multiple &self references can't race each other to perform a read-modify-write. This is convenient for sharing device peripherals I guess.

However in cortex-m we've used &mut self for pretty much all methods that modify the registers; I don't think there's any particular reason for that but it's what we've got at the moment. I guess we could consider changing it for a 1.0 release but for now it's simpler to keep these methods &mut too.

[adamgreig]

I'm happy with this now but would still like to hear if anyone else @rust-embedded/cortex-m has ideas on how it might be unsafe. If not I'll merge by the end of the week.

[adamgreig]

Guess we're good to go!

bors merge

[bors]

👎 Rejected by code reviews

[adamgreig]

Hmm, @thejpster please could you approve the changes or dismiss your review? Looks like it still being open is blocking bors.

[therealprof]

Let's see whether that did the trick.

bors r=adamgreig

[bors]

👎 Rejected by code reviews

[therealprof]

Damn, did not. That's so silly.

[therealprof]

Maybe try the close and re-open dance?

[adamgreig]

Nope.

[therealprof]

Seems to be a weird bors thing. Let's wait a bit for @thejpster and then override manually.

bors r+

[bors]

👎 Rejected by code reviews

[jonas-schievink]

Pushing a new commit should dispose of the old reviews

[therealprof]

Pushing a new commit should dispose of the old reviews

Yes, but only the approvals (if configured in the GH branch protection rules), not the change requests.