Re: [PATCH v2 08/23] x86: add multiboot2 protocol support

[Daniel Kiper <daniel.kiper@oracle.com>]

On Tue, Aug 18, 2015 at 02:12:58AM -0600, Jan Beulich wrote:
> >>> On 20.07.15 at 16:29, <daniel.kiper@oracle.com> wrote:

[...]

> > @@ -119,10 +213,11 @@ __start:
> >
> >          /* Save the Multiboot info struct (after relocation) for later use. */
> >          mov     $sym_phys(cpu0_stack)+1024,%esp
> > +        push    %eax                /* Multiboot magic. */
> >          push    %ebx                /* Multiboot information address. */
> >          push    %ecx                /* Boot trampoline address. */
> >          call    reloc
> > -        add     $8,%esp             /* Remove reloc() args from stack. */
> > +        add     $12,%esp            /* Remove reloc() args from stack. */
>
> The latest now it becomes clear that the arguments passed are
> kind of backwards: One would expect the qualifying value (i.e.
> the magic) to come first, then the info pointer, and last the
> trampoline address.

Yep, you are right. However, I wanted to avoid changing order of arguments
to not confuse reader. If you wish I can change that thing.

[...]

> > +        case MULTIBOOT2_TAG_TYPE_MMAP:
> > +            mbi_out->flags |= MBI_MEMMAP;
> > +            mbi_out->mmap_length = get_mb2_data(tag, multiboot2_tag_mmap_t, size);
> > +            mbi_out->mmap_length -= sizeof(multiboot2_tag_mmap_t);
> > +            mbi_out->mmap_length += sizeof(((multiboot2_tag_mmap_t){0}).entries);
>
> Afaict this sizeof() evaluates to zero. And even if it didn't I can't see
> what the line is good for.

Right, I have missed that. However, if it does not evaluate to zero then
you must add back relevant number of entries which you subtracted one
line above. Otherwise count of entries will be incorrect.

[...]

> Dropping the "static" would permit to also drop the "used" attribute.
> Since it was that way before, why didn't you keep it that way?

Yep, but I do not like this solution. Lack of static suggests that this function
is used elsewhere. I prefer to explicitly say that there are not external users
of that function and silence compiler warnings with __used.

Daniel