Re: [Xen-devel] [PATCH v4 02/19] x86/boot: remove multiboot1_header_end from symbol table

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/08/16 14:52, Jan Beulich wrote:
>>>> On 09.08.16 at 15:24, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 06/08/16 00:04, Daniel Kiper wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -32,7 +32,7 @@ multiboot1_header_start:       /*** MULTIBOOT1 HEADER 
>>> ****/
>>>          .long   MULTIBOOT_HEADER_FLAGS
>>>          /* Checksum: must be the negated sum of the first two fields. */
>>>          .long   -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
>>> -multiboot1_header_end:
>>> +.Lmultiboot1_header_end:
>> I put this in as a non local symbol for a very good reason, and see no
>> justification to change it.
>>
>> It is very important to be able to distinguish data from opcode in the
>> disassembly, and one extra global symbol will not break the bank.
> Well, I was about to commit it, but will hold of now that you object.
> Nevertheless I disagree, and would like to see the patch go in: If
> there is code starting past this label, then that code should itself
> have a label, and any padding between the end label above and the
> code start label is neither code nor data anyway.

andrewcoop@andrewcoop:/local/xen.git/xen$ objdump -d xen-syms
xen-syms:     file format elf64-x86-64


Disassembly of section .text:

ffff82d080100000 <_start>:
ffff82d080100000:       e9 2b d0 19 00          jmpq   ffff82d08029d030
<__start>
ffff82d080100005:       0f 1f 00                nopl   (%rax)

ffff82d080100008 <multiboot1_header_start>:
ffff82d080100008:       02 b0 ad 1b 03 00       add    0x31bad(%rax),%dh
ffff82d08010000e:       00 00                   add    %al,(%rax)
ffff82d080100010:       fb                      sti   
ffff82d080100011:       4f 52                   rex.WRXB push %r10
ffff82d080100013:       e4 66                   in     $0x66,%al

ffff82d080100014 <multiboot1_header_end>:
ffff82d080100014:       66 66 66 2e 0f 1f 84    data16 data16 nopw
%cs:0x0(%rax,%rax,1)
ffff82d08010001b:       00 00 00 00 00

ffff82d080100020 <__high_start>:
ffff82d080100020:       0f 01 15 df 1f 20 00    lgdt  
0x201fdf(%rip)        # ffff82d080302006 <gdt_descr>
ffff82d080100027:       b9 00 00 00 00          mov    $0x0,%ecx

There is padding, so the symbol doesn't overlap, but given that one byte
at the end of the multiboot header is indistinguishable from the the
7-byte nop immediately following it, the lack of multiboot1_header_end
is very deceptive.

Leaving this symbol in does not have any downside, and has significant
upside in terms of clarity of the disassembled source.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel