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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

> +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?

> +/* 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...

Jan

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