Re: [Xen-devel] [PATCH v2 5/5] xen/x86: Add multiboot2 protocol support

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Sep 24, 2014 at 08:14:03PM +0100, Andrew Cooper wrote:
> On 24/09/14 18:19, Daniel Kiper wrote:
> > Add multiboot2 protocol support. This way we are laying the foundation
> > for EFI + GRUB2 + Xen development.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
>
> Much clearer than before.
>
> Can I suggest retroactively renaming the old multiboot bits to

Do you mean before or after this (rewritten) patch?

> multiboot1 to make a more marked difference between between
> "multiboot1_proto" and "multiboot2_proto" ?

Do you think about labels in this file only or about all
stuff related to multiboot(1) protocol?

> It would also be better to split the changing of multiboot1 and
> shuffling of memory calculations away from the introduction of multiboot2.
>
> > ---
> > v2 - suggestions/fixes:
> >    - use "for" instead of "while" for loops
> >      (suggested by Jan Beulich),
> >    - properly parenthesize macro arguments
> >
> >     char *boot_loader_name;
> >      (suggested by Jan Beulich),
> >    - change some local variables types
> >      (suggested by Jan Beulich),
> >    - use meaningful labels
> >      (suggested by Andrew Cooper and Jan Beulich),
> >    - use local labels
> >      (suggested by Jan Beulich),
> >    - fix coding style
> >      (suggested by Jan Beulich),
> >    - patch split rearrangement
> >      (suggested by Andrew Cooper and Jan Beulich).
> > ---
> >  xen/arch/x86/boot/head.S     |  117 +++++++++++++-
> >  xen/arch/x86/boot/reloc.c    |  103 +++++++++++-
> >  xen/include/xen/multiboot2.h |  354 
> > ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 567 insertions(+), 7 deletions(-)
> >  create mode 100644 xen/include/xen/multiboot2.h
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 7e48833..4331821 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S

[...]

> > @@ -81,10 +144,56 @@ __start:
> >          mov     %ecx,%es
> >          mov     %ecx,%ss
> >
> > +        /* Assume multiboot[12].mem_lower is 0 if not set by bootloader */
> > +        xor     %edx,%edx
> > +
>
> This change does not match its comment, and seems to have appeared from
> nowhere.

I still do not understand what is wrong with that. We need
to set %edx to known value here (0 in this case). If MBI_MEMLIMITS
or MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO is not set by bootloader
and we do not initialize %edx then we will use uninitialized
value later.

> >          /* Check for Multiboot bootloader */
> >          cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> > -        jne     not_multiboot
> > +        je      multiboot_proto
> > +
> > +        /* Check for Multiboot2 bootloader */
> > +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> > +        je      multiboot2_proto
> > +
> > +        jmp     not_multiboot
> > +
> > +multiboot_proto:
> > +        /* Get mem_lower from Multiboot information */
> > +        testb   $MBI_MEMLIMITS,(%ebx)
> > +        jz      trampoline_setup    /* not available? BDA value will be 
> > fine */
> > +
> > +        mov     4(%ebx),%edx
> > +        shl     $10-4,%edx
> > +        jmp     trampoline_setup
> >
> > +multiboot2_proto:
> > +        /* Get Multiboot2 information address */
> > +        mov     %ebx,%ecx
> > +
> > +        /* Skip Multiboot2 information fixed part */
> > +        add     $8,%ecx
> > +
> > +0:
> > +        /* Get mem_lower from Multiboot2 information */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
> > +        jne     1f
> > +
> > +        mov     8(%ecx),%edx
> > +        shl     $10-4,%edx
> > +        jmp     trampoline_setup
> > +
> > +1:
> > +        /* Is it the end of Multiboot2 information? */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_END,(%ecx)
> > +        je      trampoline_setup
> > +
> > +        /* Go to next Multiboot2 information tag */
> > +        add     4(%ecx),%ecx
> > +        add     $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +        jmp     0b
> > +
> > +trampoline_setup:
> >          /* Set up trampoline segment 64k below EBDA */
> >          movzwl  0x40e,%ecx          /* EBDA segment */
> >          cmp     $0xa000,%ecx        /* sanity check (high) */
> > @@ -99,12 +208,8 @@ __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     4(%ebx),%edx
> > -        cmp     $0x100,%edx         /* is the multiboot value too small? */
> > +        cmp     $0x4000,%edx        /* is the multiboot value too small? */
>
> You are going to need some justification for changing this lower bound
> of memory.

It is not changed. I did "shl $10-4,%edx" above so I must change
relevant value here too. However, it looks that it left from my
earlier work and I am able to remove that change safely.

[...]

> > diff --git a/xen/include/xen/multiboot2.h b/xen/include/xen/multiboot2.h
>
> This multiboot2.h is huge - can we get away with only including struct
> definitions for tags we currently use/support?

I did that because multiboot.h contains all multiboot(1) related stuff.
I think that it is nice to have all defs for multiboot2 protocol too,
however, I do not insist on that.

Daniel

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