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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.08.15 at 14:00, <daniel.kiper@xxxxxxxxxx> wrote:
> On Tue, Aug 18, 2015 at 02:12:58AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, <daniel.kiper@xxxxxxxxxx> 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.

Yes please: Between confusing the reader of the patch and the reader
of the resulting code, the former is the less problematic one. Furthermore
with the function having just one argument before your series you have
all options to avoid confusing the reader of the patches - just insert the
new arguments at the right slot in each patch adding one.

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

There's no count being subtracted afaict - what is being subtracted
is the size of the rest of the structure.

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

If you want to do this, then not in the middle of some already big
patch doing something completely different, but in a separate
cleanup patch explaining what you explained above (which is not
to say that I'm convinced, and hence I can't promise such a change
to ultimately go in).

Jan


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