On 25.11.2013 23:37, Jon McCune wrote:
> [v0] new grub-*-setup flag to disable insertion of reed solomon codes
> [v0] grub-install support for option --no-rs-codes
> [v1] bugfix: also change the maxsec value in util/setup.c
> [v2] Modified to support new C-language install utilities, added
> documentation
>
> Signed-off-by: Jon McCune <jonmcc...@google.com>
> ---
> docs/grub.texi | 12 +++++++++-
> include/grub/util/install.h | 4 ++--
> util/grub-install.c | 20 ++++++++++++-----
> util/grub-setup.c | 11 ++++++++--
> util/setup.c | 53
> +++++++++++++++++++++++++--------------------
> 5 files changed, 66 insertions(+), 34 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 6aee292..29547cd 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -5937,8 +5937,18 @@ mounted on
> Recheck the device map, even if @file{/boot/grub/device.map} already
> exists. You should use this option whenever you add/remove a disk
> into/from your computer.
> -@end table
>
> +@item --no-rs-codes
> +By default, @command{grub-install} will use some extra space in the
It must be clear that you speak about BIOS only.
> +bootloader embedding area for Reed-Solomon error-correcting
> +codes. This enables GRUB to still boot successfully if one single
> +block is corrupted.
We can survive more than one defective sector. If we have r sectors of
redundancy we can survive up to r/2 corrupted ones
> This redundancy may be cumbersome if attempting
> +to cryptographically validate the contents of the bootloader embedding
> +area, or in more modern systems with GPT-style partition tables
> +(@pxref{BIOS installation}) where GRUB does not reside in any
> +unpartitioned space outside of the MBR. Disable the Reed-Solomon
What's the reasonning behind GPT part?
> +codes with this option.
> +@end table
>
> @node Invoking grub-mkconfig
> @chapter Invoking grub-mkconfig
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index 5cb33fc..2bfd2ec 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -176,12 +176,12 @@ void
> grub_util_bios_setup (const char *dir,
> const char *boot_file, const char *core_file,
> const char *dest, int force,
> - int fs_probe, int allow_floppy);
> + int fs_probe, int allow_floppy, int no_rs_codes);
> void
> grub_util_sparc_setup (const char *dir,
> const char *boot_file, const char *core_file,
> const char *dest, int force,
> - int fs_probe, int allow_floppy);
> + int fs_probe, int allow_floppy, int no_rs_codes);
>
> char *
> grub_install_get_image_targets_string (void);
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 5354a6d..12d0bc2 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -68,6 +68,7 @@ static int have_load_cfg = 0;
> static FILE * load_cfg_f = NULL;
> static char *load_cfg;
> static int install_bootsector = 1;
> +static int no_rs_codes = 0;
>
Negative logic variables are unreadable and prone to errors. Could you
use positive logic?
> enum
> {
> @@ -93,7 +94,8 @@ enum
> OPTION_DEBUG_IMAGE,
> OPTION_NO_FLOPPY,
> OPTION_DISK_MODULE,
> - OPTION_NO_BOOTSECTOR
> + OPTION_NO_BOOTSECTOR,
> + OPTION_NO_RS_CODES,
> };
>
> static int fs_probe = 1;
> @@ -180,6 +182,10 @@ argp_parser (int key, char *arg, struct argp_state
> *state)
> install_bootsector = 0;
> return 0;
>
> + case OPTION_NO_RS_CODES:
> + no_rs_codes = 1;
> + return 0;
> +
> case OPTION_DEBUG:
> verbosity++;
> return 0;
> @@ -238,6 +244,8 @@ static struct argp_option options[] = {
> N_("do not probe for filesystems in DEVICE"), 0},
> {"no-bootsector", OPTION_NO_BOOTSECTOR, 0, 0,
> N_("do not install bootsector"), 0},
> + {"no-rs-codes", OPTION_NO_RS_CODES, 0, 0,
> + N_("Do not apply any reed-solomon codes when embedding core.img, even if
> there is enough space."), 0},
>
> {"debug", OPTION_DEBUG, 0, OPTION_HIDDEN, 0, 2},
> {"no-floppy", OPTION_NO_FLOPPY, 0, OPTION_HIDDEN, 0, 2},
> @@ -1412,12 +1420,13 @@ main (int argc, char *argv[])
> "boot.img");
> grub_install_copy_file (boot_img_src, boot_img, 1);
>
> - grub_util_info ("%sgrub-bios-setup %s %s %s %s --directory='%s'
> --device-map='%s' '%s'",
> + grub_util_info ("%sgrub-bios-setup %s %s %s %s %s --directory='%s'
> --device-map='%s' '%s'",
> install_bootsector ? "" : "NOT RUNNING: ",
> allow_floppy ? "--allow-floppy " : "",
> verbosity ? "--verbose " : "",
> force ? "--force " : "",
> !fs_probe ? "--skip-fs-probe" : "",
> + no_rs_codes ? "--no-rs-codes" : "",
> platdir,
> device_map,
> install_device);
> @@ -1426,7 +1435,7 @@ main (int argc, char *argv[])
> if (install_bootsector)
> grub_util_bios_setup (platdir, "boot.img", "core.img",
> install_drive, force,
> - fs_probe, allow_floppy);
> + fs_probe, allow_floppy, no_rs_codes);
> break;
> }
> case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
> @@ -1438,12 +1447,13 @@ main (int argc, char *argv[])
> "boot.img");
> grub_install_copy_file (boot_img_src, boot_img, 1);
>
> - grub_util_info ("%sgrub-sparc64-setup %s %s %s %s --directory='%s'
> --device-map='%s' '%s'",
> + grub_util_info ("%sgrub-sparc64-setup %s %s %s %s %s --directory='%s'
> --device-map='%s' '%s'",
> install_bootsector ? "" : "NOT RUNNING: ",
> allow_floppy ? "--allow-floppy " : "",
> verbosity ? "--verbose " : "",
> force ? "--force " : "",
> !fs_probe ? "--skip-fs-probe" : "",
> + no_rs_codes ? "--no-rs-codes" : "",
On sparc it has no effect.
> platdir,
> device_map,
> install_drive);
> @@ -1452,7 +1462,7 @@ main (int argc, char *argv[])
> if (install_bootsector)
> grub_util_sparc_setup (platdir, "boot.img", "core.img",
> install_device, force,
> - fs_probe, allow_floppy);
> + fs_probe, allow_floppy, no_rs_codes);
> break;
> }
>
> diff --git a/util/grub-setup.c b/util/grub-setup.c
> index 90b9de0..41344d2 100644
> --- a/util/grub-setup.c
> +++ b/util/grub-setup.c
> @@ -82,7 +82,8 @@ static struct argp_option options[] = {
> /* TRANSLATORS: The potential breakage isn't limited to floppies but it's
> likely to make the install unbootable from HDD. */
> N_("make the drive also bootable as floppy (default for fdX devices). May
> break on some BIOSes."), 0},
> -
> + {"no-rs-codes", 'n', 0, 0,
> + N_("Do not apply any reed-solomon codes when embedding core.img, even if
> there is enough space."), 0},
It doesn't seem to be important enough to give it a letter of its own.
> { 0, 0, 0, 0, 0, 0 }
> };
>
Don't add it on sparc, it doesn't have any effect there.
> @@ -118,6 +119,7 @@ struct arguments
> int fs_probe;
> int allow_floppy;
> char *device;
> + int no_rs_codes;
> };
>
> static error_t
> @@ -173,6 +175,10 @@ argp_parser (int key, char *arg, struct argp_state
> *state)
> verbosity++;
> break;
>
> + case 'n':
> + arguments->no_rs_codes = 1;
> + break;
> +
> case ARGP_KEY_ARG:
> if (state->arg_num == 0)
> arguments->device = xstrdup(arg);
> @@ -292,7 +298,8 @@ main (int argc, char *argv[])
> arguments.boot_file ? : DEFAULT_BOOT_FILE,
> arguments.core_file ? : DEFAULT_CORE_FILE,
> dest_dev, arguments.force,
> - arguments.fs_probe, arguments.allow_floppy);
> + arguments.fs_probe, arguments.allow_floppy,
> + arguments.no_rs_codes);
>
> /* Free resources. */
> grub_fini_all ();
> diff --git a/util/setup.c b/util/setup.c
> index 337c304..b60a109 100644
> --- a/util/setup.c
> +++ b/util/setup.c
> @@ -248,7 +248,7 @@ void
> SETUP (const char *dir,
> const char *boot_file, const char *core_file,
> const char *dest, int force,
> - int fs_probe, int allow_floppy)
> + int fs_probe, int allow_floppy, int no_rs_codes)
> {
> char *core_path;
> char *boot_img, *core_img, *boot_path;
> @@ -486,7 +486,11 @@ SETUP (const char *dir,
>
> nsec = core_sectors;
>
> - maxsec = 2 * core_sectors;
> + if (!no_rs_codes)
> + maxsec = 2 * core_sectors;
> + else
> + maxsec = core_sectors;
> +
> if (maxsec > ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
> >> GRUB_DISK_SECTOR_BITS))
> maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
> @@ -547,27 +551,29 @@ SETUP (const char *dir,
> bl.first_block = (struct grub_boot_blocklist *) (core_img
> + GRUB_DISK_SECTOR_SIZE
> - sizeof (*bl.block));
> -
> - grub_size_t no_rs_length;
> - grub_set_unaligned32 ((core_img + GRUB_DISK_SECTOR_SIZE
> - + GRUB_KERNEL_I386_PC_REED_SOLOMON_REDUNDANCY),
> - grub_host_to_target32 (nsec * GRUB_DISK_SECTOR_SIZE -
> core_size));
> - no_rs_length = grub_target_to_host16
> - (grub_get_unaligned16 (core_img
> - + GRUB_DISK_SECTOR_SIZE
> - + GRUB_KERNEL_I386_PC_NO_REED_SOLOMON_LENGTH));
> -
> - if (no_rs_length == 0xffff)
> - grub_util_error ("%s", _("core.img version mismatch"));
> -
> - void *tmp = xmalloc (core_size);
> - grub_memcpy (tmp, core_img, core_size);
> - grub_reed_solomon_add_redundancy (core_img + no_rs_length +
> GRUB_DISK_SECTOR_SIZE,
> - core_size - no_rs_length -
> GRUB_DISK_SECTOR_SIZE,
> - nsec * GRUB_DISK_SECTOR_SIZE
> - - core_size);
> - assert (grub_memcmp (tmp, core_img, core_size) == 0);
> - free (tmp);
> + if (!no_rs_codes)
> + {
> + grub_size_t no_rs_length;
> + grub_set_unaligned32 ((core_img + GRUB_DISK_SECTOR_SIZE
> + +
> GRUB_KERNEL_I386_PC_REED_SOLOMON_REDUNDANCY),
> + grub_host_to_target32 (nsec *
> GRUB_DISK_SECTOR_SIZE - core_size));
> + no_rs_length = grub_target_to_host16
> + (grub_get_unaligned16 (core_img
> + + GRUB_DISK_SECTOR_SIZE
> + +
> GRUB_KERNEL_I386_PC_NO_REED_SOLOMON_LENGTH));
> +
> + if (no_rs_length == 0xffff)
> + grub_util_error ("%s", _("core.img version mismatch"));
> +
I'd keep this check
> + void *tmp = xmalloc (core_size);
> + grub_memcpy (tmp, core_img, core_size);
> + grub_reed_solomon_add_redundancy (core_img + no_rs_length +
> GRUB_DISK_SECTOR_SIZE,
> + core_size - no_rs_length -
> GRUB_DISK_SECTOR_SIZE,
> + nsec * GRUB_DISK_SECTOR_SIZE
> + - core_size);
> + assert (grub_memcmp (tmp, core_img, core_size) == 0);
> + free (tmp);
> + }
>
> /* Write the core image onto the disk. */
> for (i = 0; i < nsec; i++)
> @@ -762,4 +768,3 @@ unable_to_embed:
> grub_device_close (dest_dev);
> grub_device_close (root_dev);
> }
> -
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
