Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-06 Thread Brian Gerst 
On Sat, Aug 6, 2016 at 1:25 AM, Borislav Petkov  wrote:
> On Fri, Aug 05, 2016 at 11:01:57AM -0500, Josh Poimboeuf wrote:
>> The 8 should be changed to SIZEOF_PTREGS in a later patch
>> ("x86/asm/head: standardize the end of the stack for idle tasks").
>
> But SIZEOF_PTREGS is 21*8. I don't understand.

This patch is only for the boot cpu's idle thread.  All other kernel
threads, including idle threads for the secondary cpus, already have
the pt_regs area reserved.  My best guess for the current 8 byte
padding is to make sure thread_info is calculated properly (by masking
off the low bits from RSP).

Also, this fix should be applied to 32-bit, but make sure to account
for TOP_OF_KERNEL_STACK_PADDING.

--
Brian Gerst

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-06 Thread Brian Gerst 
On Sat, Aug 6, 2016 at 1:25 AM, Borislav Petkov  wrote:
> On Fri, Aug 05, 2016 at 11:01:57AM -0500, Josh Poimboeuf wrote:
>> The 8 should be changed to SIZEOF_PTREGS in a later patch
>> ("x86/asm/head: standardize the end of the stack for idle tasks").
>
> But SIZEOF_PTREGS is 21*8. I don't understand.

This patch is only for the boot cpu's idle thread.  All other kernel
threads, including idle threads for the secondary cpus, already have
the pt_regs area reserved.  My best guess for the current 8 byte
padding is to make sure thread_info is calculated properly (by masking
off the low bits from RSP).

Also, this fix should be applied to 32-bit, but make sure to account
for TOP_OF_KERNEL_STACK_PADDING.

--
Brian Gerst

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-06 Thread Josh Poimboeuf 
On Sat, Aug 06, 2016 at 09:15:30AM -0400, Brian Gerst wrote:
> On Sat, Aug 6, 2016 at 1:25 AM, Borislav Petkov  wrote:
> > On Fri, Aug 05, 2016 at 11:01:57AM -0500, Josh Poimboeuf wrote:
> >> The 8 should be changed to SIZEOF_PTREGS in a later patch
> >> ("x86/asm/head: standardize the end of the stack for idle tasks").
> >
> > But SIZEOF_PTREGS is 21*8. I don't understand.
> 
> This patch is only for the boot cpu's idle thread.  All other kernel
> threads, including idle threads for the secondary cpus, already have
> the pt_regs area reserved.

Ah, you're right, it does only affect the boot CPU's idle thread.

(To be clear, I think you're talking about patch 41/44, and not the
temporary stack for the verify_cpu() call which I referred to above.)

> My best guess for the current 8 byte
> padding is to make sure thread_info is calculated properly (by masking
> off the low bits from RSP).
>
> Also, this fix should be applied to 32-bit, but make sure to account
> for TOP_OF_KERNEL_STACK_PADDING.

Ok.

-- 
Josh

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-06 Thread Josh Poimboeuf 
On Sat, Aug 06, 2016 at 09:15:30AM -0400, Brian Gerst wrote:
> On Sat, Aug 6, 2016 at 1:25 AM, Borislav Petkov  wrote:
> > On Fri, Aug 05, 2016 at 11:01:57AM -0500, Josh Poimboeuf wrote:
> >> The 8 should be changed to SIZEOF_PTREGS in a later patch
> >> ("x86/asm/head: standardize the end of the stack for idle tasks").
> >
> > But SIZEOF_PTREGS is 21*8. I don't understand.
> 
> This patch is only for the boot cpu's idle thread.  All other kernel
> threads, including idle threads for the secondary cpus, already have
> the pt_regs area reserved.

Ah, you're right, it does only affect the boot CPU's idle thread.

(To be clear, I think you're talking about patch 41/44, and not the
temporary stack for the verify_cpu() call which I referred to above.)

> My best guess for the current 8 byte
> padding is to make sure thread_info is calculated properly (by masking
> off the low bits from RSP).
>
> Also, this fix should be applied to 32-bit, but make sure to account
> for TOP_OF_KERNEL_STACK_PADDING.

Ok.

-- 
Josh

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-06 Thread Borislav Petkov 
On Fri, Aug 05, 2016 at 11:01:57AM -0500, Josh Poimboeuf wrote:
> The 8 should be changed to SIZEOF_PTREGS in a later patch
> ("x86/asm/head: standardize the end of the stack for idle tasks").

But SIZEOF_PTREGS is 21*8. I don't understand.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-06 Thread Borislav Petkov 
On Fri, Aug 05, 2016 at 11:01:57AM -0500, Josh Poimboeuf wrote:
> The 8 should be changed to SIZEOF_PTREGS in a later patch
> ("x86/asm/head: standardize the end of the stack for idle tasks").

But SIZEOF_PTREGS is 21*8. I don't understand.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-06 Thread Josh Poimboeuf 
On Sat, Aug 06, 2016 at 07:25:21AM +0200, Borislav Petkov wrote:
> On Fri, Aug 05, 2016 at 11:01:57AM -0500, Josh Poimboeuf wrote:
> > The 8 should be changed to SIZEOF_PTREGS in a later patch
> > ("x86/asm/head: standardize the end of the stack for idle tasks").
> 
> But SIZEOF_PTREGS is 21*8. I don't understand.

