[PATCH v3 0/2] i386: Code cleanup and new MSR modules

[PATCH v3 0/2] i386: Code cleanup and new MSR modules

[Jesús Diéguez Fernández]

This patch series consist in a code cleanup and two new modules to access MSR:

The first patch replaces __asm__ __volatile__ with asm volatile.
The second patch adds the modules rdmsr and wrmsr, which register two commands 
with the same name to read from and write to MSR.

Currently, it lacks a GP# handler, and therefore accessing a reserved or 
unimplemented MSR, results in a general protection exception and system reboot.

Changes in v3:

    - Add a cover letter to the patch series.
    - In Makefile.core.def, leave only x86.
    - Fixed incorrect #endif positioning.
    - Fixed variable types and parameter parsing.
    - Check that the CPU supports the MSR instructions.
    - Add TODO comments indicating that a GP# handler should be implemented.
    - Add a warning to the documentation, explaining that accessing a reserved
      or unimplemented MSR address results in a general protection exception.

Changes in 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 submitted with Signed-off-by.

Jesús Diéguez Fernández (2):
  Replace __asm__ __volatile__ with asm volatile.
  Add new MSR modules (rdmsr/wrmsr)

 docs/grub.texi                     |  53 +++++++++++++--
 grub-core/Makefile.core.def        |  10 +++
 grub-core/commands/efi/shim_lock.c |   2 +-
 grub-core/commands/i386/rdmsr.c    | 101 +++++++++++++++++++++++++++++
 grub-core/commands/i386/wrmsr.c    |  92 ++++++++++++++++++++++++++
 grub-core/efiemu/runtime/efiemu.c  |  10 +--
 grub-core/lib/i386/halt.c          |   2 +-
 include/grub/arm/time.h            |   2 +-
 include/grub/arm64/time.h          |   2 +-
 include/grub/i386/io.h             |  12 ++--
 include/grub/i386/rdmsr.h          |  34 ++++++++++
 include/grub/i386/time.h           |   2 +-
 include/grub/i386/tsc.h            |   2 +-
 include/grub/i386/wrmsr.h          |  32 +++++++++
 include/grub/x86_64/time.h         |   2 +-
 15 files changed, 335 insertions(+), 23 deletions(-)
 create mode 100644 grub-core/commands/i386/rdmsr.c
 create mode 100644 grub-core/commands/i386/wrmsr.c
 create mode 100644 include/grub/i386/rdmsr.h
 create mode 100644 include/grub/i386/wrmsr.h

-- 
2.17.1

[PATCH v3 1/2] Replace __asm__ __volatile__ with asm volatile.

[Jesús Diéguez Fernández]

In order to maintain the coding style consistency, it was requested to
replace the methods that use __asm__ __volatile__ with asm volatile.

Signed-off-by: Jesús Diéguez Fernández <jesusdf@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/efiemu/runtime/efiemu.c | 10 +++++-----
 grub-core/lib/i386/halt.c         |  2 +-
 include/grub/arm/time.h           |  2 +-
 include/grub/arm64/time.h         |  2 +-
 include/grub/i386/io.h            | 12 ++++++------
 include/grub/i386/time.h          |  2 +-
 include/grub/i386/tsc.h           |  2 +-
 include/grub/x86_64/time.h        |  2 +-
 8 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/grub-core/efiemu/runtime/efiemu.c b/grub-core/efiemu/runtime/efiemu.c
index d923e409f..5db1f347b 100644
--- a/grub-core/efiemu/runtime/efiemu.c
+++ b/grub-core/efiemu/runtime/efiemu.c
@@ -174,17 +174,17 @@ efiemu_memset (grub_uint8_t *a, grub_uint8_t b, grub_size_t n)
 static inline void
 write_cmos (grub_uint8_t addr, grub_uint8_t val)
 {
-  __asm__ __volatile__ ("outb %%al,$0x70\n"
-			"mov %%cl, %%al\n"
-			"outb %%al,$0x71": :"a" (addr), "c" (val));
+  asm volatile ("outb %%al,$0x70\n"
+		"mov %%cl, %%al\n"
+		"outb %%al,$0x71": :"a" (addr), "c" (val));
 }
 
 static inline grub_uint8_t
 read_cmos (grub_uint8_t addr)
 {
   grub_uint8_t ret;
-  __asm__ __volatile__ ("outb %%al, $0x70\n"
-			"inb $0x71, %%al": "=a"(ret) :"a" (addr));
+  asm volatile ("outb %%al, $0x70\n"
+		"inb $0x71, %%al": "=a"(ret) :"a" (addr));
   return ret;
 }
 
diff --git a/grub-core/lib/i386/halt.c b/grub-core/lib/i386/halt.c
index 9f8405494..2364fe4d7 100644
--- a/grub-core/lib/i386/halt.c
+++ b/grub-core/lib/i386/halt.c
@@ -66,7 +66,7 @@ grub_halt (void)
 #endif
 
   /* Disable interrupts.  */
-  __asm__ __volatile__ ("cli");
+  asm volatile ("cli");
 
   /* Bochs, QEMU, etc. Removed in newer QEMU releases.  */
   for (i = 0; i < sizeof (bochs_shutdown) - 1; i++)
diff --git a/include/grub/arm/time.h b/include/grub/arm/time.h
index 4128506cb..c5a685225 100644
--- a/include/grub/arm/time.h
+++ b/include/grub/arm/time.h
@@ -23,7 +23,7 @@ static __inline void
 grub_cpu_idle (void)
 {
   /* FIXME: this can't work until we handle interrupts.  */
-/*  __asm__ __volatile__ ("wfi"); */
+/*  asm volatile ("wfi"); */
 }
 
 #endif /* ! KERNEL_CPU_TIME_HEADER */
