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

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

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

So, I understand this as "Go for it!".

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

OK, I will do this as you wish.

> >> > @@ -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?

This func is clearly static. Why do not mark it as is and use __used.
What is wrong with that?

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

Should not we be in line with GRUB2 source?
Even it sounds strange. Anyway, I will check
GRUB2 source and maybe we can also remove __packed
from it. This way everything will be OK.

Daniel