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

[Daniel Kiper]

On Wed, Mar 13, 2019 at 12:35:48AM +0100, Jesús Diéguez Fernández wrote:
> 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.

Great!

> 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.

Yep! These were the main culprits.

>       - The includes should be sorted alphabetically.

This is a matter of taste. I just like this if possible.
I have changed the order because I had to change path
for cpuid.h file too. However, I am not going to insist
alphabetic order too much here.

>       - Phrases in comments should end with a dot.

Again I like this but there is no strict requirement for that.

>       - Some spacing and line adjustment changes.
>       - Indent with two spaces.

Yes, I have missed that during review. In general two spaces of
TABs plus spaces if there are more than 8 spaces.

>       - Multi-line comments reformatted with asterisks.

Yes, please.

> Did I miss anything else?

I think that the only change in cpuid.h path.

> 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?

Right now manually but I would like to get it automated at some point.
This is getting boring...

> 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.

Do not worry. This is a nightmare in the GRUB code because it is
a mixture of various styles. I think that the problem is that the
style was not enforced much earlier. I am going to change that and
automate most of the task if time allows.

> 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. */

Yeah, I had to fix it. In general I do not like the former because in
long comments with code snippets sometimes it is not easy to catch the
difference between the real code and comment. So, I prefer the latter
with the last "*/" put alone in last comment line. However, you are
right that I had to update the docs. I will update it.

> Thank you for your time and patience.

You are welcome.

Daniel