[patch] Bugs in the multiboot example kernel implementation

[Soeren D. Schulze]

I think there are two bugs in the example implementation of the
multiboot standard version 0.6.96.  Both concern the display of the
memory map.

First, the `multiboot_uint64_t' type does not seem to be working as
expected. I always get 32-bit there.  I personally wouldn't use that
type anyway, so I used two 32-bit variables in place.

Second, the memory addresses *must* be zero-padded when chaining the
output of two `itoa' calls this way, so I introduced the (non-standard)
'%X' specifier.

As this is the first time I tinker with GRUB code and also almost the
first time I edit kernel code, it probably breaks lots of conventions...
But I hope it can be turned into something useful.

For the legal stuff:
I hereby put this patch into the public domain; as I'm not sure if this
is possible in Germany, I explicitly grant anyone the right to use this
patch in any thinkable way, including removing any reference to me and
relicensing, as a whole or in parts, and claiming copyright for it.

diff -aur multiboot-0.6.96/docs/kernel.c multiboot-0.6.96.new/docs/kernel.c
--- multiboot-0.6.96/docs/kernel.c      2009-12-24 15:23:08.000000000 +0100
+++ multiboot-0.6.96.new/docs/kernel.c  2010-02-27 02:34:52.000000000 +0100
@@ -141,13 +141,13 @@
           (unsigned long) mmap < mbi->mmap_addr + mbi->mmap_length;
           mmap = (multiboot_memory_map_t *) ((unsigned long) mmap
                                    + mmap->size + sizeof (mmap->size)))
-       printf (" size = 0x%x, base_addr = 0x%x%x,"
-               " length = 0x%x%x, type = 0x%x\n",
+       printf (" size = 0x%x, base_addr = 0x%X%X,"
+               " length = 0x%X%X, type = 0x%x\n",
                (unsigned) mmap->size,
-               mmap->addr >> 32,
-               mmap->addr & 0xffffffff,
-               mmap->len >> 32,
-               mmap->len & 0xffffffff,
+               mmap->addr1,
+               mmap->addr0,
+               mmap->len1,
+               mmap->len0,
                (unsigned) mmap->type);
     }
 }
@@ -185,7 +185,7 @@
       buf++;
       ud = -d;
     }
-  else if (base == 'x')
+  else if (base == 'x' || base == 'X')
     divisor = 16;

   /* Divide UD by DIVISOR until UD == 0.  */
@@ -213,6 +213,23 @@
     }
 }

+static void
+pad0 (char *numstr, int n)
+{
+  int i, len;
+
+  /* strlen() */
+  for (len = 0; numstr[len]; len++);
+
+  /* shift right */
+  for (i = len - 1; i >= 0; i--)
+    numstr[i + n - len] = numstr[i];
+
+  /* pad */
+  for (i = 0; i < n - len; i++)
+    numstr[i] = '0';
+}
+
 /* Put the character C on the screen.  */
 static void
 putchar (int c)
@@ -260,8 +277,11 @@
            case 'd':
            case 'u':
            case 'x':
+           case 'X':
              itoa (buf, c, *((int *) arg++));
              p = buf;
+             if (c == 'X')
+               pad0 (p, 8);
              goto string;
              break;

diff -aur multiboot-0.6.96/docs/kernel.c.texi
multiboot-0.6.96.new/docs/kernel.c.texi
--- multiboot-0.6.96/docs/kernel.c.texi 2009-12-24 15:23:11.000000000 +0100
+++ multiboot-0.6.96.new/docs/kernel.c.texi     2010-02-27
02:35:32.000000000 +0100
@@ -141,13 +141,13 @@
            (unsigned long) mmap < mbi->mmap_addr + mbi->mmap_length;
            mmap = (multiboot_memory_map_t *) ((unsigned long) mmap
                                     + mmap->size + sizeof (mmap->size)))
-        printf (" size = 0x%x, base_addr = 0x%x%x,"
-                " length = 0x%x%x, type = 0x%x\n",
+        printf (" size = 0x%x, base_addr = 0x%X%X,"
+                " length = 0x%X%X, type = 0x%x\n",
                 (unsigned) mmap->size,
-                mmap->addr >> 32,
-                mmap->addr & 0xffffffff,
-                mmap->len >> 32,
-                mmap->len & 0xffffffff,
+                mmap->addr1,
+                mmap->addr0,
+                mmap->len1,
+                mmap->len0,
                 (unsigned) mmap->type);
     @}
 @}
@@ -185,7 +185,7 @@
       buf++;
       ud = -d;
     @}
-  else if (base == 'x')
+  else if (base == 'x' || base == 'X')
     divisor = 16;

   /* @r{Divide UD by DIVISOR until UD == 0.} */
@@ -213,6 +213,23 @@
     @}
 @}

+static void
+pad0 (char *numstr, int n)
address@hidden
+  int i, len;
+
+  /* @r{strlen()} */
+  for (len = 0; numstr[len]; len++);
+
+  /* @r{shift right} */
+  for (i = len - 1; i >= 0; i--)
+    numstr[i + n - len] = numstr[i];
+
+  /* @r{pad} */
+  for (i = 0; i < n - len; i++)
+    numstr[i] = '0';
address@hidden
+
 /* @r{Put the character C on the screen.} */
 static void
 putchar (int c)
@@ -260,8 +277,11 @@
             case 'd':
             case 'u':
             case 'x':
+            case 'X':
               itoa (buf, c, *((int *) arg++));
               p = buf;
+              if (c == 'X')
+                pad0 (p, 8);
               goto string;
               break;

