Re: [PATCH v12 02/10] x86: add multiboot2 protocol support

[Doug Goldstein <cardoe@cardoe.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 14273 bytes --]

On 1/19/17 8:34 PM, 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 <daniel.kiper@oracle.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
> ---
> v12 - suggestions/fixes:
>     - replace TABs with spaces in xen/include/xen/multiboot2.h
>       (suggested by Doug Goldstein).
> 
> v9 - suggestions/fixes:
>    - use .L label instead of numeric one in multiboot2 data scanning loop;
>      I hope that this change does not invalidate Jan's Reviewed-by
>      (suggested by Jan Beulich).
> 
> v8 - suggestions/fixes:
>    - use sizeof(<var>/<expr>) instead of sizeof(<type>)
>      if it is possible
>      (suggested by Jan Beulich).
> 
> v7 - suggestions/fixes:
>    - rename mbi_mbi/mbi2_mbi to mbi_reloc/mbi2_reloc respectively
>      (suggested by Jan Beulich),
>    - initialize mbi_out->flags using "|=" instead of "="
>      (suggested by Jan Beulich),
>    - use sizeof(*mmap_dst) instead of sizeof(memory_map_t)
>      if it makes sense
>      (suggested by Jan Beulich).
> 
> v6 - suggestions/fixes:
>    - properly index multiboot2_tag_mmap_t.entries[]
>      (suggested by Jan Beulich),
>    - do not index mbi_out_mods[] beyond its end
>      (suggested by Andrew Cooper),
>    - reduce number of casts
>      (suggested by Andrew Cooper and Jan Beulich),
>    - add braces to increase code readability
>      (suggested by Andrew Cooper).
> 
> v5 - suggestions/fixes:
>    - check multiboot2_tag_mmap_t.entry_size before
>      multiboot2_tag_mmap_t.entries[] use
>      (suggested by Jan Beulich),
>    - properly index multiboot2_tag_mmap_t.entries[]
>      (suggested by Jan Beulich),
>    - use "type name[]" instad of "type name[0]"
>      in xen/include/xen/multiboot2.h
>      (suggested by Jan Beulich),
>    - remove unneeded comment
>      (suggested by Jan Beulich).
> 
> v4 - suggestions/fixes:
>    - avoid assembly usage in xen/arch/x86/boot/reloc.c,
>    - fix boundary check issue and optimize
>      for() loops in mbi2_mbi(),
>    - move to stdcall calling convention,
>    - remove unneeded typeof() from ALIGN_UP() macro
>      (suggested by Jan Beulich),
>    - add and use NULL definition in xen/arch/x86/boot/reloc.c
>      (suggested by Jan Beulich),
>    - do not read data beyond the end of multiboot2
>      information in xen/arch/x86/boot/head.S
>      (suggested by Jan Beulich),
>    - add :req to some .macro arguments
>      (suggested by Jan Beulich),
>    - use cmovcc if possible,
>    - add .L to multiboot2_header_end label
>      (suggested by Jan Beulich),
>    - add .L to multiboot2_proto label
>      (suggested by Jan Beulich),
>    - improve label names
>      (suggested by Jan Beulich).
> 
> v3 - suggestions/fixes:
>    - reorder reloc() arguments
>      (suggested by Jan Beulich),
>    - remove .L from multiboot2 header labels
>      (suggested by Andrew Cooper, Jan Beulich and Konrad Rzeszutek Wilk),
>    - take into account alignment when skipping multiboot2 fixed part
>      (suggested by Konrad Rzeszutek Wilk),
>    - create modules data if modules count != 0
>      (suggested by Jan Beulich),
>    - improve macros
>      (suggested by Jan Beulich),
>    - reduce number of casts
>      (suggested by Jan Beulich),
>    - use const if possible
>      (suggested by Jan Beulich),
>    - drop static and __used__ attribute from reloc()
>      (suggested by Jan Beulich),
>    - remove isolated/stray __packed attribute from
>      multiboot2_memory_map_t type definition
>      (suggested by Jan Beulich),
>    - reformat xen/include/xen/multiboot2.h
>      (suggested by Konrad Rzeszutek Wilk),
>    - improve comments
>      (suggested by Konrad Rzeszutek Wilk),
>    - remove hard tabs
>      (suggested by Jan Beulich and Konrad Rzeszutek Wilk).
> 
> 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:
>    - 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          |  107 ++++++++++++++++++++++-
>  xen/arch/x86/boot/reloc.c         |  144 +++++++++++++++++++++++++++++--
>  xen/arch/x86/x86_64/asm-offsets.c |    9 ++
>  xen/include/xen/multiboot2.h      |  169 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 422 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 6d20646..c6246c8 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -4,7 +4,8 @@ DEFS_H_DEPS = defs.h $(BASEDIR)/include/xen/stdbool.h
>  
>  CMDLINE_DEPS = $(DEFS_H_DEPS) video.h
>  
> -RELOC_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h
> +RELOC_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h \
> +	     $(BASEDIR)/include/xen/multiboot2.h
>  
>  head.o: cmdline.S reloc.S
>  
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 126e2e2..84cf44d 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:req, args:vararg
> +        .long \arg
> +        .ifnb \args
> +        mb2ht_args \args
> +        .endif
> +        .endm
> +
> +        .macro mb2ht_init type:req, req:req, args:vararg
> +        .align MULTIBOOT2_TAG_ALIGN
> +.Lmb2ht_init_start\@:
> +        .short \type
> +        .short \req
> +        .long .Lmb2ht_init_end\@ - .Lmb2ht_init_start\@
> +        .ifnb \args
> +        mb2ht_args \args
> +        .endif
> +.Lmb2ht_init_end\@:
> +        .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
> +
> +multiboot2_header_start:
> +        /* Magic number indicating a Multiboot2 header. */
> +        .long   MULTIBOOT2_HEADER_MAGIC
> +        /* Architecture: i386. */
> +        .long   MULTIBOOT2_ARCHITECTURE_I386
> +        /* Multiboot2 header length. */
> +        .long   .Lmultiboot2_header_end - multiboot2_header_start
> +        /* Multiboot2 header checksum. */
> +        .long   -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \
> +                        (.Lmultiboot2_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)
> +.Lmultiboot2_header_end:
> +
>          .section .init.rodata, "a", @progbits
>          .align 4
>  
> @@ -82,10 +141,52 @@ __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      .Lmultiboot2_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
> +
> +.Lmultiboot2_proto:
> +        /* Skip Multiboot2 information fixed part. */
> +        lea     (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%ebx),%ecx
> +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +
> +.Lmb2_tsize:
> +        /* Check Multiboot2 information total size. */
> +        mov     %ecx,%edi
> +        sub     %ebx,%edi
> +        cmp     %edi,MB2_fixed_total_size(%ebx)
> +        jbe     trampoline_setup
> +
> +        /* Get mem_lower from Multiboot2 information. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
> +        cmove   MB2_mem_lower(%ecx),%edx
> +        je      trampoline_setup
> +
> +        /* 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     .Lmb2_tsize
> +
> +trampoline_setup:
>          /* Set up trampoline segment 64k below EBDA */
>          movzwl  0x40e,%ecx          /* EBDA segment */
>          cmp     $0xa000,%ecx        /* sanity check (high) */
> @@ -100,9 +201,6 @@ __start:
>           * Compare the value in the BDA with the information from the
>           * multiboot structure (if available) and use the smallest.
>           */
> -        testb   $MBI_MEMLIMITS,(%ebx)
> -        jz      2f                  /* not available? BDA value will be fine */
> -        mov     MB_mem_lower(%ebx),%edx
>          cmp     $0x100,%edx         /* is the multiboot value too small? */
>          jb      2f                  /* if so, do not use it */
>          shl     $10-4,%edx
> @@ -121,6 +219,7 @@ __start:
>          mov     $sym_phys(cpu0_stack)+1024,%esp
>          push    %ecx                /* Boot trampoline address. */
>          push    %ebx                /* Multiboot information address. */
> +        push    %eax                /* Multiboot magic. */
>          call    reloc
>          mov     %eax,sym_phys(multiboot_ptr)
>  
> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> index 91405e9..0f2e372 100644
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -5,15 +5,18 @@
>   * and modules. This is most easily done early with paging disabled.
>   *
>   * Copyright (c) 2009, Citrix Systems, Inc.
> + * Copyright (c) 2013-2016 Oracle and/or its affiliates. All rights reserved.
>   *
>   * Authors:
>   *    Keir Fraser <keir@xen.org>
> + *    Daniel Kiper <daniel.kiper@oracle.com>
>   */
>  
>  /*
>   * This entry point is entered from xen/arch/x86/boot/head.S with:
> - *   - 0x4(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
> - *   - 0x8(%esp) = BOOT_TRAMPOLINE_ADDRESS.
> + *   - 0x4(%esp) = MULTIBOOT_MAGIC,
> + *   - 0x8(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
> + *   - 0xc(%esp) = BOOT_TRAMPOLINE_ADDRESS.
>   */
>  asm (
>      "    .text                         \n"
> @@ -24,6 +27,10 @@ asm (
>  
>  #include "defs.h"
>  #include "../../../include/xen/multiboot.h"
> +#include "../../../include/xen/multiboot2.h"
> +
> +#define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t *)(tag))->member)
> +#define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, member))
>  
>  static u32 alloc;
>  
> @@ -32,6 +39,12 @@ static u32 alloc_mem(u32 bytes)
>      return alloc -= ALIGN_UP(bytes, 16);

So this works its way DOWN from the supplied trampoline start address
which is coming from cfg.addr correct? So you set cfg.size to
TRAMPOLINE_SIZE + TRAMPOLINE_STACK_SIZE + MBI_SIZE but rather than
starting at cfg.addr + MBI_SIZE you are growing down into the area
before cfg.addr. You've changed this series to use AllocatePages() now
which is allocating from conventional memory of cfg.size immediately
after a non-conventional memory region. The result is this math is
growing into the non-conventional memory and just blowing it away.
Previously you allocated manually by going to the end of the
conventional region and walking backwards cfg.size. It happened to have
at least MBI_SIZE of space available before it so it caused it to work.

Please someone tell me if my conclusions are incorrect here.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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