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 <jesu...@gmail.com>
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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
