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

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

>>> On 25.05.16 at 18:34, <daniel.kiper@oracle.com> wrote:
> On Tue, May 24, 2016 at 09:46:13AM -0600, Jan Beulich wrote:
>> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote:
>> > @@ -19,6 +20,28 @@
>> >  #define BOOT_PSEUDORM_CS 0x0020
>> >  #define BOOT_PSEUDORM_DS 0x0028
>> >
>> > +#define MB2_HT(name)      (MULTIBOOT2_HEADER_TAG_##name)
>> > +#define MB2_TT(name)      (MULTIBOOT2_TAG_TYPE_##name)
>> > +
>> > +        .macro mb2ht_args arg, args:vararg
>> > +        .long \arg
>> > +        .ifnb \args
>> > +        mb2ht_args \args
>> > +        .endif
>> > +        .endm
>> > +
>> > +        .macro mb2ht_init type, req, args:vararg
>>
>> If you already use :vararg here and above, please also use :req on
>> the other macro arguments.
> 
> Why?

Because they're not allowed to be blank, yet it looks like if they
are left blank no error would otherwise be reported by gas?

>> > @@ -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.

Sure. My main point (as always) is that stuff that's there without a
good reason shouldn't be cloned. At least the clone should be done
right from the beginning. Cleaning up existing code is appreciated,
but secondary.

Jan


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