grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/1] Add a new grub module called msr that registers two comm


From: Daniel Kiper
Subject: 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.
Date: Mon, 18 Feb 2019 14:24:30 +0100
User-agent: NeoMutt/20170113 (1.7.2)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]