diff --git a/include/grub/arm64/time.h b/include/grub/arm64/time.h
index 4128506cb..c5a685225 100644
--- a/include/grub/arm64/time.h
+++ b/include/grub/arm64/time.h
@@ -23,7 +23,7 @@ static __inline void
 grub_cpu_idle (void)
 {
   /* FIXME: this can't work until we handle interrupts.  */
-/*  __asm__ __volatile__ ("wfi"); */
+/*  asm volatile ("wfi"); */
 }
 
 #endif /* ! KERNEL_CPU_TIME_HEADER */
diff --git a/include/grub/i386/io.h b/include/grub/i386/io.h
index ae12a3e3d..e9cd80957 100644
--- a/include/grub/i386/io.h
+++ b/include/grub/i386/io.h
@@ -28,7 +28,7 @@ grub_inb (unsigned short int port)
 {
   unsigned char _v;
 
-  __asm__ __volatile__ ("inb %w1,%0":"=a" (_v):"Nd" (port));
+  asm volatile ("inb %w1,%0":"=a" (_v):"Nd" (port));
   return _v;
 }
 
@@ -37,7 +37,7 @@ grub_inw (unsigned short int port)
 {
   unsigned short _v;
 
-  __asm__ __volatile__ ("inw %w1,%0":"=a" (_v):"Nd" (port));
+  asm volatile ("inw %w1,%0":"=a" (_v):"Nd" (port));
   return _v;
 }
 
@@ -46,27 +46,27 @@ grub_inl (unsigned short int port)
 {
   unsigned int _v;
 
-  __asm__ __volatile__ ("inl %w1,%0":"=a" (_v):"Nd" (port));
+  asm volatile ("inl %w1,%0":"=a" (_v):"Nd" (port));
   return _v;
 }
 
 static __inline void
 grub_outb (unsigned char value, unsigned short int port)
 {
-  __asm__ __volatile__ ("outb %b0,%w1": :"a" (value), "Nd" (port));
+  asm volatile ("outb %b0,%w1": :"a" (value), "Nd" (port));
 }
 
 static __inline void
 grub_outw (unsigned short int value, unsigned short int port)
 {
-  __asm__ __volatile__ ("outw %w0,%w1": :"a" (value), "Nd" (port));
+  asm volatile ("outw %w0,%w1": :"a" (value), "Nd" (port));
 
 }
 
 static __inline void
 grub_outl (unsigned int value, unsigned short int port)
 {
-  __asm__ __volatile__ ("outl %0,%w1": :"a" (value), "Nd" (port));
+  asm volatile ("outl %0,%w1": :"a" (value), "Nd" (port));
 }
 
 #endif /* _SYS_IO_H */
diff --git a/include/grub/i386/time.h b/include/grub/i386/time.h
index 842882cf2..4da5ae93c 100644
--- a/include/grub/i386/time.h
+++ b/include/grub/i386/time.h
@@ -23,7 +23,7 @@ static __inline void
 grub_cpu_idle (void)
 {
   /* FIXME: this can't work until we handle interrupts.  */
-/*  __asm__ __volatile__ ("hlt"); */
+/*  asm volatile ("hlt"); */
 }
 
 #endif /* ! KERNEL_CPU_TIME_HEADER */
diff --git a/include/grub/i386/tsc.h b/include/grub/i386/tsc.h
index 324174ded..947e62fa1 100644
--- a/include/grub/i386/tsc.h
+++ b/include/grub/i386/tsc.h
@@ -46,7 +46,7 @@ grub_get_tsc (void)
   grub_cpuid (0,a,b,c,d);
   /* Read TSC value.  We cannot use "=A", since this would use
      %rax on x86_64. */
-  __asm__ __volatile__ ("rdtsc":"=a" (lo), "=d" (hi));
+  asm volatile ("rdtsc":"=a" (lo), "=d" (hi));
 
   return (((grub_uint64_t) hi) << 32) | lo;
 }
diff --git a/include/grub/x86_64/time.h b/include/grub/x86_64/time.h
index 842882cf2..4da5ae93c 100644
--- a/include/grub/x86_64/time.h
+++ b/include/grub/x86_64/time.h
@@ -23,7 +23,7 @@ static __inline void
 grub_cpu_idle (void)
 {
   /* FIXME: this can't work until we handle interrupts.  */
-/*  __asm__ __volatile__ ("hlt"); */
+/*  asm volatile ("hlt"); */
 }
 
 #endif /* ! KERNEL_CPU_TIME_HEADER */
-- 
2.17.1

[PATCH v3 2/2] Add new MSR modules (rdmsr/wrmsr)

[Jesús Diéguez Fernández]

In order to be able to read from and write 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.

Also, if you specify a reserved or unimplemented MSR address, it will
cause a general protection exception (which is not currently being handled)
and the system will reboot.

Signed-off-by: Jesús Diéguez Fernández <jesusdf@gmail.com>
---
 docs/grub.texi                     |  53 +++++++++++++--
 grub-core/Makefile.core.def        |  10 +++
 grub-core/commands/efi/shim_lock.c |   2 +-
 grub-core/commands/i386/rdmsr.c    | 101 +++++++++++++++++++++++++++++
 grub-core/commands/i386/wrmsr.c    |  92 ++++++++++++++++++++++++++
 include/grub/i386/rdmsr.h          |  34 ++++++++++
 include/grub/i386/wrmsr.h          |  32 +++++++++
 7 files changed, 318 insertions(+), 6 deletions(-)
 create mode 100644 grub-core/commands/i386/rdmsr.c
 create mode 100644 grub-core/commands/i386/wrmsr.c
 create mode 100644 include/grub/i386/rdmsr.h
 create mode 100644 include/grub/i386/wrmsr.h

diff --git a/docs/grub.texi b/docs/grub.texi
index ecaba9d5c..4a636319f 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3931,6 +3931,7 @@ you forget a command, you can run the command @command{help}
 * play::                        Play a tune
 * probe::                       Retrieve device info
 * pxe_unload::                  Unload the PXE environment
