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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> wrote:
> Add multiboot2 protocol support for relocatable images. Only GRUB2 with
> "multiboot2: Add support for relocatable images" patch understands
> that feature. Older multiboot protocol (regardless of version)
> compatible loaders ignore it and everything works as usual.

So with that I'm now sure that the previous patch is in need of a
better description.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -79,6 +79,13 @@ multiboot2_header_start:
>          /* 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_offset(start), /* Min load address. */ \
> +                   0xffffffff, /* Max load address (4 GiB - 1). */ \

Hardly - that would allow us to be loaded at 4G - 2M, no matter
how large the image. Or else the comment is misleading.

> @@ -178,30 +185,39 @@ efi_multiboot2_proto:
>          and     $~(MULTIBOOT2_TAG_ALIGN-1),%rcx
>  
>  0:
> +        /* Get Xen image load base address from Multiboot2 information. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%rcx)
> +        jne     1f
> +
> +        mov     MB2_load_base_addr(%rcx),%r15d
> +        sub     $XEN_IMG_OFFSET,%r15
> +        jmp     4f

Why do we need to read this from the table? Can't we easily calculate
this ourselves?

> +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 clear BSS twice and do not go into real mode. */
>          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

See why numeric labels are bad in situations like this? The (much)
earlier patch should use .L labels here, and the patch here then
should simply follow suit.

> @@ -313,14 +329,23 @@ multiboot2_proto:
>          and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>  
>  0:
> +        /* Get Xen image load base address from Multiboot2 information. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%ecx)
> +        jne     1f
> +
> +        mov     MB2_load_base_addr(%ecx),%esi
> +        sub     $XEN_IMG_OFFSET,%esi
> +        jmp     3f

The redundancy once again suggests some form of abstraction
(helper function, macro, ...).

Jan


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