Hi,
I have some doubts that I comment below.
El 5/3/19 a las 12:09, Daniel Kiper escribió:
> On Fri, Mar 01, 2019 at 06:17:42PM +0100, Jesús Diéguez Fernández wrote:
>> In order to be able to read and write from/to model-specific registers,
>> two new modules are added. They are i386 specific, as the cpuid module.
>>
>> rdmsr module registers the command rdmsr that allows reading from a MSR.
>> wrmsr module registers the command wrmsr that allows writing to a MSR.
>>
>> wrmsr module is disabled if UEFI secure boot is enabled.
>>
>> Please note that on SMP systems, interacting with a MSR that has a
>> scope per hardware thread, implies that the value only applies to
>> the particular cpu/core/thread that ran the command.
>>
>> Changelog v1 -> v2:
>>
>> - Patch all source code files with s/__asm__ __volatile__/asm volatile/g.
>> - Split the module in two (rdmsr/wrmsr).
>> - Include the wrmsr module in the forbidden modules efi list.
>> - Code indentation and cleanup.
>> - Copyright year update.
>> - Implicit casting mask removed.
>> - Use the same assembly code for x86 and x86_64.
>> - Add missing documentation.
>> - Patch submited with Signed-off-by.
>>
>> Signed-off-by: Jesús Diéguez Fernández <jesu...@gmail.com>
>
> In general it looks much better but I still have some concerns...
>
> First of all, two patches and more should have mail introducing them.
> Good example you can find here
> http://lists.gnu.org/archive/html/grub-devel/2018-10/msg00201.html
> or here
> http://lists.gnu.org/archive/html/grub-devel/2018-11/msg00282.html
>
> git send-email --compose ... is your friend...
>
> [...]
I thought about it before sending the e-mail, but I chose the wrong
option. :-\
For the v3, now that the patch [1/2] has already been reviewed, should I
send it again, or should I skip it?
>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 2346bd291..a966a8f28 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -2484,3 +2484,19 @@ module = {
>> common = loader/i386/xen_file64.c;
>> extra_dist = loader/i386/xen_fileXX.c;
>> };
>> +module = {
>> + name = rdmsr;
>> + common = commands/i386/rdmsr.c;
>> + enable = x86;
>> + enable = i386_xen_pvh;
>> + enable = i386_xen;
>> + enable = x86_64_xen;
>
> I think that x86 should suffice? Does not it?
I used the cpuid module as an example since it has many similarities.
Even so, I thought that accessing MSR from a Xen guest could be interesting:
http://www.brendangregg.com/blog/2014-09-15/the-msrs-of-ec2.html
For my use case, leaving alone x86 is enough. I'll modify it as you tell
me to.
>
>> +};
>> +module = {
>> + name = wrmsr;
>> + common = commands/i386/wrmsr.c;
>> + enable = x86;
>> + enable = i386_xen_pvh;
>> + enable = i386_xen;
>> + enable = x86_64_xen;
>
> Ditto.
>
[...]
>> +static grub_err_t
>> +grub_cmd_msr_read (grub_extcmd_context_t ctxt, int argc, char **argv)
>> +{
>> + grub_uint64_t addr, value;
>> + char *ptr;
>> + char buf[sizeof("XXXXXXXX")];
>> +
>> + if (argc != 1)
>> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument
>> expected"));
>> +
>> + grub_errno = GRUB_ERR_NONE;
>> + ptr = argv[0];
>> + addr = grub_strtoul (ptr, &ptr, 0);
>> +
>> + if (grub_errno != GRUB_ERR_NONE)
>> + return grub_errno;
>
> Should not you display a blurb to the user what happened here?
> Like you do below...
I used the same method that is used in grub-core/term/terminfo.c
static grub_err_t
grub_cmd_terminfo (grub_extcmd_context_t ctxt, int argc, char **args)
{
...
char *ptr = state[OPTION_GEOMETRY].arg;
w = grub_strtoul (ptr, &ptr, 0);
if (grub_errno)
return grub_errno;
if (*ptr != 'x')
return grub_error (GRUB_ERR_BAD_ARGUMENT,
N_("incorrect terminal dimensions
specification"));
I tested this with:
rdmsr z => shows "unrecognized number"
rdmsr 0x1122334455667788991100 => shows "overflow detected"
I can change it to:
if (grub_errno != GRUB_ERR_NONE)
return grub_error (grub_errno, N_("invalid argument"));
If that is any better.
>
>> + if (*ptr != '\0')
>> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
>> +
>> + 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;
>> +}
[...]
>> +
>> +GRUB_MOD_LICENSE("GPLv3+");
>> +
>> +static grub_command_t cmd_write;
>> +
>> +static grub_err_t
>> +grub_cmd_msr_write (grub_command_t cmd, int argc, char **argv)
>> +{
>> + grub_uint64_t addr, value;
>> + char *ptr;
>> +
>> + if (argc != 2)
>> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments
>> expected"));
>> +
>> + grub_errno = GRUB_ERR_NONE;
>> + ptr = argv[0];
>> + addr = grub_strtoul (ptr, &ptr, 0);
>> +
>> + if (grub_errno != GRUB_ERR_NONE)
>> + return grub_errno;
>
> Ditto.
>
>> + if (*ptr != '\0')
>> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
>> +
>> + ptr = argv[1];
>> + value = grub_strtoul (ptr, &ptr, 0);
>> +
>> + if (grub_errno != GRUB_ERR_NONE)
>> + return grub_errno;
>
> Ditto.
>
>> + if (*ptr != '\0')
>> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
>> +
I found a bug in this function, it is possible to write 64 bit values to
the MSR, so I should have used grub_strtoull when reading the value.
It will be fixed in the v3.
>> + grub_msr_write (addr, value);
>> +
>> + return GRUB_ERR_NONE;
>> +}
>> +
>> +GRUB_MOD_INIT(wrmsr)
>> +{
>> + 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(wrmsr)
>> +{
>> + grub_unregister_command (cmd_write);
>> +}
[...]
>
> And this #endif should go below too. Like in rdmsr.h.
>
>> +extern __inline void grub_msr_write(grub_uint64_t msr_id, grub_uint64_t
>> msr_value)
>> +{
>> + grub_uint32_t low_id = msr_id, low = msr_value, high = msr_value >> 32;
>> +
>> + asm volatile ( "wrmsr" : : "c"(low_id), "a"(low), "d"(high) );
>> +}
>
> Anyway, thank you for doing the work. Looking forward for next version
> of patches. Please post them quickly because freeze date is nearing.
>
> Daniel
>
I'm also interested in getting this patch into the next version. I'll
send you the v3 as soon as this doubts are cleared.
Jesus
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
