|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 08/23] x86: add multiboot2 protocol support
On Mon, Aug 10, 2015 at 03:17:48PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Jul 20, 2015 at 04:29:03PM +0200, 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 <daniel.kiper@xxxxxxxxxx>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> > 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:
>
> You have two 'v2'
Yep, but it says "not fixed in v2".
> > - 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 | 105 +++++++++++++++++++++--
> > xen/arch/x86/boot/reloc.c | 146 +++++++++++++++++++++++++++++++-
> > xen/arch/x86/x86_64/asm-offsets.c | 7 ++
> > xen/include/xen/multiboot2.h | 169
> > +++++++++++++++++++++++++++++++++++++
> > 5 files changed, 420 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 77e7da9..57197db 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -1,5 +1,6 @@
> > #include <xen/config.h>
> > #include <xen/multiboot.h>
> > +#include <xen/multiboot2.h>
> > #include <public/xen.h>
> > #include <asm/asm_defns.h>
> > #include <asm/desc.h>
> > @@ -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
> > + 0:
> > + .short \type
> > + .short \req
> > + .long 1f - 0b
> > + .ifnb \args
> > + mb2ht_args \args
> > + .endif
> > + 1:
> > + .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
> > +
> > +.Lmultiboot2_header:
>
> How come you use .L? It makes this hidden while the multiboot1 headers
> are visible? Makes it a bit harder to see the contents of this under
> an debugger.
Good point. IIRC, Jan asked about that. I will remove .L if he does not object.
> > + /* 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 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)
> > +.Lmultiboot2_header_end:
> > +
> > .section .init.rodata, "a", @progbits
> > .align 4
> >
> > @@ -82,10 +141,48 @@ __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(%ebx),%ecx
>
> Is there any point of double checking the aligment? That is
> and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>
> ?
>
> Looking at the grub code it requests the memory to be aligned properly
> so in practice this will never fail. But since somebody might for fun
> allocate the structure to not be aligned and the fixed part could finish
> on non-aligned space..
You are right, we should check it. multiboot2 spec states:
3.4.2 Basic tags structure
Boot information consists of fixed part and a series of tags. Its start
is 8-bytes aligned.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |