[PATCH 04/18] xen/x86: add multiboot2 protocol support
Add multiboot2 protocol support. Alter min memory limit handling as we now may not find it from either multiboot (v1) or multiboot2. This way we are laying the foundation for EFI + GRUB2 + Xen development. Signed-off-by: Daniel Kiper --- xen/arch/x86/boot/Makefile|3 +- xen/arch/x86/boot/head.S | 104 -- xen/arch/x86/boot/reloc.c | 174 - xen/arch/x86/x86_64/asm-offsets.c |6 ++ xen/include/xen/multiboot2.h | 169 +++ 5 files changed, 429 insertions(+), 27 deletions(-) create mode 100644 xen/include/xen/multiboot2.h diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile index 5fdb5ae..0efa7d2 100644 --- 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 head.o: reloc.S diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 6180783..7861057 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -32,6 +33,59 @@ ENTRY(start) .long MULTIBOOT_HEADER_FLAGS /* Checksum: must be the negated sum of the first two fields. */ .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS) + +/*** MULTIBOOT2 HEADER / +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S file. */ +.align MULTIBOOT2_HEADER_ALIGN + +.Lmultiboot2_header: +/* Magic number indicating a Multiboot2 header. */ +.long MULTIBOOT2_HEADER_MAGIC +/* Architecture: i386. */ +.long MULTIBOOT2_ARCHITECTURE_I386 +/* Multiboot2 header length. */ +.long .Lmultiboot2_header_end - .Lmultiboot2_header +/* Multiboot2 header checksum. */ +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \ +(.Lmultiboot2_header_end - .Lmultiboot2_header)) + +/* Multiboot2 tags... */ +.Lmultiboot2_info_req: +/* Multiboot2 information request tag. */ +.short MULTIBOOT2_HEADER_TAG_INFORMATION_REQUEST +.short MULTIBOOT2_HEADER_TAG_REQUIRED +.long .Lmultiboot2_info_req_end - .Lmultiboot2_info_req +.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. */ + +/* Console flags tag. */ +.align MULTIBOOT2_TAG_ALIGN +.short MULTIBOOT2_HEADER_TAG_CONSOLE_FLAGS +.short MULTIBOOT2_HEADER_TAG_OPTIONAL +.long 12 /* Tag size. */ +.long MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED + +/* Framebuffer tag. */ +.align MULTIBOOT2_TAG_ALIGN +.short MULTIBOOT2_HEADER_TAG_FRAMEBUFFER +.short MULTIBOOT2_HEADER_TAG_OPTIONAL +.long 20 /* Tag size. */ +.long 0 /* Number of the columns - no preference. */ +.long 0 /* Number of the lines - no preference. */ +.long 0 /* Number of bits per pixel - no preference. */ + +/* Multiboot2 header end tag. */ +.align MULTIBOOT2_TAG_ALIGN +.short MULTIBOOT2_HEADER_TAG_END +.short 0 +.long 8 /* Tag size. */ +.Lmultiboot2_header_end: .section .init.rodata, "a", @progbits .align 4 @@ -81,10 +135,51 @@ __start: mov %ecx,%es mov %ecx,%ss +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value */ +xor %edx,%edx + /* Check for Multiboot bootloader */ cmp $MULTIBOOT_BOOTLOADER_MAGIC,%eax -jne not_multiboot +je multiboot1_proto +/* Check for Multiboot2 bootloader */ +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax +je multiboot2_proto + +jmp not_multiboot + +multiboot1_proto: +/* Get mem_lower from Multiboot information */ +testb $MBI_MEMLIMITS,(%ebx) +jz trampoline_setup/* not available? BDA value will be fine */ + +mov MB_mem_lower(%ebx),%edx +jmp trampoline_setup + +multiboot2_proto: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx + +0: +/* Get mem_lower from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx) +jne 1f + +mov MB2_mem_lower(%ecx),%edx +jmp trampoline_setup + +1: +/* Is it the end of Multiboot2 information? */ +cmpl$MULT
Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support
On 30/01/15 17:54, Daniel Kiper wrote: > Add multiboot2 protocol support. Alter min memory limit handling as we > now may not find it from either multiboot (v1) or multiboot2. > > This way we are laying the foundation for EFI + GRUB2 + Xen development. > > Signed-off-by: Daniel Kiper I have not reviewed the multiboot2 protocol in detail, but it appears sane and I presume it is suitably tested and works. As far as the mb1/mb2 interaction goes, this is looking far better. Reviewed-by: Andrew Cooper > --- > xen/arch/x86/boot/Makefile|3 +- > xen/arch/x86/boot/head.S | 104 -- > xen/arch/x86/boot/reloc.c | 174 > - > xen/arch/x86/x86_64/asm-offsets.c |6 ++ > xen/include/xen/multiboot2.h | 169 +++ > 5 files changed, 429 insertions(+), 27 deletions(-) > create mode 100644 xen/include/xen/multiboot2.h > > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile > index 5fdb5ae..0efa7d2 100644 > --- 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 > > head.o: reloc.S > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index 6180783..7861057 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -1,5 +1,6 @@ > #include > #include > +#include > #include > #include > #include > @@ -32,6 +33,59 @@ ENTRY(start) > .long MULTIBOOT_HEADER_FLAGS > /* Checksum: must be the negated sum of the first two fields. */ > .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS) > + > +/*** MULTIBOOT2 HEADER / > +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S > file. */ > +.align MULTIBOOT2_HEADER_ALIGN > + > +.Lmultiboot2_header: > +/* Magic number indicating a Multiboot2 header. */ > +.long MULTIBOOT2_HEADER_MAGIC > +/* Architecture: i386. */ > +.long MULTIBOOT2_ARCHITECTURE_I386 > +/* Multiboot2 header length. */ > +.long .Lmultiboot2_header_end - .Lmultiboot2_header > +/* Multiboot2 header checksum. */ > +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \ > +(.Lmultiboot2_header_end - .Lmultiboot2_header)) > + > +/* Multiboot2 tags... */ > +.Lmultiboot2_info_req: > +/* Multiboot2 information request tag. */ > +.short MULTIBOOT2_HEADER_TAG_INFORMATION_REQUEST > +.short MULTIBOOT2_HEADER_TAG_REQUIRED > +.long .Lmultiboot2_info_req_end - .Lmultiboot2_info_req > +.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. */ > + > +/* Console flags tag. */ > +.align MULTIBOOT2_TAG_ALIGN > +.short MULTIBOOT2_HEADER_TAG_CONSOLE_FLAGS > +.short MULTIBOOT2_HEADER_TAG_OPTIONAL > +.long 12 /* Tag size. */ > +.long MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED > + > +/* Framebuffer tag. */ > +.align MULTIBOOT2_TAG_ALIGN > +.short MULTIBOOT2_HEADER_TAG_FRAMEBUFFER > +.short MULTIBOOT2_HEADER_TAG_OPTIONAL > +.long 20 /* Tag size. */ > +.long 0 /* Number of the columns - no preference. */ > +.long 0 /* Number of the lines - no preference. */ > +.long 0 /* Number of bits per pixel - no preference. */ > + > +/* Multiboot2 header end tag. */ > +.align MULTIBOOT2_TAG_ALIGN > +.short MULTIBOOT2_HEADER_TAG_END > +.short 0 > +.long 8 /* Tag size. */ > +.Lmultiboot2_header_end: > > .section .init.rodata, "a", @progbits > .align 4 > @@ -81,10 +135,51 @@ __start: > mov %ecx,%es > mov %ecx,%ss > > +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value */ > +xor %edx,%edx > + > /* Check for Multiboot bootloader */ > cmp $MULTIBOOT_BOOTLOADER_MAGIC,%eax > -jne not_multiboot > +je multiboot1_proto > > +/* Check for Multiboot2 bootloader */ > +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax > +je multiboot2_proto > + > +jmp not_multiboot > + > +multiboot1_proto: > +/* Get mem_lower from Multiboot information */ > +testb $MBI_MEMLIMITS,(%ebx) > +jz trampoline_setup/* not available? BDA value will be
Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support
>>> On 30.01.15 at 18:54, 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?
> @@ -32,6 +33,59 @@ ENTRY(start)
> .long MULTIBOOT_HEADER_FLAGS
> /* Checksum: must be the negated sum of the first two fields. */
> .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
> +
> +/*** MULTIBOOT2 HEADER /
> +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S
> file. */
> +.align MULTIBOOT2_HEADER_ALIGN
> +
> +.Lmultiboot2_header:
> +/* Magic number indicating a Multiboot2 header. */
> +.long MULTIBOOT2_HEADER_MAGIC
> +/* Architecture: i386. */
> +.long MULTIBOOT2_ARCHITECTURE_I386
> +/* Multiboot2 header length. */
> +.long .Lmultiboot2_header_end - .Lmultiboot2_header
> +/* Multiboot2 header checksum. */
> +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \
> +(.Lmultiboot2_header_end - .Lmultiboot2_header))
So here ...
> +/* Multiboot2 tags... */
> +.Lmultiboot2_info_req:
> +/* Multiboot2 information request tag. */
> +.short MULTIBOOT2_HEADER_TAG_INFORMATION_REQUEST
> +.short MULTIBOOT2_HEADER_TAG_REQUIRED
> +.long .Lmultiboot2_info_req_end - .Lmultiboot2_info_req
.. and here you properly calculate the length, while ...
> +.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).
> +
> +/* Console flags tag. */
> +.align MULTIBOOT2_TAG_ALIGN
> +.short MULTIBOOT2_HEADER_TAG_CONSOLE_FLAGS
> +.short MULTIBOOT2_HEADER_TAG_OPTIONAL
> +.long 12 /* Tag size. */
> +.long MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
> +
> +/* Framebuffer tag. */
> +.align MULTIBOOT2_TAG_ALIGN
> +.short MULTIBOOT2_HEADER_TAG_FRAMEBUFFER
> +.short MULTIBOOT2_HEADER_TAG_OPTIONAL
> +.long 20 /* Tag size. */
> +.long 0 /* Number of the columns - no preference. */
> +.long 0 /* Number of the lines - no preference. */
> +.long 0 /* Number of bits per pixel - no preference. */
> +
> +/* Multiboot2 header end tag. */
> +.align MULTIBOOT2_TAG_ALIGN
> +.short MULTIBOOT2_HEADER_TAG_END
> +.short 0
> +.long 8 /* Tag size. */
> +.Lmultiboot2_header_end:
>
> .section .init.rodata, "a", @progbits
> .align 4
> @@ -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.
Also please don't say 12 here - I first even mistook this as an array
index, but you seem to mean 1 or 2. Please use {1,2} instead.
> +xor %edx,%edx
> +
> /* Check for Multiboot bootloader */
> cmp $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> -jne not_multiboot
> +je multiboot1_proto
>
> +/* Check for Multiboot2 bootloader */
> +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> +je multiboot2_proto
> +
> +jmp not_multiboot
> +
> +multiboot1_proto:
> +/* Get mem_lower from Multiboot information */
> +testb $MBI_MEMLIMITS,(%ebx)
MB_flags(%ebx)
> +jz trampoline_setup/* not available? BDA value will be fine
> */
> +
> +mov MB_mem_lower(%ebx),%edx
Please use "cmovnz" or "or" instead of "jz" and "mov".
> +jmp trampoline_setup
> +
> +multiboot2_proto:
> +/* Skip Multiboot2 information fixed part */
> +lea MB2_fixed_sizeof(%ebx),%ecx
> +
> +0:
> +/* Get mem_lower from Multiboot2 information */
> +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
> +jne 1f
> +
> +mov MB2_mem_lower(%ecx),%edx
> +jmp trampoline_setup
> +
> +1:
> +/* Is it the end of Multiboot2 information? */
> +cmpl$MULTIBOOT2_TAG_TYPE_END,(%ecx)
This
Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support
On Fri, Feb 20, 2015 at 04:06:26PM +, Jan Beulich wrote:
> >>> On 30.01.15 at 18:54, 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.
> > @@ -32,6 +33,59 @@ ENTRY(start)
> > .long MULTIBOOT_HEADER_FLAGS
> > /* Checksum: must be the negated sum of the first two fields. */
> > .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
> > +
> > +/*** MULTIBOOT2 HEADER /
> > +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S
> > file. */
> > +.align MULTIBOOT2_HEADER_ALIGN
> > +
> > +.Lmultiboot2_header:
> > +/* Magic number indicating a Multiboot2 header. */
> > +.long MULTIBOOT2_HEADER_MAGIC
> > +/* Architecture: i386. */
> > +.long MULTIBOOT2_ARCHITECTURE_I386
> > +/* Multiboot2 header length. */
> > +.long .Lmultiboot2_header_end - .Lmultiboot2_header
> > +/* Multiboot2 header checksum. */
> > +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 +
> > \
> > +(.Lmultiboot2_header_end - .Lmultiboot2_header))
>
> So here ...
>
> > +/* Multiboot2 tags... */
> > +.Lmultiboot2_info_req:
> > +/* Multiboot2 information request tag. */
> > +.short MULTIBOOT2_HEADER_TAG_INFORMATION_REQUEST
> > +.short MULTIBOOT2_HEADER_TAG_REQUIRED
> > +.long .Lmultiboot2_info_req_end - .Lmultiboot2_info_req
>
> .. and here you properly calculate the length, while ...
>
> > +.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?
> > +/* Console flags tag. */
> > +.align MULTIBOOT2_TAG_ALIGN
> > +.short MULTIBOOT2_HEADER_TAG_CONSOLE_FLAGS
> > +.short MULTIBOOT2_HEADER_TAG_OPTIONAL
> > +.long 12 /* Tag size. */
> > +.long MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
> > +
> > +/* Framebuffer tag. */
> > +.align MULTIBOOT2_TAG_ALIGN
> > +.short MULTIBOOT2_HEADER_TAG_FRAMEBUFFER
> > +.short MULTIBOOT2_HEADER_TAG_OPTIONAL
> > +.long 20 /* Tag size. */
> > +.long 0 /* Number of the columns - no preference. */
> > +.long 0 /* Number of the lines - no preference. */
> > +.long 0 /* Number of bits per pixel - no preference. */
> > +
> > +/* Multiboot2 header end tag. */
> > +.align MULTIBOOT2_TAG_ALIGN
> > +.short MULTIBOOT2_HEADER_TAG_END
> > +.short 0
> > +.long 8 /* Tag size. */
> > +.Lmultiboot2_header_end:
> >
> > .section .init.rodata, "a", @progbits
> > .align 4
> > @@ -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?
> Also please don't say 12 here - I first even mistook this as an array
> index, but you seem to mean 1 or 2. Please use {1,2} instead.
OK.
> > +xor %edx,%edx
> > +
> > /* Check for Multiboot bootloader */
> > cmp $MULTIB
Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support
>>> On 27.03.15 at 11:56, wrote:
> On Fri, Feb 20, 2015 at 04:06:26PM +, Jan Beulich wrote:
>> >>> On 30.01.15 at 18:54, 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
Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support
On Fri, Mar 27, 2015 at 11:20:10AM +, Jan Beulich wrote:
> >>> On 27.03.15 at 11:56, wrote:
> > On Fri, Feb 20, 2015 at 04:06:26PM +, Jan Beulich wrote:
> >> >>> On 30.01.15 at 18:54, 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
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support
>>> On 27.03.15 at 13:22, wrote:
> On Fri, Mar 27, 2015 at 11:20:10AM +, Jan Beulich wrote:
>> >>> On 27.03.15 at 11:56, wrote:
>> > On Fri, Feb 20, 2015 at 04:06:26PM +, Jan Beulich wrote:
>> >> >>> On 30.01.15 at 18:54, wrote:
>> >> > @@ -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?
#include-s of the above kind generally indicate badness, so we
should try to limit them to the absolute minimum.
>> >> > +
>> >> > +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.
Now that's the right course of action.
Jan
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
