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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>, Daniel Kiper <daniel.kiper@oracle.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, cardoe@cardoe.com, pgnet.dev@gmail.com,
	ning.sun@intel.com, david.vrabel@citrix.com,
	xen-devel@lists.xenproject.org, qiaowei.ren@intel.com,
	richard.l.maliszewski@intel.com, gang.wei@intel.com,
	fu.wei@linaro.org
Subject: Re: [PATCH v4 02/19] x86/boot: remove multiboot1_header_end from symbol table
Date: Tue, 9 Aug 2016 15:09:19 +0100	[thread overview]
Message-ID: <576074cd-934e-7dae-3ce2-ed1fb7c2156c@citrix.com> (raw)
In-Reply-To: <57A9FC2202000078001044DD@prv-mh.provo.novell.com>

On 09/08/16 14:52, Jan Beulich wrote:
>>>> On 09.08.16 at 15:24, <andrew.cooper3@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel