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