percpu: Optimize percpu accesses

* Support an option

CONFIG_HAVE_ZERO_BASED_PER_CPU

to make offsets for per cpu variables to start at zero.

If a percpu area starts at zero then:

- We do not need RELOC_HIDE anymore

- Provides for the future capability of architectures providing a per cpu allocator that returns offsets instead of pointers. The offsets would be independent of the processor so that address calculations can be done in a processor independent way. Per cpu instructions can then add the processor specific offset at the last minute possibly in an atomic instruction.

The data the linker provides is different for zero based percpu segments:

__per_cpu_load -> The address at which the percpu area was loaded
__per_cpu_size -> The length of the per cpu area

* Removes the &__per_cpu_x in lockdep. The __per_cpu_x are already pointers. There is no need to take the address.

* Updates kernel/module.c to be able to deal with a percpu area that is loaded at __per_cpu_load but is accessed at __per_cpu_start.

Based on linux-2.6.tip

* x86 percpu ops now will work on 64 bit too, so add the missing 8 byte cases.

* Add a few atomic ops that will be useful in the future:

x86_xchg_percpu()
x86_cmpxchg_percpu().

x86_inc_percpu() - Increment by one can generate more efficient
x86_dec_percpu() instructions and inc/dec will be supported by
cpu ops later.

* Use per_cpu_var() instead of per_cpu__##xxx.

Based on linux-2.6.tip

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area, cpu_pda() is no longer needed and per_cpu() can be used instead. This also makes the _cpu_pda[] table obsolete.

* Since %gs is pointing to the pda, it will then also point to the per cpu variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

Quoted 9 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area, cpu_pda() is no longer needed and per_cpu() can be used instead. This also
makes

    the _cpu_pda[] table obsolete.

Quoted 6 lines

* Since %gs is pointing to the pda, it will then also point to the
per cpu
variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

Unfortunately that doesn't actually work, because you can't have a reloc with two variables.

In something like:

    mov %gs:per_cpu__foo - 12345, %rax
    mov %gs:per_cpu__foo, %rax
    mov %gs:per_cpu__foo - 12345(%rip), %rax
    mov %gs:per_cpu__foo(%rip), %rax
    mov %gs:per_cpu__foo - __per_cpu_start, %rax
    mov %gs:per_cpu__foo - __per_cpu_start(%rip), %rax

the last two lines will not assemble:

t.S:5: Error: can't resolve per_cpu__foo' {*UND* section} - __per_cpu_start' {*UND* section}
t.S:6: Error: can't resolve per_cpu__foo' {*UND* section} - __per_cpu_start' {*UND* section}

Unfortunately, the only way I can think of fixing this is to compute the offset into a temp register, then use that:

    lea per_cpu__foo(%rip), %rax
    mov %gs:__per_cpu_offset(%rax), %rax

(where __per_cpu_offset is defined in the linker script as -__per_cpu_start).

This seems to be a general problem with zero-offset per-cpu. And its unfortunate, because no-register access to per-cpu variables is nice to have.

The other alternative - and I have no idea whether this is practical or possible - is to define a complete set of pre-offset per_cpu symbols.

J

Quoted 11 lines · Jeremy Fitzhardinge

Quoted 9 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area, cpu_pda() is no longer needed and per_cpu() can be used instead. This also
makes

    the _cpu_pda[] table obsolete.

Quoted 7 lines

Quoted 7 lines

* Since %gs is pointing to the pda, it will then also point to the
per cpu
variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

The above is only a partial story (I folded the two patches but didn't update the comments correctly.] The variables are already offset from __per_cpu_start by virtue of the .data.percpu section being based at zero. Therefore only the %gs register needs to be set to the base of each cpu's percpu section to resolve the target address:

        %gs:&per_cpu_xxxx

Quoted 28 lines · Mike Travis

Quoted 12 lines · Jeremy Fitzhardinge

Quoted 10 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area,
cpu_pda()
is no longer needed and per_cpu() can be used instead. This also
makes

    the _cpu_pda[] table obsolete.

Quoted 7 lines

Quoted 7 lines

* Since %gs is pointing to the pda, it will then also point to the
per cpu
variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

The above is only a partial story (I folded the two patches but didn't
update the comments correctly.] The variables are already offset from __per_cpu_start by virtue of the .data.percpu section being based at zero. Therefore only the %gs register needs to be set to the base of each cpu's percpu section to resolve the target address:

Quoted 2 lines

        %gs:&per_cpu_xxxx

Oh, good. I'd played with trying to make that work at one point, and got lost in linker bugs and/or random version-specific strangeness.
J

Quoted 2 lines

	%gs:[&per_cpu_xxxx - __per_cpu_start]

Unfortunately that doesn't actually work, because you can't have a reloc with two variables.

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area, cpu_pda() is no longer needed and per_cpu() can be used instead. This also makes the _cpu_pda[] table obsolete.

* Since %gs is pointing to the pda, it will then also point to the per cpu variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

Based on linux-2.6.tip

Quoted 8 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area, cpu_pda() is no longer needed and per_cpu() can be used instead. This also makes the _cpu_pda[] table obsolete.

Quoted 6 lines

* Since %gs is pointing to the pda, it will then also point to the per cpu variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

Based on linux-2.6.tip

-tip testing found an instantaneous reboot crash on 64-bit x86, with this config:

http://redhat.com/~mingo/misc/config-Thu_Jun__5_11_43_51_CEST_2008.bad

there is no boot log as the instantaneous reboot happens before anything is printed to the (early-) serial console. I have bisected it down to:

| 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f is first bad commit
| commit 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f
| Author: Mike Travis <[email protected]>
| Date:   Tue Jun 3 17:30:21 2008 -0700
|
|     x86_64: Fold pda into per cpu area

the big problem is not just this crash, but that the patch is _way_ too big:

 arch/x86/Kconfig                 |    3 +
 arch/x86/kernel/head64.c         |   34 ++++++--------
 arch/x86/kernel/irq_64.c         |   36 ++++++++-------
 arch/x86/kernel/setup.c          |   90 ++++++++++++---------------------------
 arch/x86/kernel/setup64.c        |    5 --
 arch/x86/kernel/smpboot.c        |   51 ----------------------
 arch/x86/kernel/traps_64.c       |   11 +++-
 arch/x86/kernel/vmlinux_64.lds.S |    1 
 include/asm-x86/percpu.h         |   48 ++++++--------------
 9 files changed, 89 insertions(+), 190 deletions(-)

considering the danger involved, this is just way too large, and there's no reasonable debugging i can do in the bisection to narrow it down any further.

Please resubmit with the bug fixed and with a proper splitup, the more patches you manage to create, the better. For a dangerous code area like this, with a track record of frequent breakages in the past, i would not mind a "one line of code changed per patch" splitup either. (Feel free to send a git tree link for us to try as well.)

Ingo

Quoted 47 lines · Ingo Molnar

Quoted 8 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area, cpu_pda() is no longer needed and per_cpu() can be used instead. This also makes the _cpu_pda[] table obsolete.

Quoted 7 lines

* Since %gs is pointing to the pda, it will then also point to the per cpu variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

Based on linux-2.6.tip

-tip testing found an instantaneous reboot crash on 64-bit x86, with this config:

http://redhat.com/~mingo/misc/config-Thu_Jun__5_11_43_51_CEST_2008.bad

there is no boot log as the instantaneous reboot happens before anything is printed to the (early-) serial console. I have bisected it down to:

| 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f is first bad commit
| commit 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f
| Author: Mike Travis <[email protected]>
| Date:   Tue Jun 3 17:30:21 2008 -0700
|
|     x86_64: Fold pda into per cpu area

the big problem is not just this crash, but that the patch is _way_ too big:

 arch/x86/Kconfig                 |    3 +
 arch/x86/kernel/head64.c         |   34 ++++++--------
 arch/x86/kernel/irq_64.c         |   36 ++++++++-------
 arch/x86/kernel/setup.c          |   90 ++++++++++++---------------------------
 arch/x86/kernel/setup64.c        |    5 --
 arch/x86/kernel/smpboot.c        |   51 ----------------------
 arch/x86/kernel/traps_64.c       |   11 +++-
 arch/x86/kernel/vmlinux_64.lds.S |    1 
 include/asm-x86/percpu.h         |   48 ++++++--------------
 9 files changed, 89 insertions(+), 190 deletions(-)

considering the danger involved, this is just way too large, and there's no reasonable debugging i can do in the bisection to narrow it down any further.

Quoted 4 lines

Please resubmit with the bug fixed and with a proper splitup, the more patches you manage to create, the better. For a dangerous code area like this, with a track record of frequent breakages in the past, i would not mind a "one line of code changed per patch" splitup either. (Feel free to send a git tree link for us to try as well.)

Ingo

Thanks for the feedback Ingo. I'll test the above config and look at splitting up the patch. The difficulty is making each patch independently compilable and testable.

Quoted 64 lines · Mike Travis

Quoted 62 lines · Ingo Molnar

Quoted 10 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area,
cpu_pda()
is no longer needed and per_cpu() can be used instead. This
also makes

    the _cpu_pda[] table obsolete.

Quoted 8 lines

* Since %gs is pointing to the pda, it will then also point to the
per cpu
variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

Based on linux-2.6.tip

-tip testing found an instantaneous reboot crash on 64-bit x86, with this config:

http://redhat.com/~mingo/misc/config-Thu_Jun__5_11_43_51_CEST_2008.bad

there is no boot log as the instantaneous reboot happens before anything is printed to the (early-) serial console. I have bisected it down to:

| 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f is first bad commit
| commit 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f
| Author: Mike Travis <[email protected]>
| Date:   Tue Jun 3 17:30:21 2008 -0700
|
|     x86_64: Fold pda into per cpu area

the big problem is not just this crash, but that the patch is _way_ too big:

 arch/x86/Kconfig                 |    3 +
 arch/x86/kernel/head64.c         |   34 ++++++--------
 arch/x86/kernel/irq_64.c         |   36 ++++++++-------
 arch/x86/kernel/setup.c          |   90
++++++++++++---------------------------
 arch/x86/kernel/setup64.c        |    5 --
 arch/x86/kernel/smpboot.c        |   51 ----------------------
 arch/x86/kernel/traps_64.c       |   11 +++-
 arch/x86/kernel/vmlinux_64.lds.S |    1
 include/asm-x86/percpu.h         |   48 ++++++--------------
 9 files changed, 89 insertions(+), 190 deletions(-)

considering the danger involved, this is just way too large, and there's no reasonable debugging i can do in the bisection to narrow it down any further.

Please resubmit with the bug fixed and with a proper splitup, the more patches you manage to create, the better. For a dangerous code area like this, with a track record of frequent breakages in the past, i would not mind a "one line of code changed per patch" splitup either. (Feel free to send a git tree link for us to try as well.)

Quoted 7 lines

Quoted 2 lines

Ingo

Thanks for the feedback Ingo. I'll test the above config and look at splitting up the patch. The difficulty is making each patch independently
compilable and testable.

FWIW, I'm getting past the "crashes very, very early" stage with this series applied when booting under Xen. Then it crashes pretty early, but that's not your fault...

J

Quoted 77 lines · Jeremy Fitzhardinge

Quoted 67 lines · Mike Travis

Quoted 64 lines · Ingo Molnar

Quoted 10 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area,
cpu_pda()
is no longer needed and per_cpu() can be used instead. This
also makes

    the _cpu_pda[] table obsolete.

Quoted 9 lines

* Since %gs is pointing to the pda, it will then also point to the
per cpu
variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

Based on linux-2.6.tip

-tip testing found an instantaneous reboot crash on 64-bit x86, with this config:

http://redhat.com/~mingo/misc/config-Thu_Jun__5_11_43_51_CEST_2008.bad

there is no boot log as the instantaneous reboot happens before anything is printed to the (early-) serial console. I have bisected it down to:

| 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f is first bad commit
| commit 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f
| Author: Mike Travis <[email protected]>
| Date:   Tue Jun 3 17:30:21 2008 -0700
|
|     x86_64: Fold pda into per cpu area

the big problem is not just this crash, but that the patch is _way_ too big:

 arch/x86/Kconfig                 |    3 +
 arch/x86/kernel/head64.c         |   34 ++++++--------
 arch/x86/kernel/irq_64.c         |   36 ++++++++-------
 arch/x86/kernel/setup.c          |   90
++++++++++++---------------------------
 arch/x86/kernel/setup64.c        |    5 --
 arch/x86/kernel/smpboot.c        |   51 ----------------------
 arch/x86/kernel/traps_64.c       |   11 +++-
 arch/x86/kernel/vmlinux_64.lds.S |    1
 include/asm-x86/percpu.h         |   48 ++++++--------------
 9 files changed, 89 insertions(+), 190 deletions(-)

considering the danger involved, this is just way too large, and there's no reasonable debugging i can do in the bisection to narrow it down any further.

Please resubmit with the bug fixed and with a proper splitup, the more patches you manage to create, the better. For a dangerous code area like this, with a track record of frequent breakages in the past, i would not mind a "one line of code changed per patch" splitup either. (Feel free to send a git tree link for us to try as well.)

Quoted 8 lines

Quoted 3 lines

Ingo

Thanks for the feedback Ingo. I'll test the above config and look at splitting up the patch. The difficulty is making each patch independently
compilable and testable.

Quoted 6 lines

FWIW, I'm getting past the "crashes very, very early" stage with this series applied when booting under Xen. Then it crashes pretty early, but that's not your fault...

J

Hi Jeremy,

Yes we have a simulator for Nahelem that also breezes past the boot up problem (actually makes it to the kernel login prompt.) Weirdly, the problem doesn't exist in an earlier code base so my changes are tickling something else newly introduced. I'm attempting to see if I can use GRUB 2 with the GDB stubs to track it down (which is time consuming in itself to setup.)

It is definitely related to basing percpu variable offsets from %gs and (I think) interrupts.

Quoted 11 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area, cpu_pda() is no longer needed and per_cpu() can be used instead. This also makes the _cpu_pda[] table obsolete.

* Since %gs is pointing to the pda, it will then also point to the per cpu variables and can be accessed thusly:

Quoted 3 lines

	%gs:[&per_cpu_xxxx - __per_cpu_start]

Based on linux-2.6.tip

-tip testing found an instantaneous reboot crash on 64-bit x86, with this config:

http://redhat.com/~mingo/misc/config-Thu_Jun__5_11_43_51_CEST_2008.bad

there is no boot log as the instantaneous reboot happens before anything is printed to the (early-) serial console. I have bisected it down to:

| 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f is first bad commit
| commit 7670dc09e89a2b151a1cf49eccebc07c41c2ce9f
| Author: Mike Travis <[email protected]>
| Date:   Tue Jun 3 17:30:21 2008 -0700
|
|     x86_64: Fold pda into per cpu area

the big problem is not just this crash, but that the patch is _way_ too big:

 arch/x86/Kconfig                 |    3 +
 arch/x86/kernel/head64.c         |   34 ++++++--------
 arch/x86/kernel/irq_64.c         |   36 ++++++++-------
 arch/x86/kernel/setup.c          |   90 ++++++++++++---------------------------
 arch/x86/kernel/setup64.c        |    5 --
 arch/x86/kernel/smpboot.c        |   51 ----------------------
 arch/x86/kernel/traps_64.c       |   11 +++-
 arch/x86/kernel/vmlinux_64.lds.S |    1 
 include/asm-x86/percpu.h         |   48 ++++++--------------
 9 files changed, 89 insertions(+), 190 deletions(-)

considering the danger involved, this is just way too large, and there's no reasonable debugging i can do in the bisection to narrow it down any further.

Please resubmit with the bug fixed and with a proper splitup, the more patches you manage to create, the better. For a dangerous code area like this, with a track record of frequent breakages in the past, i would not mind a "one line of code changed per patch" splitup either. (Feel free to send a git tree link for us to try as well.)

Ingo

Quoted 21 lines · Ingo Molnar

Quoted 8 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area, cpu_pda() is no longer needed and per_cpu() can be used instead. This also makes the _cpu_pda[] table obsolete.

Quoted 7 lines

* Since %gs is pointing to the pda, it will then also point to the per cpu variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

Based on linux-2.6.tip

-tip testing found an instantaneous reboot crash on 64-bit x86, with this config:

Quoted 2 lines

http://redhat.com/~mingo/misc/config-Thu_Jun__5_11_43_51_CEST_2008.bad

I'm still stuck on this one. One new development is that the current -tip branch without the patches boots to the kernel prompt then hangs after a few moments and then reboots. It seems you can tickle it using ^C to abort a process.

Quoted 29 lines · Mike Travis

Quoted 17 lines · Ingo Molnar

Quoted 14 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area,
cpu_pda()
is no longer needed and per_cpu() can be used instead. This
also makes

    the _cpu_pda[] table obsolete.

* Since %gs is pointing to the pda, it will then also point to the
per cpu
variables and can be accessed thusly:

Quoted 9 lines

Quoted 4 lines

    %gs:[&per_cpu_xxxx - __per_cpu_start]

Based on linux-2.6.tip

-tip testing found an instantaneous reboot crash on 64-bit x86, with this config:

http://redhat.com/~mingo/misc/config-Thu_Jun__5_11_43_51_CEST_2008.bad

Quoted 6 lines

I'm still stuck on this one. One new development is that the current -tip
branch without the patches boots to the kernel prompt then hangs after a few
moments and then reboots. It seems you can tickle it using ^C to abort a process.

Hi Mike,

I added some instrumentation to Xen to print the cpu state on triple-fault, which highlights an obvious-looking problem.

(XEN) hvm.c:767:d1 Triple fault on VCPU0 - invoking HVM system reset. (XEN) ----[ Xen-3.3-unstable x86_64 debug=y Not tainted ]----
(XEN) CPU: 1
(XEN) RIP: 0010:[<ffffffff80200160>]
(XEN) RFLAGS: 0000000000010002 CONTEXT: hvm

(XEN) rax: 0000000000000018   rbx: 0000000000000000   rcx: 00000000c0000080
(XEN) rdx: 0000000000000000   rsi: 0000000000092f40   rdi: 0000000020100800
(XEN) rbp: 0000000000000000   rsp: ffffffff807dfff8   r8:  0000000000208000
(XEN) r9:  0000000000000000   r10: 0000000000000000   r11: 00000000000000de
(XEN) r12: 0000000000000000   r13: 0000000000000000   r14: 0000000000000000
(XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4: 00000000000000a0
(XEN) cr3: 0000000000201000   cr2: 0000000000000000

(XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: 0000 cs: 0010

The rip is:

(gdb) x/i 0xffffffff80200160
0xffffffff80200160 <secondary_startup_64+96>: movl %eax,%ds

which is:

    lgdt    early_gdt_descr(%rip)

    /* set up data segments. actually 0 would do too */
    movl $__KERNEL_DS,%eax
    movl %eax,%ds
    movl %eax,%ss
    movl %eax,%es

And early_gdt_descr is:

.globl early_gdt_descr
early_gdt_descr:
.word GDT_ENTRIES*8-1
.quad per_cpu__gdt_page

and per_cpu__gdt_page is zero-based, and therefore not a directly addressable symbol.

I tried this patch, but it didn't work. Perhaps I'm missing something.

diff -r bf5a46e13f78 arch/x86/kernel/head_64.S

arch/x86/kernel/head_64.S Wed Jun 18 10:34:24 2008 -0700 renamed from arch/x86/kernel/head_64.S Tue Jun 17 22:10:51 2008 -0700 +2 -0

@@ -94,6 +94,8 @@
 
    addq    %rbp, level2_fixmap_pgt + (506*8)(%rip)
 
+    addq    $__per_cpu_load, early_gdt_descr+2(%rip)
+   
    /* Add an Identity mapping if I am above 1G */
    leaq    _text(%rip), %rdi
    andq    $PMD_PAGE_MASK, %rdi

J

Hi Jeremy,

I'm not finding that code in the tip/latest or linux-next branches... ?

I can send you my latest version of the patch which is better than the previous but still is having problems with the config file that Ingo sent out. (It also has a weird quirk that it will hang and reboot after about 30 seconds with or without my patch.)

Quoted 101 lines · Jeremy Fitzhardinge

Quoted 32 lines · Mike Travis

Quoted 20 lines · Ingo Molnar

Quoted 16 lines · Mike Travis

* Declare the pda as a per cpu variable.

* Make the x86_64 per cpu area start at zero.

* Since the pda is now the first element of the per_cpu area,
cpu_pda()
is no longer needed and per_cpu() can be used instead. This
also makes

    the _cpu_pda[] table obsolete.

* Since %gs is pointing to the pda, it will then also point to the
per cpu
variables and can be accessed thusly:

%gs:[&per_cpu_xxxx - __per_cpu_start]

Quoted 9 lines

Quoted 3 lines

Based on linux-2.6.tip

-tip testing found an instantaneous reboot crash on 64-bit x86, with this config:

http://redhat.com/~mingo/misc/config-Thu_Jun__5_11_43_51_CEST_2008.bad

Quoted 7 lines

I'm still stuck on this one. One new development is that the current -tip
branch without the patches boots to the kernel prompt then hangs after a few
moments and then reboots. It seems you can tickle it using ^C to abort a process.

Hi Mike,

I added some instrumentation to Xen to print the cpu state on triple-fault, which highlights an obvious-looking problem.

(XEN) hvm.c:767:d1 Triple fault on VCPU0 - invoking HVM system reset. (XEN) ----[ Xen-3.3-unstable x86_64 debug=y Not tainted ]----
(XEN) CPU: 1
(XEN) RIP: 0010:[<ffffffff80200160>]
(XEN) RFLAGS: 0000000000010002 CONTEXT: hvm

(XEN) rax: 0000000000000018   rbx: 0000000000000000   rcx: 00000000c0000080
(XEN) rdx: 0000000000000000   rsi: 0000000000092f40   rdi: 0000000020100800
(XEN) rbp: 0000000000000000   rsp: ffffffff807dfff8   r8:  0000000000208000
(XEN) r9:  0000000000000000   r10: 0000000000000000   r11: 00000000000000de
(XEN) r12: 0000000000000000   r13: 0000000000000000   r14: 0000000000000000
(XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4: 00000000000000a0
(XEN) cr3: 0000000000201000   cr2: 0000000000000000

(XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: 0000 cs: 0010

The rip is:

(gdb) x/i 0xffffffff80200160
0xffffffff80200160 <secondary_startup_64+96>: movl %eax,%ds

which is:

    lgdt    early_gdt_descr(%rip)

    /* set up data segments. actually 0 would do too */
    movl $__KERNEL_DS,%eax
    movl %eax,%ds
    movl %eax,%ss
    movl %eax,%es

And early_gdt_descr is:

.globl early_gdt_descr
early_gdt_descr:
.word GDT_ENTRIES*8-1
.quad per_cpu__gdt_page

and per_cpu__gdt_page is zero-based, and therefore not a directly addressable symbol.

I tried this patch, but it didn't work. Perhaps I'm missing something.

diff -r bf5a46e13f78 arch/x86/kernel/head_64.S

arch/x86/kernel/head_64.S Wed Jun 18 10:34:24 2008 -0700 renamed from arch/x86/kernel/head_64.S Tue Jun 17 22:10:51 2008 -0700 +2 -0

@@ -94,6 +94,8 @@
 
    addq    %rbp, level2_fixmap_pgt + (506*8)(%rip)
 
+    addq    $__per_cpu_load, early_gdt_descr+2(%rip)
+   
    /* Add an Identity mapping if I am above 1G */
    leaq    _text(%rip), %rdi
    andq    $PMD_PAGE_MASK, %rdi

Quoted 2 lines

J

Hi Jeremy,

I'm not finding that code in the tip/latest or linux-next branches... ?

I can send you my latest version of the patch which is better than the previous but still is having problems with the config file that Ingo sent out. (It also has a weird quirk that it will hang and reboot after about 30 seconds with or without my patch.)

Oh yeah, is it alright to re-use the pda in the static percpu load area for each startup cpu, or should it be adjusted to use the areas allocated by setup_per_cpu_areas()? pda_init() is called in x86_64_start_kernel so it would only be for anything that occurs before then. (And I moved the call to pda_init() to before the early_idt_handlers are setup.)

I hadn't realized that this code is executed for cpus other than the boot cpu. Is there a way to find out if this is the boot cpu (and/or the initial execution)?

If it's the boot cpu, then this would work for the gdt, yes?

        leaq    early_gdt_descr_base(%rip), %edi
        movq    0(%edi), %rax
        addq    $__per_cpu_load, %rax
        movq    %rax, 0(%edi)
        lgdt    early_gdt_descr(%rip)

But it should only be executed for the boot because do_boot_cpu() does this:

        early_gdt_descr.address = (unsigned long)get_cpu_gdt_table(cpu);

static inline struct desc_struct *get_cpu_gdt_table(unsigned int cpu)

{
        return per_cpu(gdt_page, cpu).gdt;
}

Btw, I've only been testing on an x86_64 system. I'm sure I've got things to fix up for i386.

64-bit inherits 32-bit's use of per-cpu gdts, though its mostly useless on 64-bit.

Why not use the real pda for all cpus?

Do you move the boot-cpu's per-cpu data? (Please don't) If not, you can just use percpu__pda from the start without having to do anything else, and then set up %gs pointing to the pda base for each secondary cpu.

Quoted 2 lines · Jeremy Fitzhardinge

Why not use the real pda for all cpus?

Yeah, I figured that out after doing some more thinking... ;-)

Quoted 4 lines

Do you move the boot-cpu's per-cpu data? (Please don't) If not, you can just use percpu__pda from the start without having to do anything else, and then set up %gs pointing to the pda base for each secondary cpu.

The problem is that the static percpu area is removed as it lies in the initdata section, so the pda is removed as well.

But I took your suggestion to move the fixup to before secondary_startup.

Below is a revised version.

It builds but I'll have to test it tomorrow.
Note the addition of:

+ initial_pda = (unsigned long)get_percpu_pda(cpu);

in do_boot_cpu.

I'm not sure yet what to put into acpi_save_state_mem:

        initial_code = (unsigned long)wakeup_long64;

+ /* ZZZ initial_pda = (unsigned long)?; */

Mike Travis wrote:

Quoted 5 lines

I'm not sure yet what to put into acpi_save_state_mem:

        initial_code = (unsigned long)wakeup_long64;

+ /* ZZZ initial_pda = (unsigned long)?; */

You'll need to change wakeup_long64 to load the right value into the GS_BASE msr anyway.

J

Quoted 17 lines · Jeremy Fitzhardinge

Review ident_complete: · line 143

Quoted 12 lines · Mike Travis

(quoted diff) +6 -0

@@ -132,6 +133,12 @@ ident_complete:
 #ifdef CONFIG_SMP
     addq    %rbp, trampoline_level4_pgt + 0(%rip)
     addq    %rbp, trampoline_level4_pgt + (511*8)(%rip)
+
+    /*
+     * Fix up per_cpu__gdt_page offset when basing percpu
+     * variables at zero.  This is only needed for the boot cpu.
+     */
+    addq    $__per_cpu_load, early_gdt_descr_base
 
 

This needs to be rip-relative. An absolute reference here will fail because you're still running in physical addresses.

Quoted 2 lines

J

Still bombs right at boot up... ;-(

So it seems the problem is that the pre-initialized gdt_page is being lost and replaced with zero. Linker script bug?

Quoted 2 lines · Jeremy Fitzhardinge

So it seems the problem is that the pre-initialized gdt_page is being lost and replaced with zero. Linker script bug?

Is the pre initialized gdt page in the per cpu area? Does not look like it. The loader setup for the percpu section changes with zero basing. Maybe that has bad side effects?

Yes, it is... The fixup logic is this:

0000000000004000 D per_cpu__gdt_page
ffffffff81911000 A __per_cpu_load

arch/x86/kernel/cpu/common.c:

DEFINE_PER_CPU_PAGE_ALIGNED(struct gdt_page, gdt_page) = { .gdt = {
        [GDT_ENTRY_KERNEL_CS] = { { { 0x0000ffff, 0x00cf9a00 } } },

Quoted 3 lines · Jeremy Fitzhardinge

So it seems the problem is that the pre-initialized gdt_page is being lost and replaced with zero. Linker script bug?

Is the pre initialized gdt page in the per cpu area? Does not look like it.

The loader setup for the percpu section changes with zero basing. Maybe that has bad side effects

Quoted 2 lines

The loader setup for the percpu section changes with zero basing. Maybe that has bad side effects

How does it work? The symbols in the percpu segment are 0-based, but where does the data for the sections which correspond to that segment go?

So the question is what kernel virtual address is it being loaded to? __per_cpu_load is ffffffff808d1000, so ffffffff808d6000 is what you'd expect...

Hm, but what happens when this gets converted to bzImage? Hm, looks OK, I think.

BTW, I think __per_cpu_load will cause trouble if you make a relocatable kernel, being an absolute symbol. But I have relocation off at the moment.

Quoted 6 lines · Jeremy Fitzhardinge

Quoted 3 lines

The loader setup for the percpu section changes with zero basing. Maybe that has bad side effects

How does it work? The symbols in the percpu segment are 0-based, but where does the data for the sections which correspond to that segment go?

Its loaded at __per_cpu_load but the symbols have addresses starting at 0.

Quoted 4 lines

So the question is what kernel virtual address is it being loaded to? __per_cpu_load is ffffffff808d1000, so ffffffff808d6000 is what you'd expect...

Correct.

Quoted 6 lines

Hm, but what happens when this gets converted to bzImage? Hm, looks OK, I think.

BTW, I think __per_cpu_load will cause trouble if you make a relocatable kernel, being an absolute symbol. But I have relocation off at the moment.

Hmmm.... we could add the relocation offset to __per_cpu_load? __per_cpu_load is used very sparingly. Basically only useful during early boot and when a new per cpu area has to be setup. In that case we want to copy from __per_cpu_load to the newly allocated percpu area.

Quoted 2 lines

Its loaded at __per_cpu_load but the symbols have addresses starting at 0.

Yes, which leads to an odd-looking ELF file where the Phdrs aren't sorted by virtual address order. I'm wondering what would happen if a bootloader that actually understood ELF files tried to load it as an actual ELF file...

Quoted 1 line

Hmmm.... we could add the relocation offset to __per_cpu_load? __per_cpu_load is used very sparingly. Basically only useful during early boot and when a new per cpu area has to be setup. In that case we want to copy from __per_cpu_load to the newly allocated percpu area.

Yes, it should be fairly easy to manually relocate it by applying the (load - link) offset to it.

Quoted 10 lines · Christoph Lameter

Quoted 6 lines · Jeremy Fitzhardinge

Quoted 3 lines

The loader setup for the percpu section changes with zero basing. Maybe that has bad side effects

How does it work? The symbols in the percpu segment are 0-based, but where does the data for the sections which correspond to that segment go?

Quoted 2 lines

Its loaded at __per_cpu_load but the symbols have addresses starting at 0.

Yes, which leads to an odd-looking ELF file where the Phdrs aren't sorted by virtual address order. I'm wondering what would happen if a bootloader that actually understood ELF files tried to load it as an actual ELF file...

Quoted 7 lines

Quoted 4 lines

So the question is what kernel virtual address is it being loaded to? __per_cpu_load is ffffffff808d1000, so ffffffff808d6000 is what you'd expect...

Correct.

Well, reading back from that address got zeros, so something is amiss.

Quoted 7 lines

Quoted 6 lines

Hm, but what happens when this gets converted to bzImage? Hm, looks OK, I think.

BTW, I think __per_cpu_load will cause trouble if you make a relocatable kernel, being an absolute symbol. But I have relocation off at the moment.

Quoted 5 lines

Hmmm.... we could add the relocation offset to __per_cpu_load? __per_cpu_load is used very sparingly. Basically only useful during early boot and when a new per cpu area has to be setup. In that case we want to copy from __per_cpu_load to the newly allocated percpu area.

Yes, it should be fairly easy to manually relocate it by applying the (load - link) offset to it.

Quoted 10 lines · Jeremy Fitzhardinge

Quoted 9 lines · Christoph Lameter

Quoted 6 lines · Jeremy Fitzhardinge

Quoted 3 lines

The loader setup for the percpu section changes with zero basing. Maybe that has bad side effects

How does it work? The symbols in the percpu segment are 0-based, but where does the data for the sections which correspond to that segment go?

Quoted 6 lines

Quoted 2 lines

Its loaded at __per_cpu_load but the symbols have addresses starting at 0.

Yes, which leads to an odd-looking ELF file where the Phdrs aren't sorted by virtual address order. I'm wondering what would happen if a bootloader that actually understood ELF files tried to load it as an actual ELF file...

Well /sbin/kexec looks at the physical addresses not the virtual ones so that may not be a problem.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

So the question is what kernel virtual address is it being loaded to? __per_cpu_load is ffffffff808d1000, so ffffffff808d6000 is what you'd expect...

Correct.

Well, reading back from that address got zeros, so something is amiss.

Weird.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Hm, but what happens when this gets converted to bzImage? Hm, looks OK, I think.

BTW, I think __per_cpu_load will cause trouble if you make a relocatable kernel, being an absolute symbol. But I have relocation off at the moment.

Quoted 7 lines

Quoted 5 lines

Hmmm.... we could add the relocation offset to __per_cpu_load? __per_cpu_load is used very sparingly. Basically only useful during early boot and when a new per cpu area has to be setup. In that case we want to copy from __per_cpu_load to the newly allocated percpu area.

Yes, it should be fairly easy to manually relocate it by applying the (load - link) offset to it.

For x86_64 all kernels are built relocatable as the only cost was changing the physical addresses in the initial page tables. The virtual address always remain the same but the physical addresses change. So that could be part of what is going on.

Is this a change that only got tested on x86_32?

As long as we are not changing the way the kernel virtual address are actually being used we should be ok with a change to make the pda 0 based. Still it is an area you need to be especially careful with.

Eric

Quoted 1 line

Is this a change that only got tested on x86_32?

I'm only testing this on x86_64. The zero-based percpu/pda changes worked fine up until just recently. At first it was one of Ingo's "randconfig" config files that was tripping it up, but lately it's not working on any config.

BTW, I think __per_cpu_load will cause trouble if you make a relocatable kernel, being an absolute symbol. But I have relocation off at the moment.

Quoted 5 lines · Jeremy Fitzhardinge

BTW, I think __per_cpu_load will cause trouble if you make a relocatable kernel, being an absolute symbol. But I have relocation off at the moment.

...
Here's where it's defined (in include/asm-generic/vmlinux.lds.h):

#ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU

#define PERCPU(align)                                                   \
        . = ALIGN(align);                                               \
        percpu : { } :percpu                                            \
        __per_cpu_load = .;                                             \
        .data.percpu 0 : AT(__per_cpu_load - LOAD_OFFSET) {             \
                *(.data.percpu.first)                                   \
                *(.data.percpu.shared_aligned)                          \
                *(.data.percpu)                                         \
                *(.data.percpu.page_aligned)                            \
                ____per_cpu_size = .;                                   \
        }                                                               \
        . = __per_cpu_load + ____per_cpu_size;                          \

data : { } :data
#else

Can we generate a new symbol which would account for LOAD_OFFSET?

at __per_coup_load. But it is absolute Ugh.

It might be worth saying something like.

.data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {
	DATA(0)                   
	. = ALIGN(align);
        __per_cpu_load = . ;                   
}

To make __per_cpu_load a relative symbol. ld has a bad habit of taking symbols out of empty sections and making them absolute. Which is why I added the DATA(0).

Still I don't think that would be the 64bit problem.

Quoted 21 lines · Mike Travis

Quoted 4 lines · Jeremy Fitzhardinge

BTW, I think __per_cpu_load will cause trouble if you make a relocatable kernel, being an absolute symbol. But I have relocation off at the moment.

...
Here's where it's defined (in include/asm-generic/vmlinux.lds.h):

#ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU

#define PERCPU(align)                                                   \
        . = ALIGN(align);                                               \
        percpu : { } :percpu                                            \
        __per_cpu_load = .;                                             \
        .data.percpu 0 : AT(__per_cpu_load - LOAD_OFFSET) {             \
                *(.data.percpu.first)                                   \
                *(.data.percpu.shared_aligned)                          \
                *(.data.percpu)                                         \
                *(.data.percpu.page_aligned)                            \
                ____per_cpu_size = .;                                   \
        }                                                               \
        . = __per_cpu_load + ____per_cpu_size;                          \

Quoted 4 lines

data : { } :data
#else

Can we generate a new symbol which would account for LOAD_OFFSET?

Ouch. Absolute symbols indeed. On the 32bit kernel that may play havoc with the relocatable kernel, although we have had similar absolute logic for the last year. With __per_cpu_start and __per_cpu_end so it may not be a problem.

To initialize the percpu data you do want to talk to the virtual address at __per_coup_load. But it is absolute Ugh.

It might be worth saying something like.

.data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {
	DATA(0)                   
	. = ALIGN(align);
        __per_cpu_load = . ;                   
}

To make __per_cpu_load a relative symbol. ld has a bad habit of taking symbols out of empty sections and making them absolute. Which is why I added the DATA(0).

Still I don't think that would be the 64bit problem.

Eric

Quoted 44 lines · Eric W. Biederman

Quoted 22 lines · Mike Travis

Quoted 4 lines · Jeremy Fitzhardinge

BTW, I think __per_cpu_load will cause trouble if you make a relocatable kernel, being an absolute symbol. But I have relocation off at the moment.

...
Here's where it's defined (in include/asm-generic/vmlinux.lds.h):

#ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU

#define PERCPU(align)                                                   \
        . = ALIGN(align);                                               \
        percpu : { } :percpu                                            \
        __per_cpu_load = .;                                             \
        .data.percpu 0 : AT(__per_cpu_load - LOAD_OFFSET) {             \
                *(.data.percpu.first)                                   \
                *(.data.percpu.shared_aligned)                          \
                *(.data.percpu)                                         \
                *(.data.percpu.page_aligned)                            \
                ____per_cpu_size = .;                                   \
        }                                                               \
        . = __per_cpu_load + ____per_cpu_size;                          \

Quoted 5 lines

data : { } :data
#else

Can we generate a new symbol which would account for LOAD_OFFSET?

Ouch. Absolute symbols indeed. On the 32bit kernel that may play havoc with the relocatable kernel, although we have had similar absolute logic for the last year. With __per_cpu_start and __per_cpu_end so it may not be a problem.

To initialize the percpu data you do want to talk to the virtual address at __per_coup_load. But it is absolute Ugh.

It might be worth saying something like.

.data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {
	DATA(0)                   
	. = ALIGN(align);
        __per_cpu_load = . ;                   
}

Quoted 8 lines

To make __per_cpu_load a relative symbol. ld has a bad habit of taking symbols out of empty sections and making them absolute. Which is why I added the DATA(0).

Still I don't think that would be the 64bit problem.

Eric

I'm not sure I understand the linker lingo enough to fill in the rest of the blanks... I've tried various versions around this framework and none have been accepted yet.

#ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU

#define PERCPU(align)                                                   \
        .data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {     \
                DATA(0)                                                 \
                . = ALIGN(align);                                       \
                __per_cpu_load = .;                                     \
                *(.data.percpu.first)                                   \
                *(.data.percpu.shared_aligned)                          \
                *(.data.percpu)                                         \
                *(.data.percpu.page_aligned)                            \
                ____per_cpu_size = . - __per_cpu_load                   \
        }                                                               \

#else

Quoted 82 lines · Mike Travis

Quoted 55 lines · Eric W. Biederman

Quoted 33 lines · Mike Travis

Quoted 6 lines · Jeremy Fitzhardinge

BTW, I think __per_cpu_load will cause trouble if you make a relocatable
kernel, being an absolute symbol. But I have relocation off at the moment.

...
Here's where it's defined (in include/asm-generic/vmlinux.lds.h):

#ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU
#define

PERCPU(align)                                                   \
        . =
ALIGN(align);                                               \
        percpu : { }
:percpu                                            \
        __per_cpu_load =

.; \
.data.percpu 0 : AT(__per_cpu_load - LOAD_OFFSET)
{ \

*(.data.percpu.first) \

*(.data.percpu.shared_aligned) \

*(.data.percpu) \

*(.data.percpu.page_aligned)                            \
                ____per_cpu_size =

.; \

Quoted 8 lines

}                                                               \
        . = __per_cpu_load +

____per_cpu_size; \
data : { } :data
#else

Can we generate a new symbol which would account for LOAD_OFFSET?

Ouch. Absolute symbols indeed. On the 32bit kernel that may play havoc with the relocatable kernel, although we have had similar absolute logic for the last year. With __per_cpu_start and __per_cpu_end so it may not be a problem.

To initialize the percpu data you do want to talk to the virtual address at __per_coup_load. But it is absolute Ugh.
It might be worth saying something like.

.data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {
    DATA(0)                       . = ALIGN(align);
        __per_cpu_load = . ;                   }

Quoted 8 lines

To make __per_cpu_load a relative symbol. ld has a bad habit of taking symbols out of empty sections and making them absolute. Which is why I added the DATA(0).

Still I don't think that would be the 64bit problem.

Eric

I'm not sure I understand the linker lingo enough to fill in the rest of the blanks... I've tried various versions around this framework and none have been accepted yet.

#ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU

#define PERCPU(align)                                                   \
        .data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {     \
                DATA(0)                                                 \
                . = ALIGN(align);                                       \
                __per_cpu_load = .;                                     \
                *(.data.percpu.first)                                   \
                *(.data.percpu.shared_aligned)                          \
                *(.data.percpu)                                         \
                *(.data.percpu.page_aligned)                            \
                ____per_cpu_size = . - __per_cpu_load                   \
        }                                                               \

Quoted 2 lines

#else

That looks OK to me. Does it work?

J

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

To initialize the percpu data you do want to talk to the virtual address at __per_coup_load. But it is absolute Ugh.
It might be worth saying something like.

.data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {
    DATA(0)                       . = ALIGN(align);
        __per_cpu_load = . ;                   }

To make __per_cpu_load a relative symbol. ld has a bad habit of taking symbols out of empty sections and making them absolute. Which is why I added the DATA(0).

Quoted 17 lines

Quoted 17 lines

I'm not sure I understand the linker lingo enough to fill in the rest of the blanks... I've tried various versions around this framework and none have been accepted yet.

#ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU

#define PERCPU(align)                                                   \
        .data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {     \
                DATA(0)                                                 \
                . = ALIGN(align);                                       \
                __per_cpu_load = .;                                     \
                *(.data.percpu.first)                                   \
                *(.data.percpu.shared_aligned)                          \
                *(.data.percpu)                                         \
                *(.data.percpu.page_aligned)                            \
                ____per_cpu_size = . - __per_cpu_load                   \
        }                                                               \

#else

The syntax error is at this "DATA(0)" statement. I don't find this as a linker script command or a macro. What is it we're trying to do with this?

Quoted 4 lines · Mike Travis

The syntax error is at this "DATA(0)" statement. I don't find this as a linker script command or a macro. What is it we're trying to do with this?

In Eric's sample, it's intended to prevent there being an empty section, which can cause linker bugs. In your case it probably isn't necessary, since you're also putting the percpu data in that section.

"DATA" is probably a typo. It should be "LONG" or something like that. (See "3.6.5 Output Section Data" in the linker manual.)

J

Mike Travis <[email protected]> writes:

Quoted 1 line

Can we generate a new symbol which would account for LOAD_OFFSET?

Ouch. Absolute symbols indeed. On the 32bit kernel that may play havoc with the relocatable kernel, although we have had similar absolute logic for the last year. With __per_cpu_start and __per_cpu_end so it may not be a problem.

To initialize the percpu data you do want to talk to the virtual address at __per_coup_load. But it is absolute Ugh.

It might be worth saying something like.

.data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {
	DATA(0)                   
	. = ALIGN(align);
        __per_cpu_load = . ;                   
}

To make __per_cpu_load a relative symbol. ld has a bad habit of taking symbols out of empty sections and making them absolute. Which is why I added the DATA(0).

Still I don't think that would be the 64bit problem.

Eric

FYI, I did try this out and it caused the bootloader to scramble the loaded data. The first corruption I found was the .x86cpuvendor.init section contained all zeroes.

Quoted 5 lines · Mike Travis

FYI, I did try this out and it caused the bootloader to scramble the loaded data. The first corruption I found was the .x86cpuvendor.init section contained all zeroes.

Explain what you mean with "the bootloader" in this context.

-hpa

Quoted 10 lines · H. Peter Anvin

Quoted 5 lines · Mike Travis

FYI, I did try this out and it caused the bootloader to scramble the loaded data. The first corruption I found was the .x86cpuvendor.init section contained all zeroes.

Explain what you mean with "the bootloader" in this context.

-hpa

After the code was loaded (the compressed code, it seems that my GRUB doesn't support uncompressed loading), the above section contained zeroes. I snapped it fairly early, around secondary_startup_64, and then printed it in x86_64_start_kernel.

The object file had the correct data (as displayed by objdump) so I'm assuming that the bootloading process didn't load the section correctly.

Below was the linker script I used:

include/asm-generic/vmlinux.lds.h +8 -4

@@ -373,9 +373,13 @@
 
 #ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU
 #define PERCPU(align)                                                  \
-       . = ALIGN(align);                                               \
+       .data.percpu.abs = .;                                           \
        percpu : { } :percpu                                            \
-       __per_cpu_load = .;                                             \
+       .data.percpu.rel : AT(.data.percpu.abs - LOAD_OFFSET) {         \
+               BYTE(0)                                                 \
+               . = ALIGN(align);                                       \
+               __per_cpu_load = .;                                     \
+       }                                                               \
        .data.percpu 0 : AT(__per_cpu_load - LOAD_OFFSET) {             \
                *(.data.percpu.first)                                   \
                *(.data.percpu.shared_aligned)                          \

@@ -383,8 +387,8 @@
                *(.data.percpu.page_aligned)                            \
                ____per_cpu_size = .;                                   \
        }                                                               \
-       . = __per_cpu_load + ____per_cpu_size;                          \
-       data : { } :data
+       . = __per_cpu_load + ____per_cpu_size;
+
 #else
 #define PERCPU(align)                                                  \
        . = ALIGN(align);                                               \

It showed all the correct address in the map and __per_cpu_load was a relative symbol (which was the objective.)

Btw, our simulator, which only loads uncompressed code, had the data correct, so it *may* only be a result of the code being compressed.

Quoted 53 lines · Mike Travis

Quoted 11 lines · H. Peter Anvin

Quoted 5 lines · Mike Travis

FYI, I did try this out and it caused the bootloader to scramble the loaded data. The first corruption I found was the .x86cpuvendor.init section contained all zeroes.

Explain what you mean with "the bootloader" in this context.

-hpa

After the code was loaded (the compressed code, it seems that my GRUB doesn't support uncompressed loading), the above section contained zeroes. I snapped it fairly early, around secondary_startup_64, and then printed it in x86_64_start_kernel.

The object file had the correct data (as displayed by objdump) so I'm assuming that the bootloading process didn't load the section correctly.

Below was the linker script I used:

include/asm-generic/vmlinux.lds.h +8 -4

@@ -373,9 +373,13 @@
 
 #ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU
 #define PERCPU(align)                                                  \
-       . = ALIGN(align);                                               \
+       .data.percpu.abs = .;                                           \
        percpu : { } :percpu                                            \
-       __per_cpu_load = .;                                             \
+       .data.percpu.rel : AT(.data.percpu.abs - LOAD_OFFSET) {         \
+               BYTE(0)                                                 \
+               . = ALIGN(align);                                       \
+               __per_cpu_load = .;                                     \
+       }                                                               \
        .data.percpu 0 : AT(__per_cpu_load - LOAD_OFFSET) {             \
                *(.data.percpu.first)                                   \
                *(.data.percpu.shared_aligned)                          \

@@ -383,8 +387,8 @@
                *(.data.percpu.page_aligned)                            \
                ____per_cpu_size = .;                                   \
        }                                                               \
-       . = __per_cpu_load + ____per_cpu_size;                          \
-       data : { } :data
+       . = __per_cpu_load + ____per_cpu_size;
+
 #else
 #define PERCPU(align)                                                  \
        . = ALIGN(align);                                               \

Quoted 6 lines

It showed all the correct address in the map and __per_cpu_load was a relative symbol (which was the objective.)

Btw, our simulator, which only loads uncompressed code, had the data correct, so it *may* only be a result of the code being compressed.

Weird. Grub doesn't get involved in the decompression the kernel does it all itself so we should be able to track where things go bad.

Last I looked the compressed code was formed by essentially. objcopy vmlinux -O binary vmlinux.bin
gzip vmlinux.bin
And then we take on a magic header to the gzip compressed file.

Are things only bad with the change above?

No, the original crash being discussed was a GP fault in head_64.S as it tries to initialize the kernel segments. The cause was that the prototype GDT is all zero, even though it's an initialized variable, and inspection of vmlinux shows that it has the right contents. But somehow it's either 1) getting zeroed on load, or 2) is loaded to the wrong place.

The zero-based PDA mechanism requires the introduction of a new ELF segment based at vaddr 0 which is sufficiently unusual that it wouldn't surprise me if its triggering some toolchain bug.

Mike: what would happen if the PDA were based at 4k rather than 0? The stack canary would still be at its small offset (0x20?), but it doesn't need to be initialized. I'm not sure if doing so would fix anything, however.

Quoted 10 lines · Jeremy Fitzhardinge

No, the original crash being discussed was a GP fault in head_64.S as it tries to initialize the kernel segments. The cause was that the prototype GDT is all zero, even though it's an initialized variable, and inspection of vmlinux shows that it has the right contents. But somehow it's either 1) getting zeroed on load, or 2) is loaded to the wrong place.

The zero-based PDA mechanism requires the introduction of a new ELF segment based at vaddr 0 which is sufficiently unusual that it wouldn't surprise me if its triggering some toolchain bug.

Agreed. Given the previous description my hunch is that the bug is occurring during objcopy. If vmlinux is good and the compressed kernel is bad.

It should be possible to look at vmlinux.bin and see if that was generated properly.

Quoted 4 lines

Mike: what would happen if the PDA were based at 4k rather than 0? The stack canary would still be at its small offset (0x20?), but it doesn't need to be initialized. I'm not sure if doing so would fix anything, however.

I'm dense today. Why are we doing a zero based pda? That seems the most likely culprit of linker trouble, and we should be able to put a smaller offset in the segment register to allow for everything to work as expected.

Quoted 36 lines · Eric W. Biederman

Quoted 10 lines · Jeremy Fitzhardinge

No, the original crash being discussed was a GP fault in head_64.S as it tries
to initialize the kernel segments. The cause was that the prototype GDT is all
zero, even though it's an initialized variable, and inspection of vmlinux shows
that it has the right contents. But somehow it's either 1) getting zeroed on
load, or 2) is loaded to the wrong place.

Quoted 6 lines

The zero-based PDA mechanism requires the introduction of a new ELF segment
based at vaddr 0 which is sufficiently unusual that it wouldn't surprise me if
its triggering some toolchain bug.

Agreed. Given the previous description my hunch is that the bug is occurring
during objcopy. If vmlinux is good and the compressed kernel is bad.

It should be possible to look at vmlinux.bin and see if that was generated
properly.

Quoted 6 lines

Mike: what would happen if the PDA were based at 4k rather than 0? The stack
canary would still be at its small offset (0x20?), but it doesn't need to be
initialized. I'm not sure if doing so would fix anything, however.

Quoted 5 lines

I'm dense today. Why are we doing a zero based pda? That seems the most likely culprit of linker trouble, and we should be able to put a smaller offset in the segment register to allow for everything to work as expected.

The only reason we need to do a zero-based PDA is because of the boneheaded gcc/x86_64 ABI decision to put the stack canary at a fixed offset from %gs (all they had to do was define it as a weak symbol we could override). If we want to support stack-protector and unify the handling of per-cpu variables, we need to rebase the per-cpu area at zero, starting with the PDA.

My own inclination would be to drop stack-protector support until gcc gets fixed, rather than letting it prevent us from unifying an area which is in need of unification...

J

Quoted 4 lines

The zero-based PDA mechanism requires the introduction of a new ELF segment based at vaddr 0 which is sufficiently unusual that it wouldn't surprise me if its triggering some toolchain bug.

Agreed. Given the previous description my hunch is that the bug is occurring during objcopy. If vmlinux is good and the compressed kernel is bad.

Quoted 8 lines · Eric W. Biederman

Quoted 4 lines

The zero-based PDA mechanism requires the introduction of a new ELF segment based at vaddr 0 which is sufficiently unusual that it wouldn't surprise me if its triggering some toolchain bug.

Agreed. Given the previous description my hunch is that the bug is occurring during objcopy. If vmlinux is good and the compressed kernel is bad.

Actually, it's not all that unusual... it's pretty common in various restricted environments. That being said, it's probably uncommon for *64-bit* code.

Quoted 11 lines · H. Peter Anvin

Quoted 9 lines · Eric W. Biederman

Quoted 5 lines

The zero-based PDA mechanism requires the introduction of a new ELF segment based at vaddr 0 which is sufficiently unusual that it wouldn't surprise me if
its triggering some toolchain bug.

Agreed. Given the previous description my hunch is that the bug is occurring during objcopy. If vmlinux is good and the compressed kernel is bad.

Quoted 3 lines

Actually, it's not all that unusual... it's pretty common in various restricted environments. That being said, it's probably uncommon for *64-bit* code.

Well, it's also unusual because 1) it's vaddr 0, but paddr <high>, and 2) the PHDRs are not sorted by vaddr order. 2) might actually be a bug.

Quoted 17 lines · Jeremy Fitzhardinge

Quoted 15 lines · H. Peter Anvin

Quoted 9 lines · Eric W. Biederman

Quoted 5 lines

The zero-based PDA mechanism requires the introduction of a new ELF segment based at vaddr 0 which is sufficiently unusual that it wouldn't surprise me if
its triggering some toolchain bug.

Agreed. Given the previous description my hunch is that the bug is occurring during objcopy. If vmlinux is good and the compressed kernel is bad.

Actually, it's not all that unusual... it's pretty common in various restricted environments. That being said, it's probably uncommon for *64-bit* code.

Quoted 3 lines

Well, it's also unusual because 1) it's vaddr 0, but paddr <high>, and 2) the PHDRs are not sorted by vaddr order. 2) might actually be a bug.

I just looked and gcc does not use this technique for thread local data.

My initial concern about all of this was not making symbols section relative is relieved as this all appears to be a 64bit arch thing where that doesn't matter.

Has anyone investigated using the technique gcc uses for thread local storage?
http://people.redhat.com/drepper/tls.pdf

In particular using the local exec model so we can say:
movq %fs:x@tpoff,%rax

To load the contents of a per cpu variable x into %rax ?

If we can use that model it should make it easier to interface with things like the stack protector code. Although we would still need to be very careful about thread switches.

Quoted 2 lines

I just looked and gcc does not use this technique for thread local data.

Which technique?

It does assume you put the thread-local data near %gs (%fs in userspace), and it uses a small offset (positive or negative) to reach it.

At present, the x86-64 only uses %gs-relative addressing to reach the pda, which are always small positive offsets. It always accesses per-cpu data in a two-step process of getting the base of per-cpu data, then offsetting to find the particular variable.

x86-32 has no pda, and arranges %fs so that %fs:variable gets the percpu variant of variable. The offsets are always quite large.

Quoted 4 lines

My initial concern about all of this was not making symbols section relative is relieved as this all appears to be a 64bit arch thing where that doesn't matter.

Why's that? I thought you cared particularly about making the x86-64 kernel relocatable for kdump, and that using non-absolute symbols was part of that?

Quoted 3 lines

Has anyone investigated using the technique gcc uses for thread local storage?
http://people.redhat.com/drepper/tls.pdf

The powerpc guys tried using gcc-level thread-local storage, but it doesn't work well. per-cpu data and per-thread data have different constraints, and its hard to tell gcc about them. For example, if you have a section of preemptable code in your function, it's hard to tell gcc not to cache a "thread-local" variable across it, even though we could have switched CPUs in the meantime.

Quoted 5 lines

In particular using the local exec model so we can say:
movq %fs:x@tpoff,%rax

To load the contents of a per cpu variable x into %rax ?

Quoted 1 line

If we can use that model it should make it easier to interface with things

like

Quoted 3 lines

the stack protector code. Although we would still need to be very careful about thread switches.

You mean cpu switches? We don't really have a notion of thread-local data in the kernel, other than things hanging off the kernel stack.

Nope. It achieves that affect with a magic set of relocations instead of linker magic.

Quoted 5 lines

At present, the x86-64 only uses %gs-relative addressing to reach the pda, which are always small positive offsets. It always accesses per-cpu data in a two-step process of getting the base of per-cpu data, then offsetting to find the particular variable.

Quoted 3 lines

x86-32 has no pda, and arranges %fs so that %fs:variable gets the percpu variant of variable. The offsets are always quite large.

As a practical matter I like that approach (except for extra code size of the offsets).

Quoted 6 lines

The powerpc guys tried using gcc-level thread-local storage, but it doesn't work well. per-cpu data and per-thread data have different constraints, and its hard to tell gcc about them. For example, if you have a section of preemptable code in your function, it's hard to tell gcc not to cache a "thread-local" variable across it, even though we could have switched CPUs in the meantime.

Yes, I completely agree with that. It doesn't mean however that we can't keep gcc ignorant and generate the same code manually.

Well I was thinking threads switching on a cpu having the kinds of problems you described when it was tried on ppc.

Yes, and there's no reason we couldn't do the same on 64-bit, aside from the stack-protector's use of %gs:40. There's no code-size cost in large offsets, since they're always 32-bits anyway (there's no short absolute addressing mode).

If we manually generate %gs-relative references to percpu data, then it's no different to what we do with 32-bit, whether it be a specific symbol address or using the TLS relocations.

Quoted 10 lines · Jeremy Fitzhardinge

Yes, and there's no reason we couldn't do the same on 64-bit, aside from the stack-protector's use of %gs:40. There's no code-size cost in large offsets, since they're always 32-bits anyway (there's no short absolute addressing mode).

If we manually generate %gs-relative references to percpu data, then it's no different to what we do with 32-bit, whether it be a specific symbol address or using the TLS relocations.

If we think the problem is the zero-basing triggering linker bugs, we should probably just use a small offset, like 64 (put a small dummy section before the .percpu.data section to occupy this section.)

I'm going to play with this a bit and see if I come up with something sanish.

-hpa

Quoted 12 lines · H. Peter Anvin

Quoted 10 lines · Jeremy Fitzhardinge

Yes, and there's no reason we couldn't do the same on 64-bit, aside from the stack-protector's use of %gs:40. There's no code-size cost in large offsets, since they're always 32-bits anyway (there's no short absolute addressing mode).

If we manually generate %gs-relative references to percpu data, then it's no different to what we do with 32-bit, whether it be a specific symbol address or using the TLS relocations.

Quoted 8 lines

If we think the problem is the zero-basing triggering linker bugs, we should probably just use a small offset, like 64 (put a small dummy section before the .percpu.data section to occupy this section.)

I'm going to play with this a bit and see if I come up with something sanish.

-hpa

One interesting thing I've discovered is the gcc --version may make a difference.

The kernel panic that occurred from Ingo's config, I was able to replicate with GCC 4.2.0 (which is on our devel server). But this one complained about not being able to handle the STACK-PROTECTOR option so I moved everything to another machine that has 4.2.4, and now it seems that it works fine. I'm still re-verifying that the source bits and config options are identical (it was a later git-remote update), and that in fact it is the gcc --version, but that may be the conclusion. (My code also has some patches submitted but not yet included in the tip/master tree. Curiously just enabling some debug options changed the footprint of the panic.)

Are we allowed to insist on a specific level of GCC for compiling the kernel?

Quoted 32 lines · Mike Travis

Quoted 10 lines · H. Peter Anvin

Quoted 9 lines · Jeremy Fitzhardinge

Yes, and there's no reason we couldn't do the same on 64-bit, aside from the stack-protector's use of %gs:40. There's no code-size cost in large offsets, since they're always 32-bits anyway (there's no short absolute addressing mode).

If we manually generate %gs-relative references to percpu data, then it's no different to what we do with 32-bit, whether it be a specific symbol address or using the TLS relocations.

Quoted 8 lines

If we think the problem is the zero-basing triggering linker bugs, we should probably just use a small offset, like 64 (put a small dummy section before the .percpu.data section to occupy this section.)

I'm going to play with this a bit and see if I come up with something sanish.

-hpa

One interesting thing I've discovered is the gcc --version may make a difference.

The kernel panic that occurred from Ingo's config, I was able to replicate with GCC 4.2.0 (which is on our devel server). But this one complained about not being able to handle the STACK-PROTECTOR option so I moved everything to another machine that has 4.2.4, and now it seems that it works fine. I'm still re-verifying that the source bits and config options are identical (it was a later git-remote update), and that in fact it is the gcc --version, but that may be the conclusion. (My code also has some patches submitted but not yet included in the tip/master tree. Curiously just enabling some debug options changed the footprint of the panic.)

Quoted 2 lines

Are we allowed to insist on a specific level of GCC for compiling the kernel?

Depends on the root cause. If it turns out to be something that is buggy in gcc and we can't work around. We might do something. I don't recall that kind of thing happening often. I think our minimum gcc is currently gcc-3.4.

Eric

Ouch. How far into it do we need to investigate? I can surely compare the vmlinux object files, but I'm not cognizant enough about the linker internals to examine much more than that.

One interesting thing I've discovered is the gcc --version may make a difference.

The kernel panic that occurred from Ingo's config, I was able to replicate
with GCC 4.2.0 (which is on our devel server). But this one complained
about not being able to handle the STACK-PROTECTOR option so I moved
everything to another machine that has 4.2.4, and now it seems that it
works fine. I'm still re-verifying that the source bits and config options are identical (it was a later git-remote update), and that in fact it is the gcc --version, but that may be the conclusion. (My code also has some
patches submitted but not yet included in the tip/master tree. Curiously
just enabling some debug options changed the footprint of the panic.)

Are we allowed to insist on a specific level of GCC for compiling the kernel?

are identical (it was a later git-remote update), and that in fact it is the gcc --version, but that may be the conclusion.

# Tue Jul 1 16:09:33 2008

CONFIG_LOCALVERSION="-ingo-test-0701"

Mike Travis wrote:
... I'm still re-verifying that the source bits and config options

Quoted 2 lines

are identical (it was a later git-remote update), and that in fact it is the gcc --version, but that may be the conclusion.

...

Yup, it's the gcc --version that makes the difference. GCC 4.2.0 couldn't boot past the grub screen, GCC 4.2.4 made it to the login prompt.

Quoted 10 lines · Mike Travis

Mike Travis wrote:
... I'm still re-verifying that the source bits and config options

Quoted 2 lines

are identical (it was a later git-remote update), and that in fact it is the gcc --version, but that may be the conclusion.

...

Yup, it's the gcc --version that makes the difference. GCC 4.2.0 couldn't
boot past the grub screen, GCC 4.2.4 made it to the login prompt.

IIRC, 4.2.0, 4.2.1 and 4.3.0 are known to miscompile the kernel in one way or another, however, that is from memory so don't quote me on it.

-hpa

Quoted 10 lines · Mike Travis

Mike Travis wrote:
... I'm still re-verifying that the source bits and config options

Quoted 2 lines

are identical (it was a later git-remote update), and that in fact it is the gcc --version, but that may be the conclusion.

...

Yup, it's the gcc --version that makes the difference. GCC 4.2.0 couldn't
boot past the grub screen, GCC 4.2.4 made it to the login prompt.

IIRC, 4.2.0, 4.2.1 and 4.3.0 are known to miscompile the kernel in one way or another, however, that is from memory so don't quote me on it.

-hpa

will only boot and run with gcc-4.2.0, and does the "failure at grub" screen with
gcc-4.2.4 ...!

Any other suggestions?

Quoted 3 lines · Mike Travis

will only boot and run with gcc-4.2.0, and does the "failure at grub" screen with
gcc-4.2.4 ...!

Do these tests have an early serial console enabled? I want to confirm that the failure is very early in boot before or just as we reach C code.

If you don't have an early console we could be failing late in the process just not have visibility.

Quoted 1 line

Any other suggestions?

If you have a serial console to add a little bit of instrumentation super duper early. Although looking at the generated objects might be just as interesting.

Eric

Hi,

Sorry for the delay, been off the past 4 days.

I'll see how closely I can narrow down where the fault is. I have been inserting
very early printk's to track how far into the startup it gets before bailing.

Quoted 4 lines · Mike Travis

Hi,

Sorry for the delay, been off the past 4 days.

Quoted 3 lines

I'll see how closely I can narrow down where the fault is. I have been inserting
very early printk's to track how far into the startup it gets before bailing.

Thanks. That should help narrow down what is going on.

Eric

Unfortunately it's back to the problem of faulting before x86_64_start_kernel() and grub just immediately reboots. So I'm back at analyzing assembler and config differences.

Quoted 4 lines · Mike Travis

Unfortunately it's back to the problem of faulting before x86_64_start_kernel() and grub just immediately reboots. So I'm back at analyzing assembler and config differences.

Ok. That is a narrow window of code. So it shouldn't be too bad, nasty though.

If you would like to trace through it I have attached my serial port debugging routines that I use in that part of the code.

Quoted 10 lines · Eric W. Biederman

Quoted 6 lines · Mike Travis

Unfortunately it's back to the problem of faulting before x86_64_start_kernel()
and grub just immediately reboots. So I'm back at analyzing assembler and
config differences.

Quoted 5 lines

Ok. That is a narrow window of code. So it shouldn't be too bad, nasty though.

If you would like to trace through it I have attached my serial port debugging routines that I use in that part of the code.

Last time it was doing this, it was a result of a triple-fault caused by loading %ds with an all-zero gdt. I modified Xen to dump the CPU state on triple-faults, so it was easy to pinpoint. I can do that again if it helps.

J

Last time it was doing this, it was a result of a triple-fault caused by loading %ds with an all-zero gdt. I modified Xen to dump the CPU state on triple-faults, so it was easy to pinpoint. I can do that again if it helps.

J

Jeremy Fitzhardinge wrote:
...

Quoted 7 lines

Last time it was doing this, it was a result of a triple-fault caused by loading %ds with an all-zero gdt. I modified Xen to dump the CPU state on triple-faults, so it was easy to pinpoint. I can do that again if it helps.

J

Hi Jeremy,

There are two question marks for my patchset. The first is in

arch/x86/xen/smp.c:xen_cpu_up()

287 #ifdef CONFIG_X86_64

288         /* Allocate node local memory for AP pdas */
289         WARN_ON(cpu == 0);
290         if (cpu > 0) {

291 rc = get_local_pda(cpu);
292 if (rc)
293 return rc;
294 }
295 #endif

and the second is at:

arch/x86/xen/enlighten.c:xen_start_kernel()

1748 #ifdef CONFIG_X86_64

1749         /* Disable until direct per-cpu data access. */
1750         have_vcpu_info_placement = 0;
1751         x86_64_init_pda();

1752 #endif

I believe with the pda folded into the percpu area, get_local_pda() and x86_64_init_pda() have been removed, so these are no longer required, yes?

Also, arch/x86/kernel/acpi/sleep.c:acpi_save_state_mem() sets up the startup code address with:

102 initial_code = (unsigned long)wakeup_long64;
103 saved_magic = 0x123456789abcdef0;

Should the pda and gdt_page address also be setup as is done in smpboot.c:do_boot_cpu():

(CONFIG_X86_64)
801 initial_pda = (unsigned long)get_cpu_pda(cpu);
802 #endif
803 early_gdt_descr.address = (unsigned long)get_cpu_gdt_table(cpu);
804 initial_code = (unsigned long)start_secondary;

Quoted 19 lines · Mike Travis

... The first is in

arch/x86/xen/smp.c:xen_cpu_up()

287 #ifdef CONFIG_X86_64

288         /* Allocate node local memory for AP pdas */
289         WARN_ON(cpu == 0);
290         if (cpu > 0) {

291 rc = get_local_pda(cpu);
292 if (rc)
293 return rc;
294 }
295 #endif

and the second is at:

arch/x86/xen/enlighten.c:xen_start_kernel()

1748 #ifdef CONFIG_X86_64

Quoted 9 lines

1749         /* Disable until direct per-cpu data access. */
1750         have_vcpu_info_placement = 0;
1751         x86_64_init_pda();

1752 #endif

I believe with the pda folded into the percpu area, get_local_pda() and x86_64_init_pda() have been removed, so these are no longer required, yes?

Well, presumably they need to be replaced with whatever setup you need to do now.

xen_start_kernel() is the first function called after a Xen kernel boot, and so it must make sure the early percpu setup is done before it can start using percpu variables.

xen_cpu_up() needs to do whatever initialization needed for a new cpu's percpu area (presumably whatever do_boot_cpu() does).

Is this for the boot cpu (0), or for all cpus? For the boot cpu, I have this now in arch/x86/kernel/setup_percpu.c:

(quoted diff) +8 -0

+#ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU
+
+/* Initialize percpu offset for boot cpu (0) */
+unsigned long __per_cpu_offset[NR_CPUS] __read_mostly = {
+       [0] = (unsigned long)__per_cpu_load
+};
+#else
 unsigned long __per_cpu_offset[NR_CPUS] __read_mostly;
+#endif

So this should apply as well to the xen startup?

Quoted 4 lines

xen_cpu_up() needs to do whatever initialization needed for a new cpu's percpu area (presumably whatever do_boot_cpu() does).

Does the startup include executing arch/x86/kernel/head_64.S:startup_64() ? I see arch/x86/xen/xen-head.S:startup_xen() so I'm guessing not?

For the real startup, I do the following two things. But I'm not comfortable enough with xen to think I'll get it right putting this in xen-head.S.

Quoted 13 lines · Mike Travis

Is this for the boot cpu (0), or for all cpus? For the boot cpu, I have this now in arch/x86/kernel/setup_percpu.c:

(quoted diff) +8 -0

+#ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU
+
+/* Initialize percpu offset for boot cpu (0) */
+unsigned long __per_cpu_offset[NR_CPUS] __read_mostly = {
+       [0] = (unsigned long)__per_cpu_load
+};
+#else
 unsigned long __per_cpu_offset[NR_CPUS] __read_mostly;
+#endif

Quoted 2 lines

So this should apply as well to the xen startup?

If it's just a static initialization, then it should be fine. But some equivalent of your head_64.S changes are needed to actually set things up?

Quoted 9 lines

Quoted 4 lines

xen_cpu_up() needs to do whatever initialization needed for a new cpu's percpu area (presumably whatever do_boot_cpu() does).

Does the startup include executing arch/x86/kernel/head_64.S:startup_64() ?
I see arch/x86/xen/xen-head.S:startup_xen() so I'm guessing not?

No, it doesn't. It bypasses all that startup code. Aside from the few instructions in xen-head.S, xen_start_kernel() is the first thing to get run.

But when bringing up a secondary cpu, where does the new percpu memory actually get allocated?

Quoted 4 lines

For the real startup, I do the following two things. But I'm not comfortable
enough with xen to think I'll get it right putting this in xen-head.S.

Yes, it needn't be in the asm code. I'll work out what to do. Looks like I just need to do an appropriate wrmsr(MSR_GS_BASE, ).

J

Quoted 3 lines · Mike Travis

Unfortunately it's back to the problem of faulting before x86_64_start_kernel() and grub just immediately reboots. So I'm back at analyzing assembler and config differences.

Ok. That is a narrow window of code. So it shouldn't be too bad, nasty though.

If you would like to trace through it I have attached my serial port debugging routines that I use in that part of the code.

Eric

Very cool, thanks!!! I will start using this. (I have been using the trick to replace printk with early_printk so messages come out immediately instead of from the log buf.)

I've been able to make some more progress. I've gotten to a point where it panics from stack overflow. I've verified this by bumping THREAD_ORDER and it boots fine. Now tracking down stack usages. (I have found a couple of new functions using set_cpus_allowed(..., CPU_MASK_ALL) instead of set_cpus_allowed_ptr(... , CPU_MASK_ALL_PTR). But these are not in the calling sequence so subsequently are not the cause.

One weird thing is early_idt_handler seems to have been called and that's one thing our simulator does not mimic for standard Intel FSB systems - early pending
interrupts. (It's designed after all to mimic our h/w, and of course it's been booting fine under that environment.)

Only a few of these though I would think might get called early in the boot, that might also be contributing to the stack overflow.

Oh yeah, I looked very closely at the differences in the assembler for vmlinux when compiled with 4.2.0 (fails) and 4.2.4 (which boots with the above mentioned THREAD_ORDER change) and except for some weirdness around ident_complete it seems to be the same code. But the per_cpu variables are in a completely different address order. I wouldn't think that the -j10 for make could cause this but I can verify that with -j1. But in any case, I'm sticking with 4.2.4 for now.

Mike Travis <[email protected]> writes:

Quoted 2 lines

to replace printk with early_printk so messages come out immediately instead of from the log buf.)

Just passing early_printk=xxx on the command line should have that effect. Although I do admit you have to be a little bit into the boot before early_printk is setup.

Quoted 7 lines

I've been able to make some more progress. I've gotten to a point where it panics from stack overflow. I've verified this by bumping THREAD_ORDER and it boots fine. Now tracking down stack usages. (I have found a couple of new functions using set_cpus_allowed(..., CPU_MASK_ALL) instead of set_cpus_allowed_ptr(... , CPU_MASK_ALL_PTR). But these are not in the calling sequence so subsequently are not the cause.

Is stack overflow the only problem you are seeing or are there still other mysteries?

Quoted 6 lines

One weird thing is early_idt_handler seems to have been called and that's one thing our simulator does not mimic for standard Intel FSB systems - early pending
interrupts. (It's designed after all to mimic our h/w, and of course it's been booting fine under that environment.)

