Re: [Xen-devel] [PATCH v3 08/16] x86: add multiboot2 protocol support
>>> On 27.05.16 at 10:13, wrote: > On 27/05/16 09:08, Jan Beulich wrote: > On 26.05.16 at 12:28, wrote: >> @@ -34,6 +57,42 @@ multiboot1_header_start: /*** MULTIBOOT1 HEADER >>> / >> .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS) >> multiboot1_header_end: >> >> +/*** MULTIBOOT2 HEADER / >> +/* Some ideas are taken from >> grub-2.00/grub-core/tests/boot/kernel-i386.S >>> file. */ >> +.align MULTIBOOT2_HEADER_ALIGN >> + >> +multiboot2_header_start: >> +/* Magic number indicating a Multiboot2 header. */ >> +.long MULTIBOOT2_HEADER_MAGIC >> +/* Architecture: i386. */ >> +.long MULTIBOOT2_ARCHITECTURE_I386 >> +/* Multiboot2 header length. */ >> +.long multiboot2_header_end - multiboot2_header_start >> +/* Multiboot2 header checksum. */ >> +.long -(MULTIBOOT2_HEADER_MAGIC + >> MULTIBOOT2_ARCHITECTURE_I386 + >>> \ >> +(multiboot2_header_end - >> multiboot2_header_start)) >> + >> +/* Multiboot2 information request tag. */ >> +mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \ >> + MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP) >> + >> +/* Align modules at page boundry. */ >> +mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED) >> + >> +/* Console flags tag. */ >> +mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \ >> + MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED >> + >> +/* Framebuffer tag. */ >> +mb2ht_init MB2_HT(FRAMEBUFFER), MB2_HT(OPTIONAL), \ >> + 0, /* Number of the columns - no preference. */ \ >> + 0, /* Number of the lines - no preference. */ \ >> + 0 /* Number of bits per pixel - no preference. */ >> + >> +/* Multiboot2 header end tag. */ >> +mb2ht_init MB2_HT(END), MB2_HT(REQUIRED) >> +multiboot2_header_end: > Imo "end" labels should always preferably be .L-prefixed, to avoid > them getting used by a consumer instead of another "proper" label > starting whatever comes next. Make sense, however, I am in line with multiboot1_header_end label here. So, if we wish .L here then we should change multiboot1_header_end label above too. Of course in separate patch. >>> The multiboot1 header is very specifically not a local label, so you can >>> distinguish the actual header from the 3 nops following it in the >>> disassembly. >> I don't follow: Those NOPs (also not sure why you think it's three of >> them) are there just for padding (alignment), so no need to label >> them. > > That wasn't the point I was trying to make. > > This is data in a code segment, so objdump/gdb disassembly tries to > disassemble the data as instructions. > > While the instruction decode of the header is definitely junk, having > the end label proves an exact boundary for the data in terms of reported > raw bytes, as well as prevent the following nops being subsumed into the > bogus decode. Where the NOPs go doesn't matter. The only relevant thing for disassembly is that the next actual instruction gets decoded correctly. And that next instruction follows its own label (__high_start). Let's please not carry more symbols at runtime than are really useful. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/16] x86: add multiboot2 protocol support
On 27/05/16 09:08, Jan Beulich wrote: On 26.05.16 at 12:28, wrote: > @@ -34,6 +57,42 @@ multiboot1_header_start: /*** MULTIBOOT1 HEADER >> / > .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS) > multiboot1_header_end: > > +/*** MULTIBOOT2 HEADER / > +/* Some ideas are taken from > grub-2.00/grub-core/tests/boot/kernel-i386.S >> file. */ > +.align MULTIBOOT2_HEADER_ALIGN > + > +multiboot2_header_start: > +/* Magic number indicating a Multiboot2 header. */ > +.long MULTIBOOT2_HEADER_MAGIC > +/* Architecture: i386. */ > +.long MULTIBOOT2_ARCHITECTURE_I386 > +/* Multiboot2 header length. */ > +.long multiboot2_header_end - multiboot2_header_start > +/* Multiboot2 header checksum. */ > +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 > + >> \ > +(multiboot2_header_end - > multiboot2_header_start)) > + > +/* Multiboot2 information request tag. */ > +mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \ > + MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP) > + > +/* Align modules at page boundry. */ > +mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED) > + > +/* Console flags tag. */ > +mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \ > + MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED > + > +/* Framebuffer tag. */ > +mb2ht_init MB2_HT(FRAMEBUFFER), MB2_HT(OPTIONAL), \ > + 0, /* Number of the columns - no preference. */ \ > + 0, /* Number of the lines - no preference. */ \ > + 0 /* Number of bits per pixel - no preference. */ > + > +/* Multiboot2 header end tag. */ > +mb2ht_init MB2_HT(END), MB2_HT(REQUIRED) > +multiboot2_header_end: Imo "end" labels should always preferably be .L-prefixed, to avoid them getting used by a consumer instead of another "proper" label starting whatever comes next. >>> Make sense, however, I am in line with multiboot1_header_end label here. >>> So, if we wish .L here then we should change multiboot1_header_end label >>> above too. Of course in separate patch. >> The multiboot1 header is very specifically not a local label, so you can >> distinguish the actual header from the 3 nops following it in the >> disassembly. > I don't follow: Those NOPs (also not sure why you think it's three of > them) are there just for padding (alignment), so no need to label > them. That wasn't the point I was trying to make. This is data in a code segment, so objdump/gdb disassembly tries to disassemble the data as instructions. While the instruction decode of the header is definitely junk, having the end label proves an exact boundary for the data in terms of reported raw bytes, as well as prevent the following nops being subsumed into the bogus decode. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/16] x86: add multiboot2 protocol support
>>> On 25.05.16 at 18:34, wrote: > On Tue, May 24, 2016 at 09:46:13AM -0600, Jan Beulich wrote: >> >>> On 15.04.16 at 14:33, wrote: >> > @@ -19,6 +20,28 @@ >> > #define BOOT_PSEUDORM_CS 0x0020 >> > #define BOOT_PSEUDORM_DS 0x0028 >> > >> > +#define MB2_HT(name) (MULTIBOOT2_HEADER_TAG_##name) >> > +#define MB2_TT(name) (MULTIBOOT2_TAG_TYPE_##name) >> > + >> > +.macro mb2ht_args arg, args:vararg >> > +.long \arg >> > +.ifnb \args >> > +mb2ht_args \args >> > +.endif >> > +.endm >> > + >> > +.macro mb2ht_init type, req, args:vararg >> >> If you already use :vararg here and above, please also use :req on >> the other macro arguments. > > Why? Because they're not allowed to be blank, yet it looks like if they are left blank no error would otherwise be reported by gas? >> > @@ -34,6 +57,42 @@ multiboot1_header_start: /*** MULTIBOOT1 HEADER >> > / >> > .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS) >> > multiboot1_header_end: >> > >> > +/*** MULTIBOOT2 HEADER / >> > +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S >> > file. */ >> > +.align MULTIBOOT2_HEADER_ALIGN >> > + >> > +multiboot2_header_start: >> > +/* Magic number indicating a Multiboot2 header. */ >> > +.long MULTIBOOT2_HEADER_MAGIC >> > +/* Architecture: i386. */ >> > +.long MULTIBOOT2_ARCHITECTURE_I386 >> > +/* Multiboot2 header length. */ >> > +.long multiboot2_header_end - multiboot2_header_start >> > +/* Multiboot2 header checksum. */ >> > +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 >> > + \ >> > +(multiboot2_header_end - multiboot2_header_start)) >> > + >> > +/* Multiboot2 information request tag. */ >> > +mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \ >> > + MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP) >> > + >> > +/* Align modules at page boundry. */ >> > +mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED) >> > + >> > +/* Console flags tag. */ >> > +mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \ >> > + MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED >> > + >> > +/* Framebuffer tag. */ >> > +mb2ht_init MB2_HT(FRAMEBUFFER), MB2_HT(OPTIONAL), \ >> > + 0, /* Number of the columns - no preference. */ \ >> > + 0, /* Number of the lines - no preference. */ \ >> > + 0 /* Number of bits per pixel - no preference. */ >> > + >> > +/* Multiboot2 header end tag. */ >> > +mb2ht_init MB2_HT(END), MB2_HT(REQUIRED) >> > +multiboot2_header_end: >> >> Imo "end" labels should always preferably be .L-prefixed, to avoid >> them getting used by a consumer instead of another "proper" label >> starting whatever comes next. > > Make sense, however, I am in line with multiboot1_header_end label here. > So, if we wish .L here then we should change multiboot1_header_end label > above too. Of course in separate patch. Sure. My main point (as always) is that stuff that's there without a good reason shouldn't be cloned. At least the clone should be done right from the beginning. Cleaning up existing code is appreciated, but secondary. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/16] x86: add multiboot2 protocol support
>>> On 26.05.16 at 12:28, wrote: @@ -34,6 +57,42 @@ multiboot1_header_start: /*** MULTIBOOT1 HEADER > / .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS) multiboot1_header_end: +/*** MULTIBOOT2 HEADER / +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S > file. */ +.align MULTIBOOT2_HEADER_ALIGN + +multiboot2_header_start: +/* Magic number indicating a Multiboot2 header. */ +.long MULTIBOOT2_HEADER_MAGIC +/* Architecture: i386. */ +.long MULTIBOOT2_ARCHITECTURE_I386 +/* Multiboot2 header length. */ +.long multiboot2_header_end - multiboot2_header_start +/* Multiboot2 header checksum. */ +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + > \ +(multiboot2_header_end - multiboot2_header_start)) + +/* Multiboot2 information request tag. */ +mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \ + MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP) + +/* Align modules at page boundry. */ +mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED) + +/* Console flags tag. */ +mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \ + MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED + +/* Framebuffer tag. */ +mb2ht_init MB2_HT(FRAMEBUFFER), MB2_HT(OPTIONAL), \ + 0, /* Number of the columns - no preference. */ \ + 0, /* Number of the lines - no preference. */ \ + 0 /* Number of bits per pixel - no preference. */ + +/* Multiboot2 header end tag. */ +mb2ht_init MB2_HT(END), MB2_HT(REQUIRED) +multiboot2_header_end: >>> Imo "end" labels should always preferably be .L-prefixed, to avoid >>> them getting used by a consumer instead of another "proper" label >>> starting whatever comes next. >> Make sense, however, I am in line with multiboot1_header_end label here. >> So, if we wish .L here then we should change multiboot1_header_end label >> above too. Of course in separate patch. > > The multiboot1 header is very specifically not a local label, so you can > distinguish the actual header from the 3 nops following it in the > disassembly. I don't follow: Those NOPs (also not sure why you think it's three of them) are there just for padding (alignment), so no need to label them. Plus with the patch in place they would now appear after the multiboot2 header. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/16] x86: add multiboot2 protocol support
>>> @@ -34,6 +57,42 @@ multiboot1_header_start: /*** MULTIBOOT1 HEADER >>> / >>> .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS) >>> multiboot1_header_end: >>> >>> +/*** MULTIBOOT2 HEADER / >>> +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S >>> file. */ >>> +.align MULTIBOOT2_HEADER_ALIGN >>> + >>> +multiboot2_header_start: >>> +/* Magic number indicating a Multiboot2 header. */ >>> +.long MULTIBOOT2_HEADER_MAGIC >>> +/* Architecture: i386. */ >>> +.long MULTIBOOT2_ARCHITECTURE_I386 >>> +/* Multiboot2 header length. */ >>> +.long multiboot2_header_end - multiboot2_header_start >>> +/* Multiboot2 header checksum. */ >>> +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + >>> \ >>> +(multiboot2_header_end - multiboot2_header_start)) >>> + >>> +/* Multiboot2 information request tag. */ >>> +mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \ >>> + MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP) >>> + >>> +/* Align modules at page boundry. */ >>> +mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED) >>> + >>> +/* Console flags tag. */ >>> +mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \ >>> + MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED >>> + >>> +/* Framebuffer tag. */ >>> +mb2ht_init MB2_HT(FRAMEBUFFER), MB2_HT(OPTIONAL), \ >>> + 0, /* Number of the columns - no preference. */ \ >>> + 0, /* Number of the lines - no preference. */ \ >>> + 0 /* Number of bits per pixel - no preference. */ >>> + >>> +/* Multiboot2 header end tag. */ >>> +mb2ht_init MB2_HT(END), MB2_HT(REQUIRED) >>> +multiboot2_header_end: >> Imo "end" labels should always preferably be .L-prefixed, to avoid >> them getting used by a consumer instead of another "proper" label >> starting whatever comes next. > Make sense, however, I am in line with multiboot1_header_end label here. > So, if we wish .L here then we should change multiboot1_header_end label > above too. Of course in separate patch. The multiboot1 header is very specifically not a local label, so you can distinguish the actual header from the 3 nops following it in the disassembly. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/16] x86: add multiboot2 protocol support
On Tue, May 24, 2016 at 09:46:13AM -0600, Jan Beulich wrote:
> >>> On 15.04.16 at 14:33, wrote:
> > @@ -19,6 +20,28 @@
> > #define BOOT_PSEUDORM_CS 0x0020
> > #define BOOT_PSEUDORM_DS 0x0028
> >
> > +#define MB2_HT(name) (MULTIBOOT2_HEADER_TAG_##name)
> > +#define MB2_TT(name) (MULTIBOOT2_TAG_TYPE_##name)
> > +
> > +.macro mb2ht_args arg, args:vararg
> > +.long \arg
> > +.ifnb \args
> > +mb2ht_args \args
> > +.endif
> > +.endm
> > +
> > +.macro mb2ht_init type, req, args:vararg
>
> If you already use :vararg here and above, please also use :req on
> the other macro arguments.
Why?
> > @@ -34,6 +57,42 @@ multiboot1_header_start: /*** MULTIBOOT1 HEADER
> > /
> > .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
> > multiboot1_header_end:
> >
> > +/*** MULTIBOOT2 HEADER /
> > +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S
> > file. */
> > +.align MULTIBOOT2_HEADER_ALIGN
> > +
> > +multiboot2_header_start:
> > +/* Magic number indicating a Multiboot2 header. */
> > +.long MULTIBOOT2_HEADER_MAGIC
> > +/* Architecture: i386. */
> > +.long MULTIBOOT2_ARCHITECTURE_I386
> > +/* Multiboot2 header length. */
> > +.long multiboot2_header_end - multiboot2_header_start
> > +/* Multiboot2 header checksum. */
> > +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 +
> > \
> > +(multiboot2_header_end - multiboot2_header_start))
> > +
> > +/* Multiboot2 information request tag. */
> > +mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \
> > + MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP)
> > +
> > +/* Align modules at page boundry. */
> > +mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED)
> > +
> > +/* Console flags tag. */
> > +mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \
> > + MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
> > +
> > +/* Framebuffer tag. */
> > +mb2ht_init MB2_HT(FRAMEBUFFER), MB2_HT(OPTIONAL), \
> > + 0, /* Number of the columns - no preference. */ \
> > + 0, /* Number of the lines - no preference. */ \
> > + 0 /* Number of bits per pixel - no preference. */
> > +
> > +/* Multiboot2 header end tag. */
> > +mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
> > +multiboot2_header_end:
>
> Imo "end" labels should always preferably be .L-prefixed, to avoid
> them getting used by a consumer instead of another "proper" label
> starting whatever comes next.
Make sense, however, I am in line with multiboot1_header_end label here.
So, if we wish .L here then we should change multiboot1_header_end label
above too. Of course in separate patch.
> > @@ -82,10 +141,49 @@ __start:
> > mov %ecx,%es
> > mov %ecx,%ss
> >
> > -/* Check for Multiboot bootloader */
> > +/* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero
> > value. */
> > +xor %edx,%edx
> > +
> > +/* Check for Multiboot2 bootloader. */
> > +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> > +je multiboot2_proto
> > +
> > +/* Check for Multiboot bootloader. */
> > cmp $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> > jne not_multiboot
> >
> > +/* Get mem_lower from Multiboot information. */
> > +testb $MBI_MEMLIMITS,MB_flags(%ebx)
> > +
> > +/* Not available? BDA value will be fine. */
> > +cmovnz MB_mem_lower(%ebx),%edx
> > +jmp trampoline_setup
> > +
> > +multiboot2_proto:
> > +/* Skip Multiboot2 information fixed part. */
> > +lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%ebx),%ecx
> > +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +
> > +0:
> > +/* Get mem_lower from Multiboot2 information. */
> > +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%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,MB2_tag_type(%ecx)
> > +je trampoline_setup
> > +
> > +/* Go to next Multiboot2 information tag. */
> > +add MB2_tag_size(%ecx),%ecx
> > +add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +jmp 0b
>
> I'm missing a total size check, matching what meanwhile got added
> to the C equivalent(s) of this loop. There's little point in doing it
> there if it doesn't also get done here.
OK.
> > @@ -41,7 +45,16 @@ asm (
> > );
> >
> > typedef unsigned int u32;
> > +typedef unsigned long long u64;
> > +
> > #include "../../../include/xen/multiboot.h"
> > +#include "../..
Re: [Xen-devel] [PATCH v3 08/16] x86: add multiboot2 protocol support
>>> On 15.04.16 at 14:33, wrote:
> @@ -19,6 +20,28 @@
> #define BOOT_PSEUDORM_CS 0x0020
> #define BOOT_PSEUDORM_DS 0x0028
>
> +#define MB2_HT(name) (MULTIBOOT2_HEADER_TAG_##name)
> +#define MB2_TT(name) (MULTIBOOT2_TAG_TYPE_##name)
> +
> +.macro mb2ht_args arg, args:vararg
> +.long \arg
> +.ifnb \args
> +mb2ht_args \args
> +.endif
> +.endm
> +
> +.macro mb2ht_init type, req, args:vararg
If you already use :vararg here and above, please also use :req on
the other macro arguments.
> @@ -34,6 +57,42 @@ multiboot1_header_start: /*** MULTIBOOT1 HEADER /
> .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
> multiboot1_header_end:
>
> +/*** MULTIBOOT2 HEADER /
> +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S
> file. */
> +.align MULTIBOOT2_HEADER_ALIGN
> +
> +multiboot2_header_start:
> +/* Magic number indicating a Multiboot2 header. */
> +.long MULTIBOOT2_HEADER_MAGIC
> +/* Architecture: i386. */
> +.long MULTIBOOT2_ARCHITECTURE_I386
> +/* Multiboot2 header length. */
> +.long multiboot2_header_end - multiboot2_header_start
> +/* Multiboot2 header checksum. */
> +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \
> +(multiboot2_header_end - multiboot2_header_start))
> +
> +/* Multiboot2 information request tag. */
> +mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \
> + MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP)
> +
> +/* Align modules at page boundry. */
> +mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED)
> +
> +/* Console flags tag. */
> +mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \
> + MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
> +
> +/* Framebuffer tag. */
> +mb2ht_init MB2_HT(FRAMEBUFFER), MB2_HT(OPTIONAL), \
> + 0, /* Number of the columns - no preference. */ \
> + 0, /* Number of the lines - no preference. */ \
> + 0 /* Number of bits per pixel - no preference. */
> +
> +/* Multiboot2 header end tag. */
> +mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
> +multiboot2_header_end:
Imo "end" labels should always preferably be .L-prefixed, to avoid
them getting used by a consumer instead of another "proper" label
starting whatever comes next.
> @@ -82,10 +141,49 @@ __start:
> mov %ecx,%es
> mov %ecx,%ss
>
> -/* Check for Multiboot bootloader */
> +/* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value.
> */
> +xor %edx,%edx
> +
> +/* Check for Multiboot2 bootloader. */
> +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> +je multiboot2_proto
> +
> +/* Check for Multiboot bootloader. */
> cmp $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> jne not_multiboot
>
> +/* Get mem_lower from Multiboot information. */
> +testb $MBI_MEMLIMITS,MB_flags(%ebx)
> +
> +/* Not available? BDA value will be fine. */
> +cmovnz MB_mem_lower(%ebx),%edx
> +jmp trampoline_setup
> +
> +multiboot2_proto:
> +/* Skip Multiboot2 information fixed part. */
> +lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%ebx),%ecx
> +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +
> +0:
> +/* Get mem_lower from Multiboot2 information. */
> +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%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,MB2_tag_type(%ecx)
> +je trampoline_setup
> +
> +/* Go to next Multiboot2 information tag. */
> +add MB2_tag_size(%ecx),%ecx
> +add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +jmp 0b
I'm missing a total size check, matching what meanwhile got added
to the C equivalent(s) of this loop. There's little point in doing it
there if it doesn't also get done here.
> @@ -41,7 +45,16 @@ asm (
> );
>
> typedef unsigned int u32;
> +typedef unsigned long long u64;
> +
> #include "../../../include/xen/multiboot.h"
> +#include "../../../include/xen/multiboot2.h"
> +
> +#define ALIGN_UP(addr, align) \
> +(((addr) + (typeof(addr))(align) - 1) &
> ~((typeof(addr))(align) - 1))
What is the left typeof() needed for here? (I can see the point of
the right one.)
> +static multiboot_info_t *mbi2_mbi(u32 mbi_in)
> +{
> +const multiboot2_memory_map_t *mmap_src;
> +const multiboot2_tag_t *tag;
> +/* Do not complain that mbi_out_mods is not initialized. */
> +module_t *mbi_out_mods = (module_t *)0;
D
[Xen-devel] [PATCH v3 08/16] 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 --- v3 - suggestions/fixes: - reorder reloc() arguments (suggested by Jan Beulich), - remove .L from multiboot2 header labels (suggested by Andrew Cooper, Jan Beulich and Konrad Rzeszutek Wilk), - take into account alignment when skipping multiboot2 fixed part (suggested by Konrad Rzeszutek Wilk), - create modules data if modules count != 0 (suggested by Jan Beulich), - improve macros (suggested by Jan Beulich), - reduce number of casts (suggested by Jan Beulich), - use const if possible (suggested by Jan Beulich), - drop static and __used__ attribute from reloc() (suggested by Jan Beulich), - remove isolated/stray __packed attribute from multiboot2_memory_map_t type definition (suggested by Jan Beulich), - reformat xen/include/xen/multiboot2.h (suggested by Konrad Rzeszutek Wilk), - improve comments (suggested by Konrad Rzeszutek Wilk), - remove hard tabs (suggested by Jan Beulich and Konrad Rzeszutek Wilk). v2 - suggestions/fixes: - generate multiboot2 header using macros (suggested by Jan Beulich), - improve comments (suggested by Jan Beulich), - simplify assembly in xen/arch/x86/boot/head.S (suggested by Jan Beulich), - do not include include/xen/compiler.h in xen/arch/x86/boot/reloc.c (suggested by Jan Beulich), - do not read data beyond the end of Multiboot2 information (suggested by Jan Beulich). v2 - not fixed yet: - dynamic dependency generation for xen/arch/x86/boot/reloc.S; this requires more work; I am not sure that it pays because potential patch requires more changes than addition of just multiboot2.h to Makefile (suggested by Jan Beulich), - isolated/stray __packed attribute usage for multiboot2_memory_map_t (suggested by Jan Beulich). --- xen/arch/x86/boot/Makefile|3 +- xen/arch/x86/boot/head.S | 106 +-- xen/arch/x86/boot/reloc.c | 152 - xen/arch/x86/x86_64/asm-offsets.c |7 ++ xen/include/xen/multiboot2.h | 169 + 5 files changed, 427 insertions(+), 10 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..06893d8 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/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 1ff5937..e46d691 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 @@ -19,6 +20,28 @@ #define BOOT_PSEUDORM_CS 0x0020 #define BOOT_PSEUDORM_DS 0x0028 +#define MB2_HT(name) (MULTIBOOT2_HEADER_TAG_##name) +#define MB2_TT(name) (MULTIBOOT2_TAG_TYPE_##name) + +.macro mb2ht_args arg, args:vararg +.long \arg +.ifnb \args +mb2ht_args \args +.endif +.endm + +.macro mb2ht_init type, req, args:vararg +.align MULTIBOOT2_TAG_ALIGN +.Lmb2ht_init_start\@: +.short \type +.short \req +.long .Lmb2ht_init_end\@ - .Lmb2ht_init_start\@ +.ifnb \args +mb2ht_args \args +.endif +.Lmb2ht_init_end\@: +.endm + ENTRY(start) jmp __start @@ -34,6 +57,42 @@ multiboot1_header_start: /*** MULTIBOOT1 HEADER / .long -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS) multiboot1_header_end: +/*** MULTIBOOT2 HEADER / +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S file. */ +.align MULTIBOOT2_HEADER_ALIGN + +multiboot2_header_start: +/* Magic number indicating a Multiboot2 header. */ +.long MULTIBOOT2_HEADER_MAGIC +/* Architecture: i386. */ +.long MULTIBOOT2_ARCHITECTURE_I386 +/* Multiboot2 header length. */ +.long multiboot2_header_end - multiboot2_header_start +/* Multiboot2 header checksum. */ +.long -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \ +(multiboot2_header_end - multiboot2_header_start)) + +/* Multiboot2 information request tag. */ +mb2ht_init MB2_HT(INFORMATION_REQUEST), MB2_HT(REQUIRED), \ + MB2_TT(BASIC_MEMINFO), MB2_TT(MMAP) + +/* Align modules at page boundry. */ +mb2ht_init MB2_HT(MODU
