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