[PATCH] Remove root drive support

[PATCH] Remove root drive support

[Pavel Roskin]

Root drive support is not actually used by the installer.  Root drive is
superseded by grub_prefix with UUID, which is much more flexible, is
already used by grub-setup for cross-drive installs and doesn't require
a single byte in the boot sector.

ChangeLog:
	* boot/i386/pc/boot.S: Remove root_drive.  Ensure that
	boot_drive_check is where boot.h expects it to be.  Don't save
	%dx, we only need %dl and we never change it.
	* boot/i386/pc/cdboot.S: Don't set the root drive.
	* boot/i386/pc/pxeboot.S: Likewise.
	* include/grub/i386/pc/boot.h: Remove
	GRUB_BOOT_MACHINE_ROOT_DRIVE, adjust
	GRUB_BOOT_MACHINE_DRIVE_CHECK.
	* include/grub/i386/pc/kernel.h: Remove grub_root_drive.
	* kern/i386/pc/init.c (make_install_device): Remove references
	to grub_root_drive.
	* kern/i386/pc/startup.S: Likewise.
	* util/i386/pc/grub-setup.c (setup): Don't set root_drive.
---

 boot/i386/pc/boot.S           |   11 +++--------
 boot/i386/pc/cdboot.S         |    3 ---
 boot/i386/pc/pxeboot.S        |    3 +--
 include/grub/i386/pc/boot.h   |    5 +----
 include/grub/i386/pc/kernel.h |    3 ---
 kern/i386/pc/init.c           |   11 ++++-------
 kern/i386/pc/startup.S        |    6 +-----
 util/i386/pc/grub-setup.c     |    5 +----
 8 files changed, 11 insertions(+), 36 deletions(-)

diff --git a/boot/i386/pc/boot.S b/boot/i386/pc/boot.S
index 2cd505e..8d8c27c 100644
--- a/boot/i386/pc/boot.S
+++ b/boot/i386/pc/boot.S
@@ -102,8 +102,6 @@ kernel_sector:
 boot_drive:
 	.byte 0xff	/* the disk to load kernel from */
 			/* 0xff means use the boot drive */
-root_drive:
-        .byte 0xff
 
 after_BPB:
 
@@ -118,6 +116,7 @@ after_BPB:
          * possible boot drive. If GRUB is installed into a floppy,
          * this does nothing (only jump).
          */
+	. = _start + GRUB_BOOT_MACHINE_DRIVE_CHECK
 boot_drive_check:
         jmp     1f	/* grub-setup may overwrite this jump */
         testb   $0x80, %dl
@@ -151,14 +150,12 @@ real_start:
 	/*
 	 *  Check if we have a forced disk reference here
 	 */
-        /* assign root_drive at the same time */
 #ifdef APPLE_CC
 	boot_drive_abs = ABS (boot_drive)
-	movw    boot_drive_abs, %ax
+	movb    boot_drive_abs, %al
 #else
-	movw    ABS(boot_drive), %ax
+	movb   ABS(boot_drive), %al
 #endif
-        movb    %ah, %dh
 	cmpb	$0xff, %al
 	je	1f
 	movb	%al, %dl
@@ -343,7 +340,6 @@ setup_sectors:
 
 	/* restore %dl */
 	popw	%dx
-        pushw   %dx
 
 	/* head start */
 	movb	%al, %dh
@@ -399,7 +395,6 @@ copy_buffer:
 
 	popw	%ds
 	popa
-        popw    %dx
 
 	/* boot kernel */
 #ifdef APPLE_CC
diff --git a/boot/i386/pc/cdboot.S b/boot/i386/pc/cdboot.S
index 688b26c..efa65f5 100644
--- a/boot/i386/pc/cdboot.S
+++ b/boot/i386/pc/cdboot.S
@@ -86,9 +86,6 @@ bi_reserved:
 
 	call	read_cdrom
 
-        /* Root drive will default to boot drive */
-        movb    $0xFF, %dh
-
 	ljmp	$(DATA_ADDR >> 4), $0
 
 /*
diff --git a/boot/i386/pc/pxeboot.S b/boot/i386/pc/pxeboot.S
index 1b51127..2fc53bc 100644
--- a/boot/i386/pc/pxeboot.S
+++ b/boot/i386/pc/pxeboot.S
@@ -27,8 +27,7 @@
 _start:
 start:
 
-        /* Root drive will default to boot drive */
-        movb	$0xFF, %dh
+	/* Use drive number 0x7F for PXE */
         movb	$0x7F, %dl
 
 	/* Jump to the real world */
diff --git a/include/grub/i386/pc/boot.h b/include/grub/i386/pc/boot.h
index 3862214..f35cb3a 100644
--- a/include/grub/i386/pc/boot.h
+++ b/include/grub/i386/pc/boot.h
@@ -34,9 +34,6 @@
 /* The offset of BOOT_DRIVE.  */
 #define GRUB_BOOT_MACHINE_BOOT_DRIVE	0x4c
 
-/* The offset of ROOT_DRIVE.  */
-#define GRUB_BOOT_MACHINE_ROOT_DRIVE	0x4d
-
 /* The offset of KERNEL_ADDRESS.  */
 #define GRUB_BOOT_MACHINE_KERNEL_ADDRESS	0x40
 
@@ -47,7 +44,7 @@
 #define GRUB_BOOT_MACHINE_KERNEL_SEGMENT	0x42
 
 /* The offset of BOOT_DRIVE_CHECK.  */
-#define GRUB_BOOT_MACHINE_DRIVE_CHECK	0x4f
+#define GRUB_BOOT_MACHINE_DRIVE_CHECK	0x4e
 
 /* The offset of a magic number used by Windows NT.  */
 #define GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC	0x1b8
diff --git a/include/grub/i386/pc/kernel.h b/include/grub/i386/pc/kernel.h
index 5acc883..5b9d8dc 100644
--- a/include/grub/i386/pc/kernel.h
+++ b/include/grub/i386/pc/kernel.h
@@ -71,9 +71,6 @@ extern char grub_prefix[];
 /* The boot BIOS drive number.  */
 extern grub_uint8_t EXPORT_VAR(grub_boot_drive);
 
-/* The root BIOS drive number.  */
-extern grub_uint8_t grub_root_drive;
-
 #endif /* ! ASM_FILE */
 
 #endif /* ! KERNEL_MACHINE_HEADER */
diff --git a/kern/i386/pc/init.c b/kern/i386/pc/init.c
index b533921..274ba15 100644
--- a/kern/i386/pc/init.c
+++ b/kern/i386/pc/init.c
@@ -60,13 +60,10 @@ make_install_device (void)
 
   if (grub_prefix[0] != '(')
     {
-      /* If the root drive is not set explicitly, assume that it is identical
-         to the boot drive.  */
-      if (grub_root_drive == 0xFF)
-        grub_root_drive = grub_boot_drive;
-
-      grub_sprintf (dev, "(%cd%u", (grub_root_drive & 0x80) ? 'h' : 'f',
-                    grub_root_drive & 0x7f);
+      /* Need to make the root partition from the boot drive and the partition
+         number encoded at the install time.  */
+      grub_sprintf (dev, "(%cd%u", (grub_boot_drive & 0x80) ? 'h' : 'f',
+                    grub_boot_drive & 0x7f);
 
       if (grub_install_dos_part >= 0)
 	grub_sprintf (dev + grub_strlen (dev), ",%u", grub_install_dos_part + 1);
diff --git a/kern/i386/pc/startup.S b/kern/i386/pc/startup.S
index 0f80e8a..f77d7db 100644
--- a/kern/i386/pc/startup.S
+++ b/kern/i386/pc/startup.S
@@ -195,9 +195,8 @@ codestart:
 
 	sti		/* we're safe again */
 
-	/* save boot and root drive references */
+	/* save the boot drive */
 	ADDR32	movb	%dl, EXT_C(grub_boot_drive)
-	ADDR32	movb	%dh, EXT_C(grub_root_drive)
 
 	/* reset disk system (%ah = 0) */
 	int	$0x13
@@ -300,9 +299,6 @@ codestart:
 VARIABLE(grub_boot_drive)
 	.byte	0
 
-VARIABLE(grub_root_drive)
-	.byte	0
-
 	.p2align	2	/* force 4-byte alignment */
 
 /*
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index bdf234c..5a51964 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -94,7 +94,7 @@ setup (const char *dir,
   grub_uint16_t core_sectors;
   grub_device_t root_dev, dest_dev;
   const char *dest_partmap;
-  grub_uint8_t *boot_drive, *root_drive;
+  grub_uint8_t *boot_drive;
   grub_disk_addr_t *kernel_sector;
   grub_uint16_t *boot_drive_check;
   struct boot_blocklist *first_block, *block;
@@ -207,7 +207,6 @@ setup (const char *dir,
 
   /* Set the addresses of variables in the boot image.  */
   boot_drive = (grub_uint8_t *) (boot_img + GRUB_BOOT_MACHINE_BOOT_DRIVE);
-  root_drive = (grub_uint8_t *) (boot_img + GRUB_BOOT_MACHINE_ROOT_DRIVE);
   kernel_sector = (grub_disk_addr_t *) (boot_img
 				     + GRUB_BOOT_MACHINE_KERNEL_SECTOR);
   boot_drive_check = (grub_uint16_t *) (boot_img
@@ -379,7 +378,6 @@ setup (const char *dir,
 
   /* FIXME: can this be skipped?  */
   *boot_drive = 0xFF;
-  *root_drive = 0xFF;
 
   *kernel_sector = grub_cpu_to_le64 (embed_region.start);
 
@@ -513,7 +511,6 @@ unable_to_embed:
 
   /* FIXME: can this be skipped?  */
   *boot_drive = 0xFF;
-  *root_drive = 0xFF;
 
   *install_dos_part = grub_cpu_to_le32 (dos_part);
   *install_bsd_part = grub_cpu_to_le32 (bsd_part);

Re: [PATCH] Remove root drive support

[Vladimir 'phcoder' Serbinenko]

On Fri, Jun 12, 2009 at 10:45 PM, Pavel Roskin<proski@gnu.org> wrote:
> Root drive support is not actually used by the installer.  Root drive is
> superseded by grub_prefix with UUID, which is much more flexible, is
> already used by grub-setup for cross-drive installs and doesn't require
> a single byte in the boot sector.
I successfully tested this patch in qemu with both grub-mkrescue and
multiboot load grub2-by-grub2. As I see root drive was always set to
0xFF and so this field is useless and as it's in size-critical part.
However reading grub-install revealed that cross-drive install would
fail if target filesystem has no UUIDs or if UUIDs are unsupported by
grub. Currently it includes following filesystems:
affs,afs,cpio/tar,hfs,jfs,minix,sfs, udf, ufs
As you can see it includes even important filesystems like ufs. Now
the question to discuss is whether we want to implement UUIDs for this
fileystems, let it be like it is or have a cross-drive install w/o
UUIDs. Even if we choose the last option I think root field still has
to be removed and we can just use non-UUID drive name for prefix
>
> ChangeLog:
>        * boot/i386/pc/boot.S: Remove root_drive.  Ensure that
>        boot_drive_check is where boot.h expects it to be.  Don't save
>        %dx, we only need %dl and we never change it.
>        * boot/i386/pc/cdboot.S: Don't set the root drive.
>        * boot/i386/pc/pxeboot.S: Likewise.
>        * include/grub/i386/pc/boot.h: Remove
>        GRUB_BOOT_MACHINE_ROOT_DRIVE, adjust
>        GRUB_BOOT_MACHINE_DRIVE_CHECK.
>        * include/grub/i386/pc/kernel.h: Remove grub_root_drive.
>        * kern/i386/pc/init.c (make_install_device): Remove references
>        to grub_root_drive.
>        * kern/i386/pc/startup.S: Likewise.
>        * util/i386/pc/grub-setup.c (setup): Don't set root_drive.
> ---
>
>  boot/i386/pc/boot.S           |   11 +++--------
>  boot/i386/pc/cdboot.S         |    3 ---
>  boot/i386/pc/pxeboot.S        |    3 +--
>  include/grub/i386/pc/boot.h   |    5 +----
>  include/grub/i386/pc/kernel.h |    3 ---
>  kern/i386/pc/init.c           |   11 ++++-------
>  kern/i386/pc/startup.S        |    6 +-----
>  util/i386/pc/grub-setup.c     |    5 +----
>  8 files changed, 11 insertions(+), 36 deletions(-)
>
> diff --git a/boot/i386/pc/boot.S b/boot/i386/pc/boot.S
> index 2cd505e..8d8c27c 100644
> --- a/boot/i386/pc/boot.S
> +++ b/boot/i386/pc/boot.S
> @@ -102,8 +102,6 @@ kernel_sector:
>  boot_drive:
>        .byte 0xff      /* the disk to load kernel from */
>                        /* 0xff means use the boot drive */
> -root_drive:
> -        .byte 0xff
>
>  after_BPB:
>
> @@ -118,6 +116,7 @@ after_BPB:
>          * possible boot drive. If GRUB is installed into a floppy,
>          * this does nothing (only jump).
>          */
> +       . = _start + GRUB_BOOT_MACHINE_DRIVE_CHECK
>  boot_drive_check:
>         jmp     1f     /* grub-setup may overwrite this jump */
>         testb   $0x80, %dl
> @@ -151,14 +150,12 @@ real_start:
>        /*
>         *  Check if we have a forced disk reference here
>         */
> -        /* assign root_drive at the same time */
>  #ifdef APPLE_CC
>        boot_drive_abs = ABS (boot_drive)
> -       movw    boot_drive_abs, %ax
> +       movb    boot_drive_abs, %al
>  #else
> -       movw    ABS(boot_drive), %ax
> +       movb   ABS(boot_drive), %al
>  #endif
> -        movb    %ah, %dh
>        cmpb    $0xff, %al
>        je      1f
>        movb    %al, %dl
> @@ -343,7 +340,6 @@ setup_sectors:
>
>        /* restore %dl */
>        popw    %dx
> -        pushw   %dx
>
>        /* head start */
>        movb    %al, %dh
> @@ -399,7 +395,6 @@ copy_buffer:
>
>        popw    %ds
>        popa
> -        popw    %dx
>
>        /* boot kernel */
>  #ifdef APPLE_CC
> diff --git a/boot/i386/pc/cdboot.S b/boot/i386/pc/cdboot.S
> index 688b26c..efa65f5 100644
> --- a/boot/i386/pc/cdboot.S
> +++ b/boot/i386/pc/cdboot.S
> @@ -86,9 +86,6 @@ bi_reserved:
>
>        call    read_cdrom
>
> -        /* Root drive will default to boot drive */
> -        movb    $0xFF, %dh
> -
>        ljmp    $(DATA_ADDR >> 4), $0
>
>  /*
> diff --git a/boot/i386/pc/pxeboot.S b/boot/i386/pc/pxeboot.S
> index 1b51127..2fc53bc 100644
> --- a/boot/i386/pc/pxeboot.S
> +++ b/boot/i386/pc/pxeboot.S
> @@ -27,8 +27,7 @@
>  _start:
>  start:
>
> -        /* Root drive will default to boot drive */
> -        movb   $0xFF, %dh
> +       /* Use drive number 0x7F for PXE */
>         movb   $0x7F, %dl
>
>        /* Jump to the real world */
> diff --git a/include/grub/i386/pc/boot.h b/include/grub/i386/pc/boot.h
> index 3862214..f35cb3a 100644
> --- a/include/grub/i386/pc/boot.h
> +++ b/include/grub/i386/pc/boot.h
> @@ -34,9 +34,6 @@
>  /* The offset of BOOT_DRIVE.  */
>  #define GRUB_BOOT_MACHINE_BOOT_DRIVE   0x4c
>
> -/* The offset of ROOT_DRIVE.  */
> -#define GRUB_BOOT_MACHINE_ROOT_DRIVE   0x4d
> -
>  /* The offset of KERNEL_ADDRESS.  */
>  #define GRUB_BOOT_MACHINE_KERNEL_ADDRESS       0x40
>
> @@ -47,7 +44,7 @@
>  #define GRUB_BOOT_MACHINE_KERNEL_SEGMENT       0x42
>
>  /* The offset of BOOT_DRIVE_CHECK.  */
> -#define GRUB_BOOT_MACHINE_DRIVE_CHECK  0x4f
> +#define GRUB_BOOT_MACHINE_DRIVE_CHECK  0x4e
>
>  /* The offset of a magic number used by Windows NT.  */
>  #define GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC     0x1b8
> diff --git a/include/grub/i386/pc/kernel.h b/include/grub/i386/pc/kernel.h
> index 5acc883..5b9d8dc 100644
> --- a/include/grub/i386/pc/kernel.h
> +++ b/include/grub/i386/pc/kernel.h
> @@ -71,9 +71,6 @@ extern char grub_prefix[];
>  /* The boot BIOS drive number.  */
>  extern grub_uint8_t EXPORT_VAR(grub_boot_drive);
>
> -/* The root BIOS drive number.  */
> -extern grub_uint8_t grub_root_drive;
> -
>  #endif /* ! ASM_FILE */
>
>  #endif /* ! KERNEL_MACHINE_HEADER */
> diff --git a/kern/i386/pc/init.c b/kern/i386/pc/init.c
> index b533921..274ba15 100644
> --- a/kern/i386/pc/init.c
> +++ b/kern/i386/pc/init.c
> @@ -60,13 +60,10 @@ make_install_device (void)
>
>   if (grub_prefix[0] != '(')
>     {
> -      /* If the root drive is not set explicitly, assume that it is identical
> -         to the boot drive.  */
> -      if (grub_root_drive == 0xFF)
> -        grub_root_drive = grub_boot_drive;
> -
> -      grub_sprintf (dev, "(%cd%u", (grub_root_drive & 0x80) ? 'h' : 'f',
> -                    grub_root_drive & 0x7f);
> +      /* Need to make the root partition from the boot drive and the partition
> +         number encoded at the install time.  */
> +      grub_sprintf (dev, "(%cd%u", (grub_boot_drive & 0x80) ? 'h' : 'f',
> +                    grub_boot_drive & 0x7f);
>
>       if (grub_install_dos_part >= 0)
>        grub_sprintf (dev + grub_strlen (dev), ",%u", grub_install_dos_part + 1);
> diff --git a/kern/i386/pc/startup.S b/kern/i386/pc/startup.S
> index 0f80e8a..f77d7db 100644
> --- a/kern/i386/pc/startup.S
> +++ b/kern/i386/pc/startup.S
> @@ -195,9 +195,8 @@ codestart:
>
>        sti             /* we're safe again */
>
> -       /* save boot and root drive references */
> +       /* save the boot drive */
>        ADDR32  movb    %dl, EXT_C(grub_boot_drive)
> -       ADDR32  movb    %dh, EXT_C(grub_root_drive)
>
>        /* reset disk system (%ah = 0) */
>        int     $0x13
> @@ -300,9 +299,6 @@ codestart:
>  VARIABLE(grub_boot_drive)
>        .byte   0
>
> -VARIABLE(grub_root_drive)
> -       .byte   0
> -
>        .p2align        2       /* force 4-byte alignment */
>
>  /*
> diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
> index bdf234c..5a51964 100644
> --- a/util/i386/pc/grub-setup.c
> +++ b/util/i386/pc/grub-setup.c
> @@ -94,7 +94,7 @@ setup (const char *dir,
>   grub_uint16_t core_sectors;
>   grub_device_t root_dev, dest_dev;
>   const char *dest_partmap;
> -  grub_uint8_t *boot_drive, *root_drive;
> +  grub_uint8_t *boot_drive;
>   grub_disk_addr_t *kernel_sector;
>   grub_uint16_t *boot_drive_check;
>   struct boot_blocklist *first_block, *block;
> @@ -207,7 +207,6 @@ setup (const char *dir,
>
>   /* Set the addresses of variables in the boot image.  */
>   boot_drive = (grub_uint8_t *) (boot_img + GRUB_BOOT_MACHINE_BOOT_DRIVE);
> -  root_drive = (grub_uint8_t *) (boot_img + GRUB_BOOT_MACHINE_ROOT_DRIVE);
>   kernel_sector = (grub_disk_addr_t *) (boot_img
>                                     + GRUB_BOOT_MACHINE_KERNEL_SECTOR);
>   boot_drive_check = (grub_uint16_t *) (boot_img
> @@ -379,7 +378,6 @@ setup (const char *dir,
>
>   /* FIXME: can this be skipped?  */
>   *boot_drive = 0xFF;
> -  *root_drive = 0xFF;
>
>   *kernel_sector = grub_cpu_to_le64 (embed_region.start);
>
> @@ -513,7 +511,6 @@ unable_to_embed:
>
>   /* FIXME: can this be skipped?  */
>   *boot_drive = 0xFF;
> -  *root_drive = 0xFF;
>
>   *install_dos_part = grub_cpu_to_le32 (dos_part);
>   *install_bsd_part = grub_cpu_to_le32 (bsd_part);
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] Remove root drive support

[Pavel Roskin]

On Tue, 2009-06-16 at 01:41 +0200, Vladimir 'phcoder' Serbinenko wrote:

> I successfully tested this patch in qemu with both grub-mkrescue and
> multiboot load grub2-by-grub2. As I see root drive was always set to
> 0xFF and so this field is useless and as it's in size-critical part.
> However reading grub-install revealed that cross-drive install would
> fail if target filesystem has no UUIDs or if UUIDs are unsupported by
> grub. Currently it includes following filesystems:
> affs,afs,cpio/tar,hfs,jfs,minix,sfs, udf, ufs
> As you can see it includes even important filesystems like ufs. Now
> the question to discuss is whether we want to implement UUIDs for this
> fileystems, let it be like it is or have a cross-drive install w/o
> UUIDs. Even if we choose the last option I think root field still has
> to be removed and we can just use non-UUID drive name for prefix

I've committed the patch just before I got this message.  I'm glad you
didn't object.

I think we could implement partition addressing by a contained filename.
As long as GRUB can read the filesystem at all, it should be able to
read filenames.  Then we would specify grub_prefix as

(FILE=/boot/grub/FSID)/boot/grub

FSID could be a random string or is could be the filesystem UUID as read
by the OS.  grub-setup would create such file and hardcode the
corresponding grub_prefix into core.img.

grub_prefix should be writable by grub-setup, so I don't expect any
problems.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Remove root drive support

[Vladimir 'phcoder' Serbinenko]

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

Hello

On Tue, Jun 16, 2009 at 2:15 AM, Pavel Roskin <proski@gnu.org> wrote:

> On Tue, 2009-06-16 at 01:41 +0200, Vladimir 'phcoder' Serbinenko wrote:
>
> > I successfully tested this patch in qemu with both grub-mkrescue and
> > multiboot load grub2-by-grub2. As I see root drive was always set to
> > 0xFF and so this field is useless and as it's in size-critical part.
> > However reading grub-install revealed that cross-drive install would
> > fail if target filesystem has no UUIDs or if UUIDs are unsupported by
> > grub. Currently it includes following filesystems:
> > affs,afs,cpio/tar,hfs,jfs,minix,sfs, udf, ufs
> > As you can see it includes even important filesystems like ufs. Now
> > the question to discuss is whether we want to implement UUIDs for this
> > fileystems, let it be like it is or have a cross-drive install w/o
> > UUIDs. Even if we choose the last option I think root field still has
> > to be removed and we can just use non-UUID drive name for prefix
>
> I've committed the patch just before I got this message.  I'm glad you
> didn't object.
>
 Usually you do a fine job so there is simply no need to object ;)

>
> I think we could implement partition addressing by a contained filename.
> As long as GRUB can read the filesystem at all, it should be able to
> read filenames.

Otherwise the filesystem needs improvement

>  Then we would specify grub_prefix as
>
> (FILE=/boot/grub/FSID)/boot/grub
>
> FSID could be a random string or is could be the filesystem UUID as read
> by the OS.  grub-setup would create such file and hardcode the
> corresponding grub_prefix into core.img.

Good idea. Do you want to implement it?

>
>
> grub_prefix should be writable by grub-setup, so I don't expect any
> problems.
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 3139 bytes --]

Re: [PATCH] Remove root drive support

[Pavel Roskin]

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

On Tue, 2009-06-16 at 12:30 +0200, Vladimir 'phcoder' Serbinenko wrote:

>         I think we could implement partition addressing by a contained
>         filename.
>         As long as GRUB can read the filesystem at all, it should be
>         able to
>         read filenames.
> Otherwise the filesystem needs improvement 

The great thing is that we don't even need the ability to read
directories, so we can support PXE and other very limited filesystems.

>          Then we would specify grub_prefix as
>         
>         (FILE=/boot/grub/FSID)/boot/grub
>         
>         FSID could be a random string or is could be the filesystem
>         UUID as read
>         by the OS.  grub-setup would create such file and hardcode the
>         corresponding grub_prefix into core.img.
> Good idea. Do you want to implement it? 

The kernel side was easy.  The patch is attached.  Actually, I'm not
sure I got the disk ID right.  I copied it from fs_uuid.c, but I need to
recheck it.

We still need the userspace changes.

-- 
Regards,
Pavel Roskin

[-- Attachment #2: 01-fs_file.patch --]
[-- Type: text/x-patch, Size: 5598 bytes --]

Allow referencing a filesystem by a file it contains

From: Pavel Roskin <proski@gnu.org>

ChangeLog:
	* conf/common.rmk: Add fs_file.mod.
	* disk/fs_file.c: New file.
	* include/grub/disk.h (enum grub_disk_dev_id): Add
	GRUB_DISK_DEVICE_FILE_ID.
---

 conf/common.rmk     |    9 +++
 disk/fs_file.c      |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/grub/disk.h |    1 
 3 files changed, 144 insertions(+), 2 deletions(-)
 create mode 100644 disk/fs_file.c


diff --git a/conf/common.rmk b/conf/common.rmk
index d0c142b..fbca2e4 100644
--- a/conf/common.rmk
+++ b/conf/common.rmk
@@ -349,8 +349,8 @@ scsi_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 # Commands.
 pkglib_MODULES += minicmd.mod extcmd.mod hello.mod handler.mod	\
-	 ls.mod	cmp.mod cat.mod help.mod search.mod		\
-	loopback.mod fs_uuid.mod configfile.mod echo.mod	\
+	ls.mod cmp.mod cat.mod help.mod search.mod loopback.mod	\
+	fs_file.mod fs_uuid.mod configfile.mod echo.mod		\
 	terminfo.mod test.mod blocklist.mod hexdump.mod		\
 	read.mod sleep.mod loadenv.mod crc.mod parttool.mod	\
 	pcpart.mod memrw.mod normal.mod sh.mod lua.mod	\
@@ -431,6 +431,11 @@ loopback_mod_SOURCES = disk/loopback.c
 loopback_mod_CFLAGS = $(COMMON_CFLAGS)
 loopback_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
+# For fs_file.mod
+fs_file_mod_SOURCES = disk/fs_file.c
+fs_file_mod_CFLAGS = $(COMMON_CFLAGS)
+fs_file_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 # For fs_uuid.mod
 fs_uuid_mod_SOURCES = disk/fs_uuid.c
 fs_uuid_mod_CFLAGS = $(COMMON_CFLAGS)
diff --git a/disk/fs_file.c b/disk/fs_file.c
new file mode 100644
index 0000000..d81a971
--- /dev/null
+++ b/disk/fs_file.c
@@ -0,0 +1,136 @@
+/* fs_file.c - Access partition by a file it contains.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2007,2008  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/dl.h>
+#include <grub/file.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <grub/partition.h>
+
+static grub_device_t
+search_fs_file (const char *key, unsigned long *count)
+{
+  char *filename = NULL;
+  grub_device_t ret = NULL;
+  *count = 0;
+
+  auto int iterate_device (const char *name);
+  int iterate_device (const char *name)
+  {
+    int len;
+    grub_file_t file;
+
+    (*count)++;
+
+    len = grub_strlen (name) + 2 + grub_strlen (key) + 1;
+    filename = grub_realloc (filename, len);
+    if (! filename)
+      return 1;
+
+    grub_sprintf (filename, "(%s)%s", name, key);
+    file = grub_file_open (filename);
+    if (file)
+      {
+	grub_file_close (file);
+	ret = grub_device_open (name);
+	return 1;
+      }
+
+    grub_errno = GRUB_ERR_NONE;
+    return 0;
+  }
+
+  grub_device_iterate (iterate_device);
+  grub_free (filename);
+
+  return ret;
+}
+
+static grub_err_t
+grub_fs_file_open (const char *name, grub_disk_t disk)
+{
+  grub_device_t dev;
+
+  if (grub_strncmp (name, "FILE=", sizeof ("FILE=") - 1))
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a FILE virtual volume");
+
+  dev = search_fs_file (name + sizeof ("FILE=") - 1, &disk->id);
+  if (! dev)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "no matching file found");
+
+  disk->total_sectors = dev->disk->total_sectors;
+  disk->has_partitions = 0;
+  if (dev->disk->partition)
+    {
+      disk->partition = grub_malloc (sizeof (*disk->partition));
+      if (disk->partition)
+	grub_memcpy (disk->partition, dev->disk->partition,
+		     sizeof (*disk->partition));
+    }
+  else
+    disk->partition = NULL;
+
+  disk->data = dev;
+
+  return GRUB_ERR_NONE;
+}
+
+static void
+grub_fs_file_close (grub_disk_t disk)
+{
+  grub_device_t parent = disk->data;
+  grub_device_close (parent);
+}
+
+static grub_err_t
+grub_fs_file_read (grub_disk_t disk, grub_disk_addr_t sector,
+		   grub_size_t size, char *buf)
+{
+  grub_device_t parent = disk->data;
+  return parent->disk->dev->read (parent->disk, sector, size, buf);
+}
+
+static grub_err_t
+grub_fs_file_write (grub_disk_t disk, grub_disk_addr_t sector,
+		    grub_size_t size, const char *buf)
+{
+  grub_device_t parent = disk->data;
+  return parent->disk->dev->write (parent->disk, sector, size, buf);
+}
+
+static struct grub_disk_dev grub_fs_file_dev = {
+  .name = "fs_file",
+  .id = GRUB_DISK_DEVICE_FILE_ID,
+  .open = grub_fs_file_open,
+  .close = grub_fs_file_close,
+  .read = grub_fs_file_read,
+  .write = grub_fs_file_write,
+  .next = 0
+};
+
+GRUB_MOD_INIT (fs_file)
+{
+  grub_disk_dev_register (&grub_fs_file_dev);
+}
+
+GRUB_MOD_FINI (fs_file)
+{
+  grub_disk_dev_unregister (&grub_fs_file_dev);
+}
diff --git a/include/grub/disk.h b/include/grub/disk.h
index a0cc642..1f96111 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -38,6 +38,7 @@ enum grub_disk_dev_id
     GRUB_DISK_DEVICE_ATA_ID,
     GRUB_DISK_DEVICE_MEMDISK_ID,
     GRUB_DISK_DEVICE_NAND_ID,
+    GRUB_DISK_DEVICE_FILE_ID,
     GRUB_DISK_DEVICE_UUID_ID,
     GRUB_DISK_DEVICE_PXE_ID,
     GRUB_DISK_DEVICE_SCSI_ID,

Re: [PATCH] Remove root drive support

[Vladimir 'phcoder' Serbinenko]

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

On Tue, Jun 16, 2009 at 8:09 PM, Pavel Roskin <proski@gnu.org> wrote:

> On Tue, 2009-06-16 at 12:30 +0200, Vladimir 'phcoder' Serbinenko wrote:
>
> >         I think we could implement partition addressing by a contained
> >         filename.
> >         As long as GRUB can read the filesystem at all, it should be
> >         able to
> >         read filenames.
> > Otherwise the filesystem needs improvement
>
> The great thing is that we don't even need the ability to read
> directories, so we can support PXE and other very limited filesystems.
>
> >          Then we would specify grub_prefix as
> >
> >         (FILE=/boot/grub/FSID)/boot/grub
> >
> >         FSID could be a random string or is could be the filesystem
> >         UUID as read
> >         by the OS.  grub-setup would create such file and hardcode the
> >         corresponding grub_prefix into core.img.
> > Good idea. Do you want to implement it?
>
> The kernel side was easy.  The patch is attached.  Actually, I'm not
> sure I got the disk ID right.  I copied it from fs_uuid.c, but I need to
> recheck it.
>
Update copyright year. I suggest putting new id in grub_disk_dev_id to the
end. You also forgot to add it to grub-emu. Under qemu it worked well except
that it wasn't autoloaded but I looked and seen that we have no module
autoloading for device drivers, perhaps because some of them don't coexist
well. Except issues mentioned patch looks fine and I think you can commit it
even if we don't have userspace part yet

>
> We still need the userspace changes.
>
> --
> Regards,
> Pavel Roskin
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 2604 bytes --]

Re: [PATCH] Remove root drive support

[Pavel Roskin]

On Thu, 2009-06-18 at 18:57 +0200, Vladimir 'phcoder' Serbinenko wrote:

> Update copyright year.

Done.

> I suggest putting new id in grub_disk_dev_id to the end.

Done.  Actually, using modules compiled from different sources is asking
for trouble, but it's not like this list is well sorted now.

> You also forgot to add it to grub-emu.

Actually, I didn't add it because fs_uuid.c is not compiled for
grub-emu.  If you want, you can add them both.

> Under qemu it worked well except that it wasn't autoloaded but I
> looked and seen that we have no module autoloading for device drivers,
> perhaps because some of them don't coexist well.

We'll need a list of drive names with corresponding modules, something
like:

hd: biosdisk
fd: biosdisk
FILE=: fs_file
UUID=: fs_uuid
ata: ata

It's true that autoloading of ata may break biosdisk.  But nobody is
forces to use ata devices on the command line.  but it is becomes an
issue, ata could be excluded from that list.

> Except issues mentioned patch looks fine and I think you can commit it
> even if we don't have userspace part yet 

Committed.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Remove root drive support

[Robert Millan]

On Tue, Jun 16, 2009 at 01:41:18AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Fri, Jun 12, 2009 at 10:45 PM, Pavel Roskin<proski@gnu.org> wrote:
> > Root drive support is not actually used by the installer.  Root drive is
> > superseded by grub_prefix with UUID, which is much more flexible, is
> > already used by grub-setup for cross-drive installs and doesn't require
> > a single byte in the boot sector.
> I successfully tested this patch in qemu with both grub-mkrescue and
> multiboot load grub2-by-grub2. As I see root drive was always set to
> 0xFF and so this field is useless and as it's in size-critical part.
> However reading grub-install revealed that cross-drive install would
> fail if target filesystem has no UUIDs or if UUIDs are unsupported by
> grub. Currently it includes following filesystems:
> affs,afs,cpio/tar,hfs,jfs,minix,sfs, udf, ufs
> As you can see it includes even important filesystems like ufs. Now
> the question to discuss is whether we want to implement UUIDs for this
> fileystems, let it be like it is or have a cross-drive install w/o
> UUIDs. Even if we choose the last option I think root field still has
> to be removed and we can just use non-UUID drive name for prefix

Cross-drive installs are not that important;  I removed support for them
on non-UUID filesystems quite a while ago, and so far nobody has complained.

In case a user with one of these filesystems wants to go cross-drive (which
isn't really a good idea anyway), implementing UUID support wouldn't be
too hard.

-- 
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."