Re: [Xen-devel] [PATCH v2 08/23] x86: add multiboot2 protocol support

[Daniel Kiper]

On Mon, Aug 10, 2015 at 03:17:48PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Jul 20, 2015 at 04:29:03PM +0200, Daniel Kiper wrote:
> > Add multiboot2 protocol support. Alter min memory limit handling as we
> > now may not find it from either multiboot (v1) or multiboot2.
> >
> > This way we are laying the foundation for EFI + GRUB2 + Xen development.
> >
> > Signed-off-by: Daniel Kiper <address@hidden>
> > Reviewed-by: Andrew Cooper <address@hidden>
> > ---
> > v2 - suggestions/fixes:
> >    - generate multiboot2 header using macros
> >      (suggested by Jan Beulich),
> >    - improve comments
> >      (suggested by Jan Beulich),
> >    - simplify assembly in xen/arch/x86/boot/head.S
> >      (suggested by Jan Beulich),
> >    - do not include include/xen/compiler.h
> >      in xen/arch/x86/boot/reloc.c
> >      (suggested by Jan Beulich),
> >    - do not read data beyond the end of Multiboot2 information
> >      (suggested by Jan Beulich).
> >
> > v2 - not fixed yet:
>
> You have two 'v2'

Yep, but it says "not fixed in v2".

> >    - dynamic dependency generation for xen/arch/x86/boot/reloc.S;
> >      this requires more work; I am not sure that it pays because
> >      potential patch requires more changes than addition of just
> >      multiboot2.h to Makefile
> >      (suggested by Jan Beulich),
> >    - isolated/stray __packed attribute usage for multiboot2_memory_map_t
> >      (suggested by Jan Beulich).
> > ---
> >  xen/arch/x86/boot/Makefile        |    3 +-
> >  xen/arch/x86/boot/head.S          |  105 +++++++++++++++++++++--
> >  xen/arch/x86/boot/reloc.c         |  146 +++++++++++++++++++++++++++++++-
> >  xen/arch/x86/x86_64/asm-offsets.c |    7 ++
> >  xen/include/xen/multiboot2.h      |  169 
> > +++++++++++++++++++++++++++++++++++++
> >  5 files changed, 420 insertions(+), 10 deletions(-)
> >  create mode 100644 xen/include/xen/multiboot2.h
> >
> > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> > index 5fdb5ae..06893d8 100644
> > --- a/xen/arch/x86/boot/Makefile
> > +++ b/xen/arch/x86/boot/Makefile
> > @@ -1,6 +1,7 @@
> >  obj-bin-y += head.o
> >
> > -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
> > $(BASEDIR)/include/xen/multiboot.h
> > +RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
> > $(BASEDIR)/include/xen/multiboot.h \
> > +        $(BASEDIR)/include/xen/multiboot2.h
> >
> >  head.o: reloc.S
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 77e7da9..57197db 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -1,5 +1,6 @@
> >  #include <xen/config.h>
> >  #include <xen/multiboot.h>
> > +#include <xen/multiboot2.h>
> >  #include <public/xen.h>
> >  #include <asm/asm_defns.h>
> >  #include <asm/desc.h>
> > @@ -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
> > +        .align MULTIBOOT2_TAG_ALIGN
> > +        0:
> > +        .short \type
> > +        .short \req
> > +        .long 1f - 0b
> > +        .ifnb \args
> > +        mb2ht_args \args
> > +        .endif
> > +        1:
> > +        .endm
> > +
> >  ENTRY(start)
> >          jmp     __start
> >
> > @@ -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
> > +
> > +.Lmultiboot2_header:
>
> How come you use .L? It makes this hidden while the multiboot1 headers
> are visible? Makes it a bit harder to see the contents of this under
> an debugger.

Good point. IIRC, Jan asked about that. I will remove .L if he does not object.

> > +        /* Magic number indicating a Multiboot2 header. */
> > +        .long   MULTIBOOT2_HEADER_MAGIC
> > +        /* Architecture: i386. */
> > +        .long   MULTIBOOT2_ARCHITECTURE_I386
> > +        /* Multiboot2 header length. */
> > +        .long   .Lmultiboot2_header_end - .Lmultiboot2_header
> > +        /* Multiboot2 header checksum. */
> > +        .long   -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + 
> > \
> > +                        (.Lmultiboot2_header_end - .Lmultiboot2_header))
> > +
> > +        /* 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)
> > +.Lmultiboot2_header_end:
> > +
> >          .section .init.rodata, "a", @progbits
> >          .align 4
> >
> > @@ -82,10 +141,48 @@ __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(%ebx),%ecx
>
> Is there any point of double checking the aligment? That is
>         and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>
> ?
>
> Looking at the grub code it requests the memory to be aligned properly
> so in practice this will never fail. But since somebody might for fun
> allocate the structure to not be aligned and the fixed part could finish
> on non-aligned space..

You are right, we should check it. multiboot2 spec states:

3.4.2 Basic tags structure

Boot information consists of fixed part and a series of tags. Its start
is 8-bytes aligned.

Daniel