+* rdmsr::                       Read values from model-specific registers
 * read::                        Read user input
 * reboot::                      Reboot your computer
 * regexp::                      Test if regular expression matches string
@@ -3953,6 +3954,7 @@ you forget a command, you can run the command @command{help}
 * verify_detached::             Verify detached digital signature
 * videoinfo::                   List available video modes
 @comment * xen_*::              Xen boot commands for AArch64
+* wrmsr::                       Write values to model-specific registers
 * xen_hypervisor::              Load xen hypervisor binary (only on AArch64)
 * xen_module::                  Load xen modules for xen hypervisor (only on AArch64)
 @end menu
@@ -4785,6 +4787,24 @@ This command is only available on PC BIOS systems.
 @end deffn
 
 
+@node rdmsr
+@subsection rdmsr
+
+@deffn Command: rdmsr 0xADDR [-v VARNAME]
+Read a model-specific register at address 0xADDR. If the parameter
+@option{-v} is used and an environment variable @var{VARNAME} is 
+given, set that environment variable to the value that was read.
+
+Please note that on SMP systems, reading from a MSR that has a
+scope per hardware thread, implies that the value that is returned
+only applies to the particular cpu/core/thread that runs the command.
+
+Also, if you specify a reserved or unimplemented MSR address, it will 
+cause a general protection exception (which is not currently being handled)
+and the system will reboot.
+@end deffn
+
+
 @node read
 @subsection read
 
@@ -5223,6 +5243,21 @@ successfully.  If validation fails, it is set to a non-zero value.
 List available video modes. If resolution is given, show only matching modes.
 @end deffn
 
+@node wrmsr
+@subsection wrmsr
+
+@deffn Command: wrmsr 0xADDR 0xVALUE
+Write a 0xVALUE to a model-specific register at address 0xADDR.
+
+Please note that on SMP systems, writing to a MSR that has a scope 
+per hardware thread, implies that the value that is written
+only applies to the particular cpu/core/thread that runs the command.
+
+Also, if you specify a reserved or unimplemented MSR address, it will 
+cause a general protection exception (which is not currently being handled)
+and the system will reboot.
+@end deffn
+
 @node xen_hypervisor
 @subsection xen_hypervisor
 
@@ -5716,11 +5751,11 @@ boot and the shim. This functionality is provided by the shim_lock module. It
 is recommend to build in this and other required modules into the @file{core.img}.
 All modules not stored in the @file{core.img} and the ACPI tables for the
 @command{acpi} command have to be signed, e.g. using PGP. Additionally, the
-@command{iorw} and the @command{memrw} commands are prohibited if the UEFI
-secure boot is enabled. This is done due to security reasons. All above
-mentioned requirements are enforced by the shim_lock module. And itself it
-is a persistent module which means that it cannot be unloaded if it was
-loaded into the memory.
+@command{iorw}, the @command{memrw} and the @command{wrmsr} commands are 
+prohibited if the UEFI secure boot is enabled.  This is done due to 
+security reasons.  All above mentioned requirements are enforced by the 
+shim_lock module. And itself it is a persistent module which means that 
+it cannot be unloaded if it was loaded into the memory.
 
 @node Measured Boot
 @section Measuring boot components
@@ -5831,6 +5866,8 @@ to install to is specified, UUID is used instead as well.
 @item USB                @tab yes     @tab yes      @tab yes          @tab yes
 @item chainloader        @tab local   @tab yes      @tab yes          @tab no
 @item cpuid              @tab partial @tab partial  @tab partial      @tab partial
+@item rdmsr              @tab partial @tab partial  @tab partial      @tab partial
+@item wrmsr              @tab partial @tab partial  @tab partial      @tab partial
 @item hints              @tab guess   @tab guess    @tab guess        @tab guess
 @item PCI                @tab yes     @tab yes      @tab yes          @tab yes
 @item badram             @tab yes     @tab yes      @tab yes          @tab yes
@@ -5850,6 +5887,8 @@ to install to is specified, UUID is used instead as well.
 @item USB                @tab yes         @tab yes       @tab yes           @tab no
 @item chainloader        @tab local       @tab local     @tab no            @tab local
 @item cpuid              @tab partial     @tab partial   @tab partial       @tab no
+@item rdmsr              @tab partial     @tab partial   @tab partial       @tab no
+@item wrmsr              @tab partial     @tab partial   @tab partial       @tab no
 @item hints              @tab guess       @tab guess     @tab good          @tab guess
 @item PCI                @tab yes         @tab yes       @tab yes           @tab no
 @item badram             @tab yes         @tab yes       @tab no            @tab yes
@@ -5869,6 +5908,8 @@ to install to is specified, UUID is used instead as well.
 @item USB                @tab yes         @tab no      @tab no      @tab no
 @item chainloader        @tab yes         @tab no      @tab no      @tab no
 @item cpuid              @tab no          @tab no      @tab no      @tab no
+@item rdmsr              @tab no          @tab no      @tab no      @tab no
+@item wrmsr              @tab no          @tab no      @tab no      @tab no
 @item hints              @tab good        @tab good    @tab good    @tab no
 @item PCI                @tab yes         @tab no      @tab no      @tab no
 @item badram             @tab yes (*)     @tab no      @tab no      @tab no
@@ -5888,6 +5929,8 @@ to install to is specified, UUID is used instead as well.
 @item USB                @tab N/A       @tab yes         @tab no
 @item chainloader        @tab yes       @tab no          @tab yes
 @item cpuid              @tab no        @tab no          @tab yes
+@item rdmsr              @tab no        @tab no          @tab yes
+@item wrmsr              @tab no        @tab no          @tab yes
 @item hints              @tab guess     @tab no          @tab no
 @item PCI                @tab no        @tab no          @tab no
 @item badram             @tab yes (*)   @tab no          @tab no
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 58df4ab07..dbc7eb3a5 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2484,3 +2484,13 @@ module = {
   common = loader/i386/xen_file64.c;
   extra_dist = loader/i386/xen_fileXX.c;
 };
