Re: [PATCH v2] Support to disable reed-solomon codes

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

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

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 <jonmccune@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);
>  }
> -
> 



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