[PATCH 0/4] Improvements to x86 boot code

[PATCH 0/4] 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 completely removes pointless data which was in .init.text
Patch 2 moves some data from .init.text to .init.data
Patch 3 moves some code and data from .text to .init.text and .init.data
Patch 4 adds hlt instructions to terminal loops

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

[PATCH 1/4] x86/boot: Remove high_start and ret_point

[Andrew Cooper]

They are not needed.  This form is a few bytes leaner overall, but
usefully removes pieces of data from the middle of the code section.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/boot/trampoline.S |    5 +----
 xen/arch/x86/boot/wakeup.S     |    5 +----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 4f5f8d7..ccb40fb 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -109,12 +109,9 @@ trampoline_protmode_entry:
         .code64
 start64:
         /* Jump to high mappings. */
-        mov     high_start(%rip),%rax
+        movabs  $__high_start,%rax
         jmpq    *%rax
 
-high_start:
-        .quad   __high_start
-
         .code32
 trampoline_boot_cpu_entry:
         cmpb    $0,bootsym_rel(skip_realmode,5)
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index a3883c1..08ea9b2 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -166,12 +166,9 @@ wakeup_32:
         .code64
 wakeup_64:
         /* Jump to high mappings and the higher-level wakeup code. */
-        movq    ret_point(%rip), %rbx
+        movabs  $__ret_point, %rbx
         jmp     *%rbx
 
-ret_point:
-        .quad   __ret_point
-
 bogus_saved_magic:
         movw    $0x0e00 + 'S', 0xb8014
         jmp     bogus_saved_magic
-- 
1.7.10.4

[PATCH 2/4] x86/boot: Early data should live in init.data