+module = {
+  name = rdmsr;
+  common = commands/i386/rdmsr.c;
+  enable = x86;
+};
+module = {
+  name = wrmsr;
+  common = commands/i386/wrmsr.c;
+  enable = x86;
+};
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..0ec91e2e5
--- /dev/null
+++ b/grub-core/commands/i386/rdmsr.c
@@ -0,0 +1,101 @@
+/* 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>
+#include <grub/cpu/cpuid.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_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
+    grub_uint64_t value;
+    char *ptr;
+    char buf[sizeof("1122334455667788")];
+
+    /* The CPUID instruction should be used to determine whether MSRs 
+       are supported. (CPUID.01H:EDX[5] = 1) */
+    if (! grub_cpu_is_cpuid_supported ())
+        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+
+    grub_cpuid (0, max_cpuid, manufacturer[0], manufacturer[2], manufacturer[1]);
+
+    if (max_cpuid < 1)
+        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+
+    grub_cpuid (1, a, b, c, features);
+
+    if (! (features & (1 << 5)))
+        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+
+    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;
+    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..8ebc21150
--- /dev/null
+++ b/grub-core/commands/i386/wrmsr.c
@@ -0,0 +1,92 @@
+/* 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>
+#include <grub/cpu/cpuid.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_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
+    grub_uint64_t value;
+    char *ptr;
+
+    /* The CPUID instruction should be used to determine whether MSRs 
+       are supported. (CPUID.01H:EDX[5] = 1) */
+    if (! grub_cpu_is_cpuid_supported ())
+        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+
+    grub_cpuid (0, max_cpuid, manufacturer[0], manufacturer[2], manufacturer[1]);
+
+    if (max_cpuid < 1)
+        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+
+    grub_cpuid (1, a, b, c, features);
+
+    if (! (features & (1 << 5)))
+        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+
+    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;
+    if (*ptr != '\0')
+        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
+
+    ptr = argv[1];
+    value = grub_strtoull (ptr, &ptr, 0);
+
+    if (grub_errno != GRUB_ERR_NONE)
+        return grub_errno;
+    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..f6d7b72ca
--- /dev/null
+++ b/include/grub/i386/rdmsr.h
@@ -0,0 +1,34 @@
+/*
+ *  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_RDMSR_H
+#define GRUB_RDMSR_H 1
+
+/* TODO: Add a general protection exception handler.
+         Accessing a reserved or unimplemented MSR address results in a GP#. */
+
+extern __inline grub_uint64_t grub_msr_read (grub_uint32_t msr_id)
+{
+    grub_uint32_t low, high;
+
+    asm volatile ( "rdmsr"  : "=a"(low), "=d"(high) : "c"(msr_id) );
+
+    return ((grub_uint64_t)high << 32) | low;
+}
+
+#endif /* GRUB_RDMSR_H */
diff --git a/include/grub/i386/wrmsr.h b/include/grub/i386/wrmsr.h
new file mode 100644
index 000000000..f20f5fdf2
--- /dev/null
+++ b/include/grub/i386/wrmsr.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_WRMSR_H
+#define GRUB_WRMSR_H 1
+
+/* TODO: Add a general protection exception handler.
+         Accessing a reserved or unimplemented MSR address results in a GP#. */
+
+extern __inline void grub_msr_write(grub_uint32_t msr_id, grub_uint64_t msr_value)
+{
+    grub_uint32_t low = msr_value, high = msr_value >> 32;
+
+    asm volatile ( "wrmsr" : : "c"(msr_id), "a"(low), "d"(high) );
+}
+
+#endif /* GRUB_WRMSR_H */
-- 
2.17.1

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

[Daniel Kiper]

On Fri, Mar 08, 2019 at 01:26:37AM +0100, Jesús Diéguez Fernández wrote:
> In order to be able to read from and write 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.
>
> Also, if you specify a reserved or unimplemented MSR address, it will
> cause a general protection exception (which is not currently being handled)
> and the system will reboot.
>
> Signed-off-by: Jesús Diéguez Fernández <jesusdf@gmail.com>

LGTM. Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

There are a few comments nitpicks which I fix before committing the patches.

If there are no objections I will commit the patches next week.

Thank you for doing the work.

Daniel

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

[Daniel Kiper]

On Fri, Mar 08, 2019 at 12:14:15PM +0100, Daniel Kiper wrote:
> On Fri, Mar 08, 2019 at 01:26:37AM +0100, Jesús Diéguez Fernández wrote:
> > In order to be able to read from and write 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.
> >
> > Also, if you specify a reserved or unimplemented MSR address, it will
> > cause a general protection exception (which is not currently being handled)
> > and the system will reboot.
> >
> > Signed-off-by: Jesús Diéguez Fernández <jesusdf@gmail.com>
>
> LGTM. Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>
> There are a few comments nitpicks which I fix before committing the patches.
>
> If there are no objections I will commit the patches next week.
>
> Thank you for doing the work.

I had to tweak your patches because they broke build at least for coreboot
and x86-64 UEFI. Now they are in.

Daniel

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

[Jesús Diéguez Fernández]

El 12/3/19 a las 20:12, Daniel Kiper escribió:
> On Fri, Mar 08, 2019 at 12:14:15PM +0100, Daniel Kiper wrote:
>> On Fri, Mar 08, 2019 at 01:26:37AM +0100, Jesús Diéguez Fernández wrote:
>>> In order to be able to read from and write 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.
>>>
>>> Also, if you specify a reserved or unimplemented MSR address, it will
>>> cause a general protection exception (which is not currently being handled)
>>> and the system will reboot.
>>>
>>> Signed-off-by: Jesús Diéguez Fernández <jesusdf@gmail.com>
>>
>> LGTM. Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>>
>> There are a few comments nitpicks which I fix before committing the patches.
>>
>> If there are no objections I will commit the patches next week.
>>
>> Thank you for doing the work.
> 
> I had to tweak your patches because they broke build at least for coreboot
> and x86-64 UEFI. Now they are in.
> 
> Daniel
> 

