[PATCH 0/1] New MSR module

[PATCH 0/1] New MSR module

[JesusDF]

Hi all,
    I'm completely new to this list and I've never sent a patch before, so excuse me if I missed anything.
    I've built a new grub module to access MSR. I've done it since it allows you to make tweaks before any OS boots.
    It also matches an old request in this same list: https://lists.gnu.org/archive/html/grub-devel/2012-09/msg00089.html

Cheers
    Jesus

JesusDF (1):
  Add a new grub module called msr that registers two commands (rdmsr
    and wrmsr) to be able to read and write to the model-specific
    registers. It is i386 specific, as the cpuid module. The name of the
    module and commands match the linux kernel module and intel commands
    to interact with it.

 grub-core/Makefile.core.def   |  10 ++++
 grub-core/commands/i386/msr.c | 106 ++++++++++++++++++++++++++++++++++
 include/grub/i386/msr.h       |  67 +++++++++++++++++++++
 3 files changed, 183 insertions(+)
 create mode 100644 grub-core/commands/i386/msr.c
 create mode 100644 include/grub/i386/msr.h

-- 
2.17.1

[PATCH 1/1] Add a new grub module called msr that registers two commands (rdmsr and wrmsr) to be able to read and write to the model-specific registers. It is i386 specific, as the cpuid module. The name of the module and commands match the linux kernel module and intel commands to interact with it.

[JesusDF]

---
 grub-core/Makefile.core.def   |  10 ++++
 grub-core/commands/i386/msr.c | 106 ++++++++++++++++++++++++++++++++++
 include/grub/i386/msr.h       |  67 +++++++++++++++++++++
 3 files changed, 183 insertions(+)
 create mode 100644 grub-core/commands/i386/msr.c
 create mode 100644 include/grub/i386/msr.h

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index e16fb06ba..836a353d0 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2455,3 +2455,13 @@ module = {
   common = loader/i386/xen_file64.c;
   extra_dist = loader/i386/xen_fileXX.c;
 };
