Re: [PATCH] Add commands for reading and writing raw bytes to CMOS

From: Daniel Kiper <dkiper@net-space.pl>
To: The development of GNU GRUB <grub-devel@gnu.org>
Cc: Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>,
	development@efficientek.com
Subject: Re: [PATCH] Add commands for reading and writing raw bytes to CMOS
Date: Wed, 10 Apr 2024 16:56:46 +0200	[thread overview]
Message-ID: <20240410145646.udtllqhtkopxhjyo@tomti.i.net-space.pl> (raw)
In-Reply-To: <20240403165957.058fb402@crass-HP-ZBook-15-G2>

On Wed, Apr 03, 2024 at 04:59:57PM -0500, Glenn Washburn wrote:
> On Fri, 15 Mar 2024 22:45:53 +0300
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote:
>
> > With some coreboot configs setting a byte to a magic value
> > changes behaviour on next boot. Setting bit-by-bit is
> > possible but not convenient. Add cmosread and cmoswrite for
> > convenience.
> >
> > Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
> >
>
> Small nit, I think its customary to use '-s' in commands for setting
> variables. '-v' would intuitively be for verbosity. Also, it would be

I agree with Glenn.

> nice if you permanently configure your system to send patches as inline
> and not as attachments. Are you not using git-send?

I concur...

> From a29cfa77e292fdd0792eeb0bc2a0287524c1de1f Mon Sep 17 00:00:00 2001
> From: Vladimir Serbinenko <phcoder@gmail.com>
> Date: Mon, 18 Sep 2023 02:20:13 +0200
> Subject: [PATCH] Add commands for reading and writing raw bytes to CMOS
>
> With some coreboot configs setting a byte to a magic value
> changes behaviour on next boot. Setting bit-by-bit is
> possible but not convenient. Add cmosread and cmoswrite for
> convenience.
>
> Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
> ---
>  grub-core/commands/i386/cmostest.c | 77 +++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/commands/i386/cmostest.c b/grub-core/commands/i386/cmostest.c
> index 1f0c5341d..920cd81eb 100644
> --- a/grub-core/commands/i386/cmostest.c
> +++ b/grub-core/commands/i386/cmostest.c
> @@ -18,9 +18,12 @@
>
>  #include <grub/dl.h>
>  #include <grub/command.h>
> +#include <grub/extcmd.h>
>  #include <grub/misc.h>
>  #include <grub/cmos.h>
>  #include <grub/i18n.h>
> +#include <grub/mm.h>
> +#include <grub/env.h>
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -99,7 +102,71 @@ grub_cmd_cmosset (struct grub_command *cmd __attribute__ ((unused)),
>    return grub_cmos_write (byte, value | (1 << bit));
>  }
>
> -static grub_command_t cmd, cmd_clean, cmd_set;
> +static grub_err_t
> +grub_cmd_cmoswrite (struct grub_command *cmd __attribute__ ((unused)),
> +		    int argc, char *argv[])
> +{
> +  int byte = -1, value = -1;

The byte and value should be defined as an unsigned long.

> +
> +  if (argc != 2)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
> +
> +  byte = grub_strtoul (argv[0], NULL, 0);
> +  if (grub_errno)

In general I think we should not use shortened checks for enums and NULL.
Even if they work...

> +    return grub_errno;

AFAICT the grub_errno is not reliable here. The good example how the
grub_strtoul() result should be checked is in the commit ac8a37dda
(net/http: Allow use of non-standard TCP/IP ports).

> +  if (byte < 0 || byte >= 0x100)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid address"));
> +
> +  value = grub_strtoul (argv[1], NULL, 0);

Ditto and below please...

> +  if (grub_errno)
> +    return grub_errno;
> +
> +  if (value < 0 || value >= 0x100)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid value"));
> +
> +  return grub_cmos_write (byte, value);
> +}
> +
> +static grub_err_t
> +grub_cmd_cmosread (grub_extcmd_context_t ctxt, int argc, char **argv)

Could you put this function before grub_cmd_cmoswrite()?

> +{
> +  int byte = -1;

The byte should be defined as an unsigned long.

> +  grub_uint8_t value = 0;
> +  grub_err_t err;
> +
> +  if (argc != 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
> +
> +  byte = grub_strtoul (argv[0], NULL, 0);
> +  if (grub_errno)
> +    return grub_errno;
> +
> +  if (byte < 0 || byte >= 0x100)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid address"));
> +
> +  err = grub_cmos_read (byte, &value);
> +  if (err)
> +    return err;
> +
> +  if (ctxt->state[2].set) {
> +    char buf[sizeof ("XX")];

I would prefer if this is defined at the top of the function.

> +    grub_snprintf (buf, sizeof (buf), "%x", value);
> +    grub_env_set(ctxt->state[2].arg, buf);
> +  } else
> +    grub_printf_("CMOS value at 0x%x is 0x%x\n", byte, value);
> +  return GRUB_ERR_NONE;
> +}
> +
> +static const struct grub_arg_option read_options[] =
> +  {
> +    {0, 'v', 0, N_("Save read value into variable VARNAME."),
> +     N_("VARNAME"), ARG_TYPE_STRING},
> +    {0, 0, 0, 0, 0, 0}
> +  };
> +
> +static grub_command_t cmd, cmd_clean, cmd_set, cmd_write;
> +static grub_extcmd_t cmd_read;
>
>  \f
>  GRUB_MOD_INIT(cmostest)
> @@ -114,6 +181,12 @@ GRUB_MOD_INIT(cmostest)
>  				   N_("BYTE:BIT"),
>  				   /* TRANSLATORS: A bit may be either set (1) or clear (0).  */
>  				   N_("Set bit at BYTE:BIT in CMOS."));
> +  cmd_read = grub_register_extcmd_lockdown ("cmosread", grub_cmd_cmosread, 0,
> +				       N_("[-v VAR] ADDR"),
> +				       N_("Read CMOS byte at ADDR."), read_options);
> +  cmd_write = grub_register_command_lockdown ("cmoswrite", grub_cmd_cmoswrite,
> +					      N_("ADDR VALUE"),
> +					      N_("Set CMOS byte at ADDR to VALUE."));

Please add commands descriptions to the GRUB documentation.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel