Enabling DragonFly BSD disklabel64 read support

Enabling DragonFly BSD disklabel64 read support

[Radosław Szymczyszyn]

Hello everyone,

I'd like to extend GRUB to be able to read the DragonFly BSD's
disklabel64 format. On my way there, adding support for disklabel32 -
unreadable by GRUB now, but similar to Free/Open/NetBSD disklabel - is
an option, too.

I've already played with GRUB a bit, run its test suite and prepared
some disk images with MBR and FBSD disklabel, DFly disklabel and DFly
disklabel64. I've also extended the build system to build and link
with libgrubmods.a a new module placed in grub-core/partmap/dfly.c.
These changes aren't available anywhere on the net, yet.

One thing I've found lacking in the documentation is the grub-shell's
requirement for xorriso. It's nothing fancy, but the program is used
on a script line whose both outputs are redirected to /dev/null, so
finding out what's missing might not be that straightforward.

On, to my questions:
- Is there any up-to-date git mirror of GRUB available? Learning bzr
isn't a showstopper, but staying with the tool I know would be
preferable.
- The source code includes some grub_dprintf diagnostics, but neither
the tests nor running grub_shell shows any of this output. What's the
proper way of enabling these diagnostic messages?

Thanks in advance.

Regards,
Radosław Szymczyszyn

Re: Enabling DragonFly BSD disklabel64 read support

[Andrey Borzenkov]

В Tue, 15 Jan 2013 18:48:20 +0100
Radosław Szymczyszyn <lavrin@gmail.com> пишет:

> - Is there any up-to-date git mirror of GRUB available? Learning bzr
> isn't a showstopper, but staying with the tool I know would be
> preferable.

I use git-bzr-ng: https://github.com/termie/git-bzr-ng

it needs a couple of additional Python modules, listed there.

Re: Enabling DragonFly BSD disklabel64 read support

[Radosław Szymczyszyn]

2013/1/15 Andrey Borzenkov <arvidjaar@gmail.com>:
> I use git-bzr-ng: https://github.com/termie/git-bzr-ng

Hmmm, I'm a little worried, as my bzr version is 2.6 and the github
page mentions 2.3 as the newest. The cloning went fine though, so I'll
give it a try. Thanks for the hint.

Re: Enabling DragonFly BSD disklabel64 read support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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


> - The source code includes some grub_dprintf diagnostics, but neither
> the tests nor running grub_shell shows any of this output. What's the
> proper way of enabling these diagnostic messages?

Setting debug variable.
E.g. debug=all or debug=usb

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


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

Re: Enabling DragonFly BSD disklabel64 read support

[Radosław Szymczyszyn]

2013/1/16 Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com>:
> Setting debug variable.
> E.g. debug=all or debug=usb

Thank you.

Re: Enabling DragonFly BSD disklabel64 read support

[Radosław Szymczyszyn]

Hello,

The code to boot from disklabel64 is in place at my git mirror on
bitbucket [1]. Alternatively, for a quick look at the patch I paste it
at the bottom of this mail.

Of course, it's only possible to boot from the UFS filesystem, not
from HAMMER, as GRUB doesn't support the latter.

I tested it on two setups. The first is a Virtualbox drive partitioned
into 4 msdos partitions,
the last of which is subpartitioned using disklabel64. One of the
subpartitions contained a Multiboot example kernel from GRUB
distribution. The other setup is qemu and a small (20MB) file with
msdos+disklabel64 - similarly as the GRUB partmap tests are done. A
short video showing this test is available at [2]. I've written a
similar test, but as Parted can't create disklabel64 tables, I had to
include an exemplar image to test against.

I hope the code will be integrated into GRUB. I'm open to critique in
case of it not being ready for merge in its current state. Should it
be important (license issues?), the structure definitions are based on
disklabel64.h from the DragonFly BSD's source tree - I didn't strip
out the original comments coming from that header file. Everything
else is based on apple.c and bsdlabel.c from grub-core/partmap/.

I've got some questions, too.

1) Firstly, the test I mentioned uses a prepared beforehand image to
test against, but it's the best that can be done without a Linux tool
being able to create disklabel64 labels - is it acceptable?

2) Secondly, I see discrepancies between the ways struct
grub_partition.index is initialized in different modules. Its comment
says it is "the index of the partition in the partition table."

I refer to files under grub-core/partmap. In apple.c it is set like that:

    part.index = partno;

Which would agree with the comment. However, in bsdlabel.c we see:

    pos = sizeof (label) + sector * GRUB_DISK_SECTOR_SIZE;
    ...
    p.offset = pos / GRUB_DISK_SECTOR_SIZE;
    p.index = pos % GRUB_DISK_SECTOR_SIZE;

I suppose it's just used as a temporary for a following
grub_disk_read() call as it's used in a similar way in apple.c.
However, apple.c later resets it as quoted above, while in bsdlabel.c
it just stays at some more or less arbitrary value between 0 and 512.

Which is the correct way? Since both seem to work, as GRUB certainly
is able to read disklabel.

I'm looking forward to the inclusion of this code into GRUB or any
comments before it might be possible.

Regards,
Radosław Szymczyszyn

----
[1]: https://bitbucket.org/erszcz/grub
[2]: http://student.agh.edu.pl/~sradosla/multiboot-example-disklabel64.ogv

----
diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..e107348
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,169 @@
+*.o
+*.yy.?
+*.tab.?
+*.mod
+*.module
+*.marker
+*.lst
+.dirstamp
+.deps-core
+
+.deps-util/
+00_header
+10_linux
+20_linux_xen
+30_os-prober
+40_custom
+41_custom
+Makefile
+Makefile.in
+Makefile.tpl
+Makefile.util.am
+Makefile.utilgcry.def
+aclocal.m4
+autom4te.cache/
+build-aux/compile
+build-aux/config.guess
+build-aux/config.sub
+build-aux/depcomp
+build-aux/install-sh
+build-aux/mdate-sh
+build-aux/missing
+build-aux/texinfo.tex
+config-util.h
+config-util.h.in
+config.h
+config.log
+config.status
+configure
+docs/Makefile
+docs/Makefile.in
+docs/grub-dev.info
+docs/grub.info
+docs/stamp-1
+docs/stamp-vti
+docs/version-dev.texi
+docs/version.texi
+grub-bin2h
+grub-bios-setup
+grub-core/.deps-util/
+grub-core/Makefile
+grub-core/Makefile.core.am
+grub-core/Makefile.gcry.def
+grub-core/Makefile.in
+grub-core/boot.image
+grub-core/boot.img
+grub-core/cdboot.image
+grub-core/cdboot.img
+grub-core/commands/.deps-util/
+grub-core/config.log
+grub-core/disk/.deps-util/
+grub-core/diskboot.image
+grub-core/diskboot.img
+grub-core/fs/.deps-util/
+grub-core/fs/zfs/.deps-util/
+grub-core/gdb_grub
+grub-core/genmod.sh
+grub-core/gensyminfo.sh
+grub-core/gentrigtables
+grub-core/gmodule.pl
+grub-core/gnulib/.deps/
+grub-core/gnulib/Makefile
+grub-core/gnulib/Makefile.in
+grub-core/gnulib/alloca.h
+grub-core/gnulib/arg-nonnull.h
+grub-core/gnulib/c++defs.h
+grub-core/gnulib/charset.alias
+grub-core/gnulib/configmake.h
+grub-core/gnulib/getopt.h
+grub-core/gnulib/langinfo.h
+grub-core/gnulib/libgnu.a
+grub-core/gnulib/ref-add.sed
+grub-core/gnulib/ref-del.sed
+grub-core/gnulib/stdio.h
+grub-core/gnulib/stdlib.h
+grub-core/gnulib/string.h
+grub-core/gnulib/strings.h
+grub-core/gnulib/sys/
+grub-core/gnulib/unistd.h
+grub-core/gnulib/warn-on-use.h
+grub-core/gnulib/wchar.h
+grub-core/gnulib/wctype.h
+grub-core/io/.deps-util/
+grub-core/kern/.deps-util/
+grub-core/kern/emu/.deps-util/
+grub-core/kern/ia64/.deps-util/
+grub-core/kernel.exec
+grub-core/kernel.img
+grub-core/lib/.deps-util/
+grub-core/lib/i386/pc/.deps-util/
+grub-core/lib/libgcrypt-grub/
+grub-core/lib/minilzo/.deps-util/
+grub-core/lib/xzembed/.deps-util/
+grub-core/libgnulib.a
+grub-core/lnxboot.image
+grub-core/lnxboot.img
+grub-core/lzma_decompress.image
+grub-core/lzma_decompress.img
+grub-core/modinfo.sh
+grub-core/normal/.deps-util/
+grub-core/partmap/.deps-util/
+grub-core/pxeboot.image
+grub-core/pxeboot.img
+grub-core/rs_decoder.S
+grub-core/script/.deps-util/
+grub-core/symlist.c
+grub-core/symlist.h
+grub-core/tests/lib/.deps-util/
+grub-core/trigtables.c
+grub-core/unidata.c
+grub-editenv
+grub-fstest
+grub-install
+grub-kbdcomp
+grub-menulst2cfg
+grub-mkconfig
+grub-mkconfig_lib
+grub-mkimage
+grub-mklayout
+grub-mknetdir
+grub-mkpasswd-pbkdf2
+grub-mkrelpath
+grub-mkrescue
+grub-mkstandalone
+grub-ofpathname
+grub-probe
+grub-reboot
+grub-script-check
+grub-set-default
+grub-shell
+grub-shell-tester
+grub-sparc64-setup
+grub_fstest.pp
+grub_fstest_init.c
+include/grub/cpu
+include/grub/gcrypt/g10lib.h
+include/grub/gcrypt/gcrypt.h
+include/grub/machine
+libgrub.pp
+libgrub_a_init.c
+libgrubgcry.a
+libgrubkern.a
+libgrubmods.a
+po/Makefile
+po/Makefile.in
+po/POTFILES
+po/grub.pot
+po/remove-potcdate.sed
+po/stamp-po
+stamp-h
+stamp-h.in
+stamp-h1
+tests/.deps-util/
+tests/lib/.deps-util/
+util/.deps-util/
+util/bash-completion.d/Makefile
+util/bash-completion.d/Makefile.in
+util/bash-completion.d/config.log
+util/bash-completion.d/grub
+util/ieee1275/.deps-util/
diff --git a/INSTALL b/INSTALL
index e325fa2..b92753d 100644
--- a/INSTALL
+++ b/INSTALL
@@ -45,6 +45,7 @@ need the following.
 Prerequisites for make-check:

 * qemu, specifically the binary 'qemu-system-i386'