Sorry to hear that. I'm fine with the changes, specially with the error
fixes.

To prevent repeating the same mistakes, I've compared the patches and
I've spot the following differences:

	+ Two missing casts to unsigned long long.
	+ An unused parameter in wrmsr command.
	- The includes should be sorted alphabetically.
	- Phrases in comments should end with a dot.
	- Some spacing and line adjustment changes.
	- Indent with two spaces.
	- Multi-line comments reformatted with asterisks.

Did I miss anything else?

About the formatting issues, before the v2 I found the GNU Indent
program. I tested it but it didn't quite follow up the final desired
format, so I didn't use it at all.
Do you use it or do you normally adjust everything manually?

I'm used to work in different environments with different coding styles
and rules (although all of them are less restrictive than GRUB), so I
tried to mimic the style of other preexisting files. Next time I need
more time and care before submitting the patch.

About the multi-line comments, I think that maybe they were ok? (at
least by the grub-dev docs specific rules):

Comments spanning multiple lines shall be formatted with all lines after
the first aligned with the first line.

Asterisk characters should not be repeated a the start of each
subsequent line.

Acceptable:

/* This is a comment
   which spans multiple lines.
   It is long.  */

Unacceptable:

/*
 * This is a comment
 * which spans multiple lines.
 * It is long. */


Thank you for your time and patience.

Jesus

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

[Daniel Kiper]

On Wed, Mar 13, 2019 at 12:35:48AM +0100, Jesús Diéguez Fernández wrote:
> El 12/3/19 a las 20:12, Daniel Kiper escribió:
> > On Fri, Mar 08, 2019 at 12:14:15PM +0100, Daniel Kiper wrote:
> >> On Fri, Mar 08, 2019 at 01:26:37AM +0100, Jesús Diéguez Fernández wrote:
> >>> In order to be able to read from and write 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.
> >>>
> >>> Also, if you specify a reserved or unimplemented MSR address, it will
> >>> cause a general protection exception (which is not currently being handled)
> >>> and the system will reboot.
> >>>
> >>> Signed-off-by: Jesús Diéguez Fernández <jesusdf@gmail.com>
> >>
> >> LGTM. Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> >>
> >> There are a few comments nitpicks which I fix before committing the patches.
> >>
> >> If there are no objections I will commit the patches next week.
> >>
> >> Thank you for doing the work.
> >
> > I had to tweak your patches because they broke build at least for coreboot
> > and x86-64 UEFI. Now they are in.
> >
> > Daniel
> >
>
> Sorry to hear that. I'm fine with the changes, specially with the error
> fixes.

Great!

> To prevent repeating the same mistakes, I've compared the patches and
> I've spot the following differences:
>
> 	+ Two missing casts to unsigned long long.
> 	+ An unused parameter in wrmsr command.

Yep! These were the main culprits.

> 	- The includes should be sorted alphabetically.

This is a matter of taste. I just like this if possible.
I have changed the order because I had to change path
for cpuid.h file too. However, I am not going to insist
alphabetic order too much here.

> 	- Phrases in comments should end with a dot.

Again I like this but there is no strict requirement for that.

> 	- Some spacing and line adjustment changes.
> 	- Indent with two spaces.

Yes, I have missed that during review. In general two spaces of
TABs plus spaces if there are more than 8 spaces.

> 	- Multi-line comments reformatted with asterisks.

Yes, please.

> Did I miss anything else?

I think that the only change in cpuid.h path.

> About the formatting issues, before the v2 I found the GNU Indent
> program. I tested it but it didn't quite follow up the final desired
> format, so I didn't use it at all.
> Do you use it or do you normally adjust everything manually?

Right now manually but I would like to get it automated at some point.
This is getting boring...

> I'm used to work in different environments with different coding styles
> and rules (although all of them are less restrictive than GRUB), so I
> tried to mimic the style of other preexisting files. Next time I need
> more time and care before submitting the patch.

Do not worry. This is a nightmare in the GRUB code because it is
a mixture of various styles. I think that the problem is that the
style was not enforced much earlier. I am going to change that and
automate most of the task if time allows.

> About the multi-line comments, I think that maybe they were ok? (at
> least by the grub-dev docs specific rules):
>
> Comments spanning multiple lines shall be formatted with all lines after
> the first aligned with the first line.
>
> Asterisk characters should not be repeated a the start of each
> subsequent line.
>
> Acceptable:
>
> /* This is a comment
>    which spans multiple lines.
>    It is long.  */
>
> Unacceptable:
>
> /*
>  * This is a comment
>  * which spans multiple lines.
>  * It is long. */

Yeah, I had to fix it. In general I do not like the former because in
long comments with code snippets sometimes it is not easy to catch the
difference between the real code and comment. So, I prefer the latter
with the last "*/" put alone in last comment line. However, you are
right that I had to update the docs. I will update it.

> Thank you for your time and patience.

You are welcome.

Daniel

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

[Eric Snowberg]

