Re: [Xen-devel] [PATCH 04/18] xen/x86: add multiboot2 protocol support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.15 at 11:56, <daniel.kiper@xxxxxxxxxx> wrote:
> On Fri, Feb 20, 2015 at 04:06:26PM +0000, Jan Beulich wrote:
>> >>> On 30.01.15 at 18:54, <daniel.kiper@xxxxxxxxxx> 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


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