[PATCH v2 0/3] Improvements to x86 boot code

[PATCH v2 0/3] Improvements to x86 boot code

[Andrew Cooper]

These patches are ones I have had since working on c/s 1ed76797439e "x86/boot:
fix BIOS memory corruption on certain IBM systems" in the 4.4 freeze window,
which required frequent reference to the disassembly.

The early code uses mixed processor modes and objdump doesn't know when to
change between them.  It can be overridden to an appropriate mode for the boot
code using -M options, but symbol tracking becomes broken.  I ended up
referencing the 64, 32 and 16 bit disassemblies side by side; one with correct
symbols and two with correct instructions.  This series was started to remove
extraneous clutter from the text disassembly.

Patch 1 moves some data from .init.text to .init.data
Patch 2 moves some code and data from .text to .init.text and .init.data
Patch 3 adds hlt instructions to terminal loops

Updates in v2:
 * Drop original patch 1 as it is now committed
 * init.data -> init.rodata
 * Fix missing @progbits
 * .Lhalt label for long jump
 * Align gdt_boot_descr

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

[PATCH v2 1/3] x86/boot: Early data should live in init.rodata

[Andrew Cooper]

No real change as these were already in the init section, but does move it out
of a text section.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

---
v2: All data can live in init.rodata
    Fix up some missing @progbits
    Pad before gdt_boot_descr to align its long.
---
 xen/arch/x86/boot/cmdline.S |    4 ++++
 xen/arch/x86/boot/head.S    |   14 +++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
index e1f4595..00687eb 100644
--- a/xen/arch/x86/boot/cmdline.S
+++ b/xen/arch/x86/boot/cmdline.S
@@ -329,6 +329,8 @@ cmdline_parse_early:
         popa
         ret
 
+        .pushsection .init.rodata, "a", @progbits
+
 .Lvga_text_modes: /* rows, mode_number */
         .word   25,VIDEO_80x25
         .word   50,VIDEO_80x50
@@ -361,3 +363,5 @@ cmdline_parse_early:
         .asciz  "no"
 .Ledd_opt:
         .asciz  "edd"
+
+        .popsection
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 1777c17..0d87b27 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -32,11 +32,19 @@ ENTRY(start)
         /* Checksum: must be the negated sum of the first two fields. */
         .long   -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
 
-        .section .init.text, "ax"
+        .section .init.rodata, "a", @progbits
+        .align 4
+
+        .word   0
+gdt_boot_descr:
+        .word   6*8-1
+        .long   sym_phys(trampoline_gdt)
 
 .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
 .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
 
+        .section .init.text, "ax", @progbits
+
 bad_cpu:
         mov     $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
         jmp     print_err
@@ -59,10 +67,6 @@ print_err:
         stosb                  # Write an attribute to the VGA framebuffer
         jmp     1b
 
-gdt_boot_descr:
-        .word   6*8-1
-        .long   sym_phys(trampoline_gdt)
-
 __start:
         cld
         cli
-- 
1.7.10.4

[PATCH v2 2/3] x86/boot: Move some __high_start code and data into init sections

[Andrew Cooper]

Half of __high_start is strictly for the BSP and will only be run once on
boot.  To complement 'start_secondary', create 'start_bsp' and move it into
the init.text section.

The interrupt handler 'ingore_int' is patched into the BSPs IDT, but fully
replaced with real handlers early during boot.  The BSPs IDT is used by APs
until midway through start_secondary, but after the real handlers have been
installed.  Therefore, 'ignore_int' can move to init.text.  Furthermore, its
strings can move to init.rodata.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

---
v2: Move data to .init.rodata.  Add missing @progbits.

I know start_bsp doesn't need to be a GLOBAL(), but it prevents objdump
looking like:

  ffff82d08010005b:  85 db              test   %ebx,%ebx
  ffff82d08010005d:  0f 85 ae 46 08 00  jne    ffff82d080184711 <start_secondary>
  ffff82d080100063:  e9 08 0f 19 00     jmpq   ffff82d080290f70 <trampoline_end>

which is confusing to say the least.  An alternative would be to insert some
explicit alignment such that trampoline_end and start_bsp were no longer at
the same virtual address, but this feels worse than using GLOBAL().
---
 xen/arch/x86/boot/x86_64.S |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 8f92402..7ecfad3 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -29,7 +29,11 @@
 
         test    %ebx,%ebx
         jnz     start_secondary
+        jmp     start_bsp
 
