Move BSD and multiboot boot helper out of the kernel to their respective modules

Move BSD and multiboot boot helper out of the kernel to their respective modules

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 221 bytes --]

Hello, I was looking through the code and have seen that bsd and
multiboot code uses helpers in the kernel. In this patch I propose to
move these function to their respective modules
Thanks
Vladimir 'phcoder' Serbinenko


[-- Attachment #2: mbbsd.diff --]
[-- Type: text/x-diff, Size: 13121 bytes --]

Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revision 1962)
+++ conf/i386-pc.rmk	(working copy)
@@ -219,11 +219,13 @@
 
 # For _multiboot.mod.
 _multiboot_mod_SOURCES = loader/i386/pc/multiboot.c \
+			 loader/i386/multiboot.S \
                          loader/i386/pc/multiboot2.c \
                          loader/multiboot2.c \
                          loader/multiboot_loader.c
 _multiboot_mod_CFLAGS = $(COMMON_CFLAGS)
 _multiboot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+_multiboot_mod_ASFLAGS = $(COMMON_ASFLAGS)
 
 # For multiboot.mod.
 multiboot_mod_SOURCES = loader/multiboot_loader_normal.c 
@@ -282,10 +284,12 @@
 aout_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 # For _bsd.mod
-_bsd_mod_SOURCES = loader/i386/bsd.c
+_bsd_mod_SOURCES = loader/i386/bsd.c loader/i386/bsd_helper.S
 _bsd_mod_CFLAGS = $(COMMON_CFLAGS)
 _bsd_mod_LDFLAGS = $(COMMON_LDFLAGS)
+_bsd_mod_ASFLAGS = $(COMMON_ASFLAGS)
 
+
 # For bsd.mod
 bsd_mod_SOURCES = loader/i386/bsd_normal.c
 bsd_mod_CFLAGS = $(COMMON_CFLAGS)
Index: conf/i386-coreboot.rmk
===================================================================
--- conf/i386-coreboot.rmk	(revision 1962)
+++ conf/i386-coreboot.rmk	(working copy)
@@ -139,10 +139,12 @@
 # For _multiboot.mod.
 _multiboot_mod_SOURCES = loader/i386/pc/multiboot.c \
                          loader/i386/pc/multiboot2.c \
+			 loader/i386/multiboot.S \
                          loader/multiboot2.c \
                          loader/multiboot_loader.c
 _multiboot_mod_CFLAGS = $(COMMON_CFLAGS)
 _multiboot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+_multiboot_mod_ASFLAGS = $(COMMON_ASFLAGS)
 
 # For multiboot.mod.
 multiboot_mod_SOURCES = loader/multiboot_loader_normal.c 
Index: conf/i386-ieee1275.rmk
===================================================================
--- conf/i386-ieee1275.rmk	(revision 1962)
+++ conf/i386-ieee1275.rmk	(working copy)
@@ -113,10 +113,12 @@
 
 # For _multiboot.mod.
 _multiboot_mod_SOURCES = loader/ieee1275/multiboot2.c \
+			 loader/i386/multiboot.S \
 			 loader/multiboot2.c \
 			 loader/multiboot_loader.c
 _multiboot_mod_CFLAGS = $(COMMON_CFLAGS)
 _multiboot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+_multiboot_mod_ASFLAGS = $(COMMON_ASFLAGS)
 
 # For multiboot.mod.
 multiboot_mod_SOURCES = loader/multiboot_loader_normal.c 
Index: kern/i386/loader.S
===================================================================
--- kern/i386/loader.S	(revision 1962)
+++ kern/i386/loader.S	(working copy)
@@ -117,127 +117,3 @@
 linux_setup_seg:
 	.word	0
 	.code32
