arch: xtensa: add isync to interrupt vector #69923
Conversation
kv2019i
requested review from
lyakh,
abonislawski,
serhiy-katsyuba-intel,
dcpleung,
andyross and
ceolin
and removed request for
andyross,
ceolin and
dcpleung
arch/xtensa/include/xtensa_asm2_s.h
Outdated
| @@ -604,6 +604,11 @@ _Level\LVL\()Vector: | |||
| s32i a2, a1, ___xtensa_irq_bsa_t_a2_OFFSET | |||
| s32i a3, a1, ___xtensa_irq_bsa_t_a3_OFFSET | |||
|
|
|||
| #if defined(CONFIG_SOC_FAMILY_INTEL_ADSP) && defined(CONFIG_ADSP_IDLE_CLOCK_GATING) | |||
There was a problem hiding this comment.
ADSP_IDLE_CLOCK_GATING depends on SOC_FAMILY_INTEL_ADSP, so there is no need for check on SOC_FAMILY_INTEL_ADSP
There was a problem hiding this comment.
Ack @nashif , updated in V2
On Intel ADSP platforms, additional "isync" is needed in interrupt vector to synchronize icache when core is woken up from deeper sleep state by an interrupt. This is only needed if DSP clock gating is enabled. Signed-off-by: Kai Vehmanen <[email protected]>
kv2019i
dismissed stale reviews from dcpleung, serhiy-katsyuba-intel, and lyakh
via
e997e49
kv2019i
force-pushed
the
202403-isync-fix
branch
from
476623d to
e997e49
Compare
| #ifdef CONFIG_ADSP_IDLE_CLOCK_GATING | ||
| /* Needed when waking from low-power waiti state */ | ||
| isync | ||
| #endif |
There was a problem hiding this comment.
Is this the right place? At this stage the handler has already significant code, including stores.
Normally you use an isync to prevent any in-flight state changes that might affect instruction fetch (i.e. MMU/MPU changes) from affecting the next instruction in the stream. Which instruction are we trying to protect, and from what? Is this maybe something that only happens if the ISR crosses a page/cacheline/whatever boundary? If so that seems like it could be fixed with alignment instead?
My gut says that if we do need this, you almost certainly want this as the very first instruction in the vector, no? Is there an errata or something that explains what you're trying to fix?
There was a problem hiding this comment.
But right here? It's just a really weird spot.
There was a problem hiding this comment.
@andyross Following up now to move this to a more maintainable place thesofproject/sof#9031
|
So, I get that every platform has its weird nonsense to support. But here I'm wearing the hat of an arch maintainer and... this is just really weird to have in the global exception vectors. Maybe a cleaner design would be to swap VECBASE on suspend such that the WAITI returns to SOC-specific code, then do your magic, restore VECBASE, and jump to the regular handler? It would be more code but a lot less confusion in arch/xtensa (and also a lot more safety vs. maintainers coming along later and "fixing" it because they don't understand what can't be documented). Note FWIW that the call0 stuff is likely to land in the immediate future, and that has a separate interrupt entry path which would need the same treatment. |
There was a problem hiding this comment.
Drop a +1 just because there are schedule worries and this is verifiably a noop on other platforms that might include it accidentally.
But let's please remember to move this to the SOC layer over the long term. The global interrupt entry code is a really bad spot for platform support.
On Intel ADSP platforms, additional "isync" is needed in interrupt vector to synchronize icache when core is woken up from deeper sleep state by an interrupt. This is only needed if DSP clock gating is enabled.