On Mon, Feb 18, 2019 at 08:09:37PM +0100, Jesús Diéguez Fernández wrote: > Hi, > > I have some doubts that I mention below. > > El 18/2/19 a las 14:24, Daniel Kiper escribió: > > Hi, > > > > Thank you for the contribution. A few comments below... > > > > On Sat, Feb 16, 2019 at 06:29:01PM +0100, JesusDF wrote:
[...] > >> diff --git a/include/grub/i386/msr.h b/include/grub/i386/msr.h > >> new file mode 100644 > >> index 000000000..a14b3dcfb > >> --- /dev/null > >> +++ b/include/grub/i386/msr.h > >> @@ -0,0 +1,67 @@ > >> +/* > >> + * GRUB -- GRand Unified Bootloader > >> + * Copyright (C) 2019 Free Software Foundation, Inc. > >> + * > >> + * GRUB is free software: you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License as published by > >> + * the Free Software Foundation, either version 3 of the License, or > >> + * (at your option) any later version. > >> + * > >> + * GRUB is distributed in the hope that it will be useful, > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> + * GNU General Public License for more details. > >> + * > >> + * You should have received a copy of the GNU General Public License > >> + * along with GRUB. If not, see <http://www.gnu.org/licenses/>. > >> + */ > >> + > >> +#ifndef GRUB_CPU_MSR_HEADER > >> +#define GRUB_CPU_MSR_HEADER 1 > >> +#endif > >> + > >> +#ifdef __x86_64__ > >> + > >> +extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id) > >> +{ > >> + grub_uint32_t low; > >> + grub_uint32_t high; > > > > grub_uint32_t low, high; > > > > And please add empty space after it. Err... Of course empty line... > >> + __asm__ __volatile__ ( "rdmsr" > >> + : "=a"(low), "=d"(high) > >> + : "c"(msr) > >> + ); > > > > "asm volatile" seems more common in the GRUB code. If you change that > > then probably everything will fit in one line. > > > > And could you add preparatory patch which replaces each> "__asm__ > > __volatile__" occurence with "asm volatile"? > > > > Just to be sure, do you want me to patch any file in the repository? > > Those would be the affected files (excluding *.pp and > gettext_strings_test.in): > > grub-core/efiemu/runtime/efiemu.c > grub-core/gnulib/stdio.h > grub-core/gnulib/stdio.in.h > grub-core/lib/i386/halt.c > grub-core/lib/libgcrypt/cipher/bithelp.h > grub-core/lib/libgcrypt/cipher/cast5.c > grub-core/lib/libgcrypt-grub/cipher/bithelp.h > grub-core/lib/libgcrypt-grub/cipher/cast5.c > grub-core/lib/libgcrypt-grub/mpi/longlong.h > grub-core/lib/libgcrypt/mpi/longlong.h > grub-core/lib/libgcrypt/src/hmac256.c > grub-core/lib/zstd/cpu.h > include/grub/arm64/time.h > include/grub/arm/time.h > include/grub/cpu/cpuid.h > include/grub/cpu/io.h > include/grub/cpu/time.h > include/grub/cpu/tsc.h > include/grub/i386/cpuid.h > include/grub/i386/io.h > include/grub/i386/time.h > include/grub/i386/tsc.h > include/grub/x86_64/time.h Yes, please. Do this in separate patch. It can be first patch in the patch series. > >> + return ((grub_uint64_t)high << 32) | low; > >> +} > >> + > >> +extern __inline void grub_msr_write(grub_uint64_t msr, grub_uint64_t > >> value) > >> +{ > >> + grub_uint32_t low = value & 0xFFFFFFFF; > > > > Hmmm... What? > > I looked at an example on the osdev wiki which looked like it was correct: > > https://wiki.osdev.org/Inline_Assembly/Examples#WRMSR > > In 32 bit mode it uses the A constraint to assign the 64 bit value (it > uses both EAX and EDX). In 64 bit mode the A constraint would use either > EAX or EDX, so it can't be used. > > There's also an example on the gcc docs using rdtsc: > > https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Machine-Constraints.html#Machine-Constraints I do not complain about constraints. value & 0xFFFFFFFF looks strange to me. I have a feeling that this is a copy paste mistake. Probably low or variable named differently was earlier 64-bit. Then it made sense. Right now it does not. Could you test your module without "& 0xFFFFFFFF"? How disassembly looks like with and without "& 0xFFFFFFFF"? There is a chance that compiler will drop it. > >> + grub_uint32_t high = value >> 32; > >> + __asm__ __volatile__ ( > >> + "wrmsr" > >> + : > >> + : "c"(msr), "a"(low), "d"(high) > >> + ); > > > > And most comments for grub_msr_read() apply here too. > > > >> +} > >> + > >> +#else > >> + > >> +extern __inline grub_uint64_t grub_msr_read (grub_uint64_t msr_id) > >> +{ > >> + /* We use uint64 in msr_id just to keep the same function signature */ > >> + grub_uint32_t low_id = msr_id & 0xFFFFFFFF; > >> + grub_uint64_t msr_value; > >> + __asm__ __volatile__ ( "rdmsr" : "=A" (msr_value) : "c" (low_id) ); > >> + return msr_value; > > > > Ditto. > > > >> +} > >> + > >> +extern __inline void grub_msr_write(grub_uint64_t msr_id, grub_uint64_t > >> msr_value) > >> +{ > >> + /* We use uint64 in msr_id just to keep the same function signature */ > >> + grub_uint32_t low_id = msr_id & 0xFFFFFFFF; > >> + grub_uint32_t low_value = msr_value & 0xFFFFFFFF; > >> + __asm__ __volatile__ ( "wrmsr" : : "c" (low_id), "A" (low_value) ); > > > > Ditto. > > > > And please do not forget about Paul's comments too. > > Yes, I totally forgot about the docs. > > When submitting the V2 version, should I sent it as a new patch (with V2 > on the subject) or should I respond to this original thread? Please start new thread. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