+        .section .init.text, "ax", @progbits
+
+GLOBAL(start_bsp)
         /* Initialise IDT with simple error defaults. */
         leaq    ignore_int(%rip),%rcx
         movl    %ecx,%eax
@@ -55,10 +59,6 @@
         ud2     /* Force a panic (invalid opcode). */
 
 /* This is the default interrupt handler. */
-int_msg:
-        .asciz "Unknown interrupt (cr2=%016lx)\n"
-hex_msg:
-        .asciz "    %016lx"
 ignore_int:
         SAVE_ALL
         movq    %cr2,%rsi
@@ -75,6 +75,12 @@ ignore_int:
         jnz     0b
 1:      jmp     1b
 
+        .section .init.rodata, "a", @progbits
+
+int_msg:
+        .asciz "Unknown interrupt (cr2=%016lx)\n"
+hex_msg:
+        .asciz "    %016lx"
 
 /*** DESCRIPTOR TABLES ***/
 
-- 
1.7.10.4

Re: [PATCH v2 2/3] x86/boot: Move some __high_start code and data into init sections

[Jan Beulich]

>>> On 28.04.14 at 15:06, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -29,7 +29,11 @@
>  
>          test    %ebx,%ebx
>          jnz     start_secondary
> +        jmp     start_bsp
>  
> +        .section .init.text, "ax", @progbits
> +
> +GLOBAL(start_bsp)

I'm sorry, I'm afraid I overlooked this in v1 - why GLOBAL()? (The other
two patches look fine to me now, so no need to re-send those in case
this one needs changing.)

Jan

Re: [PATCH v2 2/3] x86/boot: Move some __high_start code and data into init sections

[Andrew Cooper]

On 28/04/14 15:04, Jan Beulich wrote:
>>>> On 28.04.14 at 15:06, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/boot/x86_64.S
>> +++ b/xen/arch/x86/boot/x86_64.S
>> @@ -29,7 +29,11 @@
>>  
>>          test    %ebx,%ebx
>>          jnz     start_secondary
>> +        jmp     start_bsp
>>  
>> +        .section .init.text, "ax", @progbits
>> +
>> +GLOBAL(start_bsp)
> I'm sorry, I'm afraid I overlooked this in v1 - why GLOBAL()? (The other
> two patches look fine to me now, so no need to re-send those in case
> this one needs changing.)
>
> Jan

There is an explanation below the --- in the patch.

The issues is that start_bsp and trampoline_end have the same address,
leading to a confusing disassembly.

~Andrew

Re: [PATCH v2 2/3] x86/boot: Move some __high_start code and data into init sections

[Jan Beulich]

>>> On 28.04.14 at 16:09, <andrew.cooper3@citrix.com> wrote:
> On 28/04/14 15:04, Jan Beulich wrote:
>>>>> On 28.04.14 at 15:06, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/boot/x86_64.S
>>> +++ b/xen/arch/x86/boot/x86_64.S
>>> @@ -29,7 +29,11 @@
>>>  
>>>          test    %ebx,%ebx
>>>          jnz     start_secondary
>>> +        jmp     start_bsp
>>>  
>>> +        .section .init.text, "ax", @progbits
>>> +
>>> +GLOBAL(start_bsp)
>> I'm sorry, I'm afraid I overlooked this in v1 - why GLOBAL()? (The other
>> two patches look fine to me now, so no need to re-send those in case
>> this one needs changing.)
> 
> There is an explanation below the --- in the patch.

Oops, sorry, stopped reading that part after the v2 sentence.

> The issues is that start_bsp and trampoline_end have the same address,
> leading to a confusing disassembly.

Is that really defined behavior - trampoline_end is a global too? Which
one would get used if their addresses are the same may depend on
both linker and disassembler. I therefore don't agree to this needless
global.

Jan

Re: [PATCH v2 2/3] x86/boot: Move some __high_start code and data into init sections

[Andrew Cooper]

