>>> On 27.03.15 at 11:56, <daniel.ki...@oracle.com> wrote: > On Fri, Feb 20, 2015 at 04:06:26PM +0000, Jan Beulich wrote: >> >>> On 30.01.15 at 18:54, <daniel.ki...@oracle.com> wrote: >> > --- a/xen/arch/x86/boot/Makefile >> > +++ b/xen/arch/x86/boot/Makefile >> > @@ -1,6 +1,7 @@ >> > obj-bin-y += head.o >> > >> > -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h >> > $(BASEDIR)/include/xen/multiboot.h >> > +RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h >> > $(BASEDIR)/include/xen/compiler.h \ >> > + $(BASEDIR)/include/xen/multiboot.h >> > $(BASEDIR)/include/xen/multiboot2.h >> >> Perhaps we should rather have the compiler generate the >> dependencies for us when compiling reloc.o? > > Hmmm... I am a bit confused. Here > http://lists.xen.org/archives/html/xen-devel/2014-10/msg02094.html > you told a bit different thing and relevant patch was accepted as commit > c42070df66c9fcedf477959b8371b85aa4ac4933 > (x86/boot: fix reloc.S build dependencies). I can try to do what you > suggest, this is not a problem > for me, however, I would like to be sure what is your final decision in that > case.
First of all I wasn't anticipating that this list of dependencies would further grow. And then I didn't say "don't do this", I only pointed out (albeit maybe a little too implicitly) that this would be a more complex patch. >> > + .long MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO >> > + .long MULTIBOOT2_TAG_TYPE_MMAP >> > +.Lmultiboot2_info_req_end: >> > + >> > + .align MULTIBOOT2_TAG_ALIGN >> > + .short MULTIBOOT2_HEADER_TAG_MODULE_ALIGN >> > + .short MULTIBOOT2_HEADER_TAG_REQUIRED >> > + .long 8 /* Tag size. */ >> >> ... here and further down you hard-code it. Please be consistent >> (and if you go the calculation route, perhaps introduce some >> redundancy reducing macro). > > I did this deliberately. I calculate size using labels when it is really > needed, e.g. in tags which size depends on number of elements. However, > most tags have strictly defined sizes. So, that is why I dropped labels > and entered simple numbers. Though I agree that it can be improved. > I think that we can define relevant tag structures in multiboot2.h and > generate relevant constants using asm-offsets.c. Is it OK for you? That would mean exposing stuff to other parts of the tree which doesn't need to be exposed. I don't see why you can't just do proper size calculations here. >> > @@ -81,10 +135,51 @@ __start: >> > mov %ecx,%es >> > mov %ecx,%ss >> > >> > + /* Bootloaders may set multiboot[12].mem_lower to a nonzero value >> > */ >> >> Above you meet coding style requirements, but here you stop to do >> so (ongoing below)? Even if pre-existing neighboring comments aren't >> well formed, please don't make the situation worse. > > Do you ask about ending fullstops? Am I right? Yes. >> > @@ -31,7 +38,16 @@ asm ( >> > ); >> > >> > typedef unsigned int u32; >> > +typedef unsigned long long u64; >> > + >> > +#include "../../../include/xen/compiler.h" >> >> Why? > > static multiboot_info_t __used *reloc(void *mbi_in, u32 mb_magic) > Because of this ------> ^^^^^^ And what keeps you from leaving out the "static", making the __used unnecessary? >> > + >> > +typedef struct { >> > + u32 type; >> > + u32 size; >> > + u32 mem_lower; >> > + u32 mem_upper; >> > +} multiboot2_tag_basic_meminfo_t; >> > + >> > +typedef struct __packed { >> >> Why __packed when all the others aren't? > > Ha! This thing was taken from GRUB2 source. > I was asking this question myself many times. > I have not found real explanation for this > but if you wish I can dive into code and > try to find one (if it exists). Or even better just drop the __packed. Jan _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel