Re: Enabling DragonFly BSD disklabel64 read support

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

signature.asc
Description: OpenPGP digital signature