[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>
---
 xen/arch/x86/boot/cmdline.S |    4 ++++
 xen/arch/x86/boot/head.S    |   12 +++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
index e1f4595..380b358 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.data, "aw", @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..d706c44 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -32,11 +32,17 @@ 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.data, "aw", @progbits
+
+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"
+
 bad_cpu:
         mov     $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
         jmp     print_err
@@ -59,10 +65,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

Re: [PATCH 2/4] x86/boot: Early data should live in init.data

[Jan Beulich]

>>> On 25.04.14 at 21:50, <andrew.cooper3@citrix.com> wrote:
> --- 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.data, "aw", @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

Did you not mean .init.rodata, "a", @progbits here?

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -32,11 +32,17 @@ 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.data, "aw", @progbits
> +
> +gdt_boot_descr:
> +        .word   6*8-1
> +        .long   sym_phys(trampoline_gdt)

While at it, how about putting this on a 2 mod 4 boundary?

>  .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
>  .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
>  
> +        .section .init.text, "ax"
> +

@progbits above calls for @progbits here too.

Jan

Re: [PATCH 2/4] x86/boot: Early data should live in init.data

[Andrew Cooper]

On 28/04/14 11:08, Jan Beulich wrote:
>>>> On 25.04.14 at 21:50, <andrew.cooper3@citrix.com> wrote:
>> --- 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.data, "aw", @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
> Did you not mean .init.rodata, "a", @progbits here?

Ok.

>
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -32,11 +32,17 @@ 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.data, "aw", @progbits
>> +
>> +gdt_boot_descr:
>> +        .word   6*8-1
>> +        .long   sym_phys(trampoline_gdt)
> While at it, how about putting this on a 2 mod 4 boundary?

Yes, and the strings below into .rodata

>
>>  .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
>>  .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
>>  
>> +        .section .init.text, "ax"
>> +
> @progbits above calls for @progbits here too.
>
> Jan
>

Oops - yes.

~Andrew

Re: [PATCH 2/4] x86/boot: Early data should live in init.data

[Jan Beulich]

>>> On 28.04.14 at 12:16, <andrew.cooper3@citrix.com> wrote:
> On 28/04/14 11:08, Jan Beulich wrote:
>>>>> On 25.04.14 at 21:50, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -32,11 +32,17 @@ 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.data, "aw", @progbits
>>> +
>>> +gdt_boot_descr:
>>> +        .word   6*8-1
>>> +        .long   sym_phys(trampoline_gdt)
>> While at it, how about putting this on a 2 mod 4 boundary?
> 
> Yes, and the strings below into .rodata

Actually not just them - the construct above is read-only too afaict.

Jan

>>>  .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
>>>  .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"

Re: [PATCH 2/4] x86/boot: Early data should live in init.data

[Andrew Cooper]

On 28/04/14 11:37, Jan Beulich wrote:
>>>> On 28.04.14 at 12:16, <andrew.cooper3@citrix.com> wrote:
>> On 28/04/14 11:08, Jan Beulich wrote:
>>>>>> On 25.04.14 at 21:50, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/boot/head.S
>>>> +++ b/xen/arch/x86/boot/head.S
>>>> @@ -32,11 +32,17 @@ 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.data, "aw", @progbits
>>>> +
>>>> +gdt_boot_descr:
>>>> +        .word   6*8-1
>>>> +        .long   sym_phys(trampoline_gdt)
>>> While at it, how about putting this on a 2 mod 4 boundary?
>> Yes, and the strings below into .rodata
> Actually not just them - the construct above is read-only too afaict.
>
> Jan

Yes - I noticed as much when starting to implement the changed.

~Andrew

[PATCH 3/4] 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.code 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.code.  Furthermore, its
strings can move to init.data.

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

---

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..0c086b2 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"
+
+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.data, "aw", @progbits
+
+int_msg:
+        .asciz "Unknown interrupt (cr2=%016lx)\n"
+hex_msg:
+        .asciz "    %016lx"
 
 /*** DESCRIPTOR TABLES ***/
 
-- 
1.7.10.4

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

[Jan Beulich]

>>> On 25.04.14 at 21:50, <andrew.cooper3@citrix.com> wrote:
> 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.code 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.code.  Furthermore, its

s/init[.]code/.init.text/ twice.

> strings can move to init.data.

.init.rodata again as well as consistent use of @progbits.

Jan

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

[Andrew Cooper]

On 28/04/14 11:12, Jan Beulich wrote:
>>>> On 25.04.14 at 21:50, <andrew.cooper3@citrix.com> wrote:
>> 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.code 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.code.  Furthermore, its
> s/init[.]code/.init.text/ twice.

I could have sworn I corrected that.  I will make certain for v2.

~Andrew

>
>> strings can move to init.data.
> .init.rodata again as well as consistent use of @progbits.
>
> Jan
>

[PATCH 4/4] 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>
---
 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 d706c44..06014ec 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -52,11 +52,11 @@ print_err:
         mov     $0xB8000,%edi  # VGA framebuffer
 1:      mov     (%esi),%bl
         test    %bl,%bl        # Terminate on '\0' sentinel
-2:      je      2b
+        je      3f
         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
@@ -64,6 +64,8 @@ print_err:
         mov     $7,%al
         stosb                  # Write an attribute to the VGA framebuffer
         jmp     1b
+3:      hlt
+        jmp     3b
 
 __start:
         cld
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 0c086b2..fd3d93a 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.data, "aw", @progbits
 
-- 
1.7.10.4

Re: [PATCH 4/4] x86/boot: Use 'hlt' inside terminal loops

[Jan Beulich]

>>> On 25.04.14 at 21:50, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -52,11 +52,11 @@ print_err:
>          mov     $0xB8000,%edi  # VGA framebuffer
>  1:      mov     (%esi),%bl
>          test    %bl,%bl        # Terminate on '\0' sentinel
> -2:      je      2b
> +        je      3f
>          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
> @@ -64,6 +64,8 @@ print_err:
>          mov     $7,%al
>          stosb                  # Write an attribute to the VGA framebuffer
>          jmp     1b
> +3:      hlt
> +        jmp     3b

The longer the code sequences between branch and label, the less
desirable is the use of such numeric ones. Please use .Lhalt or some
such instead here.

Jan