Re: [PATCH 1/1] Add new module msr

[Daniel Kiper]

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