I was referring to this patch:

  [PATCH v2 41/44] x86/asm/head: standardize the end of the stack for idle tasks
  
https://lkml.kernel.org/r/98f297ffbc2a23131f08c5c77c4db974e0de2ad3.1470345772.git.jpoim...@redhat.com

It changes the stack end offset from 8 to SIZEOF_PTREGS, so idle tasks
will have the same end of stack address that other tasks do.  I was
thinking we should make a similar change here, for consistency:

/*
 * Setup stack for verify_cpu(). "-8" because stack_start is defined
 * this way, see below. Our best guess is a NULL ptr for stack
 * termination heuristics and we don't want to break anything which
 * might depend on it (kgdb, ...).
 */
leaq(__end_init_task - 8)(%rip), %rsp

-- 
Josh

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-06 Thread Josh Poimboeuf 
On Sat, Aug 06, 2016 at 07:25:21AM +0200, Borislav Petkov wrote:
> On Fri, Aug 05, 2016 at 11:01:57AM -0500, Josh Poimboeuf wrote:
> > The 8 should be changed to SIZEOF_PTREGS in a later patch
> > ("x86/asm/head: standardize the end of the stack for idle tasks").
> 
> But SIZEOF_PTREGS is 21*8. I don't understand.

I was referring to this patch:

  [PATCH v2 41/44] x86/asm/head: standardize the end of the stack for idle tasks
  
https://lkml.kernel.org/r/98f297ffbc2a23131f08c5c77c4db974e0de2ad3.1470345772.git.jpoim...@redhat.com

It changes the stack end offset from 8 to SIZEOF_PTREGS, so idle tasks
will have the same end of stack address that other tasks do.  I was
thinking we should make a similar change here, for consistency:

/*
 * Setup stack for verify_cpu(). "-8" because stack_start is defined
 * this way, see below. Our best guess is a NULL ptr for stack
 * termination heuristics and we don't want to break anything which
 * might depend on it (kgdb, ...).
 */
leaq(__end_init_task - 8)(%rip), %rsp

-- 
Josh

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-05 Thread Josh Poimboeuf 
On Fri, Aug 05, 2016 at 10:28:39AM -0500, Nilay Vaish wrote:
> On 4 August 2016 at 17:21, Josh Poimboeuf  wrote:
> > The 'stack_start' variable is similar in usage to 'initial_code' and
> > 'initial_gs': they're all stored in head_64.S and they're all updated by
> > SMP and ACPI suspend before starting a CPU.
> >
> > Rename it to 'initial_stack' to be consistent with the others.
> >
> 
> May be change the following line as well:
> 
> ./arch/x86/kernel/head_64.S:69: * Setup stack for verify_cpu().
> "-8" because stack_start is defined

Ah, yeah, I missed that one.

And also, now that I see that comment, and the line below it:

  leaq(__end_init_task - 8)(%rip), %rsp

The 8 should be changed to SIZEOF_PTREGS in a later patch
("x86/asm/head: standardize the end of the stack for idle tasks").

-- 
Josh

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-05 Thread Josh Poimboeuf 
On Fri, Aug 05, 2016 at 10:28:39AM -0500, Nilay Vaish wrote:
> On 4 August 2016 at 17:21, Josh Poimboeuf  wrote:
> > The 'stack_start' variable is similar in usage to 'initial_code' and
> > 'initial_gs': they're all stored in head_64.S and they're all updated by
> > SMP and ACPI suspend before starting a CPU.
> >
> > Rename it to 'initial_stack' to be consistent with the others.
> >
> 
> May be change the following line as well:
> 
> ./arch/x86/kernel/head_64.S:69: * Setup stack for verify_cpu().
> "-8" because stack_start is defined

Ah, yeah, I missed that one.

And also, now that I see that comment, and the line below it:

  leaq(__end_init_task - 8)(%rip), %rsp

The 8 should be changed to SIZEOF_PTREGS in a later patch
("x86/asm/head: standardize the end of the stack for idle tasks").

-- 
Josh

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-05 Thread Nilay Vaish 
On 4 August 2016 at 17:21, Josh Poimboeuf  wrote:
> The 'stack_start' variable is similar in usage to 'initial_code' and
> 'initial_gs': they're all stored in head_64.S and they're all updated by
> SMP and ACPI suspend before starting a CPU.
>
> Rename it to 'initial_stack' to be consistent with the others.
>

May be change the following line as well:

./arch/x86/kernel/head_64.S:69: * Setup stack for verify_cpu().
"-8" because stack_start is defined


--
Nilay

Re: [PATCH v2 03/44] x86/asm/head: rename 'stack_start' -> 'initial_stack'

2016-08-05 Thread Nilay Vaish 
On 4 August 2016 at 17:21, Josh Poimboeuf  wrote:
> The 'stack_start' variable is similar in usage to 'initial_code' and
> 'initial_gs': they're all stored in head_64.S and they're all updated by
> SMP and ACPI suspend before starting a CPU.
>
> Rename it to 'initial_stack' to be consistent with the others.
>

May be change the following line as well:

./arch/x86/kernel/head_64.S:69: * Setup stack for verify_cpu().
"-8" because stack_start is defined


--
Nilay