Smip/Ssip hardware stacking is broken

[evgeniy-paltsev]

As per ACLIC spec (Version v0.17), we perform following sequence (in order) on interrupt entry when Smip/Ssip extensions present:

• Save registers x10-x15 (these are the caller saved registers available in RVC)
• Adjusts the stack pointer to create the stack frame
• Conditionally performs a stack pointer swap by executing cspspush sp

So, if we have S or U modes, we'll save registers on memory location which is pointed by S/U-mode x2. It can be

• stack of lower privilege mode which we don't have access to
• stack of lower privilege mode which we can overflow
• SP (x2) register which is not initialized properly and points to random memory
• x2 which is not used as SP at all

How is it supposed to work at all?

[christian-herber-nxp]

So, if we have S or U modes, we'll save registers on memory location which is pointed by S/U-mode x2. It can be

This is precisely what Zephyr does today. So you can see it working there.

• stack of lower privilege mode which we don't have access to

Fair point. At this point, access would have to be guaranteed to be there for the more privileged mode.
The RISC-V ISA already has a feature for that with mstatus.MPRV.
We should define that the save/restore happens at the effective privilege level of the previous privilege level

• stack of lower privilege mode which we can overflow

Whoever is allocating the stack sizes needs to understand that additional space for 6 registers is needed.

• SP (x2) register which is not initialized properly and points to random memory

if you have not even initialized an SP, you should not enable interrupts

• x2 which is not used as SP at all

Not sure what you mean. Why would x2 not be used as SP?

[gagachang]

Not sure what you mean. Why would x2 not be used as SP?

In some implementations requiring less code size, x2 could be used as the base address of load/store from structures other than stack.
https://docs.riscv.org/reference/isa/unpriv/c-st-ext.html#27-3-1-stack-pointer-based-loads-and-stores

[evgeniy-paltsev]

Not sure what you mean. Why would x2 not be used as SP?

Cause the ISA does not guarantee that.

From RISC-V Unprivileged Architecture:

There is no dedicated stack pointer or subroutine return address link register in the Base
Integer ISA; the instruction encoding allows any x register to be used for these purposes.

[evgeniy-paltsev]

This is precisely what Zephyr does today. So you can see it working there.

I'm a bit confused here. Are you talking about upstream Zephyr or about some custom port? I've double checked the upstream Zephyr port - and there is nothing like that there. Zephyr handles it correctly.

[christian-herber-nxp]

The RISC-V ISA does not guarantee that x2 is the sp, true.
At the same time, all ratified ABIs do guarantee that.
Also, sp-relative load/stores exist.

In order for the stack pointer to be swapped automatically, it is a requirement that the stack pointer is in a known location. x2 is the obvious choice. If an application cannot guarantee this, it cannot activate Smip.

[christian-herber-nxp]

I'm a bit confused here. Are you talking about upstream Zephyr or about some custom port? I've double checked the upstream Zephyr port - and there is nothing like that there. Zephyr handles it correctly.

You are right, Zephyr does switch to an exception stack frame before stacking caller saved register.
In the code, however, there is a comment, which indicates it is being saved on the thread stack:

	/* Save caller-saved registers on current thread stack. */
	addi sp, sp, -__struct_arch_esf_SIZEOF
	DO_CALLER_SAVED(sr)		;

The reason we decided to stack on the user stack is because it has some benefits for context switching in case the OS wants to switch to another thread after an interrupt.

I will try to dive a bit deeper into this on Monday, as this sort of thing requires a bit of a deeper analysis.

[reosdev]

Accessing memory with different privilege is not a generic solution and is potentially unsafe. When taking the interrupt trap in M-mode there is no clear path to setup proper access permissions. Depending on interrupted context ACLIC requires to save either on S or U memory pointed by either S or U register that may have no valid address inside (only guess on its content). Access privilege modification (like you did in latest fix) is extremely expensive in terms of performance (pls consult with your HW experts about the cost of switching data path to use orthogonal address protection/translation).

By the way, the fix is totally incorrect:

• csrsi mstatus, 1<<17 is invalid (argument is out of range)
• it (supposes to) enforce switch to U-mode data path, which is not only possible scenario.
• it does not change the MPRV back to original state
• it is not applicable for S-mode (there is no MPRV counterpart in sstatus CSR)

[christian-herber-nxp]

• csrsi mstatus, 1<<17 is invalid (argument is out of range)
• it (supposes to) enforce switch to U-mode data path, which is not only possible scenario.
• it does not change the MPRV back to original state

it gets reset on xret.

• it is not applicable for S-mode (there is no MPRV counterpart in sstatus CSR)

I did realize this. The bit is WPRI in sstatus. I can understand why that is the case without SPMP. I do actually wonder why this was not added when introducing the SPMP so far.

[christian-herber-nxp]

Access privilege modification (like you did in latest fix) is extremely expensive in terms of performance (pls consult with your HW experts about the cost of switching data path to use orthogonal address protection/translation).

In the spirit of a collaborative community, would it not be better if you share your input about this cost?

In this particular case, I do not see how this can introduce such a significant, as it does not increase the number of switches that have to be performed. It simply delays the moment when the switch happens. On trap, instead of changing the access privilege immediately, it defers it until after the registers have been pushed.

[christian-herber-nxp]

After looking a bit into Zephyr, I have to notice that all kinds of things are happening in the RISC-V port

1. Direct ISRs operate directly on the thread user stack, with M-mode privilege.
2. Direct ISRs never lead to an immediate context switch, but this is RISC-V specific.
3. Everything that runs through the _isr_wrapper works on a per-thread exception stack, plus interrupts work on a per CPU interrupt thread.

I have to working propositions, which I would like to check:

1. If we expect the OS to context switch msp, i.e. have a privileged sp per thread, it would be better to move the stack pointer swap back to the beginning of the push sequence
2. If the stack pointer swap is at the end of the push sequence, it is better to leave it out all together,
3. Since we simplified the interrupt push/pop sequence a lot, I think the reasons for removing it from synchronous exceptions are no longer valid, and things are cleaner (and allow a shared path between interrupts and exceptions) if both have the same push/pop behavior.
4. The use-case for the stack pointer swapping needs to be sharpened. It would be used to swap in an exception stack frame sp, i.e. a privileged per-thread stack pointer, or to swap in a global interrupt sp. The current Zephyr port is somewhat inconsistent. While it has separate IRQ stack, it does not use it for direct interrupts, which adds quite some burden onto all user stacks.
   I believe we are better off looking at the Cortex-M port and make sure our specification is compatible with that. In this scenario, I believe exceptions and interrupts share one interrupt stack. This brings me back to (3), it may make sense to go back to having the push/pop shared between interrupts and exceptions.

[reosdev]

"I did realize this. The bit is WPRI in sstatus. I can understand why that is the case without SPMP."

If you realize the problem exists why continue proposing an MPRV-based solution without explicitly acknowledging this as a fundamental limitation?

[reosdev]

"I do not see how this can introduce such a significant [cost]..."

MPRV modification is expensive because:

1. Requires FENCE - caches and TLBs may have stale privilege attributes without synchronization
2. Triggers pipeline flush - MPRV is architectural state (part of mstatus); writing it flushes the pipeline

[reosdev]

"After looking a bit into Zephyr, I have to notice that all kinds of things are happening in the RISC-V port..."

The Zephyr RISC-V port is incomplete and cannot be used as a reference for specification design. A hardware specification should be based on architectural requirements and correctness, not on the current state of one OS port.