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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Tue, Sep 09, 2014 at 03:36:44PM +0100, Jan Beulich wrote:
> >>> On 09.09.14 at 16:21, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Mon, Aug 11, 2014 at 11:33:32AM +0100, Jan Beulich wrote:
> >> >>> On 09.08.14 at 01:04, <daniel.kiper@xxxxxxxxxx> wrote:
> >> > +
> >> > +        /*
> >> > +         * Align Xen image and modules at page boundry.
> >> > +         *
> >> > +         * .balignl MULTIBOOT2_TAG_ALIGN, MULTIBOOT2_TAG_TYPE_END is a 
> >> > hack
> >> > +         * to avoid bug related to Multiboot2 information request tag 
> >> > in earlier
> >> > +         * versions of GRUB2.
> >> > +         *
> >> > +         * DO NOT MOVE THIS TAG! ANY CHANGE HERE MAY BREAK COMPATIBILITY
> >> > +         * WITH EARLIER GRUB2 VERSIONS!
> >> > +         */
> >>
> >> Question is - since your ultimate goal of getting UEFI to work this way
> >> won't be achievable with older GrUB2 anyway, do we care at all? Also,
> >
> > I think that it makes sense to have multiboot2 protocol implementation
> > working on every GRUB2 version. This way it will be quite easy to use
> > that same Xen image regardless of GRUB and multiboot protocol version.
> > Additionally, I discovered that above mentioned bug depends on
> > multiboot2_info_req
> > contents which is very annoying and not very easy to discover. So, this
> > hack ensures that regardless of multiboot2_info_req contents Xen image
> > behaves always in the same way on every GRUB2 version (with or without bug).
>
> I'm still not convinced that adding hacks for something that won't work
> right anyway is useful.
>
> >> > @@ -81,10 +144,55 @@ __start:
> >> >          mov     %ecx,%es
> >> >          mov     %ecx,%ss
> >> >
> >> > +        /* Set mem_lower to 0 */
> >> > +        xor     %edx,%edx
> >> > +
> >> >          /* Check for Multiboot bootloader */
> >> > -        cmp     $0x2BADB002,%eax
> >> > -        jne     not_multiboot
> >> > +        cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> >> > +        je      1f
> >> > +
> >> > +        /* 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
> >>
> >> This code isn't being moved here from elsewhere, but also isn't
> >> multiboot2 related - what's this about? If it's really needed for
> >> something, this should be in a separate patch imo.
> >
> > Here we are reading data from multiboot[1] protocol. Label
> > below is start of multiboot2 protocol implementation. We
> > need rearrange a bit multiboot[1] protocol implementation
> > to have both protocols working.
>
> Re-arrange to me means the same code was present elsewhere
> before. But as said in my initial reply, I can't spot any such origin.

Ahhh... I have just spotted that. I forgot to remove relevant code a few
lines below. Thanks.

> >> > --- a/xen/arch/x86/boot/reloc.c
> >> > +++ b/xen/arch/x86/boot/reloc.c
> >> > @@ -18,8 +18,12 @@ typedef unsigned long u32;
> >> >  typedef unsigned long long u64;
> >> >
> >> >  #include "../../../include/xen/multiboot.h"
> >> > +#include "../../../include/xen/multiboot2.h"
> >> >  #include "../../../include/asm/mbd.h"
> >> >
> >> > +#define ALIGN_UP(addr, align) \
> >> > +                ((addr + (typeof(addr))align - 1) & 
> >> > ~((typeof(addr))align -
> > 1))
> >>
> >> Even if just used locally, please make sure such macros are properly
> >> parenthesized.
> >
> > You mean?
>
> Which part is unclear here? Macro arguments used in expressions
> should be parenthesized.

Too fast reading. I have missed that.

> >> > +        case MULTIBOOT2_TAG_TYPE_MMAP:
> >> > +            mbd->mmap_size = ((multiboot2_tag_mmap_t *)tag)->size;
> >> > +            mbd->mmap_size -= sizeof(multiboot2_tag_mmap_t);
> >> > +            mbd->mmap_size += 
> >> > sizeof(((multiboot2_tag_mmap_t){0}).entries);
> >> > +            mbd->mmap_size /= ((multiboot2_tag_mmap_t 
> >> > *)tag)->entry_size;
> >> > +            mbd->mmap_size *= sizeof(memory_map_t);
> >> > +
> >> > +            mbd->mmap = alloc_struct(mbd->mmap_size);
> >> > +
> >> > +            mmap_src = ((multiboot2_tag_mmap_t *)tag)->entries;
> >> > +            mmap_dst = (memory_map_t *)mbd->mmap;
> >> > +
> >> > +            for (i = 0; i < mbd->mmap_size / sizeof(memory_map_t); ++i)
> >>
> >> Coding style.
> >
> > You mean?
>
> You're kidding, aren't you? I think by now you should not require
> help with seeing where your "for" above lacks blanks.

Ditto.

Thanks,

Daniel

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