[PATCH] Improve FreeDOS direct loading support compatibility.

[PATCH] Improve FreeDOS direct loading support compatibility.

[C. Masloch]

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

Hello,

A bit about me first; feel free to skip this paragraph. I know 86 assembly  
language well and have been involved in FreeDOS kernel development. On the  
freedos-user list I read that GRUB 2.00 added FreeDOS direct loading  
support. I have reviewed GRUB's implementation of the FreeDOS load  
protocol.

The attached patch improves compatibility to the FreeDOS kernel and its  
original loader as well as potential compatibility to other DOS-like  
kernels and loaders. In particular, some requirements of the current  
EDR-DOS load protocol are implemented by this as well (though possibly not  
all of them) without harming the loading of FreeDOS kernels.

Regards,
Chris Masloch

[-- Attachment #2: improve-freedos-compatibility.diff --]
[-- Type: application/octet-stream, Size: 7265 bytes --]

Subject: [PATCH] Improve FreeDOS direct loading support compatibility.
From: "C. Masloch" <pushbx@38.de>
Date: Sat Jun 30 06:46:08 2012 +0200

 ChangeLog                          |  15 ++++++
 grub-core/lib/i386/relocator.c     |   2 +
 grub-core/lib/i386/relocator16.S   |   5 ++
 grub-core/loader/i386/pc/freedos.c |  83 +++++++++++++++++++++++++++++++++++--
 include/grub/i386/relocator.h      |   1 +
 5 files changed, 101 insertions(+), 5 deletions(-)

# HG changeset patch
# User C. Masloch <pushbx@38.de>
# Date 1341031568 -7200
# Node ID 100d87aefb9b101d05dc614ead36cbcbcd5f4692
# Parent  cd406008554c8411969b8b5ab13e7b58d3f084a8
Improve FreeDOS direct loading support compatibility.

diff --git a/ChangeLog b/ChangeLog
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2012-06-30  C. Masloch  <pushbx@38.de>
+
+	Improve FreeDOS direct loading support compatibility.
+
+	* include/grub/i386/relocator.h (grub_relocator16_state):
+	New member ebp.
+	* grub-core/lib/i386/relocator.c (grub_relocator16_ebp): New extern
+	variable.
+	(grub_relocator16_boot): Handle %ebp.
+	* grub-core/lib/i386/relocator16.S: Likewise.
+	* grub-core/loader/i386/pc/freedos.c:
+	Load BPB to pass kernel which partition to load from.
+	Check that kernel file is not too large.
+	Set register dl to BIOS unit number as well.
+
 2012-06-27  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* configure.ac: Bump version to 2.00.
diff --git a/grub-core/lib/i386/relocator.c b/grub-core/lib/i386/relocator.c
--- a/grub-core/lib/i386/relocator.c
+++ b/grub-core/lib/i386/relocator.c
@@ -54,6 +54,7 @@
 extern grub_uint32_t grub_relocator16_edx;
 extern grub_uint32_t grub_relocator16_ebx;
 extern grub_uint32_t grub_relocator16_esi;
+extern grub_uint32_t grub_relocator16_ebp;
 
 extern grub_uint16_t grub_relocator16_keep_a20_enabled;
 
@@ -225,6 +226,7 @@
   grub_relocator16_ss = state.ss;
   grub_relocator16_sp = state.sp;
 
+  grub_relocator16_ebp = state.ebp;
   grub_relocator16_ebx = state.ebx;
   grub_relocator16_edx = state.edx;
   grub_relocator16_esi = state.esi;
diff --git a/grub-core/lib/i386/relocator16.S b/grub-core/lib/i386/relocator16.S
--- a/grub-core/lib/i386/relocator16.S
+++ b/grub-core/lib/i386/relocator16.S
@@ -259,6 +259,11 @@
 VARIABLE(grub_relocator16_ebx)
 	.long	0
 
+	/* movl imm32, %ebp.  */
+	.byte	0x66, 0xbd
+VARIABLE(grub_relocator16_ebp)
+	.long	0
+
 	/* Cleared direction flag is of no problem with any current
 	   payload and makes this implementation easier.  */
 	cld
diff --git a/grub-core/loader/i386/pc/freedos.c b/grub-core/loader/i386/pc/freedos.c
--- a/grub-core/loader/i386/pc/freedos.c
+++ b/grub-core/loader/i386/pc/freedos.c
@@ -32,6 +32,7 @@
 #include <grub/video.h>
 #include <grub/mm.h>
 #include <grub/cpu/relocator.h>
+#include <grub/machine/chainloader.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -40,8 +41,23 @@
 static grub_uint32_t ebx = 0xffffffff;
 
 #define GRUB_FREEDOS_SEGMENT         0x60
+#define GRUB_FREEDOS_ADDR            (GRUB_FREEDOS_SEGMENT<<4)
 #define GRUB_FREEDOS_STACK_SEGMENT         0x1fe0
-#define GRUB_FREEDOS_STACK_POINTER         0x8000
+#define GRUB_FREEDOS_STACK_BPB_POINTER     0x7c00
+#define GRUB_FREEDOS_BPB_ADDR        ((GRUB_FREEDOS_STACK_SEGMENT<<4) \
+                                      +GRUB_FREEDOS_STACK_BPB_POINTER)
+
+/* FreeDOS boot.asm passes register sp as exactly this. Importantly,
+   it must point below the BPB (to avoid overwriting any of it). */
+#define GRUB_FREEDOS_STACK_POINTER         (GRUB_FREEDOS_STACK_BPB_POINTER \
+                                            -0x60)
+
+/* In this, the additional 8192 bytes are the stack reservation; the
+   remaining parts trivially give the maximum allowed size. */
+#define GRUB_FREEDOS_MAX_SIZE        ((GRUB_FREEDOS_STACK_SEGMENT<<4) \
+                                      +GRUB_FREEDOS_STACK_POINTER \
+                                      -GRUB_FREEDOS_ADDR \
+                                      -8192)
 
 static grub_err_t
 grub_freedos_boot (void)
@@ -49,14 +65,30 @@
   struct grub_relocator16_state state = { 
     .cs = GRUB_FREEDOS_SEGMENT,
     .ip = 0,
-    .ds = 0,
+
+    /* This is not strictly necessary for the current FreeDOS kernel
+       but improves potential compatibility with the current EDR-DOS
+       load protocol.
+       There is no harm in setting this. */
+    .ds = GRUB_FREEDOS_STACK_SEGMENT,
     .es = 0,
     .fs = 0,
     .gs = 0,
     .ss = GRUB_FREEDOS_STACK_SEGMENT,
     .sp = GRUB_FREEDOS_STACK_POINTER,
+    .ebp = GRUB_FREEDOS_STACK_BPB_POINTER,
     .ebx = ebx,
-    .edx = 0,
+
+    /* This is not strictly necessary for the current FreeDOS kernel
+       but is crucial for potential compatibility with the load
+       protocols of eg the current EDR-DOS kernel, or other DOS-like
+       kernels and loaders.
+       (Among those, FreeDOS's load protocol must be considered a
+       special case in that it doesn't require register dl to pass
+       the unit number. Incidentally, the current FreeDOS boot.asm
+       does pass it in both registers.)
+       There is no harm in setting this. */
+    .edx = ebx,
     .a20 = 1
   };
   grub_video_set_mode ("text", 0, 0);
@@ -79,8 +111,9 @@
 {
   grub_file_t file = 0;
   grub_err_t err;
-  void *kernelsys;
+  void *bs, *kernelsys;
   grub_size_t kernelsyssize;
+  grub_device_t dev;
 
   if (argc == 0)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
@@ -95,12 +128,52 @@
   if (! file)
     goto fail;
 
+  {
+    grub_relocator_chunk_t ch;
+    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_BPB_ADDR,
+					   GRUB_DISK_SECTOR_SIZE);
+    if (err)
+      goto fail;
+    bs = get_virtual_current_address (ch);
+  }
+
   ebx = grub_get_root_biosnumber ();
+  dev = grub_device_open (0);
+
+  if (dev && dev->disk)
+    {
+      err = grub_disk_read (dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, bs);
+      if (err)
+	{
+	  grub_device_close (dev);
+	  goto fail;
+	}
+      grub_chainloader_patch_bpb (bs, dev, ebx);
+    }
+
+  if (dev)
+    grub_device_close (dev);
 
   kernelsyssize = grub_file_size (file);
+
+  /* This check could be considered optional, but it provides a more
+     specific error message than grub_relocator_alloc_chunk_addr would,
+     and additionally it insures that a little is set aside for the
+     initial stack as well.
+     Quirkily, because of its size constraints FreeDOS's original loader
+     doesn't perform such a check at all (and crashes instead). The file
+     size limit is documented though. */
+  if (kernelsyssize > GRUB_FREEDOS_MAX_SIZE)
+    {
+      (void)grub_error (GRUB_ERR_BAD_OS,
+		        "file `%s' is too large for a valid"
+		        " FreeDOS kernel.sys", argv[0]);
+      goto fail;
+    }
+
   {
     grub_relocator_chunk_t ch;
-    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_SEGMENT << 4,
+    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_ADDR,
 					   kernelsyssize);
     if (err)
       goto fail;
diff --git a/include/grub/i386/relocator.h b/include/grub/i386/relocator.h
--- a/include/grub/i386/relocator.h
+++ b/include/grub/i386/relocator.h
@@ -49,6 +49,7 @@
   grub_uint32_t ebx;
   grub_uint32_t edx;
   grub_uint32_t esi;
+  grub_uint32_t ebp;
   int a20;
 };
 

Re: [PATCH] Improve FreeDOS direct loading support compatibility.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 30.06.2012 07:43, C. Masloch wrote:

>  #define GRUB_FREEDOS_SEGMENT         0x60
> +#define GRUB_FREEDOS_ADDR            (GRUB_FREEDOS_SEGMENT<<4)
>  #define GRUB_FREEDOS_STACK_SEGMENT         0x1fe0
> -#define GRUB_FREEDOS_STACK_POINTER         0x8000
> +#define GRUB_FREEDOS_STACK_BPB_POINTER     0x7c00
> +#define GRUB_FREEDOS_BPB_ADDR        ((GRUB_FREEDOS_STACK_SEGMENT<<4) \
> +                                      +GRUB_FREEDOS_STACK_BPB_POINTER)
> +

Please put spaces around operators.

> +
> +    /* This is not strictly necessary for the current FreeDOS kernel
> +       but improves potential compatibility with the current EDR-DOS
> +       load protocol.

DR-DOS is less-known proprietary software which we usually don't support
as policy. While it's ok to set those registers, please avoid
referencing DR-DOS directly, even in comments.

> +  /* This check could be considered optional, but it provides a more
> +     specific error message than grub_relocator_alloc_chunk_addr would,
> +     and additionally it insures that a little is set aside for the
> +     initial stack as well.
> +     Quirkily, because of its size constraints FreeDOS's original loader
> +     doesn't perform such a check at all (and crashes instead). The file
> +     size limit is documented though. */
> +  if (kernelsyssize > GRUB_FREEDOS_MAX_SIZE)
> +    {
> +      (void)grub_error (GRUB_ERR_BAD_OS,
> +		        "file `%s' is too large for a valid"
> +		        " FreeDOS kernel.sys", argv[0]);

No need of void cast here.
-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] Improve FreeDOS direct loading support compatibility.

[C. Masloch]

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


Thanks for the help. The attached patch includes spaces around all  
operators, doesn't include the void cast, and doesn't mention (E)DR-DOS  
any longer. I recently learned that the dl register requirement is present  
in prior releases of the FreeDOS kernel too, still called DOS-C. These  
DOS-C releases already are under GPLv2+ so I hope it is okay to mention  
them in the comment, as the attached patch does.

Regards,
Chris

[-- Attachment #2: improve-freedos-compatibility-2.diff --]
[-- Type: application/octet-stream, Size: 7257 bytes --]

Subject: [PATCH] Improve FreeDOS direct loading support compatibility
Date: Sun, 08 Jul 2012 10:08:29 +0200
From: "C. Masloch" <pushbx@38.de>

 ChangeLog                          |  15 ++++++
 grub-core/lib/i386/relocator.c     |   2 +
 grub-core/lib/i386/relocator16.S   |   5 ++
 grub-core/loader/i386/pc/freedos.c |  82 +++++++++++++++++++++++++++++++++++--
 include/grub/i386/relocator.h      |   1 +
 5 files changed, 100 insertions(+), 5 deletions(-)

# HG changeset patch
# User C. Masloch <pushbx@38.de>
# Date 1341734778 -7200
# Node ID 3c587f7d6d1e80c73ec0c4a664feebb788f00ed1
# Parent  73948d0f69334ef6b60e60c16389742c402c2f3e
Improve FreeDOS direct loading support compatibility.

diff --git a/ChangeLog b/ChangeLog
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2012-06-30  C. Masloch  <pushbx@38.de>
+
+	Improve FreeDOS direct loading support compatibility.
+
+	* include/grub/i386/relocator.h (grub_relocator16_state):
+	New member ebp.
+	* grub-core/lib/i386/relocator.c (grub_relocator16_ebp): New extern
+	variable.
+	(grub_relocator16_boot): Handle %ebp.
+	* grub-core/lib/i386/relocator16.S: Likewise.
+	* grub-core/loader/i386/pc/freedos.c:
+	Load BPB to pass kernel which partition to load from.
+	Check that kernel file is not too large.
+	Set register dl to BIOS unit number as well.
+
 2012-06-27  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* configure.ac: Bump version to 2.00.
diff --git a/grub-core/lib/i386/relocator.c b/grub-core/lib/i386/relocator.c
--- a/grub-core/lib/i386/relocator.c
+++ b/grub-core/lib/i386/relocator.c
@@ -54,6 +54,7 @@
 extern grub_uint32_t grub_relocator16_edx;
 extern grub_uint32_t grub_relocator16_ebx;
 extern grub_uint32_t grub_relocator16_esi;
+extern grub_uint32_t grub_relocator16_ebp;
 
 extern grub_uint16_t grub_relocator16_keep_a20_enabled;
 
@@ -225,6 +226,7 @@
   grub_relocator16_ss = state.ss;
   grub_relocator16_sp = state.sp;
 
+  grub_relocator16_ebp = state.ebp;
   grub_relocator16_ebx = state.ebx;
   grub_relocator16_edx = state.edx;
   grub_relocator16_esi = state.esi;
diff --git a/grub-core/lib/i386/relocator16.S b/grub-core/lib/i386/relocator16.S
--- a/grub-core/lib/i386/relocator16.S
+++ b/grub-core/lib/i386/relocator16.S
@@ -259,6 +259,11 @@
 VARIABLE(grub_relocator16_ebx)
 	.long	0
 
+	/* movl imm32, %ebp.  */
+	.byte	0x66, 0xbd
+VARIABLE(grub_relocator16_ebp)
+	.long	0
+
 	/* Cleared direction flag is of no problem with any current
 	   payload and makes this implementation easier.  */
 	cld
diff --git a/grub-core/loader/i386/pc/freedos.c b/grub-core/loader/i386/pc/freedos.c
--- a/grub-core/loader/i386/pc/freedos.c
+++ b/grub-core/loader/i386/pc/freedos.c
@@ -32,6 +32,7 @@
 #include <grub/video.h>
 #include <grub/mm.h>
 #include <grub/cpu/relocator.h>
+#include <grub/machine/chainloader.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -40,8 +41,23 @@
 static grub_uint32_t ebx = 0xffffffff;
 
 #define GRUB_FREEDOS_SEGMENT         0x60
+#define GRUB_FREEDOS_ADDR            (GRUB_FREEDOS_SEGMENT << 4)
 #define GRUB_FREEDOS_STACK_SEGMENT         0x1fe0
-#define GRUB_FREEDOS_STACK_POINTER         0x8000
+#define GRUB_FREEDOS_STACK_BPB_POINTER     0x7c00
+#define GRUB_FREEDOS_BPB_ADDR        ((GRUB_FREEDOS_STACK_SEGMENT << 4) \
+                                       + GRUB_FREEDOS_STACK_BPB_POINTER)
+
+/* FreeDOS boot.asm passes register sp as exactly this. Importantly,
+   it must point below the BPB (to avoid overwriting any of it). */
+#define GRUB_FREEDOS_STACK_POINTER         (GRUB_FREEDOS_STACK_BPB_POINTER \
+                                             - 0x60)
+
+/* In this, the additional 8192 bytes are the stack reservation; the
+   remaining parts trivially give the maximum allowed size. */
+#define GRUB_FREEDOS_MAX_SIZE        ((GRUB_FREEDOS_STACK_SEGMENT << 4) \
+                                       + GRUB_FREEDOS_STACK_POINTER \
+                                       - GRUB_FREEDOS_ADDR \
+                                       - 8192)
 
 static grub_err_t
 grub_freedos_boot (void)