-
-
-/*
- * This starts the multiboot kernel.
- */
-
-VARIABLE(grub_multiboot_payload_size)
-	.long	0
-VARIABLE(grub_multiboot_payload_orig)
-	.long	0
-VARIABLE(grub_multiboot_payload_dest)
-	.long	0
-VARIABLE(grub_multiboot_payload_entry_offset)
-	.long	0
-
-/*
- * The relocators below understand the following parameters:
- * ecx:	Size of the block to be copied.
- * esi:	Where to copy from (always lowest address, even if we're relocating
- *      backwards).
- * edi:	Where to copy to (likewise).
- * edx:	Offset of the entry point (relative to the beginning of the block).
- */
-VARIABLE(grub_multiboot_forward_relocator)
-	/* Add entry offset.  */
-	addl	%edi, %edx
-
-	/* Forward copy.  */
-	cld
-	rep
-	movsb
-
-	jmp	*%edx
-VARIABLE(grub_multiboot_forward_relocator_end)
-
-VARIABLE(grub_multiboot_backward_relocator)
-	/* Add entry offset (before %edi is mangled).  */
-	addl	%edi, %edx
-
-	/* Backward movsb is implicitly off-by-one.  compensate that.  */
-	decl	%esi
-	decl	%edi
-
-	/* Backward copy.  */
-	std
-	addl	%ecx, %esi
-	addl	%ecx, %edi
-	rep
-	movsb
-
-	jmp	*%edx
-VARIABLE(grub_multiboot_backward_relocator_end)
-
-FUNCTION(grub_multiboot_real_boot)
-	/* Push the entry address on the stack.  */
-	pushl	%eax
-	/* Move the address of the multiboot information structure to ebx.  */
-	movl	%edx,%ebx
-
-	/* Unload all modules and stop the floppy driver.  */
-	call	EXT_C(grub_dl_unload_all)
-	call	EXT_C(grub_stop_floppy)
-
-	/* Interrupts should be disabled.  */
-	cli
-
-	/* Where do we copy what from.  */
-	movl	EXT_C(grub_multiboot_payload_size), %ecx
-	movl	EXT_C(grub_multiboot_payload_orig), %esi
-	movl	EXT_C(grub_multiboot_payload_dest), %edi
-	movl	EXT_C(grub_multiboot_payload_entry_offset), %edx
-
-	/* Move the magic value into eax.  */
-	movl	$MULTIBOOT_MAGIC2, %eax
-				
-	/* Jump to the relocator.  */
-	popl	%ebp
-	jmp 	*%ebp
-	
-/*
- * This starts the multiboot 2 kernel.
- */
-
-FUNCTION(grub_multiboot2_real_boot)
-        /* Push the entry address on the stack.  */
-        pushl   %eax
-        /* Move the address of the multiboot information structure to ebx.  */
-        movl    %edx,%ebx
-
-        /* Unload all modules and stop the floppy driver.  */
-        call    EXT_C(grub_dl_unload_all)
-        call    EXT_C(grub_stop_floppy)
-
-        /* Interrupts should be disabled.  */
-        cli
-
-        /* Move the magic value into eax and jump to the kernel.  */
-        movl    $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
-        popl    %ecx
-        jmp     *%ecx
-
-/*
- * Use cdecl calling convention for *BSD kernels.
- */
-
-FUNCTION(grub_unix_real_boot)
-
-        call    EXT_C(grub_dl_unload_all)
-        call    EXT_C(grub_stop_floppy)
-
-	/* Interrupts should be disabled.  */
-        cli
-
-	/* Discard `grub_unix_real_boot' return address.  */
-        popl    %eax
-
-        /* Fetch `entry' address ...  */
-        popl	%eax
-
-        /*
-         * ... and put our return address in its place. The kernel will
-         * ignore it, but it expects %esp to point to it.
-         */
-        call	*%eax
Index: include/grub/i386/loader.h
===================================================================
--- include/grub/i386/loader.h	(revision 1962)
+++ include/grub/i386/loader.h	(working copy)
@@ -22,7 +22,6 @@
 #include <grub/types.h>
 #include <grub/err.h>
 #include <grub/symbol.h>
-#include <grub/multiboot.h>
 
 extern grub_uint32_t EXPORT_VAR(grub_linux_prot_size);
 extern char *EXPORT_VAR(grub_linux_tmp_addr);
@@ -33,31 +32,12 @@
 
 grub_err_t EXPORT_FUNC(grub_linux_boot) (void);
 
-/* The asm part of the multiboot loader.  */
-void EXPORT_FUNC(grub_multiboot_real_boot) (grub_addr_t entry,
-					    struct grub_multiboot_info *mbi)
-     __attribute__ ((noreturn));
-void EXPORT_FUNC(grub_multiboot2_real_boot) (grub_addr_t entry,
-                                             struct grub_multiboot_info *mbi)
-     __attribute__ ((noreturn));
-void EXPORT_FUNC(grub_unix_real_boot) (grub_addr_t entry, ...)
-     __attribute__ ((cdecl,noreturn));
-
-extern grub_addr_t EXPORT_VAR(grub_multiboot_payload_orig);
-extern grub_addr_t EXPORT_VAR(grub_multiboot_payload_dest);
-extern grub_size_t EXPORT_VAR(grub_multiboot_payload_size);
-extern grub_uint32_t EXPORT_VAR(grub_multiboot_payload_entry_offset);
-
 /* It is necessary to export these functions, because normal mode commands
    reuse rescue mode commands.  */
 void grub_rescue_cmd_linux (int argc, char *argv[]);
 void grub_rescue_cmd_initrd (int argc, char *argv[]);
 
