Preliminary Xtensa support

[MabezDev]

Based on the work in #804.

I hope non-upstream target support is acceptable :).

[Dirbaio]

the critical section in riscv was needed to avoid a race if an irq happens between SIGNAL_WORK_THREAD_MODE.load and waiti. the load would read false so the core would be put to sleep, missing the wake.

ARM solves it with the "event register" in SEV/WFE. riscv doesn't have an equivalent and afaict xtensa doesn't either, so maybe the CS is needed here as well?

[MabezDev]

That's a good point. The issue in Xtensa is that the processor will never wake up if waiti is executed inside an interrupt-free critical section.

From TRM (emphasis mine):

In some implementations it also powers down the processor’s logic, and waits for an interrupt. After executing the interrupt handler, execution continues with the instruction following the WAITI.

It seems the interrupt can't just be pending, it has to be actually handled which can't happen when interrupts are disabled. Not really sure what to suggest here, any ideas?

[Dirbaio]

I'd maybe verify that experimentally. If it's true a CS doesn't work then there's no way to do this without races, which I'd find very surprising.

[MabezDev]

So in my testing, I've verified that the TRM is indeed correct. However, I also found some more notes on implemented critical sections on Xtensa, they should actually be done by increasing the PS.INTLEVEL such that PS.INTLEVEL > any possible interrupt level instead of disabling interrupts via the INTENABLE register. I need to do a bit more reading to verify that no interrupts can exist at the max (which is 15), the TRM is kinda vague on that. I guess NMI might be at that level, but what can ya do about that :D. By modifying PS.INTLEVEL, the waiti instruction can atomically reduce the PS.INTLEVEL whilst slipping into sleep mode, hence no race conditions :D.

So what this actually means is we need critical_section impls for Xtensa. It seems the path forward for critical_section is that every HAL/PAC will have a per chip cs impl that can handle multi core etc, so I've opened esp-rs/esp-hal#87.

I'll make sure all this theory work actually works on hardware and get back to you :).

[MabezDev]

I've now reverted to using the critical section to avoid the race condition, for reference the CS impl I used looked like this:

struct CriticalSection;
critical_section::custom_impl!(CriticalSection);

static mut VPS: u32 = 0;
unsafe impl critical_section::Impl for CriticalSection {
    unsafe fn acquire() -> u8 {
        core::arch::asm!("rsil {0}, 15", out(reg) VPS);
        0
    }

    unsafe fn release(_token: u8) {
        core::arch::asm!("wsr.ps {0}", in(reg) VPS)
    }
}

With the next release of critical_section, I believe I can get rid of the static mut and pass it through the variable-sized token instead to restore processor state.

[Dirbaio]

Hmm, if waiti 0 sets INTLEVEL=0, it's effectively exiting the critical section. I wonder if this can be used to create unsoundness, for example calling run() within a CS will cause it to end early, but I can't immediately see how to exploit that because run() is noreturn...

[MabezDev]

I'm not sure there is anything we can do here, waiti doesn't restore the previous INTLEVEL so you're right it does effectively end the CS there. However, this is only possible because we are using unsafe and we make sure that we don't include anything that needs to be inside the CS after the waiti call. This is sort of similar to cortex-m in that you can call interrupt::enable inside the critical section to end the critical section there but of course, this method is marked as unsafe in cortex-m.

[MabezDev]

FYI this file is what I am basing my implementation on, which came straight from Tensilica (now Cadence). Might be worth a quick skim read, just in case I'm missing something obvious 😅.

[MabezDev]

To the best of my knowledge, I think this is the best we can do for now. I've added a comment to warn that the CS ends early when using waiti. It kinda sucks it works this way, it means we can't implement any CPU usage tracking etc. Hopefully, I can find a better way to do this in the future.

I hope this is acceptable as is, I'd be happy to manage any issues relating to Xtensa (or RISCV) if this gets merged :).

[Dirbaio]

cool! Let's do this then! We can always change it later if it turns out to be troublesome 🚀

Thank you! :)

bors r+

[bors]

Build succeeded:

• all