@@ -49,14 +65,29 @@
   struct grub_relocator16_state state = { 
     .cs = GRUB_FREEDOS_SEGMENT,
     .ip = 0,
-    .ds = 0,
+
+    /* This is not strictly necessary for the current FreeDOS kernel
+       but improves potential compatibility with others.
+       There is no harm in setting this. */
+    .ds = GRUB_FREEDOS_STACK_SEGMENT,
     .es = 0,
     .fs = 0,
     .gs = 0,
     .ss = GRUB_FREEDOS_STACK_SEGMENT,
     .sp = GRUB_FREEDOS_STACK_POINTER,
+    .ebp = GRUB_FREEDOS_STACK_BPB_POINTER,
     .ebx = ebx,
-    .edx = 0,
+
+    /* This is not strictly necessary for the current FreeDOS kernel
+       but is crucial for potential compatibility with the load
+       protocols of other DOS-like kernels and loaders (including
+       the older FreeDOS kernel releases called DOS-C).
+       (Among those, FreeDOS's new load protocol must be considered
+       a special case in that it doesn't require register dl to pass
+       the unit number. Incidentally, the current FreeDOS boot.asm
+       does pass it in both registers.)
+       There is no harm in setting this. */
+    .edx = ebx,
     .a20 = 1
   };
   grub_video_set_mode ("text", 0, 0);
@@ -79,8 +110,9 @@
 {
   grub_file_t file = 0;
   grub_err_t err;
-  void *kernelsys;
+  void *bs, *kernelsys;
   grub_size_t kernelsyssize;
+  grub_device_t dev;
 
   if (argc == 0)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
@@ -95,12 +127,52 @@
   if (! file)
     goto fail;
 
+  {
+    grub_relocator_chunk_t ch;
+    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_BPB_ADDR,
+					   GRUB_DISK_SECTOR_SIZE);
+    if (err)
+      goto fail;
+    bs = get_virtual_current_address (ch);
+  }
+
   ebx = grub_get_root_biosnumber ();
+  dev = grub_device_open (0);
+
+  if (dev && dev->disk)
+    {
+      err = grub_disk_read (dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, bs);
+      if (err)
+	{
+	  grub_device_close (dev);
+	  goto fail;
+	}
+      grub_chainloader_patch_bpb (bs, dev, ebx);
+    }
+
+  if (dev)
+    grub_device_close (dev);
 
   kernelsyssize = grub_file_size (file);
+
+  /* This check could be considered optional, but it provides a more
+     specific error message than grub_relocator_alloc_chunk_addr would,
+     and additionally it insures that a little is set aside for the
+     initial stack as well.
+     Quirkily, because of its size constraints FreeDOS's original loader
+     doesn't perform such a check at all (and crashes instead). The file
+     size limit is documented though. */
+  if (kernelsyssize > GRUB_FREEDOS_MAX_SIZE)
+    {
+      grub_error (GRUB_ERR_BAD_OS,
+		  "file `%s' is too large for a valid"
+		  " FreeDOS kernel.sys", argv[0]);
+      goto fail;
+    }
+
   {
     grub_relocator_chunk_t ch;
-    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_SEGMENT << 4,
+    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_ADDR,
 					   kernelsyssize);
     if (err)
       goto fail;
diff --git a/include/grub/i386/relocator.h b/include/grub/i386/relocator.h
--- a/include/grub/i386/relocator.h
+++ b/include/grub/i386/relocator.h
@@ -49,6 +49,7 @@
   grub_uint32_t ebx;
   grub_uint32_t edx;
   grub_uint32_t esi;
+  grub_uint32_t ebp;
   int a20;
 };
 