-extern grub_uint8_t EXPORT_VAR(grub_multiboot_forward_relocator);
-extern grub_uint8_t EXPORT_VAR(grub_multiboot_forward_relocator_end);
-extern grub_uint8_t EXPORT_VAR(grub_multiboot_backward_relocator);
-extern grub_uint8_t EXPORT_VAR(grub_multiboot_backward_relocator_end);
+void EXPORT_FUNC(grub_stop_floppy) (void);
 
-#define RELOCATOR_SIZEOF(x)	(&grub_multiboot_##x##_relocator_end - &grub_multiboot_##x##_relocator)
 
 #endif /* ! GRUB_LOADER_CPU_HEADER */
Index: include/grub/i386/bsd.h
===================================================================
--- include/grub/i386/bsd.h	(revision 1962)
+++ include/grub/i386/bsd.h	(working copy)
@@ -229,4 +229,7 @@
 void grub_rescue_cmd_freebsd_loadenv (int argc, char *argv[]);
 void grub_rescue_cmd_freebsd_module (int argc, char *argv[]);
 
+void grub_unix_real_boot (grub_addr_t entry, ...)
+     __attribute__ ((cdecl,noreturn));
+
 #endif /* ! GRUB_BSD_CPU_HEADER */
Index: include/grub/i386/multiboot.h
===================================================================
--- include/grub/i386/multiboot.h	(revision 0)
+++ include/grub/i386/multiboot.h	(revision 0)
@@ -0,0 +1,24 @@
+#ifndef GRUB_MULTIBOOT_CPU_HEADER
+#define GRUB_MULTIBOOT_CPU_HEADER	1
+
+/* The asm part of the multiboot loader.  */
+void grub_multiboot_real_boot (grub_addr_t entry,
+			       struct grub_multiboot_info *mbi)
+     __attribute__ ((noreturn));
+void grub_multiboot2_real_boot (grub_addr_t entry,
+				struct grub_multiboot_info *mbi)
+     __attribute__ ((noreturn));
+
+extern grub_addr_t grub_multiboot_payload_orig;
+extern grub_addr_t grub_multiboot_payload_dest;
+extern grub_size_t grub_multiboot_payload_size;
+extern grub_uint32_t grub_multiboot_payload_entry_offset;
+
+extern grub_uint8_t grub_multiboot_forward_relocator;
+extern grub_uint8_t grub_multiboot_forward_relocator_end;
+extern grub_uint8_t grub_multiboot_backward_relocator;
+extern grub_uint8_t grub_multiboot_backward_relocator_end;
+
+#define RELOCATOR_SIZEOF(x)	(&grub_multiboot_##x##_relocator_end - &grub_multiboot_##x##_relocator)
+
+#endif /* ! GRUB_MULTIBOOT_CPU_HEADER */
Index: loader/i386/multiboot.S
===================================================================
--- loader/i386/multiboot.S	(revision 0)
+++ loader/i386/multiboot.S	(revision 0)
@@ -0,0 +1,103 @@
+#include <grub/symbol.h>
+#include <multiboot.h>
+#include <multiboot2.h>
+	
+	.p2align	2	/* force 4-byte alignment */
+
+/*
+ * This starts the multiboot kernel.
+ */
+
+VARIABLE(grub_multiboot_payload_size)
+	.long	0
+VARIABLE(grub_multiboot_payload_orig)
+	.long	0
+VARIABLE(grub_multiboot_payload_dest)
+	.long	0
+VARIABLE(grub_multiboot_payload_entry_offset)
+	.long	0
+
+/*
+ * The relocators below understand the following parameters:
+ * ecx:	Size of the block to be copied.
+ * esi:	Where to copy from (always lowest address, even if we're relocating
+ *      backwards).
+ * edi:	Where to copy to (likewise).
+ * edx:	Offset of the entry point (relative to the beginning of the block).
+ */
+
+VARIABLE(grub_multiboot_forward_relocator)
+	/* Add entry offset.  */
+	addl	%edi, %edx
+
+	/* Forward copy.  */
+	cld
+	rep
+	movsb
+
+	jmp	*%edx
+VARIABLE(grub_multiboot_forward_relocator_end)
+
+VARIABLE(grub_multiboot_backward_relocator)
+	/* Add entry offset (before %edi is mangled).  */
+	addl	%edi, %edx
+
+	/* Backward movsb is implicitly off-by-one.  compensate that.  */
+	decl	%esi
+	decl	%edi
+
+	/* Backward copy.  */
+	std
+	addl	%ecx, %esi
+	addl	%ecx, %edi
+	rep
+	movsb
+
+	jmp	*%edx
+VARIABLE(grub_multiboot_backward_relocator_end)
+
+FUNCTION(grub_multiboot_real_boot)
+	/* Push the entry address on the stack.  */
+	pushl	%eax
+	/* Move the address of the multiboot information structure to ebx.  */
+	movl	%edx,%ebx
+
+	/* Unload all modules and stop the floppy driver.  */
+/*	call	EXT_C(grub_dl_unload_all)
+	call	EXT_C(grub_stop_floppy)*/
+
+	/* Interrupts should be disabled.  */
+	cli
+
+	/* Where do we copy what from.  */
+	movl	EXT_C(grub_multiboot_payload_size), %ecx
+	movl	EXT_C(grub_multiboot_payload_orig), %esi
+	movl	EXT_C(grub_multiboot_payload_dest), %edi
+	movl	EXT_C(grub_multiboot_payload_entry_offset), %edx
+
+	/* Move the magic value into eax.  */
+	movl	$MULTIBOOT_MAGIC2, %eax
+				
+	/* Jump to the relocator.  */
+	popl	%ebp
+	jmp 	*%ebp
+
+/*
+ * This starts the multiboot 2 kernel.
+ */
+
+FUNCTION(grub_multiboot2_real_boot)
+        /* Push the entry address on the stack.  */
+        pushl   %eax
+        /* Move the address of the multiboot information structure to ebx.  */
+        movl    %edx,%ebx
+
+        call    EXT_C(grub_stop_floppy)
+
+        /* Interrupts should be disabled.  */
+        cli
+
+        /* Move the magic value into eax and jump to the kernel.  */
+        movl    $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
+        popl    %ecx
+        jmp     *%ecx
Index: loader/i386/pc/multiboot.c
===================================================================
--- loader/i386/pc/multiboot.c	(revision 1962)
+++ loader/i386/pc/multiboot.c	(working copy)
@@ -32,6 +32,7 @@
 #include <grub/multiboot.h>
 #include <grub/machine/init.h>
 #include <grub/machine/memory.h>