> On Mar 7, 2019, at 5:26 PM, Jesús Diéguez Fernández <jesusdf@gmail.com> wrote:
> 
> In order to be able to read from and write 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.
> 
> Also, if you specify a reserved or unimplemented MSR address, it will
> cause a general protection exception (which is not currently being handled)
> and the system will reboot.
> 
> Signed-off-by: Jesús Diéguez Fernández <jesusdf@gmail.com>
> ---
> docs/grub.texi                     |  53 +++++++++++++--
> grub-core/Makefile.core.def        |  10 +++
> grub-core/commands/efi/shim_lock.c |   2 +-
> grub-core/commands/i386/rdmsr.c    | 101 +++++++++++++++++++++++++++++
> grub-core/commands/i386/wrmsr.c    |  92 ++++++++++++++++++++++++++
> include/grub/i386/rdmsr.h          |  34 ++++++++++
> include/grub/i386/wrmsr.h          |  32 +++++++++
> 7 files changed, 318 insertions(+), 6 deletions(-)
> create mode 100644 grub-core/commands/i386/rdmsr.c
> create mode 100644 grub-core/commands/i386/wrmsr.c
> create mode 100644 include/grub/i386/rdmsr.h
> create mode 100644 include/grub/i386/wrmsr.h
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index ecaba9d5c..4a636319f 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3931,6 +3931,7 @@ you forget a command, you can run the command @command{help}
> * play::                        Play a tune
> * probe::                       Retrieve device info
> * pxe_unload::                  Unload the PXE environment
> +* rdmsr::                       Read values from model-specific registers
> * read::                        Read user input
> * reboot::                      Reboot your computer
> * regexp::                      Test if regular expression matches string
> @@ -3953,6 +3954,7 @@ you forget a command, you can run the command @command{help}
> * verify_detached::             Verify detached digital signature
> * videoinfo::                   List available video modes
> @comment * xen_*::              Xen boot commands for AArch64
> +* wrmsr::                       Write values to model-specific registers
> * xen_hypervisor::              Load xen hypervisor binary (only on AArch64)
> * xen_module::                  Load xen modules for xen hypervisor (only on AArch64)
> @end menu
> @@ -4785,6 +4787,24 @@ This command is only available on PC BIOS systems.
> @end deffn
> 
> 
> +@node rdmsr
> +@subsection rdmsr
> +
> +@deffn Command: rdmsr 0xADDR [-v VARNAME]
> +Read a model-specific register at address 0xADDR. If the parameter
> +@option{-v} is used and an environment variable @var{VARNAME} is 
> +given, set that environment variable to the value that was read.
> +
> +Please note that on SMP systems, reading from a MSR that has a
> +scope per hardware thread, implies that the value that is returned
> +only applies to the particular cpu/core/thread that runs the command.
> +
> +Also, if you specify a reserved or unimplemented MSR address, it will 
> +cause a general protection exception (which is not currently being handled)
> +and the system will reboot.
> +@end deffn
> +
> +
> @node read
> @subsection read
> 
> @@ -5223,6 +5243,21 @@ successfully.  If validation fails, it is set to a non-zero value.
> List available video modes. If resolution is given, show only matching modes.
> @end deffn
> 
> +@node wrmsr
> +@subsection wrmsr
> +
> +@deffn Command: wrmsr 0xADDR 0xVALUE
> +Write a 0xVALUE to a model-specific register at address 0xADDR.
> +
> +Please note that on SMP systems, writing to a MSR that has a scope 
> +per hardware thread, implies that the value that is written
> +only applies to the particular cpu/core/thread that runs the command.
> +
> +Also, if you specify a reserved or unimplemented MSR address, it will 
> +cause a general protection exception (which is not currently being handled)
> +and the system will reboot.
> +@end deffn
> +
> @node xen_hypervisor
> @subsection xen_hypervisor
> 
> @@ -5716,11 +5751,11 @@ boot and the shim. This functionality is provided by the shim_lock module. It
> is recommend to build in this and other required modules into the @file{core.img}.
> All modules not stored in the @file{core.img} and the ACPI tables for the
> @command{acpi} command have to be signed, e.g. using PGP. Additionally, the
> -@command{iorw} and the @command{memrw} commands are prohibited if the UEFI
> -secure boot is enabled. This is done due to security reasons. All above
> -mentioned requirements are enforced by the shim_lock module. And itself it
> -is a persistent module which means that it cannot be unloaded if it was
> -loaded into the memory.
> +@command{iorw}, the @command{memrw} and the @command{wrmsr} commands are 
> +prohibited if the UEFI secure boot is enabled.  This is done due to 
> +security reasons.  All above mentioned requirements are enforced by the 
> +shim_lock module. And itself it is a persistent module which means that 
> +it cannot be unloaded if it was loaded into the memory.
> 
> @node Measured Boot
> @section Measuring boot components
> @@ -5831,6 +5866,8 @@ to install to is specified, UUID is used instead as well.
> @item USB                @tab yes     @tab yes      @tab yes          @tab yes
> @item chainloader        @tab local   @tab yes      @tab yes          @tab no
> @item cpuid              @tab partial @tab partial  @tab partial      @tab partial
> +@item rdmsr              @tab partial @tab partial  @tab partial      @tab partial
> +@item wrmsr              @tab partial @tab partial  @tab partial      @tab partial
> @item hints              @tab guess   @tab guess    @tab guess        @tab guess
> @item PCI                @tab yes     @tab yes      @tab yes          @tab yes
> @item badram             @tab yes     @tab yes      @tab yes          @tab yes
> @@ -5850,6 +5887,8 @@ to install to is specified, UUID is used instead as well.
> @item USB                @tab yes         @tab yes       @tab yes           @tab no
> @item chainloader        @tab local       @tab local     @tab no            @tab local
> @item cpuid              @tab partial     @tab partial   @tab partial       @tab no
> +@item rdmsr              @tab partial     @tab partial   @tab partial       @tab no
> +@item wrmsr              @tab partial     @tab partial   @tab partial       @tab no
> @item hints              @tab guess       @tab guess     @tab good          @tab guess
> @item PCI                @tab yes         @tab yes       @tab yes           @tab no
> @item badram             @tab yes         @tab yes       @tab no            @tab yes
> @@ -5869,6 +5908,8 @@ to install to is specified, UUID is used instead as well.
> @item USB                @tab yes         @tab no      @tab no      @tab no
> @item chainloader        @tab yes         @tab no      @tab no      @tab no
> @item cpuid              @tab no          @tab no      @tab no      @tab no
> +@item rdmsr              @tab no          @tab no      @tab no      @tab no
> +@item wrmsr              @tab no          @tab no      @tab no      @tab no
> @item hints              @tab good        @tab good    @tab good    @tab no
> @item PCI                @tab yes         @tab no      @tab no      @tab no
> @item badram             @tab yes (*)     @tab no      @tab no      @tab no
> @@ -5888,6 +5929,8 @@ to install to is specified, UUID is used instead as well.
> @item USB                @tab N/A       @tab yes         @tab no
> @item chainloader        @tab yes       @tab no          @tab yes
> @item cpuid              @tab no        @tab no          @tab yes
> +@item rdmsr              @tab no        @tab no          @tab yes
> +@item wrmsr              @tab no        @tab no          @tab yes
> @item hints              @tab guess     @tab no          @tab no
> @item PCI                @tab no        @tab no          @tab no
> @item badram             @tab yes (*)   @tab no          @tab no
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 58df4ab07..dbc7eb3a5 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -2484,3 +2484,13 @@ module = {
>   common = loader/i386/xen_file64.c;
>   extra_dist = loader/i386/xen_fileXX.c;
> };
> +module = {
> +  name = rdmsr;
> +  common = commands/i386/rdmsr.c;
> +  enable = x86;
> +};
> +module = {
> +  name = wrmsr;
> +  common = commands/i386/wrmsr.c;
> +  enable = x86;
> +};
> 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..0ec91e2e5
> --- /dev/null
> +++ b/grub-core/commands/i386/rdmsr.c
> @@ -0,0 +1,101 @@
> +/* 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>
> +#include <grub/cpu/cpuid.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_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
> +    grub_uint64_t value;
> +    char *ptr;
> +    char buf[sizeof("1122334455667788")];
> +
> +    /* The CPUID instruction should be used to determine whether MSRs 
> +       are supported. (CPUID.01H:EDX[5] = 1) */
> +    if (! grub_cpu_is_cpuid_supported ())
> +        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
> +
> +    grub_cpuid (0, max_cpuid, manufacturer[0], manufacturer[2], manufacturer[1]);
> +
> +    if (max_cpuid < 1)
> +        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
> +
> +    grub_cpuid (1, a, b, c, features);
> +
> +    if (! (features & (1 << 5)))
> +        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
> +
> +    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;
> +    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..8ebc21150
> --- /dev/null
> +++ b/grub-core/commands/i386/wrmsr.c
> @@ -0,0 +1,92 @@
> +/* 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>
> +#include <grub/cpu/cpuid.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_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
> +    grub_uint64_t value;
> +    char *ptr;
> +
> +    /* The CPUID instruction should be used to determine whether MSRs 
> +       are supported. (CPUID.01H:EDX[5] = 1) */
> +    if (! grub_cpu_is_cpuid_supported ())
> +        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
> +
> +    grub_cpuid (0, max_cpuid, manufacturer[0], manufacturer[2], manufacturer[1]);
> +
> +    if (max_cpuid < 1)
> +        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
> +
> +    grub_cpuid (1, a, b, c, features);
> +
> +    if (! (features & (1 << 5)))
> +        return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
> +
> +    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;
> +    if (*ptr != '\0')
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
> +
> +    ptr = argv[1];
> +    value = grub_strtoull (ptr, &ptr, 0);
> +
> +    if (grub_errno != GRUB_ERR_NONE)
> +        return grub_errno;
> +    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..f6d7b72ca
> --- /dev/null
> +++ b/include/grub/i386/rdmsr.h
> @@ -0,0 +1,34 @@
> +/*
> + *  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_RDMSR_H
> +#define GRUB_RDMSR_H 1
> +
> +/* TODO: Add a general protection exception handler.
> +         Accessing a reserved or unimplemented MSR address results in a GP#. */
> +
> +extern __inline grub_uint64_t grub_msr_read (grub_uint32_t msr_id)