+
+
+module = {
+  name = msr;
+  common = commands/i386/msr.c;
+  enable = x86;
+  enable = i386_xen_pvh;
+  enable = i386_xen;
+  enable = x86_64_xen;
+};
diff --git a/grub-core/commands/i386/msr.c b/grub-core/commands/i386/msr.c
new file mode 100644
index 000000000..ea63adf97
--- /dev/null
+++ b/grub-core/commands/i386/msr.c
@@ -0,0 +1,106 @@
+/* msr.c - Read or write CPU model specific registers */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2006, 2007, 2009  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/msr.h>
+#include <grub/i18n.h>
+
+GRUB_MOD_LICENSE("GPLv3+");
+
+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;
+    grub_uint64_t value;
+    char buf[sizeof("XXXXXXXX")];
+
+    if (argc != 1)
+    {
+        return grub_error(GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
+    }
+
+    addr = grub_strtoul(argv[0], 0, 0);
+
+    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;
+}
+
+static grub_err_t
+grub_cmd_msr_write(grub_command_t cmd, int argc, char **argv)
+{
+    grub_addr_t addr;
+    grub_uint32_t value;
+
+    if (argc != 2)
+    {
+        return grub_error(GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
+    }
+
+    addr = grub_strtoul(argv[0], 0, 0);
+    value = grub_strtoul(argv[1], 0, 0);
+
+    grub_msr_write(addr, value);
+
+    return GRUB_ERR_NONE;
+}
+
+static grub_extcmd_t cmd_read;
+static grub_extcmd_t cmd_write;
+
+GRUB_MOD_INIT(msr)
+{
+    cmd_read = grub_register_extcmd("rdmsr", grub_cmd_msr_read, 0,
+                                    N_("ADDR"),
+                                    N_("Read a CPU model specific register."),
+                                    options);
+
+    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(msr)
+{
+    grub_unregister_extcmd(cmd_read);
+    grub_unregister_command(cmd_write);
+}
diff --git a/include/grub/i386/msr.h b/include/grub/i386/msr.h
new file mode 100644
index 000000000..a14b3dcfb
--- /dev/null
+++ b/include/grub/i386/msr.h
@@ -0,0 +1,67 @@
+/*
+ *  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_HEADER
+#define GRUB_CPU_MSR_HEADER 1
+#endif
+
+#ifdef __x86_64__
+
+extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id)
+{
+    grub_uint32_t low;
+    grub_uint32_t high;
+    __asm__ __volatile__ ( "rdmsr"
+                           : "=a"(low), "=d"(high)
+                           : "c"(msr)
+                           );
+    return ((grub_uint64_t)high << 32) | low;
+}
+
+extern __inline void grub_msr_write(grub_uint64_t msr, grub_uint64_t value)
+{
+    grub_uint32_t low = value & 0xFFFFFFFF;
+    grub_uint32_t high = value >> 32;
+    __asm__ __volatile__ (
+                "wrmsr"
+                :
+                : "c"(msr), "a"(low), "d"(high)
+                );
+}
+
+#else
+
+extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id)
+{
+    /* We use uint64 in msr_id just to keep the same function signature */
+    grub_uint32_t low_id = msr_id & 0xFFFFFFFF;
+    grub_uint64_t msr_value;
+    __asm__ __volatile__ ( "rdmsr" : "=A" (msr_value) : "c" (low_id) );
+    return msr_value;
+}
+
+extern __inline void grub_msr_write(grub_uint64_t msr_id, grub_uint64_t msr_value)
+{
+    /* We use uint64 in msr_id just to keep the same function signature */
+    grub_uint32_t low_id = msr_id & 0xFFFFFFFF;
+    grub_uint32_t low_value = msr_value & 0xFFFFFFFF;
+    __asm__ __volatile__ ( "wrmsr" : : "c" (low_id), "A" (low_value) );
+}
+
+#endif
+
-- 
2.17.1

Re: [PATCH 1/1] Add new module msr

[Paul Menzel]

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

Dear Jesus,


Thank you very much for your contribution.


On 02/16/19 18:29, JesusDF wrote:

> Add a new grub module called msr that registers two commands (rdmsr and wrmsr)
> to be able to read and write to the model-specific registers. It is i386
> specific, as the cpuid module. The name of the module and commands match the
> linux kernel module and intel commands to interact with it.

I moved the description away from the subject line to the body. Please read,
for example, *How to Write a Git Commit Message* [1] and reformat it with
`git commit --amend`.

I believe a Signed-off-by line is also needed. (Use the switch `-s`:
`git commit --amend -s` in this case.)

> ---
>  grub-core/Makefile.core.def   |  10 ++++
>  grub-core/commands/i386/msr.c | 106 ++++++++++++++++++++++++++++++++++
>  include/grub/i386/msr.h       |  67 +++++++++++++++++++++
>  3 files changed, 183 insertions(+)
>  create mode 100644 grub-core/commands/i386/msr.c
>  create mode 100644 include/grub/i386/msr.h

[…]

Could you please add the appropriate documentation too, and then resend as
v2?


Kind regards,

Paul


[1]: https://chris.beams.io/posts/git-commit/


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH 1/1] Add a new grub module called msr that registers two commands (rdmsr and wrmsr) to be able to read and write to the model-specific registers. It is i386 specific, as the cpuid module. The name of the module and commands match the linux kernel module and intel commands to interact with it.

[Daniel Kiper]

Hi,

Thank you for the contribution. A few comments below...

On Sat, Feb 16, 2019 at 06:29:01PM +0100, JesusDF wrote:

I think that message from mail #0 should go here. And you do not need
introduction mail just for one patch.

