diff mbox series

[0.5/2] x86/boot: Clean up early error asm

Message ID 20230616182303.3546262-1-andrew.cooper3@citrix.com (mailing list archive)
State Superseded
Headers show
Series Introduce a REQUIRE_NX Kconfig option | expand

Commit Message

Andrew Cooper June 16, 2023, 6:23 p.m. UTC
The asm forming early error handling is a mix of local and non-local symbols,
and has some pointless comments.  Drop the "# Error message" comments,
tweaking the style on modified lines, and make the symbols local.

However, leave behind one real symbol so this logic disassembles nicely
without merging in to acpi_boot_init(), which is the thing that happens to be
immediately prior in my build.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Done in order to simplfy Alejandro's NX series a little.
---
 xen/arch/x86/boot/head.S | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

Comments

Jan Beulich June 19, 2023, 8:10 a.m. UTC | #1
On 16.06.2023 20:23, Andrew Cooper wrote:
> The asm forming early error handling is a mix of local and non-local symbols,
> and has some pointless comments.  Drop the "# Error message" comments,
> tweaking the style on modified lines, and make the symbols local.
> 
> However, leave behind one real symbol so this logic disassembles nicely
> without merging in to acpi_boot_init(), which is the thing that happens to be
> immediately prior in my build.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one request:

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -142,25 +142,27 @@ efi_platform:
>  
>          .section .init.text, "ax", @progbits
>  
> -bad_cpu:
> -        add     $sym_offs(.Lbad_cpu_msg),%esi   # Error message
> +early_error:

This symbol, which isn't really used except by .size/.type below,
could do with a comment saying that it's intentionally here despite
having no real reference.

> @@ -202,6 +204,9 @@ not_multiboot:
>  .Lhalt: hlt
>          jmp     .Lhalt
>  
> +        .size early_error, . - early_error
> +        .type early_error, @function

At the 1st and 2nd glance this looks unrelated to this patch. Which
isn't to say I'm opposed, but of course once we have settled on an
annotation model, it'll need touching again anyway.

Jan
Andrew Cooper June 19, 2023, 11:20 a.m. UTC | #2
On 19/06/2023 9:10 am, Jan Beulich wrote:
> On 16.06.2023 20:23, Andrew Cooper wrote:
>> The asm forming early error handling is a mix of local and non-local symbols,
>> and has some pointless comments.  Drop the "# Error message" comments,
>> tweaking the style on modified lines, and make the symbols local.
>>
>> However, leave behind one real symbol so this logic disassembles nicely
>> without merging in to acpi_boot_init(), which is the thing that happens to be
>> immediately prior in my build.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> with one request:
>
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -142,25 +142,27 @@ efi_platform:
>>  
>>          .section .init.text, "ax", @progbits
>>  
>> -bad_cpu:
>> -        add     $sym_offs(.Lbad_cpu_msg),%esi   # Error message
>> +early_error:
> This symbol, which isn't really used except by .size/.type below,
> could do with a comment saying that it's intentionally here despite
> having no real reference.

Ok.  /* Here to make the disassembly better. */ ?

The .Lbad_cpu jmp will end up disassembling to early_error+0, so it is
technically referenced, but I see your point.

>> @@ -202,6 +204,9 @@ not_multiboot:
>>  .Lhalt: hlt
>>          jmp     .Lhalt
>>  
>> +        .size early_error, . - early_error
>> +        .type early_error, @function
> At the 1st and 2nd glance this looks unrelated to this patch. Which
> isn't to say I'm opposed, but of course once we have settled on an
> annotation model, it'll need touching again anyway.

I know it's going to need changing, but in the short term I'm at least
making sure the metadata is correct on new additions to asm code.

~Andrew
Jan Beulich June 19, 2023, 11:43 a.m. UTC | #3
On 19.06.2023 13:20, Andrew Cooper wrote:
> On 19/06/2023 9:10 am, Jan Beulich wrote:
>> On 16.06.2023 20:23, Andrew Cooper wrote:
>>> The asm forming early error handling is a mix of local and non-local symbols,
>>> and has some pointless comments.  Drop the "# Error message" comments,
>>> tweaking the style on modified lines, and make the symbols local.
>>>
>>> However, leave behind one real symbol so this logic disassembles nicely
>>> without merging in to acpi_boot_init(), which is the thing that happens to be
>>> immediately prior in my build.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks.
> 
>> with one request:
>>
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -142,25 +142,27 @@ efi_platform:
>>>  
>>>          .section .init.text, "ax", @progbits
>>>  
>>> -bad_cpu:
>>> -        add     $sym_offs(.Lbad_cpu_msg),%esi   # Error message
>>> +early_error:
>> This symbol, which isn't really used except by .size/.type below,
>> could do with a comment saying that it's intentionally here despite
>> having no real reference.
> 
> Ok.  /* Here to make the disassembly better. */ ?