+#include <grub/cpu/multiboot.h>
 #include <grub/elf.h>
 #include <grub/aout.h>
 #include <grub/file.h>
Index: loader/i386/bsd_helper.S
===================================================================
--- loader/i386/bsd_helper.S	(revision 0)
+++ loader/i386/bsd_helper.S	(revision 0)
@@ -0,0 +1,26 @@
+#include <grub/symbol.h>
+	
+	.p2align	2	/* force 4-byte alignment */
+
+/*
+ * Use cdecl calling convention for *BSD kernels.
+ */
+
+FUNCTION(grub_unix_real_boot)
+
+        call    EXT_C(grub_stop_floppy)
+
+	/* Interrupts should be disabled.  */
+        cli
+
+	/* Discard `grub_unix_real_boot' return address.  */
+        popl    %eax
+
+        /* Fetch `entry' address ...  */
+        popl	%eax
+
+        /*
+         * ... and put our return address in its place. The kernel will
+         * ignore it, but it expects %esp to point to it.
+         */
+        call	*%eax

Re: Move BSD and multiboot boot helper out of the kernel to their respective modules

[phcoder]

Hello. On the IRC discussion it was said that removing call to 
grub_dl_unload_all_modules isn't a good thing. I would like to discuss 
this on the list
1) Call to this functon is present only on i386-pc
2) I would suggest an interface for preboot hooks if modules needs to do 
something before booting
3) If it's really needed to unload modules one could add a function 
grub_dl_unload_all_modules_except (grub_dl_t *mod)
Then the module before booting kernel would call this function and after 
it no external calls are allowed.
4) Not being able to add a new kernel type by module alone is IMHO a 
severe limitation to module infrastructure
Thanks
Vladimir 'phcoder' Serbinenko
phcoder wrote:
> Hello, I was looking through the code and have seen that bsd and
> multiboot code uses helpers in the kernel. In this patch I propose to
> move these function to their respective modules
> Thanks
> Vladimir 'phcoder' Serbinenko
> 

