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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Tue, May 24, 2016 at 09:46:13AM -0600, Jan Beulich wrote:
> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> 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?

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

> > @@ -82,10 +141,49 @@ __start:
> >          mov     %ecx,%es
> >          mov     %ecx,%ss
> >
> > -        /* Check for Multiboot bootloader */
> > +        /* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero 
> > value. */
> > +        xor     %edx,%edx
> > +
> > +        /* Check for Multiboot2 bootloader. */
> > +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> > +        je      multiboot2_proto
> > +
> > +        /* Check for Multiboot bootloader. */
> >          cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> >          jne     not_multiboot
> >
> > +        /* Get mem_lower from Multiboot information. */
> > +        testb   $MBI_MEMLIMITS,MB_flags(%ebx)
> > +
> > +        /* Not available? BDA value will be fine. */
> > +        cmovnz  MB_mem_lower(%ebx),%edx
> > +        jmp     trampoline_setup
> > +
> > +multiboot2_proto:
> > +        /* Skip Multiboot2 information fixed part. */
> > +        lea     (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%ebx),%ecx
> > +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +
> > +0:
> > +        /* Get mem_lower from Multiboot2 information. */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
> > +        jne     1f
> > +
> > +        mov     MB2_mem_lower(%ecx),%edx
> > +        jmp     trampoline_setup
> > +
> > +1:
> > +        /* Is it the end of Multiboot2 information? */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%ecx)
> > +        je      trampoline_setup
> > +
> > +        /* Go to next Multiboot2 information tag. */
> > +        add     MB2_tag_size(%ecx),%ecx
> > +        add     $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +        jmp     0b
>
> I'm missing a total size check, matching what meanwhile got added
> to the C equivalent(s) of this loop. There's little point in doing it
> there if it doesn't also get done here.

OK.

> > @@ -41,7 +45,16 @@ asm (
> >      );
> >
> >  typedef unsigned int u32;
> > +typedef unsigned long long u64;
> > +
> >  #include "../../../include/xen/multiboot.h"
> > +#include "../../../include/xen/multiboot2.h"
> > +
> > +#define ALIGN_UP(addr, align) \
> > +                (((addr) + (typeof(addr))(align) - 1) & 
> > ~((typeof(addr))(align) - 1))
>
> What is the left typeof() needed for here? (I can see the point of
> the right one.)

AIUI, right typeof() is needed for "~" but it looks that left one is not
needed and could be safely removed.

> > +static multiboot_info_t *mbi2_mbi(u32 mbi_in)
> > +{
> > +    const multiboot2_memory_map_t *mmap_src;
> > +    const multiboot2_tag_t *tag;
> > +    /* Do not complain that mbi_out_mods is not initialized. */
> > +    module_t *mbi_out_mods = (module_t *)0;
>
> Do we not have a proper NULL available in this environment?

No, we should define it or include relevant header file.

> > +/* Multiboot 2 architectures. */
> > +#define MULTIBOOT2_ARCHITECTURE_I386                       0
> > +#define MULTIBOOT2_ARCHITECTURE_MIPS32                     4
>
> What's the latter good for? I can't imagine this is a complete list...

Surprisingly it is! However, TBH, it is not really used by us. Just
copied from GRUB2 multiboot2.h file for completeness.

Daniel

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