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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan

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