Re: Move BSD and multiboot boot helper out of the kernel to their respective modules

[Robert Millan]

On Thu, Jan 29, 2009 at 04:57:08PM +0100, phcoder wrote:
> Index: loader/i386/bsd_helper.S
> ===================================================================
> --- loader/i386/bsd_helper.S	(revision 0)
> +++ loader/i386/bsd_helper.S	(revision 0)
> @@ -0,0 +1,26 @@
> +#include <grub/symbol.h>
> +	
> +	.p2align	2	/* force 4-byte alignment */
> +
> +/*
> + * Use cdecl calling convention for *BSD kernels.
> + */
> +
> +FUNCTION(grub_unix_real_boot)
> +
> +        call    EXT_C(grub_stop_floppy)
> +
> +	/* Interrupts should be disabled.  */
> +        cli
> +
> +	/* Discard `grub_unix_real_boot' return address.  */
> +        popl    %eax
> +
> +        /* Fetch `entry' address ...  */
> +        popl	%eax
> +
> +        /*
> +         * ... and put our return address in its place. The kernel will
> +         * ignore it, but it expects %esp to point to it.
> +         */
> +        call	*%eax

This is small enough that it could make sense to use inline asm instead,
so we can avoid a separate file.  What do you think?

Also, it'd be better if you can submit two separate patches, one for BSD
and one for Multiboot.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Move BSD and multiboot boot helper out of the kernel to their respective modules

[Robert Millan]

On Wed, Feb 04, 2009 at 01:08:26PM +0100, phcoder wrote:
> Hello. On the IRC discussion it was said that removing call to 
> grub_dl_unload_all_modules isn't a good thing. I would like to discuss 
> this on the list
> 1) Call to this functon is present only on i386-pc
> 2) I would suggest an interface for preboot hooks if modules needs to do 
> something before booting
> 3) If it's really needed to unload modules one could add a function 
> grub_dl_unload_all_modules_except (grub_dl_t *mod)
> Then the module before booting kernel would call this function and after 
> it no external calls are allowed.
> 4) Not being able to add a new kernel type by module alone is IMHO a 
> severe limitation to module infrastructure
> Thanks

Or perhaps we could avoid grub_dl_unload_all_modules altogether.  For example,
gfxterm might think that graphical mode needs to be disabled when it's
unloaded, but the OS loader knows better.  You could want to tell your OS
about the framebuffer state so it can be used by it without glitches.

The same would apply, for example, to stopping the floppy drive.  Instead
of waiting for the drive to tell us it has stopped, we could tell the OS
it's still spinning if our loader knows how to do that (and the OS knows
how to receive this information).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Move BSD and multiboot boot helper out of the kernel to their respective modules

[phcoder]

> This is small enough that it could make sense to use inline asm instead,
> so we can avoid a separate file.  What do you think?
I agree with this. Actually in similar place in my xnu booting I used 4 
lines of inline assembly. The only reason I kept it in .S is that I was 
in one of my lazy states. In few days I'll send two patches with your 
suggestions
> Also, it'd be better if you can submit two separate patches, one for BSD
> and one for Multiboot.

Thanks
Vladimir 'phcoder' Serbinenko

Re: Move BSD and multiboot boot helper out of the kernel to their respective modules

[phcoder]

I agree. Such workarounds take boot time and create situations when 
first action of kernel is to undo last action of booter.
Sometimes it can't be avoided but if we can we should try. Also it adds 
some flexibility since last action of booter is
performed in nice unix-like environment and kernel starts in "tabula 
rasa". An example is my system with widescreen. To init
widescreen special commands specific to my framebuffer controller are 
necessary. So if grub2 sets it kernel can start in
undistorted video mode.

Thanks
Vladimir 'phcoder' Serbinenko


> Or perhaps we could avoid grub_dl_unload_all_modules altogether.  For example,
> gfxterm might think that graphical mode needs to be disabled when it's
> unloaded, but the OS loader knows better.  You could want to tell your OS
> about the framebuffer state so it can be used by it without glitches.
> 
> The same would apply, for example, to stopping the floppy drive.  Instead
> of waiting for the drive to tell us it has stopped, we could tell the OS
> it's still spinning if our loader knows how to do that (and the OS knows
> how to receive this information).
>