Re: [PATCH v3 08/16] x86: add multiboot2 protocol support

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.05.16 at 10:13, <andrew.cooper3@citrix.com> wrote:
> On 27/05/16 09:08, Jan Beulich wrote:
>>>>> On 26.05.16 at 12:28, <andrew.cooper3@citrix.com> wrote:
>>>>>> @@ -34,6 +57,42 @@ multiboot1_header_start:       /*** MULTIBOOT1 HEADER 
>>> ****/
>>>>>>          .long   -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
>>>>>>  multiboot1_header_end:
>>>>>>
>>>>>> +/*** MULTIBOOT2 HEADER ****/
>>>>>> +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S 
>>> file. */
>>>>>> +        .align  MULTIBOOT2_HEADER_ALIGN
>>>>>> +
>>>>>> +multiboot2_header_start:
>>>>>> +        /* Magic number indicating a Multiboot2 header. */
>>>>>> +        .long   MULTIBOOT2_HEADER_MAGIC
>>>>>> +        /* Architecture: i386. */
>>>>>> +        .long   MULTIBOOT2_ARCHITECTURE_I386
>>>>>> +        /* Multiboot2 header length. */
>>>>>> +        .long   multiboot2_header_end - multiboot2_header_start
>>>>>> +        /* Multiboot2 header checksum. */
>>>>>> +        .long   -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + 
>>> \
>>>>>> +                        (multiboot2_header_end - multiboot2_header_start))
>>>>>> +
>>>>>> +        /* Multiboot2 information request tag. */
>>>>>> +        mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \
>>>>>> +                   MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP)
>>>>>> +
>>>>>> +        /* Align modules at page boundry. */
>>>>>> +        mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED)
>>>>>> +
>>>>>> +        /* Console flags tag. */
>>>>>> +        mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \
>>>>>> +                   MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
>>>>>> +
>>>>>> +        /* Framebuffer tag. */
>>>>>> +        mb2ht_init MB2_HT(FRAMEBUFFER), MB2_HT(OPTIONAL), \
>>>>>> +                   0, /* Number of the columns - no preference. */ \
>>>>>> +                   0, /* Number of the lines - no preference. */ \
>>>>>> +                   0  /* Number of bits per pixel - no preference. */
>>>>>> +
>>>>>> +        /* Multiboot2 header end tag. */
>>>>>> +        mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
>>>>>> +multiboot2_header_end:
>>>>> Imo "end" labels should always preferably be .L-prefixed, to avoid
>>>>> them getting used by a consumer instead of another "proper" label
>>>>> starting whatever comes next.
>>>> Make sense, however, I am in line with multiboot1_header_end label here.
>>>> So, if we wish .L here then we should change multiboot1_header_end label
>>>> above too. Of course in separate patch.
>>> The multiboot1 header is very specifically not a local label, so you can
>>> distinguish the actual header from the 3 nops following it in the
>>> disassembly.
>> I don't follow: Those NOPs (also not sure why you think it's three of
>> them) are there just for padding (alignment), so no need to label
>> them.
> 
> That wasn't the point I was trying to make.
> 
> This is data in a code segment, so objdump/gdb disassembly tries to
> disassemble the data as instructions.
> 
> While the instruction decode of the header is definitely junk, having
> the end label proves an exact boundary for the data in terms of reported
> raw bytes, as well as prevent the following nops being subsumed into the
> bogus decode.

Where the NOPs go doesn't matter. The only relevant thing for
disassembly is that the next actual instruction gets decoded
correctly. And that next instruction follows its own label
(__high_start). Let's please not carry more symbols at runtime
than are really useful.

Jan


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