[PATCH] powerpc64/idle: Fix SP offsets when saving GPRs

[Nicholas Piggin]

Excerpts from Michael Ellerman's message of February 4, 2021 9:13 pm:
> Nicholas Piggin <npiggin at 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" <cmr at 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