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

["Jesús Diéguez Fernández" <jesusdf@gmail.com>]

[-- 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 --]