+* xorriso for grub-mkrescue to work

 Configuring the GRUB
 ====================
diff --git a/Makefile.util.def b/Makefile.util.def
index 3ee5e4e..20783b5 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -116,6 +116,7 @@ library = {
   common = grub-core/partmap/dvh.c;
   common = grub-core/partmap/sunpc.c;
   common = grub-core/partmap/bsdlabel.c;
+  common = grub-core/partmap/dfly.c;
   common = grub-core/script/function.c;
   common = grub-core/script/lexer.c;
   common = grub-core/script/main.c;
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index f4dd645..30f7f1c 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1554,6 +1554,11 @@ module = {
 };

 module = {
+  name = part_dfly;
+  common = partmap/dfly.c;
+};
+
+module = {
   name = msdospart;
   common = parttool/msdospart.c;
 };
diff --git a/grub-core/partmap/apple.c b/grub-core/partmap/apple.c
index c08cae5..1c1d3bc 100644
--- a/grub-core/partmap/apple.c
+++ b/grub-core/partmap/apple.c
@@ -118,7 +118,7 @@ apple_partition_map_iterate (grub_disk_t disk,
   if (grub_be_to_cpu16 (aheader.magic) != GRUB_APPLE_HEADER_MAGIC)
     {
       grub_dprintf ("partition",
-		    "bad magic (found 0x%x; wanted 0x%x\n",
+		    "bad magic (found 0x%x; wanted 0x%x)\n",
 		    grub_be_to_cpu16 (aheader.magic),
 		    GRUB_APPLE_HEADER_MAGIC);
       goto fail;
@@ -138,7 +138,7 @@ apple_partition_map_iterate (grub_disk_t disk,
       if (grub_be_to_cpu16 (apart.magic) != GRUB_APPLE_PART_MAGIC)
 	{
 	  grub_dprintf ("partition",
-			"partition %d: bad magic (found 0x%x; wanted 0x%x\n",
+			"partition %d: bad magic (found 0x%x; wanted 0x%x)\n",
 			partno, grub_be_to_cpu16 (apart.magic),
 			GRUB_APPLE_PART_MAGIC);
 	  break;
diff --git a/grub-core/partmap/dfly.c b/grub-core/partmap/dfly.c
new file mode 100644
index 0000000..8b06145
--- /dev/null
+++ b/grub-core/partmap/dfly.c
@@ -0,0 +1,167 @@
+/* dfly.c - Read DragonFly BSD disklabel64.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2002,2004,2005,2006,2007,2008,2009,2012  Free
Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/disk.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <grub/partition.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+#define GRUB_DFLY_DISKMAGIC64       ((grub_uint32_t)0xc4464c59)
+#ifndef GRUB_DFLY_MAXPARTITIONS64
+#define GRUB_DFLY_MAXPARTITIONS64   16
+#endif
+
+/* Note: offsets are relative to the base of the slice, NOT to
+   pbase.  Unlike 32 bit disklabels the on-disk format for
+   a 64 bit disklabel remains slice-relative.
+
+   An uninitialized partition has a boffset and bsize of 0.
+
+   If fstype is not supported for a live partition it is set
+   to FS_OTHER.  This is typically the case when the filesystem
+   is identified by its uuid. */
+struct grub_dfly_partition64      /* partition table entry */
+{
+  grub_uint64_t   boffset;        /* slice relative offset, in bytes */
+  grub_uint64_t   bsize;          /* size of partition, in bytes */
+  grub_uint8_t    fstype;
+  grub_uint8_t    unused01;       /* reserved, must be 0 */
+  grub_uint8_t    unused02;       /* reserved, must be 0 */
+  grub_uint8_t    unused03;       /* reserved, must be 0 */
+  grub_uint32_t   unused04;       /* reserved, must be 0 */
+  grub_uint32_t   unused05;       /* reserved, must be 0 */
+  grub_uint32_t   unused06;       /* reserved, must be 0 */
+  grub_uint8_t    unused07[16];   /* unused struct uuid type_uuid */
+  grub_uint8_t    unused08[16];   /* unused struct uuid stor_uuid */
+} __attribute__ ((packed));
+
+/* A disklabel64 starts at slice relative offset 0, NOT SECTOR 1.  In
+   otherwords, magic is at byte offset 512 within the slice, regardless
+   of the sector size.
+
+   The reserved0 area is not included in the crc and any kernel writeback
+   of the label will not change the reserved area on-disk.  It is purely
+   a shim to allow us to avoid sector calculations when reading or
+   writing the label.  Since byte offsets are used in our 64 bit disklabel,
+   the entire disklabel and the I/O required to access it becomes
+   sector-agnostic. */
+struct grub_dfly_disklabel64
+{
+  grub_int8_t     reserved0[512]; /* reserved or unused */
+  grub_uint32_t   magic;          /* the magic number */
+  grub_uint32_t   crc;            /* crc32() magic thru last part */
+  grub_uint32_t   align;          /* partition alignment requirement */
+  grub_uint32_t   npartitions;    /* number of partitions */
+  grub_uint8_t    unused01[16];   /* unused struct uuid stor_uuid */
+
+  grub_uint64_t   total_size;     /* total size incl everything (bytes) */
+  grub_uint64_t   bbase;          /* boot area base offset (bytes) */
+                                  /* boot area is pbase - bbase */
+  grub_uint64_t   pbase;          /* first allocatable offset (bytes) */
+  grub_uint64_t   pstop;          /* last allocatable offset+1 (bytes) */
+  grub_uint64_t   abase;          /* location of backup copy if not 0 */
+
+  grub_uint8_t    packname[64];
+  grub_uint8_t    reserved[64];
+
+  /* actually may be more than GRUB_DFLY_MAXPARTITIONS64 */
+  /*struct grub_dfly_partition64 partitions[GRUB_DFLY_MAXPARTITIONS64];*/
+} __attribute__ ((packed));
+
+static struct grub_partition_map grub_dfly_partition_map;
+
+static grub_err_t
+dfly_partition_map_iterate (grub_disk_t disk,
+                            int (*hook) (grub_disk_t disk,
+                                         const grub_partition_t partition))
+{
+  struct grub_partition part;
+  struct grub_dfly_disklabel64 label;
+  struct grub_dfly_partition64 dpart;
+  unsigned partno, pos;
+
+  part.partmap = &grub_dfly_partition_map;
+
+  if (grub_disk_read (disk, 0, 0, sizeof (label), &label))
+    return grub_errno;
+
+  if (grub_le_to_cpu32 (label.magic) != GRUB_DFLY_DISKMAGIC64)
+    {
+      grub_dprintf ("partition",
+                    "bad magic (found 0x%x; wanted 0x%x)\n",
+                    grub_le_to_cpu32 (label.magic),
+                    GRUB_DFLY_DISKMAGIC64);
+      goto fail;
+    }
+
+  pos = sizeof (label);
+
+  for (partno = 0;
+       partno < grub_le_to_cpu32 (label.npartitions); ++partno)
+    {
+      grub_disk_addr_t sector = pos / GRUB_DISK_SECTOR_SIZE;
+      grub_off_t       offset = pos % GRUB_DISK_SECTOR_SIZE;
+      pos += sizeof (dpart);
+      if (grub_disk_read (disk, sector, offset, sizeof (dpart), &dpart))
+	return grub_errno;
+
+      grub_dprintf ("partition",
+                    "partition %2d: offset 0x%llx, size 0x%llx\n",
+		    part.number,
+                    (unsigned long long) grub_le_to_cpu64 (dpart.boffset),
+                    (unsigned long long) grub_le_to_cpu64 (dpart.bsize));
+
+      /* Is partition initialized? */
+      if (! (dpart.boffset && dpart.bsize))
+	continue;
+
+      part.number = partno;
+      part.start  = grub_le_to_cpu64 (dpart.boffset) / GRUB_DISK_SECTOR_SIZE;
+      part.len    = grub_le_to_cpu64 (dpart.bsize) / GRUB_DISK_SECTOR_SIZE;
+      part.offset = pos / GRUB_DISK_SECTOR_SIZE;
+      part.index  = partno;
+
+      if (hook (disk, &part))
+	return grub_errno;
+    }
+
+  return GRUB_ERR_NONE;
+
+fail:
+  return grub_error (GRUB_ERR_BAD_PART_TABLE, "disklabel64 not found");
+}
+
+/* Partition map type.  */
+static struct grub_partition_map grub_dfly_partition_map =
+{
+    .name = "dfly",
+    .iterate = dfly_partition_map_iterate,
+};
+
+GRUB_MOD_INIT(part_dfly)
+{
+  grub_partition_map_register (&grub_dfly_partition_map);
+}
+
+GRUB_MOD_FINI(part_dfly)
+{
+  grub_partition_map_unregister (&grub_dfly_partition_map);
+}
diff --git a/tests/dfly-mbr-mbexample.img b/tests/dfly-mbr-mbexample.img
new file mode 100644
index 0000000..997e1f5
Binary files /dev/null and b/tests/dfly-mbr-mbexample.img differ
diff --git a/tests/partmap_test.in b/tests/partmap_test.in
index 1507220..6875547 100644
--- a/tests/partmap_test.in
+++ b/tests/partmap_test.in
@@ -28,6 +28,12 @@ create_disk_image () {
     dd if=/dev/zero of="${name}" bs=512 count=1 seek=$((size * 2048 -
1)) status=noxfer > /dev/null
 }

+create_dfly_image () {
+    name="$1"
+    rm -f ${name}
+    dd if="@builddir@/tests/dfly-mbr-mbexample.img" of=${name} > /dev/null
+}
+
 check_output () {
     outfile=$1
     shift
@@ -289,3 +295,13 @@ create_disk_image "${imgfile}" 128
 ${parted} -a none -s "${imgfile}" mklabel mac mkpart a 1M 10M mkpart
b 10M 20M mkpart c 20M 30M mkpart d 30M 40M mkpart e 40M 50M mkpart f
50M 60M
 list_parts part_apple "${imgfile}" "${outfile}"
 check_output "${outfile}" $disk $disk,apple1 $disk,apple2
$disk,apple4 $disk,apple5 $disk,apple6 $disk,apple7 $disk,apple8
+
+#
+# DragonFly BSD disklabel64
+#
+
+echo "Checking DragonFly BSD disklabel64..."
+
+create_dfly_image "${imgfile}"
+list_parts part_dfly "${imgfile}" "${outfile}"
+check_output "${outfile}" $disk $disk,msdos1 $disk,msdos1,dfly1
$disk,msdos1,dfly2 $disk,msdos1,dfly3

Re: Enabling DragonFly BSD disklabel64 read support

[Andrey Borzenkov]

В Mon, 21 Jan 2013 22:17:16 +0100
> ----
> diff --git a/.gitignore b/.gitignore
> new file mode 100644
> index 0000000..e107348
> --- /dev/null
> +++ b/.gitignore

As much as would welcome it - grub is using bzr as VCS, so it probably
is not in place here. I suggest you add .gitignore to .gitignore :)


> diff --git a/grub-core/partmap/apple.c b/grub-core/partmap/apple.c
> index c08cae5..1c1d3bc 100644
> --- a/grub-core/partmap/apple.c
> +++ b/grub-core/partmap/apple.c
> @@ -118,7 +118,7 @@ apple_partition_map_iterate (grub_disk_t disk,
>    if (grub_be_to_cpu16 (aheader.magic) != GRUB_APPLE_HEADER_MAGIC)
>      {
>        grub_dprintf ("partition",
> -		    "bad magic (found 0x%x; wanted 0x%x\n",
> +		    "bad magic (found 0x%x; wanted 0x%x)\n",
>  		    grub_be_to_cpu16 (aheader.magic),
>  		    GRUB_APPLE_HEADER_MAGIC);
>        goto fail;
> @@ -138,7 +138,7 @@ apple_partition_map_iterate (grub_disk_t disk,
>        if (grub_be_to_cpu16 (apart.magic) != GRUB_APPLE_PART_MAGIC)
>  	{
>  	  grub_dprintf ("partition",
> -			"partition %d: bad magic (found 0x%x; wanted 0x%x\n",
> +			"partition %d: bad magic (found 0x%x; wanted 0x%x)\n",
>  			partno, grub_be_to_cpu16 (apart.magic),
>  			GRUB_APPLE_PART_MAGIC);
>  	  break;

I would say this should go in separate clean up patch; I wonder if
Vladimir accepts mixing several unrelated changes in one commit.

> diff --git a/grub-core/partmap/dfly.c b/grub-core/partmap/dfly.c
[...]
> +static grub_err_t
> +dfly_partition_map_iterate (grub_disk_t disk,
> +                            int (*hook) (grub_disk_t disk,
> +                                         const grub_partition_t partition))
[...]
> +
> +      if (hook (disk, &part))

You need to update it to new hook API (added third parameter).

Re: Enabling DragonFly BSD disklabel64 read support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 21.01.2013 22:17, Radosław Szymczyszyn wrote:

> Hello,
> 
> The code to boot from disklabel64 is in place at my git mirror on
> bitbucket [1]. Alternatively, for a quick look at the patch I paste it
> at the bottom of this mail.
> 
> Of course, it's only possible to boot from the UFS filesystem, not
> from HAMMER, as GRUB doesn't support the latter.
> 
> I tested it on two setups. The first is a Virtualbox drive partitioned
> into 4 msdos partitions,
> the last of which is subpartitioned using disklabel64. One of the
> subpartitions contained a Multiboot example kernel from GRUB
> distribution. The other setup is qemu and a small (20MB) file with
> msdos+disklabel64 - similarly as the GRUB partmap tests are done. A
> short video showing this test is available at [2]. I've written a
> similar test, but as Parted can't create disklabel64 tables, I had to
> include an exemplar image to test against.

I think this is an issue to be taken to parted guys. If we can't have
dragonfly label handling in it, we'll look for other solutions but first
we have to try.

> 
> I hope the code will be integrated into GRUB. I'm open to critique in
> case of it not being ready for merge in its current state. Should it
> be important (license issues?), the structure definitions are based on
> disklabel64.h from the DragonFly BSD's source tree - I didn't strip
> out the original comments coming from that header file. Everything
> else is based on apple.c and bsdlabel.c from grub-core/partmap/.
> 

Can you keep it in a separate file and put original license header
there? Which exact license is it? There are different BSD license and
not all of them are compatible with GPLv2.

> I've got some questions, too.
> 
> 1) Firstly, the test I mentioned uses a prepared beforehand image to
> test against, but it's the best that can be done without a Linux tool
> being able to create disklabel64 labels - is it acceptable?
> 

Answered above.

> 2) Secondly, I see discrepancies between the ways struct
> grub_partition.index is initialized in different modules. Its comment
> says it is "the index of the partition in the partition table."
> 
> I refer to files under grub-core/partmap. In apple.c it is set like that:
> 
>     part.index = partno;
> 
> Which would agree with the comment. However, in bsdlabel.c we see:
> 
>     pos = sizeof (label) + sector * GRUB_DISK_SECTOR_SIZE;
>     ...
>     p.offset = pos / GRUB_DISK_SECTOR_SIZE;
>     p.index = pos % GRUB_DISK_SECTOR_SIZE;
> 
> I suppose it's just used as a temporary for a following
> grub_disk_read() call as it's used in a similar way in apple.c.
> However, apple.c later resets it as quoted above, while in bsdlabel.c
> it just stays at some more or less arbitrary value between 0 and 512.
> 
> Which is the correct way? Since both seem to work, as GRUB certainly
> is able to read disklabel.
> 

It's just that a code aware of that particular table would be able to
quickly and reliably read the entry from the disk

> diff --git a/.gitignore b/.gitignore
> new file mode 100644
> index 0000000..e107348
> --- /dev/null
> +++ b/.gitignore

Andrey has already commented on this one

> --- a/INSTALL
> +++ b/INSTALL
> @@ -45,6 +45,7 @@ need the following.
>  Prerequisites for make-check:
> 
>  * qemu, specifically the binary 'qemu-system-i386'
> +* xorriso for grub-mkrescue to work


On this as well

> diff --git a/Makefile.util.def b/Makefile.util.def
> index 3ee5e4e..20783b5 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -116,6 +116,7 @@ library = {
>    common = grub-core/partmap/dvh.c;
>    common = grub-core/partmap/sunpc.c;
>    common = grub-core/partmap/bsdlabel.c;
> +  common = grub-core/partmap/dfly.c;
>    common = grub-core/script/function.c;
>    common = grub-core/script/lexer.c;
>    common = grub-core/script/main.c;
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index f4dd645..30f7f1c 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1554,6 +1554,11 @@ module = {
>  };
> 
>  module = {
> +  name = part_dfly;
> +  common = partmap/dfly.c;
> +};
> +
> +module = {
>    name = msdospart;
>    common = parttool/msdospart.c;
>  };
> diff --git a/grub-core/partmap/apple.c b/grub-core/partmap/apple.c
> index c08cae5..1c1d3bc 100644
> --- a/grub-core/partmap/apple.c
> +++ b/grub-core/partmap/apple.c
> @@ -118,7 +118,7 @@ apple_partition_map_iterate (grub_disk_t disk,
>    if (grub_be_to_cpu16 (aheader.magic) != GRUB_APPLE_HEADER_MAGIC)
>      {
>        grub_dprintf ("partition",
> -		    "bad magic (found 0x%x; wanted 0x%x\n",
> +		    "bad magic (found 0x%x; wanted 0x%x)\n",

Likewise

> diff --git a/grub-core/partmap/dfly.c b/grub-core/partmap/dfly.c
> new file mode 100644
> index 0000000..8b06145
> --- /dev/null
> +++ b/grub-core/partmap/dfly.c
> @@ -0,0 +1,167 @@
> +/* dfly.c - Read DragonFly BSD disklabel64.  */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2002,2004,2005,2006,2007,2008,2009,2012  Free
> Software Foundation, Inc.

Use correct years, not just a copypaste.

> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/disk.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/partition.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +#define GRUB_DFLY_DISKMAGIC64       ((grub_uint32_t)0xc4464c59)
> +#ifndef GRUB_DFLY_MAXPARTITIONS64
> +#define GRUB_DFLY_MAXPARTITIONS64   16
> +#endif
> +
> +/* Note: offsets are relative to the base of the slice, NOT to
> +   pbase.  Unlike 32 bit disklabels the on-disk format for
> +   a 64 bit disklabel remains slice-relative.
> +
> +   An uninitialized partition has a boffset and bsize of 0.
> +
> +   If fstype is not supported for a live partition it is set
> +   to FS_OTHER.  This is typically the case when the filesystem
> +   is identified by its uuid. */
> +struct grub_dfly_partition64      /* partition table entry */
> +{
> +  grub_uint64_t   boffset;        /* slice relative offset, in bytes */
> +  grub_uint64_t   bsize;          /* size of partition, in bytes */
> +  grub_uint8_t    fstype;
> +  grub_uint8_t    unused01;       /* reserved, must be 0 */
> +  grub_uint8_t    unused02;       /* reserved, must be 0 */
> +  grub_uint8_t    unused03;       /* reserved, must be 0 */
> +  grub_uint32_t   unused04;       /* reserved, must be 0 */
> +  grub_uint32_t   unused05;       /* reserved, must be 0 */
> +  grub_uint32_t   unused06;       /* reserved, must be 0 */
> +  grub_uint8_t    unused07[16];   /* unused struct uuid type_uuid */
> +  grub_uint8_t    unused08[16];   /* unused struct uuid stor_uuid */
> +} __attribute__ ((packed));
> +
> +/* A disklabel64 starts at slice relative offset 0, NOT SECTOR 1.  In
> +   otherwords, magic is at byte offset 512 within the slice, regardless
> +   of the sector size.
> +
> +   The reserved0 area is not included in the crc and any kernel writeback
> +   of the label will not change the reserved area on-disk.  It is purely
> +   a shim to allow us to avoid sector calculations when reading or
> +   writing the label.  Since byte offsets are used in our 64 bit disklabel,
> +   the entire disklabel and the I/O required to access it becomes
> +   sector-agnostic. */
> +struct grub_dfly_disklabel64
> +{
> +  grub_int8_t     reserved0[512]; /* reserved or unused */
> +  grub_uint32_t   magic;          /* the magic number */
> +  grub_uint32_t   crc;            /* crc32() magic thru last part */
> +  grub_uint32_t   align;          /* partition alignment requirement */
> +  grub_uint32_t   npartitions;    /* number of partitions */
> +  grub_uint8_t    unused01[16];   /* unused struct uuid stor_uuid */
> +
> +  grub_uint64_t   total_size;     /* total size incl everything (bytes) */
> +  grub_uint64_t   bbase;          /* boot area base offset (bytes) */
> +                                  /* boot area is pbase - bbase */
> +  grub_uint64_t   pbase;          /* first allocatable offset (bytes) */
> +  grub_uint64_t   pstop;          /* last allocatable offset+1 (bytes) */
> +  grub_uint64_t   abase;          /* location of backup copy if not 0 */
> +
> +  grub_uint8_t    packname[64];
> +  grub_uint8_t    reserved[64];
> +
> +  /* actually may be more than GRUB_DFLY_MAXPARTITIONS64 */
> +  /*struct grub_dfly_partition64 partitions[GRUB_DFLY_MAXPARTITIONS64];*/
> +} __attribute__ ((packed));
> +
> +static struct grub_partition_map grub_dfly_partition_map;
> +
> +static grub_err_t
> +dfly_partition_map_iterate (grub_disk_t disk,
> +                            int (*hook) (grub_disk_t disk,
> +                                         const grub_partition_t partition))
> +{
> +  struct grub_partition part;
> +  struct grub_dfly_disklabel64 label;
> +  struct grub_dfly_partition64 dpart;
> +  unsigned partno, pos;
> +
> +  part.partmap = &grub_dfly_partition_map;
> +
> +  if (grub_disk_read (disk, 0, 0, sizeof (label), &label))
> +    return grub_errno;
> +
> +  if (grub_le_to_cpu32 (label.magic) != GRUB_DFLY_DISKMAGIC64)
> +    {

Please use
label.magic != grub_le_to_cpu32_compile_time (GRUB_DFLY_DISKMAGIC64)

> +      grub_dprintf ("partition",
> +                    "bad magic (found 0x%x; wanted 0x%x)\n",
> +                    grub_le_to_cpu32 (label.magic),
> +                    GRUB_DFLY_DISKMAGIC64);
> +      goto fail;
> +    }
> +
> +  pos = sizeof (label);
> +
> +  for (partno = 0;
> +       partno < grub_le_to_cpu32 (label.npartitions); ++partno)
> +    {
> +      grub_disk_addr_t sector = pos / GRUB_DISK_SECTOR_SIZE;
> +      grub_off_t       offset = pos % GRUB_DISK_SECTOR_SIZE;

Use bit operations rather than divisions

> +      pos += sizeof (dpart);
> +      if (grub_disk_read (disk, sector, offset, sizeof (dpart), &dpart))
> +	return grub_errno;
> +
> +      grub_dprintf ("partition",
> +                    "partition %2d: offset 0x%llx, size 0x%llx\n",
> +		    part.number,
> +                    (unsigned long long) grub_le_to_cpu64 (dpart.boffset),
> +                    (unsigned long long) grub_le_to_cpu64 (dpart.bsize));
> +

We have PRIxGRUB_UINT64_T for this.

> +      /* Is partition initialized? */
> +      if (! (dpart.boffset && dpart.bsize))
> +	continue;
> +
> +      part.number = partno;
> +      part.start  = grub_le_to_cpu64 (dpart.boffset) / GRUB_DISK_SECTOR_SIZE;
> +      part.len    = grub_le_to_cpu64 (dpart.bsize) / GRUB_DISK_SECTOR_SIZE;
> +      part.offset = pos / GRUB_DISK_SECTOR_SIZE;

Likewise.

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


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