Yes please.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 09bebf8635d0..d52dbc752e29 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -142,25 +142,27 @@  efi_platform:
 
         .section .init.text, "ax", @progbits
 
-bad_cpu:
-        add     $sym_offs(.Lbad_cpu_msg),%esi   # Error message
+early_error:
+
+.Lbad_cpu:
+        add     $sym_offs(.Lbad_cpu_msg), %esi
         jmp     .Lget_vtb
-not_multiboot:
-        add     $sym_offs(.Lbad_ldr_msg),%esi   # Error message
+.Lnot_multiboot:
+        add     $sym_offs(.Lbad_ldr_msg), %esi
         jmp     .Lget_vtb
 .Lnot_aligned:
-        add     $sym_offs(.Lbag_alg_msg),%esi   # Error message
+        add     $sym_offs(.Lbag_alg_msg), %esi
         jmp     .Lget_vtb
 .Lmb2_no_st:
         /*
          * Here we are on EFI platform. vga_text_buffer was zapped earlier
          * because there is pretty good chance that VGA is unavailable.
          */
-        add     $sym_offs(.Lbad_ldr_nst),%esi   # Error message
+        add     $sym_offs(.Lbad_ldr_nst), %esi
         jmp     .Lget_vtb
 .Lmb2_no_ih:
         /* Ditto. */
-        add     $sym_offs(.Lbad_ldr_nih),%esi   # Error message
+        add     $sym_offs(.Lbad_ldr_nih), %esi
         jmp     .Lget_vtb
 .Lmb2_no_bs:
         /*
@@ -168,7 +170,7 @@  not_multiboot:
          * via start label. Then reliable vga_text_buffer zap is impossible
          * in Multiboot2 scanning loop and we have to zero %edi below.
          */
-        add     $sym_offs(.Lbad_ldr_nbs),%esi   # Error message
+        add     $sym_offs(.Lbad_ldr_nbs), %esi
         xor     %edi,%edi                       # No VGA text buffer
         jmp     .Lprint_err
 .Lmb2_efi_ia_32:
@@ -176,11 +178,11 @@  not_multiboot:
          * Here we are on EFI IA-32 platform. Then reliable vga_text_buffer zap is
          * impossible in Multiboot2 scanning loop and we have to zero %edi below.
          */
-        add     $sym_offs(.Lbad_efi_msg),%esi   # Error message
+        add     $sym_offs(.Lbad_efi_msg), %esi
         xor     %edi,%edi                       # No VGA text buffer
         jmp     .Lprint_err
 .Lget_vtb:
-        mov     sym_esi(vga_text_buffer),%edi
+        mov     sym_esi(vga_text_buffer), %edi
 .Lprint_err:
         lodsb
         test    %al,%al        # Terminate on '\0' sentinel
@@ -202,6 +204,9 @@  not_multiboot:
 .Lhalt: hlt
         jmp     .Lhalt
 
+        .size early_error, . - early_error
+        .type early_error, @function
+
         .code64
 
 __efi64_mb2_start:
@@ -221,8 +226,8 @@  __efi64_mb2_start:
         cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
         je      .Lefi_multiboot2_proto
 
-        /* Jump to not_multiboot after switching CPU to x86_32 mode. */
-        lea     not_multiboot(%rip),%r15
+        /* Jump to .Lnot_multiboot after switching CPU to x86_32 mode. */
+        lea     .Lnot_multiboot(%rip), %r15
         jmp     x86_32_switch
 
 .Lefi_multiboot2_proto:
@@ -464,7 +469,7 @@  __start:
 
         /* Check for Multiboot bootloader. */
         cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
-        jne     not_multiboot
+        jne     .Lnot_multiboot
 
         /* Get mem_lower from Multiboot information. */
         testb   $MBI_MEMLIMITS,MB_flags(%ebx)
@@ -655,7 +660,7 @@  trampoline_setup:
 
         /* Check for availability of long mode. */
         bt      $cpufeat_bit(X86_FEATURE_LM),%edx
-        jnc     bad_cpu
+        jnc     .Lbad_cpu
 
         /* Stash TSC to calculate a good approximation of time-since-boot */
         rdtsc