Re: [PATCH v2 23/23] x86: add multiboot2 protocol support for relocatable images

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Jul 20, 2015 at 04:29:18PM +0200, Daniel Kiper wrote:
> Add multiboot2 protocol support for relocatable images. Only GRUB2
> with relevant patches understands that feature. Older multiboot

You may want to enumerate what those 'relevant' patches are.

> protocol (regardless of version) compatible loaders ignore it
> and everything works as usual.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  xen/arch/x86/boot/head.S          |   46 +++++++++++++++++++++++++++++--------
>  xen/arch/x86/x86_64/asm-offsets.c |    1 +
>  xen/include/xen/multiboot2.h      |   13 +++++++++++
>  3 files changed, 50 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index d484f68..2520e48 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -81,6 +81,13 @@ multiboot1_header_end:
>          /* Align modules at page boundry. */
>          mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED)
>  
> +        /* Load address preference. */
> +        mb2ht_init MB2_HT(RELOCATABLE), MB2_HT(OPTIONAL), \
> +                   sym_phys(start), /* Min load address. */ \

We could go straight to __start?

> +                   0xffffffff, /* Max load address (4 GiB - 1). */ \
> +                   0x200000, /* Load address alignment (2 MiB). */ \
> +                   MULTIBOOT2_LOAD_PREFERENCE_HIGH
> +
>          /* Console flags tag. */
>          mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \
>                     MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
> @@ -176,30 +183,39 @@ efi_multiboot2_proto:
>          lea     MB2_fixed_sizeof(%rbx),%rcx
>  
>  0:
> +        /* Get Xen image base address from Multiboot2 information. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_BASE_ADDR,MB2_tag_type(%rcx)
> +        jne     1f
> +
> +        mov     MB2_base_addr(%rcx),%ebp
> +        sub     $XEN_IMG_OFFSET,%rbp
> +        jmp     4f
> +
> +1:
>          /* Get EFI SystemTable address from Multiboot2 information. */
>          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
> -        jne     1f
> +        jne     2f
>  
>          mov     MB2_efi64_st(%rcx),%rsi
>  
>          /* Do not go into real mode on EFI platform. */
>          movb    $1,skip_realmode(%rip)
> -        jmp     3f
> +        jmp     4f
>  
> -1:
> +2:
>          /* Get EFI ImageHandle address from Multiboot2 information. */
>          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx)
> -        jne     2f
> +        jne     3f
>  
>          mov     MB2_efi64_ih(%rcx),%rdi
> -        jmp     3f
> +        jmp     4f
>  
> -2:
> +3:
>          /* Is it the end of Multiboot2 information? */
>          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
>          je      run_bs
>  
> -3:
> +4:
>          /* Go to next Multiboot2 information tag. */
>          add     MB2_tag_size(%rcx),%ecx
>          add     $(MULTIBOOT2_TAG_ALIGN-1),%rcx
> @@ -297,14 +313,23 @@ multiboot2_proto:
>          lea     MB2_fixed_sizeof(%ebx),%ecx
>  
>  0:
> +        /* Get Xen image base address from Multiboot2 information. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_BASE_ADDR,MB2_tag_type(%ecx)
> +        jne     1f
> +
> +        mov     MB2_base_addr(%ecx),%ebp
> +        sub     $XEN_IMG_OFFSET,%ebp
> +        jmp     3f
> +
> +1:
>          /* Get mem_lower from Multiboot2 information. */
>          cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
> -        jne     1f
> +        jne     2f
>  
>          mov     MB2_mem_lower(%ecx),%edx
> -        jmp     trampoline_bios_setup
> +        jmp     3f
>  
> -1:
> +2:
>          /* EFI mode is not supported via legacy BIOS path. */
>          cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
>          je      mb2_too_old
> @@ -316,6 +341,7 @@ multiboot2_proto:
>          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%ecx)
>          je      trampoline_bios_setup
>  
> +3:
>          /* Go to next Multiboot2 information tag. */
>          add     MB2_tag_size(%ecx),%ecx
>          add     $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
> index b7aed49..5345a9e 100644
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -172,6 +172,7 @@ void __dummy__(void)
>      DEFINE(MB2_fixed_sizeof, sizeof(multiboot2_fixed_t));
>      OFFSET(MB2_tag_type, multiboot2_tag_t, type);
>      OFFSET(MB2_tag_size, multiboot2_tag_t, size);
> +    OFFSET(MB2_base_addr, multiboot2_tag_base_addr_t, base_addr);
>      OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower);
>      OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer);
>      OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer);
> diff --git a/xen/include/xen/multiboot2.h b/xen/include/xen/multiboot2.h
> index 09ee64e..a63c4d6 100644
> --- a/xen/include/xen/multiboot2.h
> +++ b/xen/include/xen/multiboot2.h
> @@ -59,11 +59,17 @@
>  #define MULTIBOOT2_HEADER_TAG_EFI_BS			7
>  #define MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI32	8
>  #define MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64	9
> +#define MULTIBOOT2_HEADER_TAG_RELOCATABLE		10
>  
>  /* Header tag flags. */
>  #define MULTIBOOT2_HEADER_TAG_REQUIRED	0
>  #define MULTIBOOT2_HEADER_TAG_OPTIONAL	1
>  
> +/* Where image should be loaded (suggestion not requirement). */
> +#define MULTIBOOT2_LOAD_PREFERENCE_NONE	0
> +#define MULTIBOOT2_LOAD_PREFERENCE_LOW	1
> +#define MULTIBOOT2_LOAD_PREFERENCE_HIGH	2


Would be good for all of this (with the other MULTIBOOT2..)
to be aligned on the same column-ish.

> +
>  /* Header console tag console_flags. */
>  #define MULTIBOOT2_CONSOLE_FLAGS_CONSOLE_REQUIRED	1
>  #define MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED	2
> @@ -90,6 +96,7 @@
>  #define MULTIBOOT2_TAG_TYPE_EFI_BS		18
>  #define MULTIBOOT2_TAG_TYPE_EFI32_IH		19
>  #define MULTIBOOT2_TAG_TYPE_EFI64_IH		20
> +#define MULTIBOOT2_TAG_TYPE_BASE_ADDR		21
>  
>  /* Multiboot 2 tag alignment. */
>  #define MULTIBOOT2_TAG_ALIGN			8
> @@ -120,6 +127,12 @@ typedef struct {
>  typedef struct {
>      u32 type;
>      u32 size;
> +    u32 base_addr;
> +} multiboot2_tag_base_addr_t;
> +
> +typedef struct {
> +    u32 type;
> +    u32 size;
>      char string[0];
>  } multiboot2_tag_string_t;
>  
> -- 
> 1.7.10.4
> 

Otherwise, Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>