>>> On 18.08.15 at 14:00, <daniel.ki...@oracle.com> wrote: > On Tue, Aug 18, 2015 at 02:12:58AM -0600, Jan Beulich wrote: >> >>> On 20.07.15 at 16:29, <daniel.ki...@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.
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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel