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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Sun, Aug 10, 2014 at 06:23:55PM +0100, Andrew Cooper wrote:
> On 09/08/14 00:04, 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>
> > ---
> >  xen/arch/x86/boot/head.S  |  112 
> > ++++++++++++++++++++++++++++++++++++++++++++-
> >  xen/arch/x86/boot/reloc.c |  103 ++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 212 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S

[...]

> > @@ -81,10 +144,55 @@ __start:
> >          mov     %ecx,%es
> >          mov     %ecx,%ss
> >
> > +        /* Set mem_lower to 0 */
> > +        xor     %edx,%edx
> > +
>
> How does this affect mem_lower? it doesn't appear relevant in context.

%edx is used to store mem_lower value from multiboot[12] protocol. It must
have sensible value even if multiboot loader do not pass mem_lower value.
That is why %edx is zeroed here.

> >          /* Check for Multiboot bootloader */
> > -        cmp     $0x2BADB002,%eax
> > -        jne     not_multiboot
> > +        cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> > +        je      1f
>
> This $MULTIBOOT_BOOTLOADER_MAGIC should probably be separate, as there
> are other bits of code which should use multiboot manifest constants.

You mean?

> It looks as if this should be "je multiboot1_entry" ...

OK.

> > +
> > +        /* Check for Multiboot2 bootloader */
> > +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> > +        je      2f
> > +
> > +        jmp     not_multiboot
> > +
> > +1:
> > +        /* Get mem_lower from Multiboot information */
> > +        testb   $MBI_MEMLIMITS,(%ebx)
> > +        jz      0f                  /* not available? BDA value will be 
> > fine */
> >
> > +        mov     4(%ebx),%edx
> > +        shl     $10-4,%edx
> > +        jmp     0f
>
> and these 0f's should be "setup_trampoline" or similar.  You have also

OK.

> removed some sanity checks of the multiboot information, which need
> reintroducing.

No, here we are just reading basic data passed via multiboot[12] protocol.
All needed checks are done later. Relevant code is not changed (excluding
some register shuffling done in earlier patch).

Daniel

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