I’m seeing a compile error with this patch:

In file included from commands/i386/rdmsr.c:29:0:
../include/grub/i386/rdmsr.h:27:29: error: no previous prototype for _grub_msr_read_ [-Werror=missing-prototypes]
 extern inline grub_uint64_t grub_msr_read (grub_uint32_t msr_id)
                             ^
cc1: all warnings being treated as errors

> +{
> +    grub_uint32_t low, high;
> +
> +    asm volatile ( "rdmsr"  : "=a"(low), "=d"(high) : "c"(msr_id) );
> +
> +    return ((grub_uint64_t)high << 32) | low;
> +}
> +
> +#endif /* GRUB_RDMSR_H */
> diff --git a/include/grub/i386/wrmsr.h b/include/grub/i386/wrmsr.h
> new file mode 100644
> index 000000000..f20f5fdf2
> --- /dev/null
> +++ b/include/grub/i386/wrmsr.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_WRMSR_H
> +#define GRUB_WRMSR_H 1
> +
> +/* TODO: Add a general protection exception handler.
> +         Accessing a reserved or unimplemented MSR address results in a GP#. */
> +
> +extern __inline void grub_msr_write(grub_uint32_t msr_id, grub_uint64_t msr_value)

Also here:

In file included from commands/i386/wrmsr.c:29:0:
../include/grub/i386/wrmsr.h:27:20: error: no previous prototype for _grub_msr_write_ [-Werror=missing-prototypes]
 extern inline void grub_msr_write(grub_uint32_t msr_id, grub_uint64_t msr_value)
                    ^
cc1: all warnings being treated as errors

I believe it would be best to either provide a global definition for the function, or declare the functions as static inline, or move them into the c file, since neither is used elsewhere. If you would like me to fix it, let me know the approach you would like to see done.

> +{
> +    grub_uint32_t low = msr_value, high = msr_value >> 32;
> +
> +    asm volatile ( "wrmsr" : : "c"(msr_id), "a"(low), "d"(high) );
> +}
> +
> +#endif /* GRUB_WRMSR_H */
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Jesús DF]

El vie., 22 mar. 2019 a las 0:24, Eric Snowberg
(<eric.snowberg@oracle.com>) escribió:
>
>
> > On Mar 7, 2019, at 5:26 PM, Jesús Diéguez Fernández <jesusdf@gmail.com> wrote:
> >
> > In order to be able to read from and write to model-specific registers,
> > two new modules are added. They are i386 specific, as the cpuid module.

[...]

> > +extern __inline void grub_msr_write(grub_uint32_t msr_id, grub_uint64_t msr_value)
>
> Also here:
>
> In file included from commands/i386/wrmsr.c:29:0:
> ../include/grub/i386/wrmsr.h:27:20: error: no previous prototype for _grub_msr_write_ [-Werror=missing-prototypes]
>  extern inline void grub_msr_write(grub_uint32_t msr_id, grub_uint64_t msr_value)
>                     ^
> cc1: all warnings being treated as errors
>
> I believe it would be best to either provide a global definition for the function, or declare the functions as static inline, or move them into the c file, since neither is used elsewhere. If you would like me to fix it, let me know the approach you would like to see done.
>

I would add the missing prototype. If you can fix it, I appreciate it.
If you are busy I can send the patch late this night or tomorrow.

Thank you.

> > +{
> > +    grub_uint32_t low = msr_value, high = msr_value >> 32;
> > +
> > +    asm volatile ( "wrmsr" : : "c"(msr_id), "a"(low), "d"(high) );
> > +}
> > +
> > +#endif /* GRUB_WRMSR_H */
> > --
> > 2.17.1
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

[Daniel Kiper]

On Thu, Mar 21, 2019 at 05:23:58PM -0600, Eric Snowberg wrote:
> > On Mar 7, 2019, at 5:26 PM, Jesús Diéguez Fernández <jesusdf@gmail.com> wrote:

[...]

> > diff --git a/include/grub/i386/rdmsr.h b/include/grub/i386/rdmsr.h
> > new file mode 100644
> > index 000000000..f6d7b72ca
> > --- /dev/null
> > +++ b/include/grub/i386/rdmsr.h
> > @@ -0,0 +1,34 @@
> > +/*
> > + *  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_RDMSR_H
> > +#define GRUB_RDMSR_H 1
> > +
> > +/* TODO: Add a general protection exception handler.
> > +         Accessing a reserved or unimplemented MSR address results in a GP#. */
> > +
> > +extern __inline grub_uint64_t grub_msr_read (grub_uint32_t msr_id)
>
> I’m seeing a compile error with this patch:
>
> In file included from commands/i386/rdmsr.c:29:0:
> ../include/grub/i386/rdmsr.h:27:29: error: no previous prototype for _grub_msr_read_ [-Werror=missing-prototypes]
>  extern inline grub_uint64_t grub_msr_read (grub_uint32_t msr_id)
>                              ^
> cc1: all warnings being treated as errors

I am not able to reproduce this. Eric, could you tell us more about your
build environment and how you compile the GRUB?

Anyway, I missed that during review but

extern inline grub_uint64_t grub_msr_read (grub_uint32_t msr_id)
{
...

should be changed to

grub_uint64_t inline
grub_msr_read (grub_uint32_t msr_id)
{
...

Jesús, please post the fix for that.

Daniel

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

[Jesús Diéguez Fernández]

El 22/3/19 a las 13:16, Daniel Kiper escribió:
> On Thu, Mar 21, 2019 at 05:23:58PM -0600, Eric Snowberg wrote:
>>> On Mar 7, 2019, at 5:26 PM, Jesús Diéguez Fernández <jesusdf@gmail.com> wrote:
> 
> [...]
> 
>>> diff --git a/include/grub/i386/rdmsr.h b/include/grub/i386/rdmsr.h
>>> new file mode 100644
>>> index 000000000..f6d7b72ca
>>> --- /dev/null
>>> +++ b/include/grub/i386/rdmsr.h
>>> @@ -0,0 +1,34 @@
>>> +/*
>>> + *  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_RDMSR_H
>>> +#define GRUB_RDMSR_H 1
>>> +
>>> +/* TODO: Add a general protection exception handler.
>>> +         Accessing a reserved or unimplemented MSR address results in a GP#. */
>>> +
>>> +extern __inline grub_uint64_t grub_msr_read (grub_uint32_t msr_id)
>>
>> I’m seeing a compile error with this patch:
>>
>> In file included from commands/i386/rdmsr.c:29:0:
>> ../include/grub/i386/rdmsr.h:27:29: error: no previous prototype for _grub_msr_read_ [-Werror=missing-prototypes]
>>  extern inline grub_uint64_t grub_msr_read (grub_uint32_t msr_id)
>>                              ^
>> cc1: all warnings being treated as errors
> 
> I am not able to reproduce this. Eric, could you tell us more about your
> build environment and how you compile the GRUB?
> 
> Anyway, I missed that during review but
> 
> extern inline grub_uint64_t grub_msr_read (grub_uint32_t msr_id)
> {
> ...
> 
> should be changed to
> 
> grub_uint64_t inline
> grub_msr_read (grub_uint32_t msr_id)
> {
> ...
> 
> Jesús, please post the fix for that.
> 
> Daniel
> 

Done. I inverted the order (inline first) to prevent this warning:

In file included from commands/i386/rdmsr.c:29:0:
../include/grub/i386/rdmsr.h:27:1: warning: ‘inline’ is not at beginning
of declaration [-Wold-style-declaration]
 grub_uint64_t inline
 ^~~~~~~~~~~~~
In file included from commands/i386/wrmsr.c:29:0:
../include/grub/i386/wrmsr.h:27:1: warning: ‘inline’ is not at beginning
of declaration [-Wold-style-declaration]
 void inline
 ^~~~

I also added the missing prototypes.

Jesus.