Excerpts from Michael Ellerman's message of February 4, 2021 9:13 pm: > Nicholas Piggin <npig...@gmail.com> writes: >> Excerpts from Christopher M. Riedl's message of February 4, 2021 4:59 pm: >>> On Sat Jan 30, 2021 at 7:44 AM CST, Nicholas Piggin wrote: >>>> Excerpts from Michael Ellerman's message of January 30, 2021 9:32 pm: >>>> > "Christopher M. Riedl" <c...@codefail.de> writes: >>>> >> The idle entry/exit code saves/restores GPRs in the stack "red zone" >>>> >> (Protected Zone according to PowerPC64 ELF ABI v2). However, the offset >>>> >> used for the first GPR is incorrect and overwrites the back chain - the >>>> >> Protected Zone actually starts below the current SP. In practice this is >>>> >> probably not an issue, but it's still incorrect so fix it. >>>> > >>>> > Nice catch. >>>> > >>>> > Corrupting the back chain means you can't backtrace from there, which >>>> > could be confusing for debugging one day. >>>> >>>> Yeah, we seem to have got away without noticing because the CPU will >>>> wake up and return out of here before it tries to unwind the stack, >>>> but if you tried to walk it by hand if the CPU got stuck in idle or >>>> something, then we'd get confused. >>>> >>>> > It does make me wonder why we don't just create a stack frame and use >>>> > the normal macros? It would use a bit more stack space, but we shouldn't >>>> > be short of stack space when going idle. >>>> > >>>> > Nick, was there a particular reason for using the red zone? >>>> >>>> I don't recall a particular reason, I think a normal stack frame is >>>> probably a good idea. >>> >>> I'll send a version using STACKFRAMESIZE - I assume that's the "normal" >>> stack frame :) >>> >> >> I think STACKFRAMESIZE is STACK_FRAME_OVERHEAD + NVGPRs. LR and CR can >> be saved in the caller's frame so that should be okay. > > TBH I didn't know/had forgotten we had STACKFRAMESIZE. > > The safest is SWITCH_FRAME_SIZE, because it's calculated based on > pt_regs (which can change size): > > DEFINE(SWITCH_FRAME_SIZE, STACK_FRAME_OVERHEAD + sizeof(struct > pt_regs)); > > But the name is obviously terrible for your usage, and it will allocate > a bit more space than you need, because pt_regs has more than just the GPRs.
I don't see why that's safer if we're not using pt_regs. > >>> I admit I am a bit confused when I saw the similar but much smaller >>> STACK_FRAME_OVERHEAD which is also used in _some_ cases to save/restore >>> a few registers. >> >> Yeah if you don't need to save all nvgprs you can use caller's frame >> plus a few bytes in the minimum frame as volatile storage. >> >> STACK_FRAME_OVERHEAD should be 32 on LE, but I think the problem is a >> lot of asm uses it and hasn't necessarily been audited to make sure it's >> not assuming it's bigger. > > Yeah it's a total mess. See this ~3.5 year old issue :/ > > https://github.com/linuxppc/issues/issues/113 > >> You could actually use STACK_FRAME_MIN_SIZE for new code, maybe we add >> a STACK_FRAME_MIN_NVGPR_SIZE to match and use that. > > Something like that makes me nervous because it's so easy to > accidentally use one of the macros that expects a full pt_regs. > > I think ideally you have just two options. > > Option 1: > > You use: > STACK_FRAME_WITH_PT_REGS = STACK_FRAME_MIN_SIZE + sizeof(struct pt_regs) > > And then you can use all the macros in asm-offsets.c generated with > STACK_PT_REGS_OFFSET. I don't see a good reason to use pt_regs here, but in general sure this would be good to have and begin using. > Option 2: > > If you want to be fancy you manually allocate your frame with just > the right amount of space, but with the size there in the code, so for > example the idle code that wants 19 GPRs would do: > > stdu r1, -(STACK_FRAME_MIN_SIZE + 8 * 19)(r1) > > And then it's very obvious that you have a non-standard frame size and > need to be more careful. I would be happy with this for the idle code. Thanks, Nick
