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