Re: [PATCH v2.1 6.5/70] x86/kexec: Annotate embedded data with ELF metadata

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2022 11:01, Andrew Cooper wrote:
> Scanning for embedded endbranch instructions involves parsing the .text
> disassembly.  Data in the kexec trampoline has no ELF metadata, so objdump
> treats it as instructions and tries to disassemble.  Convert:
> 
>   ffff82d040396108 <compatibility_mode_far>:

What about the (possible) padding ahead of this? Should the .align
there perhaps specify a filler character?

>   ffff82d040396108:       00 00                   add    %al,(%rax)
>   ffff82d04039610a:       00 00                   add    %al,(%rax)
>   ffff82d04039610c:       10 00                   adc    %al,(%rax)
> 
>   ffff82d04039610e <compat_mode_gdt_desc>:
>   ffff82d04039610e:       17                      (bad)
>           ...
> 
>   ffff82d040396118 <compat_mode_gdt>:
>           ...
>   ffff82d040396120:       ff                      (bad)
>   ffff82d040396121:       ff 00                   incl   (%rax)
>   ffff82d040396123:       00 00                   add    %al,(%rax)
>   ffff82d040396125:       93                      xchg   %eax,%ebx
>   ffff82d040396126:       cf                      iret
>   ffff82d040396127:       00 ff                   add    %bh,%bh
>   ffff82d040396129:       ff 00                   incl   (%rax)
>   ffff82d04039612b:       00 00                   add    %al,(%rax)
>   ffff82d04039612d:       9b                      fwait
>   ffff82d04039612e:       cf                      iret
>           ...
> 
>   ffff82d040396130 <compat_mode_idt>:
>           ...
> 
>   ffff82d0403961b6 <kexec_reloc_size>:
>   ffff82d0403961b6:       b6 01                   mov    $0x1,%dh
>           ...
> 
> to:
> 
>   ffff82d040396108 <compatibility_mode_far>:
>   ffff82d040396108:       00 00 00 00 10 00                               
> ......
> 
>   ffff82d04039610e <compat_mode_gdt_desc>:
>   ffff82d04039610e:       17 00 00 00 00 00 00 00 00 00                   
> ..........
> 
>   ffff82d040396118 <compat_mode_gdt>:
>           ...
>   ffff82d040396120:       ff ff 00 00 00 93 cf 00 ff ff 00 00 00 9b cf 00 
> ................
> 
>   ffff82d040396130 <compat_mode_idt>:
>   ffff82d040396130:       00 00 00 00 00 00                               
> ......

With the .size directives added, can we rely on consistent (past,
present, and future) objcopy behavior for padding gaps? It just so
happens that there's no 4-byte gap between compat_mode_gdt_desc and
compat_mode_gdt. Changing the .align ahead of compatibility_mode_far
would eliminate the risk of padding appearing if the code further up
changed.

>   ffff82d040396136 <reloc_stack>:
>           ...

Now this is particularly puzzling: Us setting %rsp to an unaligned
address is clearly not ABI-conforming. Since you're fiddling with
all of this already anyway, how about fixing this at the same time?
Of course there would then appear padding ahead of the stack, unless
the stack was moved up some.

> @@ -175,10 +175,16 @@ compatibility_mode_far:
>          .long 0x00000000             /* set in call_32_bit above */
>          .word 0x0010
>  
> +        .type compatibility_mode_far, @object
> +        .size compatibility_mode_far, . - compatibility_mode_far
> +
>  compat_mode_gdt_desc:
>          .word .Lcompat_mode_gdt_end - compat_mode_gdt -1
>          .quad 0x0000000000000000     /* set in call_32_bit above */
>  
> +        .type compat_mode_gdt_desc, @object
> +        .size compat_mode_gdt_desc, . - compat_mode_gdt_desc

Side note: We really ought to gain something like OBJECT(name) to avoid
c'n'p mistakes not updating correctly all three symbol name instances.

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -87,6 +87,7 @@ SECTIONS
>         *(.text.unlikely)
>         *(.fixup)
>         *(.text.kexec)
> +       kexec_reloc_end = .;

Does this maybe want aligning on a 4- or even 8-byte boundary? If
so, imo preferably not here, but by adding a trailing .align in the
.S file.

Jan