Re: [PATCH] Improve FreeDOS direct loading support compatibility.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 08.07.2012 10:28, C. Masloch wrote:

> 
> Thanks for the help. The attached patch includes spaces around all
> operators, doesn't include the void cast, and doesn't mention (E)DR-DOS
> any longer. I recently learned that the dl register requirement is
> present in prior releases of the FreeDOS kernel too, still called DOS-C.
> These DOS-C releases already are under GPLv2+ so I hope it is okay to
> mention them in the comment, as the attached patch does.
> 
> Regards,
> Chris
> 
> improve-freedos-compatibility-2.diff
> 
> 
> Subject: [PATCH] Improve FreeDOS direct loading support compatibility
> Date: Sun, 08 Jul 2012 10:08:29 +0200
> From: "C. Masloch" <pushbx@38.de>
> 
>  ChangeLog                          |  15 ++++++
>  grub-core/lib/i386/relocator.c     |   2 +
>  grub-core/lib/i386/relocator16.S   |   5 ++
>  grub-core/loader/i386/pc/freedos.c |  82 +++++++++++++++++++++++++++++++++++--
>  include/grub/i386/relocator.h      |   1 +
>  5 files changed, 100 insertions(+), 5 deletions(-)
> 
> # HG changeset patch
> # User C. Masloch <pushbx@38.de>
> # Date 1341734778 -7200
> # Node ID 3c587f7d6d1e80c73ec0c4a664feebb788f00ed1
> # Parent  73948d0f69334ef6b60e60c16389742c402c2f3e
> Improve FreeDOS direct loading support compatibility.
> 
> diff --git a/ChangeLog b/ChangeLog
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,18 @@
> +2012-06-30  C. Masloch  <pushbx@38.de>
> +
> +	Improve FreeDOS direct loading support compatibility.
> +
> +	* include/grub/i386/relocator.h (grub_relocator16_state):
> +	New member ebp.
> +	* grub-core/lib/i386/relocator.c (grub_relocator16_ebp): New extern
> +	variable.
> +	(grub_relocator16_boot): Handle %ebp.
> +	* grub-core/lib/i386/relocator16.S: Likewise.
> +	* grub-core/loader/i386/pc/freedos.c:
> +	Load BPB to pass kernel which partition to load from.
> +	Check that kernel file is not too large.
> +	Set register dl to BIOS unit number as well.
> +
>  2012-06-27  Vladimir Serbinenko  <phcoder@gmail.com>
>  
>  	* configure.ac: Bump version to 2.00.
> diff --git a/grub-core/lib/i386/relocator.c b/grub-core/lib/i386/relocator.c
> --- a/grub-core/lib/i386/relocator.c
> +++ b/grub-core/lib/i386/relocator.c
> @@ -54,6 +54,7 @@
>  extern grub_uint32_t grub_relocator16_edx;
>  extern grub_uint32_t grub_relocator16_ebx;
>  extern grub_uint32_t grub_relocator16_esi;
> +extern grub_uint32_t grub_relocator16_ebp;
>  
>  extern grub_uint16_t grub_relocator16_keep_a20_enabled;
>  
> @@ -225,6 +226,7 @@
>    grub_relocator16_ss = state.ss;
>    grub_relocator16_sp = state.sp;
>  
> +  grub_relocator16_ebp = state.ebp;
>    grub_relocator16_ebx = state.ebx;
>    grub_relocator16_edx = state.edx;
>    grub_relocator16_esi = state.esi;
> diff --git a/grub-core/lib/i386/relocator16.S b/grub-core/lib/i386/relocator16.S
> --- a/grub-core/lib/i386/relocator16.S
> +++ b/grub-core/lib/i386/relocator16.S
> @@ -259,6 +259,11 @@
>  VARIABLE(grub_relocator16_ebx)
>  	.long	0
>  
> +	/* movl imm32, %ebp.  */
> +	.byte	0x66, 0xbd
> +VARIABLE(grub_relocator16_ebp)
> +	.long	0
> +
>  	/* Cleared direction flag is of no problem with any current
>  	   payload and makes this implementation easier.  */
>  	cld
> diff --git a/grub-core/loader/i386/pc/freedos.c b/grub-core/loader/i386/pc/freedos.c
> --- a/grub-core/loader/i386/pc/freedos.c
> +++ b/grub-core/loader/i386/pc/freedos.c
> @@ -32,6 +32,7 @@
>  #include <grub/video.h>
>  #include <grub/mm.h>
>  #include <grub/cpu/relocator.h>
> +#include <grub/machine/chainloader.h>
>  
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
> @@ -40,8 +41,23 @@
>  static grub_uint32_t ebx = 0xffffffff;
>  
>  #define GRUB_FREEDOS_SEGMENT         0x60
> +#define GRUB_FREEDOS_ADDR            (GRUB_FREEDOS_SEGMENT << 4)
>  #define GRUB_FREEDOS_STACK_SEGMENT         0x1fe0
> -#define GRUB_FREEDOS_STACK_POINTER         0x8000
> +#define GRUB_FREEDOS_STACK_BPB_POINTER     0x7c00
> +#define GRUB_FREEDOS_BPB_ADDR        ((GRUB_FREEDOS_STACK_SEGMENT << 4) \
> +                                       + GRUB_FREEDOS_STACK_BPB_POINTER)
> +
> +/* FreeDOS boot.asm passes register sp as exactly this. Importantly,
> +   it must point below the BPB (to avoid overwriting any of it). */
> +#define GRUB_FREEDOS_STACK_POINTER         (GRUB_FREEDOS_STACK_BPB_POINTER \
> +                                             - 0x60)
> +
> +/* In this, the additional 8192 bytes are the stack reservation; the
> +   remaining parts trivially give the maximum allowed size. */
> +#define GRUB_FREEDOS_MAX_SIZE        ((GRUB_FREEDOS_STACK_SEGMENT << 4) \
> +                                       + GRUB_FREEDOS_STACK_POINTER \
> +                                       - GRUB_FREEDOS_ADDR \
> +                                       - 8192)
>  
>  static grub_err_t
>  grub_freedos_boot (void)
> @@ -49,14 +65,29 @@
>    struct grub_relocator16_state state = { 
>      .cs = GRUB_FREEDOS_SEGMENT,
>      .ip = 0,
> -    .ds = 0,
> +
> +    /* This is not strictly necessary for the current FreeDOS kernel
> +       but improves potential compatibility with others.
> +       There is no harm in setting this. */
> +    .ds = GRUB_FREEDOS_STACK_SEGMENT,
>      .es = 0,
>      .fs = 0,
>      .gs = 0,
>      .ss = GRUB_FREEDOS_STACK_SEGMENT,
>      .sp = GRUB_FREEDOS_STACK_POINTER,
> +    .ebp = GRUB_FREEDOS_STACK_BPB_POINTER,
>      .ebx = ebx,
> -    .edx = 0,
> +
> +    /* This is not strictly necessary for the current FreeDOS kernel
> +       but is crucial for potential compatibility with the load
> +       protocols of other DOS-like kernels and loaders (including
> +       the older FreeDOS kernel releases called DOS-C).
> +       (Among those, FreeDOS's new load protocol must be considered
> +       a special case in that it doesn't require register dl to pass
> +       the unit number. Incidentally, the current FreeDOS boot.asm
> +       does pass it in both registers.)
> +       There is no harm in setting this. */
> +    .edx = ebx,
>      .a20 = 1
>    };
>    grub_video_set_mode ("text", 0, 0);
> @@ -79,8 +110,9 @@
>  {
>    grub_file_t file = 0;
>    grub_err_t err;
> -  void *kernelsys;
> +  void *bs, *kernelsys;
>    grub_size_t kernelsyssize;
> +  grub_device_t dev;
>  
>    if (argc == 0)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> @@ -95,12 +127,52 @@
>    if (! file)
>      goto fail;
>  
> +  {
> +    grub_relocator_chunk_t ch;
> +    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_BPB_ADDR,
> +					   GRUB_DISK_SECTOR_SIZE);

I don't understand this. Shouldn't it be at 0x7c00 ?
Do you plan to contribute more? If so I'd prefer to make you sign a
copyright assignment, otherwise this patch can go in, once it's clear
with addresses.
-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] Improve FreeDOS direct loading support compatibility.

[C. Masloch]

On 2012-09-09 21:49 +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:

>> +  {
>> +    grub_relocator_chunk_t ch;
>> +    err = grub_relocator_alloc_chunk_addr (rel, &ch,  
>> GRUB_FREEDOS_BPB_ADDR,
>> +					   GRUB_DISK_SECTOR_SIZE);
>
> I don't understand this. Shouldn't it be at 0x7c00 ?

This is intended to be at 0x27a00, not 0x7c00. GRUB_FREEDOS_BPB_ADDR  
should evaluate to 0x27a00. I'll try to explain the reasoning for this -  
if any of the following is unclear, please feel free to ask for  
clarification. Sorry that it's so verbose; I suggest you read the summary  
first if you prefer.

-

The FreeDOS kernel's original boot loader (boot/boot.asm in the kernel's  
repo) first relocates itself to linear address 2_7A00h (formed as  
segmented destination address 1FE0h:7C00h, the segmented source address  
being 0000h:7C00h). This is an adjustment from other/earlier loaders that  
left the (boot sector) loader at (linear) 0_7C00h and thus restricted the  
next stage* to be loaded from disk to about 29 KiB, if it is to be loaded  
into contiguous space starting at a lower address (used to be 0_0700h, now  
0_0600h for current FreeDOS kernel releases). With this adjustment, the  
limit is increased to about 157 KiB.

[* Earlier DOS-C (ie FreeDOS kernel) releases loaded a subsequent IPL  
stage first, possibly motivated by this ca. 29 KiB limit as well. Current  
FreeDOS kernel releases typically directly load the whole kernel.]

In principle, GRUB could of course write the BPB anywhere up to the top of  
"conventional memory" (below A_0000h, or below an EBDA if present there)  
and incidentally raise the file size limit. (Or potentially the kernel  
could be loaded to eg 0_8000h with the BPB placed somewhere below that,  
also raising the file size limit - however 0_0600h is clearly documented  
as the canonical load address of the current protocol and the kernel might  
rely on it.)

However, various programs might assume that the FreeDOS load protocol  
places the BPB at exactly 2_7A00h (with ss:bp and ds:bp specifying  
1FE0h:7C00h). I don't know whether the current kernel itself relies on  
this. I'm aware that at least MetaBoot** does search at that particular  
(linear) address, so placing the BPB/sector there is necessary (though not  
sufficient) for it. (MetaBoot's only purpose is to reload a different  
next-stage file, hence loading it from GRUB isn't important because GRUB  
can instead directly load the desired next-stage file. I merely mentioned  
MetaBoot for its assumption that this particular position is implied by  
the load protocol.)

[** MetaBoot belongs to the MetaKern loader for FreeDOS, developed by  
FreeDOS kernel maintainer Eric Auer. It is covered by the GPL 2 with an  
additional loading exception/permission, and distributed as part of  
FreeDOS.]

In addition, arguably there is little use in raising the file size limit  
above what the original loader can work with. This differs from my  
opinion, but might be a relevant view for GRUB to consider.

-

So, summing it all up, the resulting 2_7A00h address of the BPB is to most  
closely mimic the original loader, which arrived there to partially raise  
a limit caused by that loader's size constraints (from the original 29 KiB  
limit to 157 KiB). That level of compatibility might be unnecessary (for  
the kernel itself), but apart from reproducing the original's more recent  
file size limit there is no harm in it.

Note that in GRUB's ntldr.c, the BPB is written at 0_7C00h and no  
equivalent file size limit exists because the next stage (NTLDR) file is  
loaded starting at a higher address, 2_0000h.

> Do you plan to contribute more? If so I'd prefer to make you sign a
> copyright assignment, otherwise this patch can go in, once it's clear
> with addresses.

I could gladly sign an assignment, however I don't yet plan to contribute  
more.

Regards,
Chris Masloch

Re: [PATCH] Improve FreeDOS direct loading support compatibility.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

Committed with minor problems fixed. Thanks.
On 08.07.2012 10:28, C. Masloch wrote:

> diff --git a/ChangeLog b/ChangeLog
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,18 @@
> +2012-06-30  C. Masloch  <pushbx@38.de>
> +
> +	Improve FreeDOS direct loading support compatibility.
> +
> +	* include/grub/i386/relocator.h (grub_relocator16_state):
> +	New member ebp.
> +	* grub-core/lib/i386/relocator.c (grub_relocator16_ebp): New extern
> +	variable.
> +	(grub_relocator16_boot): Handle %ebp.
> +	* grub-core/lib/i386/relocator16.S: Likewise.
> +	* grub-core/loader/i386/pc/freedos.c:
> +	Load BPB to pass kernel which partition to load from.
> +	Check that kernel file is not too large.
> +	Set register dl to BIOS unit number as well.
> +
>  2012-06-27  Vladimir Serbinenko  <phcoder@gmail.com>
>  
>  	* configure.ac: Bump version to 2.00.
> diff --git a/grub-core/lib/i386/relocator.c b/grub-core/lib/i386/relocator.c
> --- a/grub-core/lib/i386/relocator.c
> +++ b/grub-core/lib/i386/relocator.c
> @@ -54,6 +54,7 @@
>  extern grub_uint32_t grub_relocator16_edx;
>  extern grub_uint32_t grub_relocator16_ebx;
>  extern grub_uint32_t grub_relocator16_esi;
> +extern grub_uint32_t grub_relocator16_ebp;
>  
>  extern grub_uint16_t grub_relocator16_keep_a20_enabled;
>  
> @@ -225,6 +226,7 @@
>    grub_relocator16_ss = state.ss;
>    grub_relocator16_sp = state.sp;
>  
> +  grub_relocator16_ebp = state.ebp;
>    grub_relocator16_ebx = state.ebx;
>    grub_relocator16_edx = state.edx;
>    grub_relocator16_esi = state.esi;
> diff --git a/grub-core/lib/i386/relocator16.S b/grub-core/lib/i386/relocator16.S
> --- a/grub-core/lib/i386/relocator16.S
> +++ b/grub-core/lib/i386/relocator16.S
> @@ -259,6 +259,11 @@
>  VARIABLE(grub_relocator16_ebx)
>  	.long	0
>  
> +	/* movl imm32, %ebp.  */
> +	.byte	0x66, 0xbd
> +VARIABLE(grub_relocator16_ebp)
> +	.long	0
> +
>  	/* Cleared direction flag is of no problem with any current
>  	   payload and makes this implementation easier.  */
>  	cld
> diff --git a/grub-core/loader/i386/pc/freedos.c b/grub-core/loader/i386/pc/freedos.c
> --- a/grub-core/loader/i386/pc/freedos.c
> +++ b/grub-core/loader/i386/pc/freedos.c
> @@ -32,6 +32,7 @@
>  #include <grub/video.h>
>  #include <grub/mm.h>
>  #include <grub/cpu/relocator.h>
> +#include <grub/machine/chainloader.h>
>  
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
> @@ -40,8 +41,23 @@
>  static grub_uint32_t ebx = 0xffffffff;
>  
>  #define GRUB_FREEDOS_SEGMENT         0x60
> +#define GRUB_FREEDOS_ADDR            (GRUB_FREEDOS_SEGMENT << 4)
>  #define GRUB_FREEDOS_STACK_SEGMENT         0x1fe0
> -#define GRUB_FREEDOS_STACK_POINTER         0x8000
> +#define GRUB_FREEDOS_STACK_BPB_POINTER     0x7c00
> +#define GRUB_FREEDOS_BPB_ADDR        ((GRUB_FREEDOS_STACK_SEGMENT << 4) \
> +                                       + GRUB_FREEDOS_STACK_BPB_POINTER)
> +
> +/* FreeDOS boot.asm passes register sp as exactly this. Importantly,
> +   it must point below the BPB (to avoid overwriting any of it). */
> +#define GRUB_FREEDOS_STACK_POINTER         (GRUB_FREEDOS_STACK_BPB_POINTER \
> +                                             - 0x60)
> +
> +/* In this, the additional 8192 bytes are the stack reservation; the
> +   remaining parts trivially give the maximum allowed size. */
> +#define GRUB_FREEDOS_MAX_SIZE        ((GRUB_FREEDOS_STACK_SEGMENT << 4) \
> +                                       + GRUB_FREEDOS_STACK_POINTER \
> +                                       - GRUB_FREEDOS_ADDR \
> +                                       - 8192)
>  
>  static grub_err_t
>  grub_freedos_boot (void)
> @@ -49,14 +65,29 @@
>    struct grub_relocator16_state state = { 
>      .cs = GRUB_FREEDOS_SEGMENT,
>      .ip = 0,
> -    .ds = 0,
> +
> +    /* This is not strictly necessary for the current FreeDOS kernel
> +       but improves potential compatibility with others.
> +       There is no harm in setting this. */
> +    .ds = GRUB_FREEDOS_STACK_SEGMENT,
>      .es = 0,
>      .fs = 0,
>      .gs = 0,
>      .ss = GRUB_FREEDOS_STACK_SEGMENT,
>      .sp = GRUB_FREEDOS_STACK_POINTER,
> +    .ebp = GRUB_FREEDOS_STACK_BPB_POINTER,
>      .ebx = ebx,
> -    .edx = 0,
> +
> +    /* This is not strictly necessary for the current FreeDOS kernel
> +       but is crucial for potential compatibility with the load
> +       protocols of other DOS-like kernels and loaders (including
> +       the older FreeDOS kernel releases called DOS-C).
> +       (Among those, FreeDOS's new load protocol must be considered
> +       a special case in that it doesn't require register dl to pass
> +       the unit number. Incidentally, the current FreeDOS boot.asm
> +       does pass it in both registers.)
> +       There is no harm in setting this. */
> +    .edx = ebx,
>      .a20 = 1
>    };
>    grub_video_set_mode ("text", 0, 0);
> @@ -79,8 +110,9 @@
>  {
>    grub_file_t file = 0;
>    grub_err_t err;
> -  void *kernelsys;
> +  void *bs, *kernelsys;
>    grub_size_t kernelsyssize;
> +  grub_device_t dev;
>  
>    if (argc == 0)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> @@ -95,12 +127,52 @@
>    if (! file)
>      goto fail;
>  
> +  {
> +    grub_relocator_chunk_t ch;
> +    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_BPB_ADDR,
> +					   GRUB_DISK_SECTOR_SIZE);
> +    if (err)
> +      goto fail;
> +    bs = get_virtual_current_address (ch);
> +  }
> +
>    ebx = grub_get_root_biosnumber ();
> +  dev = grub_device_open (0);
> +
> +  if (dev && dev->disk)
> +    {
> +      err = grub_disk_read (dev->disk, 0, 0, GRUB_DISK_SECTOR_SIZE, bs);
> +      if (err)
> +	{
> +	  grub_device_close (dev);
> +	  goto fail;
> +	}
> +      grub_chainloader_patch_bpb (bs, dev, ebx);
> +    }
> +
> +  if (dev)
> +    grub_device_close (dev);
>  
>    kernelsyssize = grub_file_size (file);
> +
> +  /* This check could be considered optional, but it provides a more
> +     specific error message than grub_relocator_alloc_chunk_addr would,
> +     and additionally it insures that a little is set aside for the
> +     initial stack as well.
> +     Quirkily, because of its size constraints FreeDOS's original loader
> +     doesn't perform such a check at all (and crashes instead). The file
> +     size limit is documented though. */
> +  if (kernelsyssize > GRUB_FREEDOS_MAX_SIZE)
> +    {
> +      grub_error (GRUB_ERR_BAD_OS,
> +		  "file `%s' is too large for a valid"
> +		  " FreeDOS kernel.sys", argv[0]);
> +      goto fail;
> +    }
> +
>    {
>      grub_relocator_chunk_t ch;
> -    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_SEGMENT << 4,
> +    err = grub_relocator_alloc_chunk_addr (rel, &ch, GRUB_FREEDOS_ADDR,
>  					   kernelsyssize);
>      if (err)
>        goto fail;
> diff --git a/include/grub/i386/relocator.h b/include/grub/i386/relocator.h
> --- a/include/grub/i386/relocator.h
> +++ b/include/grub/i386/relocator.h
> @@ -49,6 +49,7 @@
>    grub_uint32_t ebx;
>    grub_uint32_t edx;
>    grub_uint32_t esi;
> +  grub_uint32_t ebp;
>    int a20;
>  };
>  



-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]