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 <jesu...@gmail.com> > >> > >> LGTM. Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com> > >> > >> 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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