diff -aur multiboot-0.6.96/docs/multiboot.h
multiboot-0.6.96.new/docs/multiboot.h
--- multiboot-0.6.96/docs/multiboot.h   2009-12-24 15:19:40.000000000 +0100
+++ multiboot-0.6.96.new/docs/multiboot.h       2010-02-27 01:50:47.000000000
+0100
@@ -196,12 +196,14 @@
 struct multiboot_mmap_entry
 {
   multiboot_uint32_t size;
-  multiboot_uint64_t addr;
-  multiboot_uint64_t len;
+  multiboot_uint32_t addr0;
+  multiboot_uint32_t addr1;
+  multiboot_uint32_t len0;
+  multiboot_uint32_t len1;
 #define MULTIBOOT_MEMORY_AVAILABLE             1
 #define MULTIBOOT_MEMORY_RESERVED              2
   multiboot_uint32_t type;
-} __attribute__((packed));
+};
 typedef struct multiboot_mmap_entry multiboot_memory_map_t;

 struct multiboot_mod_list
diff -aur multiboot-0.6.96/docs/multiboot.h.texi
multiboot-0.6.96.new/docs/multiboot.h.texi
--- multiboot-0.6.96/docs/multiboot.h.texi      2009-12-24 15:19:41.000000000
+0100
+++ multiboot-0.6.96.new/docs/multiboot.h.texi  2010-02-27
01:51:10.000000000 +0100
@@ -196,12 +196,14 @@
 struct multiboot_mmap_entry
 @{
   multiboot_uint32_t size;
-  multiboot_uint64_t addr;
-  multiboot_uint64_t len;
+  multiboot_uint32_t addr0;
+  multiboot_uint32_t addr1;
+  multiboot_uint32_t len0;
+  multiboot_uint32_t len1;
 #define MULTIBOOT_MEMORY_AVAILABLE              1
 #define MULTIBOOT_MEMORY_RESERVED               2
   multiboot_uint32_t type;
address@hidden __attribute__((packed));
address@hidden;
 typedef struct multiboot_mmap_entry multiboot_memory_map_t;

 struct multiboot_mod_list
diff -aur multiboot-0.6.96/docs/multiboot.info
multiboot-0.6.96.new/docs/multiboot.info
--- multiboot-0.6.96/docs/multiboot.info        2009-12-24 16:38:18.000000000 
+0100
+++ multiboot-0.6.96.new/docs/multiboot.info    2010-02-27
02:35:32.000000000 +0100
@@ -1,4 +1,4 @@
-This is multiboot.info, produced by makeinfo version 4.11 from
+This is multiboot.info, produced by makeinfo version 4.13 from
 multiboot.texi.

 Copyright (C) 1995,96 Bryan Ford <address@hidden>
@@ -1269,12 +1269,14 @@
      struct multiboot_mmap_entry
      {
        multiboot_uint32_t size;
-       multiboot_uint64_t addr;
-       multiboot_uint64_t len;
+       multiboot_uint32_t addr0;
+       multiboot_uint32_t addr1;
+       multiboot_uint32_t len0;
+       multiboot_uint32_t len1;
      #define MULTIBOOT_MEMORY_AVAILABLE              1
      #define MULTIBOOT_MEMORY_RESERVED               2
        multiboot_uint32_t type;
-     } __attribute__((packed));
+     };
      typedef struct multiboot_mmap_entry multiboot_memory_map_t;

      struct multiboot_mod_list
@@ -1551,13 +1553,13 @@
                 (unsigned long) mmap < mbi->mmap_addr + mbi->mmap_length;
                 mmap = (multiboot_memory_map_t *) ((unsigned long) mmap
                                          + mmap->size + sizeof
(mmap->size)))
-             printf (" size = 0x%x, base_addr = 0x%x%x,"
-                     " length = 0x%x%x, type = 0x%x\n",
+             printf (" size = 0x%x, base_addr = 0x%X%X,"
+                     " length = 0x%X%X, type = 0x%x\n",
                      (unsigned) mmap->size,
-                     mmap->addr >> 32,
-                     mmap->addr & 0xffffffff,
-                     mmap->len >> 32,
-                     mmap->len & 0xffffffff,
+                     mmap->addr1,
+                     mmap->addr0,
+                     mmap->len1,
+                     mmap->len0,
                      (unsigned) mmap->type);
          }
      }
@@ -1595,7 +1597,7 @@
            buf++;
            ud = -d;
          }
-       else if (base == 'x')
+       else if (base == 'x' || base == 'X')
          divisor = 16;

        /* Divide UD by DIVISOR until UD == 0. */
@@ -1623,6 +1625,23 @@
          }
      }

+     static void
+     pad0 (char *numstr, int n)
+     {
+       int i, len;
+
+       /* strlen() */
+       for (len = 0; numstr[len]; len++);
+
+       /* shift right */
+       for (i = len - 1; i >= 0; i--)
+         numstr[i + n - len] = numstr[i];
+
+       /* pad */
+       for (i = 0; i < n - len; i++)
+         numstr[i] = '0';
+     }
+
      /* Put the character C on the screen. */
      static void
      putchar (int c)
@@ -1670,8 +1689,11 @@
                  case 'd':
                  case 'u':
                  case 'x':
+                 case 'X':
                    itoa (buf, c, *((int *) arg++));
                    p = buf;
+                   if (c == 'X')
+                     pad0 (p, 8);
                    goto string;
                    break;

@@ -1800,11 +1822,11 @@
 Node: I/O restriction technique42685
 Node: Example OS code43902
 Node: multiboot.h45444
-Node: boot.S53515
-Node: kernel.c56668
-Node: Other Multiboot kernels65537
-Node: Example boot loader code65968
-Node: History66494
-Node: Index67716
+Node: boot.S53558
+Node: kernel.c56711
+Node: Other Multiboot kernels65990
+Node: Example boot loader code66421
+Node: History66947
+Node: Index68169
 
 End Tag Table