Re: [PATCH v2 2/2] Add new MSR modules (rdmsr/wrmsr)

[Jesús Diéguez Fernández]

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 <address@hidden>
> 
> 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