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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/05/16 09:08, Jan Beulich wrote:
>>>> On 26.05.16 at 12:28, <andrew.cooper3@xxxxxxxxxx> 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.

~Andrew

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