On 28/04/14 15:45, Jan Beulich wrote:
>>>> On 28.04.14 at 16:09, <andrew.cooper3@citrix.com> wrote:
>> On 28/04/14 15:04, Jan Beulich wrote:
>>>>>> On 28.04.14 at 15:06, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/boot/x86_64.S
>>>> +++ b/xen/arch/x86/boot/x86_64.S
>>>> @@ -29,7 +29,11 @@
>>>>  
>>>>          test    %ebx,%ebx
>>>>          jnz     start_secondary
>>>> +        jmp     start_bsp
>>>>  
>>>> +        .section .init.text, "ax", @progbits
>>>> +
>>>> +GLOBAL(start_bsp)
>>> I'm sorry, I'm afraid I overlooked this in v1 - why GLOBAL()? (The other
>>> two patches look fine to me now, so no need to re-send those in case
>>> this one needs changing.)
>> There is an explanation below the --- in the patch.
> Oops, sorry, stopped reading that part after the v2 sentence.
>
>> The issues is that start_bsp and trampoline_end have the same address,
>> leading to a confusing disassembly.
> Is that really defined behavior - trampoline_end is a global too? Which
> one would get used if their addresses are the same may depend on
> both linker and disassembler. I therefore don't agree to this needless
> global.
>
> Jan
>

trampoline_end is a valid global, as it is referenced in the C code.

I can drop the global.  This is not the only instance of overlapping
symbols, and anyone looking at the disassembly will have reference to
the code as well.

v3 on its way.

~Andrew

[PATCH v3 2/3] x86/boot: Move some __high_start code and data into init sections

[Andrew Cooper]

Half of __high_start is strictly for the BSP and will only be run once on
boot.  To complement 'start_secondary', create 'start_bsp' and move it into
the init.text section.

The interrupt handler 'ingore_int' is patched into the BSPs IDT, but fully
replaced with real handlers early during boot.  The BSPs IDT is used by APs
until midway through start_secondary, but after the real handlers have been
installed.  Therefore, 'ignore_int' can move to init.text.  Furthermore, its
strings can move to init.rodata.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

---
v3: Drop GLOBAL() for start_bsp.
v2: Move data to .init.rodata.  Add missing @progbits.
---
 xen/arch/x86/boot/x86_64.S |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 8f92402..9652edb 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -29,7 +29,11 @@
 
         test    %ebx,%ebx
         jnz     start_secondary
+        jmp     start_bsp
 
+        .section .init.text, "ax", @progbits
+
+start_bsp:
         /* Initialise IDT with simple error defaults. */
         leaq    ignore_int(%rip),%rcx
         movl    %ecx,%eax
@@ -55,10 +59,6 @@
         ud2     /* Force a panic (invalid opcode). */
 
 /* This is the default interrupt handler. */
-int_msg:
-        .asciz "Unknown interrupt (cr2=%016lx)\n"
-hex_msg:
-        .asciz "    %016lx"
 ignore_int:
         SAVE_ALL
         movq    %cr2,%rsi
@@ -75,6 +75,12 @@ ignore_int:
         jnz     0b
 1:      jmp     1b
 
+        .section .init.rodata, "a", @progbits
+
+int_msg:
+        .asciz "Unknown interrupt (cr2=%016lx)\n"
+hex_msg:
+        .asciz "    %016lx"
 
 /*** DESCRIPTOR TABLES ***/
 
-- 
1.7.10.4

[PATCH v2 3/3] x86/boot: Use 'hlt' inside terminal loops

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

---
v2: Use '.Lhalt' in preference to '3f'
---
 xen/arch/x86/boot/head.S   |    8 +++++---
 xen/arch/x86/boot/x86_64.S |    3 ++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 0d87b27..ca0e97d 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -54,11 +54,11 @@ print_err:
         mov     $0xB8000,%edi  # VGA framebuffer
 1:      mov     (%esi),%bl
         test    %bl,%bl        # Terminate on '\0' sentinel
-2:      je      2b
+        je      .Lhalt
         mov     $0x3f8+5,%dx   # UART Line Status Register
-3:      in      %dx,%al
+2:      in      %dx,%al
         test    $0x20,%al      # Test THR Empty flag
-        je      3b
+        je      2b
         mov     $0x3f8+0,%dx   # UART Transmit Holding Register
         mov     %bl,%al
         out     %al,%dx        # Send a character over the serial line
@@ -66,6 +66,8 @@ print_err:
         mov     $7,%al
         stosb                  # Write an attribute to the VGA framebuffer
         jmp     1b
+.Lhalt: hlt
+        jmp     .Lhalt
 
 __start:
         cld
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 7ecfad3..fe5469a 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -73,7 +73,8 @@ ignore_int:
         call    printk
         testq   $0xff8,%rbp
         jnz     0b
-1:      jmp     1b
+1:      hlt
+        jmp     1b
 
         .section .init.rodata, "a", @progbits
 
-- 
1.7.10.4