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

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
	Daniel Kiper <daniel.kiper@oracle.com>
Cc: Juergen Gross <JGross@suse.com>,
	stefano.stabellini@eu.citrix.com, 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 v3 08/16] x86: add multiboot2 protocol support
Date: Fri, 27 May 2016 02:08:13 -0600	[thread overview]
Message-ID: <57481C8D02000078000EF0F6@prv-mh.provo.novell.com> (raw)
In-Reply-To: <5746CFBC.1070205@citrix.com>

>>> 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. Plus with the patch in place they would now appear after the
multiboot2 header.

Jan

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