From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1gvoIo-0005iT-3G for mharc-grub-devel@gnu.org; Mon, 18 Feb 2019 14:10:06 -0500 Received: from eggs.gnu.org ([209.51.188.92]:54716) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gvoIj-0005fj-SF for grub-devel@gnu.org; Mon, 18 Feb 2019 14:10:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gvoIh-00054s-NF for grub-devel@gnu.org; Mon, 18 Feb 2019 14:10:01 -0500 Received: from mail-wr1-x441.google.com ([2a00:1450:4864:20::441]:39046) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gvoIh-0004tp-AI for grub-devel@gnu.org; Mon, 18 Feb 2019 14:09:59 -0500 Received: by mail-wr1-x441.google.com with SMTP id l5so18490767wrw.6 for ; Mon, 18 Feb 2019 11:09:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:openpgp:autocrypt:message-id:date :user-agent:mime-version:in-reply-to; bh=9Uxlg6SIqpihK0/lwRG2hoosG+zJ1ZEq1q9TakD63YQ=; b=jSxqoAdXNGIZiUcjZ0nPz4UUKntwAJDHLf2O4KYl1yvu4mwND1QoyjGZ+vwpaNglr5 iNR8HWnYNrfi2yzOgeq4uf9GkFl8XQn2Wvjs9lsu4RCeCdHg+xNnO2rIjPUzZRzWkq+Q kmQTBzPwVIC4iSrdcWlqE/JaeV8vrL7fE9+3k1GqyiCM79Zjgvs0TZ4zbgn3C2FxElC+ CB3iZxLFBqk2dQA/D4vR5jwe33D1/mwBQFyR4BkBlynnQ5nmzx78ejTNeBwjEFZ5Z+9+ TGRXoSwEs6gGiqFM2sdOoXtizks2puvwz8jRE2qHT0Y3HjOzw/F8bXGv8FXE0JpsO7Q0 E1oA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:autocrypt :message-id:date:user-agent:mime-version:in-reply-to; bh=9Uxlg6SIqpihK0/lwRG2hoosG+zJ1ZEq1q9TakD63YQ=; b=h3BvGZTOhuNh/mUTLheh1RLx6WhowycyX30WfPZG652wpEi98Nbix7v/M0RZEgvvhr 9bZjDMcEnt18lN7yAvo4io1W/gdPQ/odTrKAnd4Nq1UaoRWJu4RcyF+2xdHLP2NHj7rx aIp82WqM2lIz7npN/0hihI9mT8LB4keO8zfrzF6pgN5MVnFSVDtAJ/v5V80V413OlWU2 T00Y2TfLR/FOGEbwZBDHbRW4POZ+FA055sv2UUfMDVVPJPFKho5qdviN2TzNmp22tNKT 68/umD+/zm/Pug33fc9RdJQzW7K62lFsAVGhUFZEOWtE8OZMNLazVdEmnPbCw5t/cV27 Lnhg== X-Gm-Message-State: AHQUAubEB+XEtcj1j+b+I97eYdY40cOUtMlTEzwIeYC7skNN1ya3dKI5 FGQWReCMvG9XFJX38chkz74C/7SVfDE= X-Google-Smtp-Source: AHgI3IYw+rvG0WXJeEOQoyQIszpj02NGxm4DDvJ+tbKPoJ9w79rPl/rvuL2P7Tz0Ze/YC7KAWDsV6A== X-Received: by 2002:a5d:680d:: with SMTP id w13mr17005676wru.196.1550516989807; Mon, 18 Feb 2019 11:09:49 -0800 (PST) Received: from [192.168.1.11] (57.red-88-19-226.staticip.rima-tde.net. [88.19.226.57]) by smtp.googlemail.com with ESMTPSA id j3sm136734wmb.39.2019.02.18.11.09.48 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 18 Feb 2019 11:09:48 -0800 (PST) Subject: Re: [PATCH 1/1] Add new module msr To: Daniel Kiper Cc: grub-devel@gnu.org References: <20190216172901.20108-1-jesusdf@gmail.com> <20190216172901.20108-2-jesusdf@gmail.com> <20190218132430.ea74imfok54uixyq@tomti.i.net-space.pl> From: =?UTF-8?B?SmVzw7pzIERpw6lndWV6IEZlcm7DoW5kZXo=?= Openpgp: id=25B5907521F72F0D2A85805AF2B86990C97D83AE; url=http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xF2B86990C97D83AE Autocrypt: addr=jesusdf@gmail.com; prefer-encrypt=mutual; keydata= mQENBE0EQN4BCADJd9SXqsXllmTx1IIMLUTnSoMAjNzD3JTakEwkDRO2mena5MQMYosSS/lv XFtlY+16IxPOP4rTIIvInqE9D7mwWvXsuqlR8GBUH7y0wssdCmm1STAQicVSfa8hW2Gi4vrk zH3aAWdguLZeuQwtITvpGHcZOcfOsmu5KesH4nHpgUmfrSY+5ZrKLeHIwOxH0K4vAX9Cu5+B RO/2PBMvuB50cCXKMrUtOrn7UitZ8UIlgVWFolRnMoZwoMTsaDR/59nYwCUZcR5U5EwcLTrx nGT23g8kAt+keCkQLyz23LpXoBLFv4umB0+ZUki18Nv5kdBxfAOiDUBSM/jj/8P8jViPABEB AAG0Lkplc8O6cyBEacOpZ3VleiBGZXJuw6FuZGV6IDxqZXN1c2RmQGdtYWlsLmNvbT6JATgE EwECACIFAk0EQN4CGwMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJEPK4aZDJfYOuAucI ALN5iJnjNvRUsRLmCghisiLPbM8C2X0ctqr+fKYmN6tSZ7YKIe4dmuqnVS6PGPm2wAaW+X9E 3C/cg/ShSe9s55+v018Fq1UUXKPd6MM1CRohr5BR8YtA2uRC5hY0DpANdDop1hAM0DOb1M46 I1c/9P5NLA/mQinFikdPglzID1p2UgzoXeCNaYQuUBcOY8nXsfZM2IMtZPzXj6QT7Hpqh1qe Jw/ujucayQ5uo2ZQH2BfLhDCDG7S9YhKiy9S8CcSmgXb4a+V7r5U4kJIeu16ioKr7wMy1bjE bmN6YAEJcsddaQgH3qLi42tS2c7oIxrCPM22aqkuuOYEWjT4UPHQ3f+5AQ0ETQRA3gEIAMm6 gXWdJN8oQvYB5qxQdzXb+ph5JTzL+URMVuGJyePHIoxpy+saYlx05pPxtWmVtqTzHhahqgDw E5c8XpaY/LjJ/q3LHkUofwF1DhxbkuHYKTbIiVTXKQRiVS596N+CTECjF1WDBAtfO+AGOp98 tpPtHa5oeBuhS8X7va3qOhTIz7NXp9cM9gfZAH1y9gspxZxikVOgOHQQK3t9F34mdmfIQZj0 D3rWwCQW6kJzy0YSsB2w/ewYb1d3fS5QMZsfoVCrcN/Q3CcTR8dRtc41zDUv5zyherrCCWSk c3ghUWt6QEF4gCNJvOf22gBQBYJI/BCrLaVZuhczU2w9qAZnwdkAEQEAAYkBHwQYAQIACQUC TQRA3gIbDAAKCRDyuGmQyX2DrjRzB/0RTqLR6P+J+znCWrG+zTx/voXgUfenB1JF/lt8wDHw 9eSgmh+bvS+I6cGF0FLz9j+zpRggDGSKWuVrDZ8FsfsrMy0V6HfRJlkNtR1DpuRvM8mnTK8o or7yrwkGeGJ7IhhYMPCEPGOrWbQwiSuHUoO1B0GKMT0gqkFoCEnjp9RKwbORMdw3ya6l6SMK xKromJkKLYfwlt0Ju77k0n1CEv6b+VSHjPlctZLqu1eUyDKsewXb/K7/VyAePzg8elWCt6qK RtOfPf+43ULVI5r5uKOxDcH/nmSMJW86Jqtn6Vlyt1/YnKQQHGeed6io8XuLpl864dzTpReX 8XRRy5VFKUa8 Message-ID: <512f59fa-e56b-8537-ea2c-bd247b49982e@gmail.com> Date: Mon, 18 Feb 2019 20:09:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <20190218132430.ea74imfok54uixyq@tomti.i.net-space.pl> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Kq06BUqZaA9enTgqkNJbBV6HP3IxJYFOX" X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2a00:1450:4864:20::441 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Feb 2019 19:10:04 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Kq06BUqZaA9enTgqkNJbBV6HP3IxJYFOX Content-Type: multipart/mixed; boundary="GIRvF6y67uXM8CFQUdsc453TysbuQHtqM"; protected-headers="v1" From: =?UTF-8?B?SmVzw7pzIERpw6lndWV6IEZlcm7DoW5kZXo=?= To: Daniel Kiper Cc: grub-devel@gnu.org Message-ID: <512f59fa-e56b-8537-ea2c-bd247b49982e@gmail.com> Subject: Re: [PATCH 1/1] Add new module msr References: <20190216172901.20108-1-jesusdf@gmail.com> <20190216172901.20108-2-jesusdf@gmail.com> <20190218132430.ea74imfok54uixyq@tomti.i.net-space.pl> In-Reply-To: <20190218132430.ea74imfok54uixyq@tomti.i.net-space.pl> --GIRvF6y67uXM8CFQUdsc453TysbuQHtqM Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi, I have some doubts that I mention below. El 18/2/19 a las 14:24, Daniel Kiper escribi=C3=B3: > Hi, >=20 > Thank you for the contribution. A few comments below... >=20 > On Sat, Feb 16, 2019 at 06:29:01PM +0100, JesusDF wrote: >=20 > I think that message from mail #0 should go here. And you do not need > introduction mail just for one patch. >=20 >> --- >> grub-core/Makefile.core.def | 10 ++++ >> grub-core/commands/i386/msr.c | 106 +++++++++++++++++++++++++++++++++= + >> include/grub/i386/msr.h | 67 +++++++++++++++++++++ >> 3 files changed, 183 insertions(+) >> create mode 100644 grub-core/commands/i386/msr.c >> create mode 100644 include/grub/i386/msr.h >> >> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def= >> index e16fb06ba..836a353d0 100644 >> --- a/grub-core/Makefile.core.def >> +++ b/grub-core/Makefile.core.def >> @@ -2455,3 +2455,13 @@ module =3D { >> common =3D loader/i386/xen_file64.c; >> extra_dist =3D loader/i386/xen_fileXX.c; >> }; >> + >> + >> +module =3D { >> + name =3D msr; >=20 > I think that you should split this into two modules. rdmsr and wrmsr. T= hen you > should add wrmsr into grub-core/commands/efi/shim_lock.c:disabled_mods = list. Ok, I'll split it. I didn't take into account secure boot. >=20 >> + common =3D commands/i386/msr.c; >> + enable =3D x86; >> + enable =3D i386_xen_pvh; >> + enable =3D i386_xen; >> + enable =3D x86_64_xen; >> +}; >> diff --git a/grub-core/commands/i386/msr.c b/grub-core/commands/i386/m= sr.c >> new file mode 100644 >> index 000000000..ea63adf97 >> --- /dev/null >> +++ b/grub-core/commands/i386/msr.c >> @@ -0,0 +1,106 @@ >> +/* msr.c - Read or write CPU model specific registers */ >> +/* >> + * GRUB -- GRand Unified Bootloader >> + * Copyright (C) 2006, 2007, 2009 Free Software Foundation, Inc. >=20 > Please update the dates accordingly> >> + * Based on gcc/gcc/config/i386/driver-i386.c >> + * >> + * 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 . >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +GRUB_MOD_LICENSE("GPLv3+"); >> + >> +static const struct grub_arg_option options[] =3D >> +{ >> +{0, 'v', 0, N_("Save read value into variable VARNAME."), >> + N_("VARNAME"), ARG_TYPE_STRING}, >> +{0, 0, 0, 0, 0, 0} >> +}; >> + >> +static grub_err_t >> +grub_cmd_msr_read(grub_extcmd_context_t ctxt, int argc, char **argv) >=20 > Space between functions name and "(" please. Same below... >=20 >> +{ >> + grub_uint64_t addr; >> + grub_uint64_t value; >> + char buf[sizeof("XXXXXXXX")]; >> + >> + if (argc !=3D 1) >> + { >> + return grub_error(GRUB_ERR_BAD_ARGUMENT, N_("one argument exp= ected")); >=20 > Space between functions name and "(" please. Same below... >=20 >> + } >=20 > You do not need curly braces here. >=20 >> + >> + addr =3D grub_strtoul(argv[0], 0, 0); >=20 > grub_strtoul() may return some errors. Please treat them properly. >=20 >> + value =3D grub_msr_read(addr); >> + >> + if (ctxt->state[0].set) >> + { >> + grub_snprintf(buf, sizeof(buf), "%llx", value); >> + grub_env_set(ctxt->state[0].arg, buf); >> + } >> + else >> + { >> + grub_printf("0x%llx\n", value); >> + } >=20 > Ditto. >=20 >> + return GRUB_ERR_NONE; >> +} >> + >> +static grub_err_t >> +grub_cmd_msr_write(grub_command_t cmd, int argc, char **argv) >> +{ >> + grub_addr_t addr; >> + grub_uint32_t value; >> + >> + if (argc !=3D 2) >> + { >> + return grub_error(GRUB_ERR_BAD_ARGUMENT, N_("two arguments ex= pected")); >> + } >=20 > Ditto. >=20 >> + addr =3D grub_strtoul(argv[0], 0, 0); >> + value =3D grub_strtoul(argv[1], 0, 0); >> + >> + grub_msr_write(addr, value); >> + >> + return GRUB_ERR_NONE; >> +} >> + >> +static grub_extcmd_t cmd_read; >> +static grub_extcmd_t cmd_write; >=20 > Please put these variables after GRUB_MOD_LICENSE(). I've also found that cmd_write should be of type grub_command_t. >=20 >> +GRUB_MOD_INIT(msr) >> +{ >> + cmd_read =3D grub_register_extcmd("rdmsr", grub_cmd_msr_read, 0, >> + N_("ADDR"), >> + N_("Read a CPU model specific reg= ister."), >> + options); >> + >> + cmd_write =3D grub_register_command("wrmsr", grub_cmd_msr_write, >> + N_("ADDR VALUE"), >> + N_("Write a value to a CPU mode= l specific register.")); >> +} >> + >> +GRUB_MOD_FINI(msr) >> +{ >> + grub_unregister_extcmd(cmd_read); >> + grub_unregister_command(cmd_write); >> +} >> 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 . >> + */ >> + >> +#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; >=20 > grub_uint32_t low, high; >=20 > And please add empty space after it. >=20 >> + __asm__ __volatile__ ( "rdmsr" >> + : "=3Da"(low), "=3Dd"(high) >> + : "c"(msr) >> + ); >=20 > "asm volatile" seems more common in the GRUB code. If you change that > then probably everything will fit in one line. >=20 > And could you add preparatory patch which replaces each> "__asm__ __vol= atile__" occurence with "asm volatile"? >=20 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 >> + 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 =3D value & 0xFFFFFFFF; >=20 > 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#Mac= hine-Constraints >=20 >> + grub_uint32_t high =3D value >> 32; >> + __asm__ __volatile__ ( >> + "wrmsr" >> + : >> + : "c"(msr), "a"(low), "d"(high) >> + ); >=20 > And most comments for grub_msr_read() apply here too. >=20 >> +} >> + >> +#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 signatu= re */ >> + grub_uint32_t low_id =3D msr_id & 0xFFFFFFFF; >> + grub_uint64_t msr_value; >> + __asm__ __volatile__ ( "rdmsr" : "=3DA" (msr_value) : "c" (low_id= ) ); >> + return msr_value; >=20 > Ditto. >=20 >> +} >> + >> +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 signatu= re */ >> + grub_uint32_t low_id =3D msr_id & 0xFFFFFFFF; >> + grub_uint32_t low_value =3D msr_value & 0xFFFFFFFF; >> + __asm__ __volatile__ ( "wrmsr" : : "c" (low_id), "A" (low_value) = ); >=20 > Ditto. >=20 > 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? >=20 > Daniel >=20 Jesus --GIRvF6y67uXM8CFQUdsc453TysbuQHtqM-- --Kq06BUqZaA9enTgqkNJbBV6HP3IxJYFOX Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCgAdFiEEJbWQdSH3Lw0qhYBa8rhpkMl9g64FAlxrAvwACgkQ8rhpkMl9 g66eaQgAgebyuIllUa8SIod6cpBUS83L0oiggThQoSePboadOBMAaAsjSn6NLlPZ gxsRnRe4p+q0lOqR72AWyBO4bPbtshbAP1w1r/I43DsmRdxkykiFjmQ4bobiGPqt YTe4gAyz3LrRIv9+OeJbRiVrd2KHqsk5LTnlbRnP/SgT8+5htJU9HtcHHE3cEsLM fINaIEXOqayxdhOdtLibcgDqsY2EnfvvpRW0R7h8QXAptf6Fm22q3i8vMdoJ8Z2d i+5097chsNj52E3r8SoBfopAwkTrkBJyyIyH96qzXF3uKC9pvUGuGrzCTGXcky/d CxVGuwpSiwOVvX2/pFivaMScd6we+A== =7cfX -----END PGP SIGNATURE----- --Kq06BUqZaA9enTgqkNJbBV6HP3IxJYFOX--