Re: [PATCH 1/3] x86: get rid of KERNEL_STACK_OFFSET

[Andy Lutomirski]

On Thu, Mar 19, 2015 at 8:28 AM, Denys Vlasenko <dvlasenk@xxxxxxxxxx> wrote:
> On 03/18/2015 09:54 PM, Andy Lutomirski wrote:
>> On Wed, Mar 18, 2015 at 12:47 PM, Denys Vlasenko <dvlasenk@xxxxxxxxxx> wrote:
>>> PER_CPU_VAR(kernel_stack) was set up in a way where it points
>>> five stack slots below the top of stack.
>>>
>>> Presumably, it was done to avoid one "sub $5*8,%rsp"
>>> in syscall/sysenter code paths, where iret frame needs to be
>>> created by hand.
>>>
>>> Ironically, none of them benefits from this optimization,
>>> since all of them need to allocate additional data on stack
>>> (struct pt_regs), so they still have to perform subtraction.
>>>
>>> This patch eliminates KERNEL_STACK_OFFSET.
>>>
>>> PER_CPU_VAR(kernel_stack) now points directly to top of stack.
>>> pt_regs allocations are adjusted to allocate iret frame as well.
>>> Hopefully we can merge it later with 32-bit specific
>>> PER_CPU_VAR(cpu_current_top_of_stack) variable...
>>>
>>> Semi-mysterious expressions THREAD_INFO(%rsp,RIP) - "why RIP??"
>>> are now replaced by more logical THREAD_INFO(%rsp,SIZEOF_PTREGS) -
>>> "calculate thread_info's address using information that
>>> rsp is SIZEOF_PTREGS bytes below the stack top".
>>>
>>> Net result in generated code is that constants in several insns
>>> are changed.
>>>
>>> This change is necessary for changing struct pt_regs creation
>>> in SYSCALL64 code path from MOV to PUSH instructions.
>>>
>>
>> Would it be reasonable to break this up into two pieces: first, remove
>> KERNEL_STACK_OFFSET from THREAD_INFO and related macros (i.e. make
>> them relative to current_top_of_stack instead)
>
> PER_CPU(cpu_current_top_of_stack) exists only in 32 bits.
> Can't use it in 64-bit code.
>
> PER_CPU(cpu_current_top_of_stack) becomes equal
> to PER_CPU(kernel_stack) only after this patch.

You could have a #define that gives the top of the stack from asm, though.

>
> I plan to clean up PER_CPU(cpu_current_top_of_stack)/
> PER_CPU(kernel_stack)/PER_CPU(tss->sp0) mess
> on top of my patches.
>
> First: this seems to be the easier way.

It's probably easier, but it's harder to review since you're making
two change at once (changing the rsp loaded during syscall and
changing the way that GET_THREAD_INFO works).

--Andy

>
> Second (why do this *after* this patch set, not in it?):
> the removal of KERNEL_STACK_OFFSET has no strong reason behind it
> unless we do MOV -> PUSH conversion - in which case it is mandatory:
> we must have RSP start at top of stack in 64-bit mode.
>
> I want to post a minimal patch set which results in PUSH conversion.
> Want to avoid polluting it with tangential goal of removing
> duplicated percpu variable.
>



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/