On Tue, Mar 05, 2019 at 11:17:01PM +0100, Jesús Diéguez Fernández wrote:
> 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. :-\
Not big deal...
> For the v3, now that the patch [1/2] has already been reviewed, should I
> send it again, or should I skip it?
Please add my Reviewed-by under your Signed-off-by (SOB) and resend 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.
I am not saying that MSR access is not interesting on Xen. I am saying
that I have a feeling that all "enable ..." except "enable = x86;" are
redundant. So, I think that "enable = x86;" will cover Xen too. Please
double check it. If it does not cover leave xen stuff as is.
> >> +};
> >> +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 your/terminfo version works correctly then I am fine with it.
You do not need to change it.
> >> + 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);
There are other issues. IMO addr should be 32-bit type instead of 64-bit.
And Intel spec says:
The CPUID instruction should be used to determine whether MSRs are
supported (CPUID.01H:EDX[5] = 1) before using this instruction.
Specifying a reserved or unimplemented MSR address in ECX will also
cause a general protection exception.
What will happen if somebody specifies invalid MSR? Does GRUB crashes?
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
