Re: [PATCH v2 2/2] Add new MSR modules (rdmsr/wrmsr)

[Daniel Kiper]

On Fri, Mar 01, 2019 at 06:17:42PM +0100, Jesús Diéguez Fernández wrote:
> In order to be able to read and write from/to model-specific registers,
> two new modules are added. They are i386 specific, as the cpuid module.
>
> rdmsr module registers the command rdmsr that allows reading from a MSR.
> wrmsr module registers the command wrmsr that allows writing to a MSR.
>
> wrmsr module is disabled if UEFI secure boot is enabled.
>
> Please note that on SMP systems, interacting with a MSR that has a
> scope per hardware thread, implies that the value only applies to
> the particular cpu/core/thread that ran the command.
>
> Changelog v1 -> v2:
>
>     - Patch all source code files with s/__asm__ __volatile__/asm volatile/g.
>     - Split the module in two (rdmsr/wrmsr).
>     - Include the wrmsr module in the forbidden modules efi list.
>     - Code indentation and cleanup.
>     - Copyright year update.
>     - Implicit casting mask removed.
>     - Use the same assembly code for x86 and x86_64.
>     - Add missing documentation.
>     - Patch submited with Signed-off-by.
>
> Signed-off-by: Jesús Diéguez Fernández <address@hidden>

In general it looks much better but I still have some concerns...

First of all, two patches and more should have mail introducing them.
Good example you can find here
  http://lists.gnu.org/archive/html/grub-devel/2018-10/msg00201.html
or here
  http://lists.gnu.org/archive/html/grub-devel/2018-11/msg00282.html

git send-email --compose ... is your friend...

[...]

> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 2346bd291..a966a8f28 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -2484,3 +2484,19 @@ module = {
>    common = loader/i386/xen_file64.c;
>    extra_dist = loader/i386/xen_fileXX.c;
>  };
> +module = {
> +  name = rdmsr;
> +  common = commands/i386/rdmsr.c;
> +  enable = x86;
> +  enable = i386_xen_pvh;
> +  enable = i386_xen;
> +  enable = x86_64_xen;

I think that x86 should suffice? Does not it?

> +};
> +module = {
> +  name = wrmsr;
> +  common = commands/i386/wrmsr.c;
> +  enable = x86;
> +  enable = i386_xen_pvh;
> +  enable = i386_xen;
> +  enable = x86_64_xen;

Ditto.

> +};
> diff --git a/grub-core/commands/efi/shim_lock.c 
> b/grub-core/commands/efi/shim_lock.c
> index 83568cb2b..764098cfc 100644
> --- a/grub-core/commands/efi/shim_lock.c
> +++ b/grub-core/commands/efi/shim_lock.c
> @@ -43,7 +43,7 @@ static grub_efi_guid_t shim_lock_guid = 
> GRUB_EFI_SHIM_LOCK_GUID;
>  static grub_efi_shim_lock_protocol_t *sl;
>
>  /* List of modules which cannot be loaded if UEFI secure boot mode is 
> enabled. */
> -static const char * const disabled_mods[] = {"iorw", "memrw", NULL};
> +static const char * const disabled_mods[] = {"iorw", "memrw", "wrmsr", NULL};
>
>  static grub_err_t
>  shim_lock_init (grub_file_t io, enum grub_file_type type,
> diff --git a/grub-core/commands/i386/rdmsr.c b/grub-core/commands/i386/rdmsr.c
> new file mode 100644
> index 000000000..08e5aee0b
> --- /dev/null
> +++ b/grub-core/commands/i386/rdmsr.c
> @@ -0,0 +1,84 @@
> +/* rdmsr.c - Read CPU model-specific registers */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2019  Free Software Foundation, Inc.
> + *  Based on gcc/gcc/config/i386/driver-i386.c
> + *
> + *  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/dl.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/env.h>
> +#include <grub/command.h>
> +#include <grub/extcmd.h>
> +#include <grub/i386/rdmsr.h>
> +#include <grub/i18n.h>
> +
> +GRUB_MOD_LICENSE("GPLv3+");
> +
> +static grub_extcmd_t cmd_read;
> +
> +static const struct grub_arg_option options[] =
> +{
> +    {0, 'v', 0, N_("Save read value into variable VARNAME."),
> +    N_("VARNAME"), ARG_TYPE_STRING},
> +    {0, 0, 0, 0, 0, 0}
> +};
> +
> +static grub_err_t
> +grub_cmd_msr_read (grub_extcmd_context_t ctxt, int argc, char **argv)
> +{
> +    grub_uint64_t addr, value;
> +    char *ptr;
> +    char buf[sizeof("XXXXXXXX")];
> +
> +    if (argc != 1)
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument 
> expected"));
> +
> +    grub_errno = GRUB_ERR_NONE;
> +    ptr = argv[0];
> +    addr = grub_strtoul (ptr, &ptr, 0);
> +
> +    if (grub_errno != GRUB_ERR_NONE)
> +        return grub_errno;

Should not you display a blurb to the user what happened here?
Like you do below...

> +    if (*ptr != '\0')
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
> +
> +    value = grub_msr_read (addr);
> +
> +    if (ctxt->state[0].set)
> +    {
> +        grub_snprintf (buf, sizeof(buf), "%llx", value);
> +        grub_env_set (ctxt->state[0].arg, buf);
> +    }
> +    else
> +        grub_printf ("0x%llx\n", value);
> +
> +    return GRUB_ERR_NONE;
> +}
> +
> +GRUB_MOD_INIT(rdmsr)
> +{
> +    cmd_read = grub_register_extcmd ("rdmsr", grub_cmd_msr_read, 0,
> +                                    N_("ADDR"),
> +                                    N_("Read a CPU model specific 
> register."),
> +                                    options);
> +}
> +
> +GRUB_MOD_FINI(rdmsr)
> +{
> +    grub_unregister_extcmd (cmd_read);
> +}
> diff --git a/grub-core/commands/i386/wrmsr.c b/grub-core/commands/i386/wrmsr.c
> new file mode 100644
> index 000000000..351b93f93
> --- /dev/null
> +++ b/grub-core/commands/i386/wrmsr.c
> @@ -0,0 +1,75 @@
> +/* wrmsr.c - Write CPU model-specific registers */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2019  Free Software Foundation, Inc.
> + *  Based on gcc/gcc/config/i386/driver-i386.c
> + *
> + *  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/dl.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/env.h>
> +#include <grub/command.h>
> +#include <grub/extcmd.h>
> +#include <grub/i386/wrmsr.h>
> +#include <grub/i18n.h>
> +
> +GRUB_MOD_LICENSE("GPLv3+");
> +
> +static grub_command_t cmd_write;
> +
> +static grub_err_t
> +grub_cmd_msr_write (grub_command_t cmd, int argc, char **argv)
> +{
> +    grub_uint64_t addr, value;
> +    char *ptr;
> +
> +    if (argc != 2)
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments 
> expected"));
> +
> +    grub_errno = GRUB_ERR_NONE;
> +    ptr = argv[0];
> +    addr = grub_strtoul (ptr, &ptr, 0);
> +
> +    if (grub_errno != GRUB_ERR_NONE)
> +        return grub_errno;

Ditto.

> +    if (*ptr != '\0')
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
> +
> +    ptr = argv[1];
> +    value = grub_strtoul (ptr, &ptr, 0);
> +
> +    if (grub_errno != GRUB_ERR_NONE)
> +        return grub_errno;

Ditto.

> +    if (*ptr != '\0')
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
> +
> +    grub_msr_write (addr, value);
> +
> +    return GRUB_ERR_NONE;
> +}
> +
> +GRUB_MOD_INIT(wrmsr)
> +{
> +    cmd_write = grub_register_command ("wrmsr", grub_cmd_msr_write,
> +                                      N_("ADDR VALUE"),
> +                                      N_("Write a value to a CPU model 
> specific register."));
> +}
> +
> +GRUB_MOD_FINI(wrmsr)
> +{
> +    grub_unregister_command (cmd_write);
> +}
> diff --git a/include/grub/i386/rdmsr.h b/include/grub/i386/rdmsr.h
> new file mode 100644
> index 000000000..e907f1052
> --- /dev/null
> +++ b/include/grub/i386/rdmsr.h
> @@ -0,0 +1,32 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2019  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/>.
> + */
> +
> +#ifndef GRUB_CPU_MSR_READ_HEADER
> +#define GRUB_CPU_MSR_READ_HEADER 1

s/GRUB_CPU_MSR_READ_HEADER/GRUB_RDMSR_H/

> +#endif

This #endif should go...

> +
> +extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id)
> +{
> +    grub_uint32_t low_id = msr_id, low, high;
> +
> +    asm volatile ( "rdmsr"  : "=a"(low), "=d"(high) : "c"(low_id) );
> +
> +    return ((grub_uint64_t)high << 32) | low;
> +}
> +

...here. Like this

#endif /* GRUB_RDMSR_H */

> +

Please drop this empty line.

> diff --git a/include/grub/i386/wrmsr.h b/include/grub/i386/wrmsr.h
> new file mode 100644
> index 000000000..2e535b8fe
> --- /dev/null
> +++ b/include/grub/i386/wrmsr.h
> @@ -0,0 +1,29 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2019  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/>.
> + */
> +
> +#ifndef GRUB_CPU_MSR_WRITE_HEADER
> +#define GRUB_CPU_MSR_WRITE_HEADER 1

s/GRUB_CPU_MSR_READ_HEADER/GRUB_WRMSR_H/

> +#endif

And this #endif should go below too. Like in rdmsr.h.

> +extern __inline void grub_msr_write(grub_uint64_t msr_id, grub_uint64_t 
> msr_value)
> +{
> +    grub_uint32_t low_id = msr_id, low = msr_value, high = msr_value >> 32;
> +
> +    asm volatile ( "wrmsr" : : "c"(low_id), "a"(low), "d"(high) );
> +}

Anyway, thank you for doing the work. Looking forward for next version
of patches. Please post them quickly because freeze date is nearing.

Daniel