> ---
>  grub-core/Makefile.core.def   |  10 ++++
>  grub-core/commands/i386/msr.c | 106 ++++++++++++++++++++++++++++++++++
>  include/grub/i386/msr.h       |  67 +++++++++++++++++++++
>  3 files changed, 183 insertions(+)
>  create mode 100644 grub-core/commands/i386/msr.c
>  create mode 100644 include/grub/i386/msr.h
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index e16fb06ba..836a353d0 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -2455,3 +2455,13 @@ module = {
>    common = loader/i386/xen_file64.c;
>    extra_dist = loader/i386/xen_fileXX.c;
>  };
> +
> +
> +module = {
> +  name = msr;

I think that you should split this into two modules. rdmsr and wrmsr. Then you
should add wrmsr into grub-core/commands/efi/shim_lock.c:disabled_mods list.

> +  common = commands/i386/msr.c;
> +  enable = x86;
> +  enable = i386_xen_pvh;
> +  enable = i386_xen;
> +  enable = x86_64_xen;
> +};
> diff --git a/grub-core/commands/i386/msr.c b/grub-core/commands/i386/msr.c
> new file mode 100644
> index 000000000..ea63adf97
> --- /dev/null
> +++ b/grub-core/commands/i386/msr.c
> @@ -0,0 +1,106 @@
> +/* msr.c - Read or write CPU model specific registers */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2006, 2007, 2009  Free Software Foundation, Inc.

Please update the dates accordingly.

> + *  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/msr.h>
> +#include <grub/i18n.h>
> +
> +GRUB_MOD_LICENSE("GPLv3+");
> +
> +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)

Space between functions name and "(" please. Same below...

> +{
> +    grub_uint64_t addr;
> +    grub_uint64_t value;
> +    char buf[sizeof("XXXXXXXX")];
> +
> +    if (argc != 1)
> +    {
> +        return grub_error(GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));

Space between functions name and "(" please. Same below...

> +    }

You do not need curly braces here.

> +
> +    addr = grub_strtoul(argv[0], 0, 0);

grub_strtoul() may return some errors. Please treat them properly.

> +    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);
> +    }

Ditto.

> +    return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_msr_write(grub_command_t cmd, int argc, char **argv)
> +{
> +    grub_addr_t addr;
> +    grub_uint32_t value;
> +
> +    if (argc != 2)
> +    {
> +        return grub_error(GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
> +    }

Ditto.

> +    addr = grub_strtoul(argv[0], 0, 0);
> +    value = grub_strtoul(argv[1], 0, 0);
> +
> +    grub_msr_write(addr, value);
> +
> +    return GRUB_ERR_NONE;
> +}
> +
> +static grub_extcmd_t cmd_read;
> +static grub_extcmd_t cmd_write;

Please put these variables after GRUB_MOD_LICENSE().

> +GRUB_MOD_INIT(msr)
> +{
> +    cmd_read = grub_register_extcmd("rdmsr", grub_cmd_msr_read, 0,
> +                                    N_("ADDR"),
> +                                    N_("Read a CPU model specific register."),
> +                                    options);
> +
> +    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(msr)
> +{
> +    grub_unregister_extcmd(cmd_read);
> +    grub_unregister_command(cmd_write);
> +}
> diff --git a/include/grub/i386/msr.h b/include/grub/i386/msr.h
> new file mode 100644
> index 000000000..a14b3dcfb
> --- /dev/null
> +++ b/include/grub/i386/msr.h
> @@ -0,0 +1,67 @@
> +/*
> + *  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_HEADER
> +#define GRUB_CPU_MSR_HEADER 1
> +#endif
> +
> +#ifdef __x86_64__
> +
> +extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id)
> +{
> +    grub_uint32_t low;
> +    grub_uint32_t high;

grub_uint32_t low, high;

And please add empty space after it.

> +    __asm__ __volatile__ ( "rdmsr"
> +                           : "=a"(low), "=d"(high)
> +                           : "c"(msr)
> +                           );

"asm volatile" seems more common in the GRUB code. If you change that
then probably everything will fit in one line.

And could you add preparatory patch which replaces each
"__asm__ __volatile__" occurence with "asm volatile"?

> +    return ((grub_uint64_t)high << 32) | low;
> +}
> +
> +extern __inline void grub_msr_write(grub_uint64_t msr, grub_uint64_t value)
> +{
> +    grub_uint32_t low = value & 0xFFFFFFFF;

Hmmm... What?

> +    grub_uint32_t high = value >> 32;
> +    __asm__ __volatile__ (
> +                "wrmsr"
> +                :
> +                : "c"(msr), "a"(low), "d"(high)
> +                );

And most comments for grub_msr_read() apply here too.

> +}
> +
> +#else
> +
> +extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id)
> +{
> +    /* We use uint64 in msr_id just to keep the same function signature */
> +    grub_uint32_t low_id = msr_id & 0xFFFFFFFF;
> +    grub_uint64_t msr_value;
> +    __asm__ __volatile__ ( "rdmsr" : "=A" (msr_value) : "c" (low_id) );
> +    return msr_value;

Ditto.

> +}
> +
> +extern __inline void grub_msr_write(grub_uint64_t msr_id, grub_uint64_t msr_value)
> +{
> +    /* We use uint64 in msr_id just to keep the same function signature */
> +    grub_uint32_t low_id = msr_id & 0xFFFFFFFF;
> +    grub_uint32_t low_value = msr_value & 0xFFFFFFFF;
> +    __asm__ __volatile__ ( "wrmsr" : : "c" (low_id), "A" (low_value) );

Ditto.

And please do not forget about Paul's comments too.

Daniel

Re: [PATCH 1/1] Add new module msr

[Jesús Diéguez Fernández]

[-- Attachment #1.1: Type: text/plain, Size: 10197 bytes --]

Hi,

    I have some doubts that I mention below.

El 18/2/19 a las 14:24, Daniel Kiper escribió:
> Hi,
> 
> Thank you for the contribution. A few comments below...
> 
> On Sat, Feb 16, 2019 at 06:29:01PM +0100, JesusDF wrote:
> 
> I think that message from mail #0 should go here. And you do not need
> introduction mail just for one patch.
> 
>> ---
>>  grub-core/Makefile.core.def   |  10 ++++
>>  grub-core/commands/i386/msr.c | 106 ++++++++++++++++++++++++++++++++++
>>  include/grub/i386/msr.h       |  67 +++++++++++++++++++++
>>  3 files changed, 183 insertions(+)
>>  create mode 100644 grub-core/commands/i386/msr.c
>>  create mode 100644 include/grub/i386/msr.h
>>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index e16fb06ba..836a353d0 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -2455,3 +2455,13 @@ module = {
>>    common = loader/i386/xen_file64.c;
>>    extra_dist = loader/i386/xen_fileXX.c;
>>  };
>> +
>> +
>> +module = {
>> +  name = msr;
> 
> I think that you should split this into two modules. rdmsr and wrmsr. Then you
> should add wrmsr into grub-core/commands/efi/shim_lock.c:disabled_mods list.

Ok, I'll split it. I didn't take into account secure boot.

> 
>> +  common = commands/i386/msr.c;
>> +  enable = x86;
>> +  enable = i386_xen_pvh;
>> +  enable = i386_xen;
>> +  enable = x86_64_xen;
>> +};
>> diff --git a/grub-core/commands/i386/msr.c b/grub-core/commands/i386/msr.c
>> new file mode 100644
>> index 000000000..ea63adf97
>> --- /dev/null
>> +++ b/grub-core/commands/i386/msr.c
>> @@ -0,0 +1,106 @@
>> +/* msr.c - Read or write CPU model specific registers */
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2006, 2007, 2009  Free Software Foundation, Inc.
> 
> Please update the dates accordingly>
>> + *  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/msr.h>
>> +#include <grub/i18n.h>
>> +
>> +GRUB_MOD_LICENSE("GPLv3+");
>> +
>> +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)
> 
> Space between functions name and "(" please. Same below...
> 
>> +{
>> +    grub_uint64_t addr;
>> +    grub_uint64_t value;
>> +    char buf[sizeof("XXXXXXXX")];
>> +
>> +    if (argc != 1)
>> +    {
>> +        return grub_error(GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
> 
> Space between functions name and "(" please. Same below...
> 
>> +    }
> 
> You do not need curly braces here.
> 
>> +
>> +    addr = grub_strtoul(argv[0], 0, 0);
> 
> grub_strtoul() may return some errors. Please treat them properly.
> 
>> +    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);
>> +    }
> 
> Ditto.
> 
>> +    return GRUB_ERR_NONE;
>> +}
>> +
>> +static grub_err_t
>> +grub_cmd_msr_write(grub_command_t cmd, int argc, char **argv)
>> +{
>> +    grub_addr_t addr;
>> +    grub_uint32_t value;
>> +
>> +    if (argc != 2)
>> +    {
>> +        return grub_error(GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
>> +    }
> 
> Ditto.
> 
>> +    addr = grub_strtoul(argv[0], 0, 0);
>> +    value = grub_strtoul(argv[1], 0, 0);
>> +
>> +    grub_msr_write(addr, value);
>> +
>> +    return GRUB_ERR_NONE;
>> +}
>> +
>> +static grub_extcmd_t cmd_read;
>> +static grub_extcmd_t cmd_write;
> 
> Please put these variables after GRUB_MOD_LICENSE().

I've also found that cmd_write should be of type grub_command_t.

> 
>> +GRUB_MOD_INIT(msr)
>> +{
>> +    cmd_read = grub_register_extcmd("rdmsr", grub_cmd_msr_read, 0,
>> +                                    N_("ADDR"),
>> +                                    N_("Read a CPU model specific register."),
>> +                                    options);
>> +
>> +    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(msr)
>> +{
>> +    grub_unregister_extcmd(cmd_read);
>> +    grub_unregister_command(cmd_write);
>> +}
>> diff --git a/include/grub/i386/msr.h b/include/grub/i386/msr.h
>> new file mode 100644
>> index 000000000..a14b3dcfb
>> --- /dev/null
>> +++ b/include/grub/i386/msr.h
>> @@ -0,0 +1,67 @@
>> +/*
>> + *  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_HEADER
>> +#define GRUB_CPU_MSR_HEADER 1
>> +#endif
>> +
>> +#ifdef __x86_64__
>> +
>> +extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id)
>> +{
>> +    grub_uint32_t low;
>> +    grub_uint32_t high;
> 
> grub_uint32_t low, high;
> 
> And please add empty space after it.
> 
>> +    __asm__ __volatile__ ( "rdmsr"
>> +                           : "=a"(low), "=d"(high)
>> +                           : "c"(msr)
>> +                           );
> 
> "asm volatile" seems more common in the GRUB code. If you change that
> then probably everything will fit in one line.
> 
> And could you add preparatory patch which replaces each> "__asm__ __volatile__" occurence with "asm volatile"?
> 

Just to be sure, do you want me to patch any file in the repository?

Those would be the affected files (excluding *.pp and
gettext_strings_test.in):

grub-core/efiemu/runtime/efiemu.c
grub-core/gnulib/stdio.h
grub-core/gnulib/stdio.in.h
grub-core/lib/i386/halt.c
grub-core/lib/libgcrypt/cipher/bithelp.h
grub-core/lib/libgcrypt/cipher/cast5.c
grub-core/lib/libgcrypt-grub/cipher/bithelp.h
grub-core/lib/libgcrypt-grub/cipher/cast5.c
grub-core/lib/libgcrypt-grub/mpi/longlong.h
grub-core/lib/libgcrypt/mpi/longlong.h
grub-core/lib/libgcrypt/src/hmac256.c
grub-core/lib/zstd/cpu.h
include/grub/arm64/time.h
include/grub/arm/time.h
include/grub/cpu/cpuid.h
include/grub/cpu/io.h
include/grub/cpu/time.h
include/grub/cpu/tsc.h
include/grub/i386/cpuid.h
include/grub/i386/io.h
include/grub/i386/time.h
include/grub/i386/tsc.h
include/grub/x86_64/time.h

>> +    return ((grub_uint64_t)high << 32) | low;
>> +}
>> +
>> +extern __inline void grub_msr_write(grub_uint64_t msr, grub_uint64_t value)
>> +{
>> +    grub_uint32_t low = value & 0xFFFFFFFF;
> 
> Hmmm... What?

I looked at an example on the osdev wiki which looked like it was correct:

https://wiki.osdev.org/Inline_Assembly/Examples#WRMSR

In 32 bit mode it uses the A constraint to assign the 64 bit value (it
uses both EAX and EDX). In 64 bit mode the A constraint would use either
EAX or EDX, so it can't be used.

There's also an example on the gcc docs using rdtsc:

https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Machine-Constraints.html#Machine-Constraints

> 
>> +    grub_uint32_t high = value >> 32;
>> +    __asm__ __volatile__ (
>> +                "wrmsr"
>> +                :
>> +                : "c"(msr), "a"(low), "d"(high)
>> +                );
> 
> And most comments for grub_msr_read() apply here too.
> 
>> +}
>> +
>> +#else
>> +
>> +extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id)
>> +{
>> +    /* We use uint64 in msr_id just to keep the same function signature */
>> +    grub_uint32_t low_id = msr_id & 0xFFFFFFFF;
>> +    grub_uint64_t msr_value;
>> +    __asm__ __volatile__ ( "rdmsr" : "=A" (msr_value) : "c" (low_id) );
>> +    return msr_value;
> 
> Ditto.
> 
>> +}
>> +
>> +extern __inline void grub_msr_write(grub_uint64_t msr_id, grub_uint64_t msr_value)
>> +{
>> +    /* We use uint64 in msr_id just to keep the same function signature */
>> +    grub_uint32_t low_id = msr_id & 0xFFFFFFFF;
>> +    grub_uint32_t low_value = msr_value & 0xFFFFFFFF;
>> +    __asm__ __volatile__ ( "wrmsr" : : "c" (low_id), "A" (low_value) );
> 
> Ditto.
> 
> And please do not forget about Paul's comments too.

Yes, I totally forgot about the docs.

When submitting the V2 version, should I sent it as a new patch (with V2
on the subject) or should I respond to this original thread?

> 
> Daniel
> 

Jesus


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

Re: [PATCH 1/1] Add new module msr

[Daniel Kiper]

On Mon, Feb 18, 2019 at 08:09:37PM +0100, Jesús Diéguez Fernández wrote:
> Hi,
>
>     I have some doubts that I mention below.
>
> El 18/2/19 a las 14:24, Daniel Kiper escribió:
> > Hi,
> >
> > Thank you for the contribution. A few comments below...
> >
> > On Sat, Feb 16, 2019 at 06:29:01PM +0100, JesusDF wrote:

[...]

> >> diff --git a/include/grub/i386/msr.h b/include/grub/i386/msr.h
> >> new file mode 100644
> >> index 000000000..a14b3dcfb
> >> --- /dev/null
> >> +++ b/include/grub/i386/msr.h
> >> @@ -0,0 +1,67 @@
> >> +/*
> >> + *  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_HEADER
> >> +#define GRUB_CPU_MSR_HEADER 1
> >> +#endif
> >> +
> >> +#ifdef __x86_64__
> >> +
> >> +extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id)
> >> +{
> >> +    grub_uint32_t low;
> >> +    grub_uint32_t high;
> >
> > grub_uint32_t low, high;
> >
> > And please add empty space after it.

Err... Of course empty line...

> >> +    __asm__ __volatile__ ( "rdmsr"
> >> +                           : "=a"(low), "=d"(high)
> >> +                           : "c"(msr)
> >> +                           );
> >
> > "asm volatile" seems more common in the GRUB code. If you change that
> > then probably everything will fit in one line.
> >
> > And could you add preparatory patch which replaces each> "__asm__ __volatile__" occurence with "asm volatile"?
> >
>
> Just to be sure, do you want me to patch any file in the repository?
>
> Those would be the affected files (excluding *.pp and
> gettext_strings_test.in):
>
> grub-core/efiemu/runtime/efiemu.c
> grub-core/gnulib/stdio.h
> grub-core/gnulib/stdio.in.h
> grub-core/lib/i386/halt.c
> grub-core/lib/libgcrypt/cipher/bithelp.h
> grub-core/lib/libgcrypt/cipher/cast5.c
> grub-core/lib/libgcrypt-grub/cipher/bithelp.h
> grub-core/lib/libgcrypt-grub/cipher/cast5.c
> grub-core/lib/libgcrypt-grub/mpi/longlong.h
> grub-core/lib/libgcrypt/mpi/longlong.h
> grub-core/lib/libgcrypt/src/hmac256.c
> grub-core/lib/zstd/cpu.h
> include/grub/arm64/time.h
> include/grub/arm/time.h
> include/grub/cpu/cpuid.h
> include/grub/cpu/io.h
> include/grub/cpu/time.h
> include/grub/cpu/tsc.h
> include/grub/i386/cpuid.h
> include/grub/i386/io.h
> include/grub/i386/time.h
> include/grub/i386/tsc.h
> include/grub/x86_64/time.h

Yes, please. Do this in separate patch. It can be first patch in the
patch series.

> >> +    return ((grub_uint64_t)high << 32) | low;
> >> +}
> >> +
> >> +extern __inline void grub_msr_write(grub_uint64_t msr, grub_uint64_t value)
> >> +{
> >> +    grub_uint32_t low = value & 0xFFFFFFFF;
> >
> > Hmmm... What?
>
> I looked at an example on the osdev wiki which looked like it was correct:
>
> https://wiki.osdev.org/Inline_Assembly/Examples#WRMSR
>
> In 32 bit mode it uses the A constraint to assign the 64 bit value (it
> uses both EAX and EDX). In 64 bit mode the A constraint would use either
> EAX or EDX, so it can't be used.
>
> There's also an example on the gcc docs using rdtsc:
>
> https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Machine-Constraints.html#Machine-Constraints

I do not complain about constraints. value & 0xFFFFFFFF looks strange to
me. I have a feeling that this is a copy paste mistake. Probably low or
variable named differently was earlier 64-bit. Then it made sense. Right
now it does not. Could you test your module without "& 0xFFFFFFFF"? How
disassembly looks like with and without "& 0xFFFFFFFF"? There is a chance
that compiler will drop it.

> >> +    grub_uint32_t high = value >> 32;
> >> +    __asm__ __volatile__ (
> >> +                "wrmsr"
> >> +                :
> >> +                : "c"(msr), "a"(low), "d"(high)
> >> +                );
> >
> > And most comments for grub_msr_read() apply here too.
> >
> >> +}
> >> +
> >> +#else
> >> +
> >> +extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id)
> >> +{
> >> +    /* We use uint64 in msr_id just to keep the same function signature */
> >> +    grub_uint32_t low_id = msr_id & 0xFFFFFFFF;
> >> +    grub_uint64_t msr_value;
> >> +    __asm__ __volatile__ ( "rdmsr" : "=A" (msr_value) : "c" (low_id) );
> >> +    return msr_value;
> >
> > Ditto.
> >
> >> +}
> >> +
> >> +extern __inline void grub_msr_write(grub_uint64_t msr_id, grub_uint64_t msr_value)
> >> +{
> >> +    /* We use uint64 in msr_id just to keep the same function signature */
> >> +    grub_uint32_t low_id = msr_id & 0xFFFFFFFF;
> >> +    grub_uint32_t low_value = msr_value & 0xFFFFFFFF;
> >> +    __asm__ __volatile__ ( "wrmsr" : : "c" (low_id), "A" (low_value) );
> >
> > Ditto.
> >
> > And please do not forget about Paul's comments too.
>
> Yes, I totally forgot about the docs.
>
> When submitting the V2 version, should I sent it as a new patch (with V2
> on the subject) or should I respond to this original thread?

Please start new thread.

Daniel