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

[Jesús Diéguez Fernández]

El 12/3/19 a las 20:12, Daniel Kiper escribió:
> On Fri, Mar 08, 2019 at 12:14:15PM +0100, Daniel Kiper wrote:
>> On Fri, Mar 08, 2019 at 01:26:37AM +0100, Jesús Diéguez Fernández wrote:
>>> In order to be able to read from and write 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.
>>>
>>> Also, if you specify a reserved or unimplemented MSR address, it will
>>> cause a general protection exception (which is not currently being handled)
>>> and the system will reboot.
>>>
>>> Signed-off-by: Jesús Diéguez Fernández <address@hidden>
>>
>> LGTM. Reviewed-by: Daniel Kiper <address@hidden>
>>
>> There are a few comments nitpicks which I fix before committing the patches.
>>
>> If there are no objections I will commit the patches next week.
>>
>> Thank you for doing the work.
> 
> I had to tweak your patches because they broke build at least for coreboot
> and x86-64 UEFI. Now they are in.
> 
> Daniel
> 

Sorry to hear that. I'm fine with the changes, specially with the error
fixes.

To prevent repeating the same mistakes, I've compared the patches and
I've spot the following differences:

        + Two missing casts to unsigned long long.
        + An unused parameter in wrmsr command.
        - The includes should be sorted alphabetically.
        - Phrases in comments should end with a dot.
        - Some spacing and line adjustment changes.
        - Indent with two spaces.
        - Multi-line comments reformatted with asterisks.

Did I miss anything else?

About the formatting issues, before the v2 I found the GNU Indent
program. I tested it but it didn't quite follow up the final desired
format, so I didn't use it at all.
Do you use it or do you normally adjust everything manually?

I'm used to work in different environments with different coding styles
and rules (although all of them are less restrictive than GRUB), so I
tried to mimic the style of other preexisting files. Next time I need
more time and care before submitting the patch.

About the multi-line comments, I think that maybe they were ok? (at
least by the grub-dev docs specific rules):

Comments spanning multiple lines shall be formatted with all lines after
the first aligned with the first line.

Asterisk characters should not be repeated a the start of each
subsequent line.

Acceptable:

/* This is a comment
   which spans multiple lines.
   It is long.  */

Unacceptable:

/*
 * This is a comment
 * which spans multiple lines.
 * It is long. */


Thank you for your time and patience.

Jesus