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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, May 25, 2016 at 05:03:20AM -0600, Jan Beulich wrote:
> >>> 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.

OK.

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

This is the highest address at which memory region allocated for image
may end. You should remember that image itself can be smaller (Xen
image is smaller) than its initial memory requirements.

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

Potentially yes but why do not use data from boot loader?

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

Then we should change legacy multiboot (v1) code too. Just to be in line
new stuff here. Does it pays? And I am not sure that patching convenience
overweight convenience of numeric labels here.

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

Do you suggest that we should macroize multiboot2 header accesses?

Daniel

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