That usually indicates you are taking an exception during boot not that you have received an external interrupt. Something like a page fault or a division by 0 error.

Quoted 3 lines

Only a few of these though I would think might get called early in the boot, that might also be contributing to the stack overflow.

Still the call chain depth shouldn't really be changing. So why should it matter? Ah. The high cpu count is growing cpumask_t so when you put it on the stack. That makes sense. So what stars out as a 4 byte variable on the stack in a normal setup winds up being a 1k variable with 4k cpus.

Reasonable. The practical problem is you are mixing a lot of changes simultaneously and it confuses things. Compiling with NR_CPUS=4096 and working out the bugs from a growing cpumask_t, putting the per cpu area in a zero based segment, and putting putting the pda into the per cpu area all at the same time.

Who knows maybe the only difference between 4.2.0 and 4.2.4 is that 4.2.4 optimizes it's stack usage a little better and you don't see a stack overflow.

It would be very very good if we could separate out these issues especially the segment for the per cpu variables. We need something like that.

What I meant was using early_printk in place of printk, which seems to stuff the messages into the log buf until the serial console is setup fairly late in start_kernel.
I did this by removing printk() and renaming early_printk() to be printk (and a couple
other things like #define early_printk printk ...

Quoted 1 line

Is stack overflow the only problem you are seeing or are there still other

mysteries?

I'm not entirely sure it's a stack overflow, the fault has a NULL dereference and
then the stack overflow message.

Quoted 8 lines

Quoted 2 lines

Only a few of these though I would think might get called early in the boot, that might also be contributing to the stack overflow.

Still the call chain depth shouldn't really be changing. So why should it matter? Ah. The high cpu count is growing cpumask_t so when you put it on the stack. That makes sense. So what stars out as a 4 byte variable on the stack in a normal setup winds up being a 1k variable with 4k cpus.

Yes, it's definitely the three related:

NR_CPUS Patch_Applied THREAD_ORDER Results

  256        NO           1        works (obviously ;-)
  256        YES          1        works
 4096        NO           1        works
 4096        YES          1        panics
 4096        YES          3        works (just happened to pick 3,

2 probably will work as well.)

I've been testing NR_CPUS=4096 for quite a while and it's been very reliable. It's just weird that this config fails with this new patch applied. (default configs and some fairly normal distro configs also work fine.) And with the zillion config straws we now have, spotting the arbitrary needle is proving difficult. ;-)

One reason I've been sticking with 4.2.4.

For i386 since virtual address space is precious and because there were concerns about putting code in __pa we actually relocate the kernel symbols during load right after decompression. When we do relocations absolute symbols are a killer.

Quoted 6 lines · Eric W. Biederman

For i386 since virtual address space is precious and because there were concerns about putting code in __pa we actually relocate the kernel symbols during load right after decompression. When we do relocations absolute symbols are a killer.

Well, it means making it clear to the relocator if it should relocate those symbols or not. Since IIRC we're doing "all or nothing" relocation, the relative offsets are always the same, even between sections.

Has anyone investigated using the technique gcc uses for thread local storage?

Quoted 2 lines · [email protected] (Eric W. Biederman) writes:

Has anyone investigated using the technique gcc uses for thread local storage?

I investigated a long time ago (given when the binutils/gcc support was much more primitive) and my conclusion back then was that doing the same for kernel module (negative addresses) would need new relocation types. And the pain of a binutils change didn't seem worth it.

Quoted 3 lines · Eric W. Biederman

Quoted 3 lines

The zero-based PDA mechanism requires the introduction of a new ELF segment based at vaddr 0 which is sufficiently unusual that it wouldn't surprise me

if

Quoted 5 lines

Quoted 1 line

its triggering some toolchain bug.

Agreed. Given the previous description my hunch is that the bug is occurring during objcopy. If vmlinux is good and the compressed kernel is bad.

Actually, it's not all that unusual... it's pretty common in various restricted environments. That being said, it's probably uncommon for *64-bit* code.

Quoted 86 lines · Eric W. Biederman

Quoted 70 lines · Mike Travis

Quoted 11 lines · H. Peter Anvin

Quoted 5 lines · Mike Travis

FYI, I did try this out and it caused the bootloader to scramble the loaded data. The first corruption I found was the .x86cpuvendor.init section contained all zeroes.

Explain what you mean with "the bootloader" in this context.

-hpa

After the code was loaded (the compressed code, it seems that my GRUB doesn't support uncompressed loading), the above section contained zeroes. I snapped it fairly early, around secondary_startup_64, and then printed it in x86_64_start_kernel.

The object file had the correct data (as displayed by objdump) so I'm assuming that the bootloading process didn't load the section correctly.

Below was the linker script I used:

include/asm-generic/vmlinux.lds.h +1 -2

@@ -373,9 +373,13 @@
 
 #ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU
 #define
 PERCPU(align)                                                  \
-       . =
 ALIGN(align);                                               \
+       .data.percpu.abs =
 .;                                           \
        percpu : { }
 :percpu                                            \
-       __per_cpu_load =

.; \
+ .data.percpu.rel : AT(.data.percpu.abs - LOAD_OFFSET)
{ \
+
BYTE(0) \
+ . =
ALIGN(align); \
+ __per_cpu_load =
.; \
+
} \
.data.percpu 0 : AT(__per_cpu_load - LOAD_OFFSET)
{ \

*(.data.percpu.first) \

*(.data.percpu.shared_aligned) \

include/asm-generic/vmlinux.lds.h +2 -2

@@ -383,8 +387,8 @@
 
 *(.data.percpu.page_aligned)                            \
                ____per_cpu_size =
 .;                                   \
 
 }                                                               \
-       . = __per_cpu_load +
 ____per_cpu_size;                          \
-       data : { } :data
+       . = __per_cpu_load + ____per_cpu_size;
+
 #else
 #define

PERCPU(align)                                                  \
        . =

Quoted 9 lines

ALIGN(align); \

It showed all the correct address in the map and __per_cpu_load was a relative symbol (which was the objective.)

Btw, our simulator, which only loads uncompressed code, had the data correct,
so it *may* only be a result of the code being compressed.

Weird. Grub doesn't get involved in the decompression the kernel does it all itself so we should be able to track where things go bad.

Quoted 6 lines

Last I looked the compressed code was formed by essentially. objcopy vmlinux -O binary vmlinux.bin
gzip vmlinux.bin
And then we take on a magic header to the gzip compressed file.

Are things only bad with the change above?

No, the original crash being discussed was a GP fault in head_64.S as it tries to initialize the kernel segments. The cause was that the prototype GDT is all zero, even though it's an initialized variable, and inspection of vmlinux shows that it has the right contents. But somehow it's either 1) getting zeroed on load, or 2) is loaded to the wrong place.

The zero-based PDA mechanism requires the introduction of a new ELF segment based at vaddr 0 which is sufficiently unusual that it wouldn't surprise me if its triggering some toolchain bug.

Mike: what would happen if the PDA were based at 4k rather than 0? The stack canary would still be at its small offset (0x20?), but it doesn't need to be initialized. I'm not sure if doing so would fix anything, however.

J

Quoted 50 lines · Mike Travis

Quoted 8 lines · H. Peter Anvin

Quoted 4 lines · Mike Travis

FYI, I did try this out and it caused the bootloader to scramble the loaded data. The first corruption I found was the .x86cpuvendor.init section contained all zeroes.

Explain what you mean with "the bootloader" in this context.

-hpa

After the code was loaded (the compressed code, it seems that my GRUB doesn't support uncompressed loading), the above section contained zeroes. I snapped it fairly early, around secondary_startup_64, and then printed it in x86_64_start_kernel.

The object file had the correct data (as displayed by objdump) so I'm assuming that the bootloading process didn't load the section correctly.

Below was the linker script I used:

include/asm-generic/vmlinux.lds.h +8 -4

@@ -373,9 +373,13 @@
 
 #ifdef CONFIG_HAVE_ZERO_BASED_PER_CPU
 #define PERCPU(align)                                                  \
-       . = ALIGN(align);                                               \
+       .data.percpu.abs = .;                                           \
        percpu : { } :percpu                                            \
-       __per_cpu_load = .;                                             \
+       .data.percpu.rel : AT(.data.percpu.abs - LOAD_OFFSET) {         \
+               BYTE(0)                                                 \
+               . = ALIGN(align);                                       \
+               __per_cpu_load = .;                                     \
+       }                                                               \
        .data.percpu 0 : AT(__per_cpu_load - LOAD_OFFSET) {             \
                *(.data.percpu.first)                                   \
                *(.data.percpu.shared_aligned)                          \

@@ -383,8 +387,8 @@
                *(.data.percpu.page_aligned)                            \
                ____per_cpu_size = .;                                   \
        }                                                               \
-       . = __per_cpu_load + ____per_cpu_size;                          \
-       data : { } :data
+       . = __per_cpu_load + ____per_cpu_size;
+
 #else
 #define PERCPU(align)                                                  \
        . = ALIGN(align);                                               \

Quoted 5 lines

It showed all the correct address in the map and __per_cpu_load was a relative symbol (which was the objective.)

Btw, our simulator, which only loads uncompressed code, had the data correct, so it *may* only be a result of the code being compressed.

Weird. Grub doesn't get involved in the decompression the kernel does it all itself so we should be able to track where things go bad.

Last I looked the compressed code was formed by essentially. objcopy vmlinux -O binary vmlinux.bin
gzip vmlinux.bin
And then we take on a magic header to the gzip compressed file.

Are things only bad with the change above?

Eric

Quoted 2 lines · Eric W. Biederman

Mike Travis <[email protected]> writes:

...

Quoted 20 lines

Quoted 2 lines

Can we generate a new symbol which would account for LOAD_OFFSET?

Ouch. Absolute symbols indeed. On the 32bit kernel that may play havoc with the relocatable kernel, although we have had similar absolute logic for the last year. With __per_cpu_start and __per_cpu_end so it may not be a problem.

To initialize the percpu data you do want to talk to the virtual address at __per_coup_load. But it is absolute Ugh.

It might be worth saying something like.

.data.percpu.start : AT(.data.percpu.dummy - LOAD_OFFSET) {
	DATA(0)                   
	. = ALIGN(align);
        __per_cpu_load = . ;                   
}

To make __per_cpu_load a relative symbol. ld has a bad habit of taking symbols out of empty sections and making them absolute. Which is why I added the DATA(0).

Quoted 4 lines

Still I don't think that would be the 64bit problem.

Eric

FYI, I did try this out and it caused the bootloader to scramble the loaded data. The first corruption I found was the .x86cpuvendor.init section contained all zeroes.

* It is now possible to use percpu operations for pda access since the pda is in the percpu area. Drop the pda operations.