From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 19E742114B137 for ; Fri, 21 Sep 2018 00:40:54 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Sep 2018 00:40:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,284,1534834800"; d="scan'208";a="259101119" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga005.jf.intel.com with ESMTP; 21 Sep 2018 00:40:39 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 21 Sep 2018 00:40:38 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.39]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.227]) with mapi id 14.03.0319.002; Fri, 21 Sep 2018 15:40:37 +0800 From: "Dong, Eric" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" , "Ni, Ruiyu" Thread-Topic: [edk2] [Patch 00/14] Update MSR definitions. Thread-Index: AQHUTvEgyHKfjrMp50ydoLVL1LN28aT1OHUAgAUlueA= Date: Fri, 21 Sep 2018 07:40:36 +0000 Message-ID: References: <20180918014330.28336-1-eric.dong@intel.com> <2a7532fe-4fcf-ddb7-95d0-acfd9b4657cf@redhat.com> In-Reply-To: <2a7532fe-4fcf-ddb7-95d0-acfd9b4657cf@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch 00/14] Update MSR definitions. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Sep 2018 07:40:55 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Tuesday, September 18, 2018 5:02 PM > To: Dong, Eric ; edk2-devel@lists.01.org > Cc: Kinney, Michael D ; Ni, Ruiyu > > Subject: Re: [edk2] [Patch 00/14] Update MSR definitions. >=20 > On 09/18/18 03:43, Eric Dong wrote: > > Current MSR definition are follow the SDM 2016-09 version. The latest > > SDM is 2018-05. This patch serial update the MSR related definition to > > follow the latest SDM 2018-05 version. MSR related defintion are saved > > at UefiCpuPkg\Include\Register\. > > > > The changes for this serial includes: > > 1. Add new MSR definition and file. > > 2. Remove old MSR definition which not defined in new SDM. > > 3. Change MSR name to follow new SDM, keep old one for compatibility. > > 4. Change MSR data structure definition to follow new SDM. > > 5. Update comments to follow the new SDM, mainly related to chapter > > info. > > > > Below changes are incompatible changes: > > 2. Remove old MSR definition which not defined in new SDM. > > For this one, i search edk2 codebase, not found any code uses it. so > > no impact for edk2 codebase. Detail changes see patch 9 ~ 11. >=20 > This sounds good. Macros and structure names are easy to grep. >=20 > > 4. Change MSR data structure definition to follow new SDM. > > For this one, new data structure just change the original reserved > > bits to valid bits, should have no impact for the current code. Detail > > see patch 8 and patch 14 >=20 > This looks more risky to me. Consider the MSR_IA32_RTIT_CTL_REGISTER > type in "UefiCpuPkg/Include/Register/ArchitecturalMsr.h" (modified by > patch 8). The patch removes (for example) the original Reserved1 field, b= ut > then reintroduces the field in place of the Reserved3 field. >=20 > IMO, this is risky: if code exists (possibly out-of-tree) that used to re= fer to > the Reserved1 field (because it actually wanted to access PwrEvtEn and > FUPonPTW, but the headers weren't up-to-date enough, so the code got > written against the Reserved1 field), then after this change, the code wi= ll > likely still *compile*, but it will not work. >=20 > Similarly for patch 14: the Reserved3 and Reserved4 fields are redefined = with > different sizes and locations. Risky move. The old field names should be > retired, and entirely new field names (with larger counter values than ev= er > before) should be introduced. >=20 Thanks for your suggestion. I think it's a good idea to use totally new Res= ervedX if the data structure is changed. I will follow this rule to update= the patches. Thanks, Eric > IMO we should consider ReservedX field names similar to PPI and protocol > GUIDs. If the change is not compatible, then the name ("GUID") must not > stay the same. >=20 > Here are some examples where edk2 code assigns (potentially) nonzero > values to Reserved fields, or it branches to different logic dependent on > reserved fields being zero vs. nonzero: >=20 > - CbParseFbInfo() > [CorebootModulePkg/Library/CbParseLib/CbParseLib.c] > - FbGopCheckForVbe() [CorebootPayloadPkg/FbGop/FbGop.c] > - BiosVideoCheckForVbe() [DuetPkg/BiosVideoThunkDxe/BiosVideo.c] > - BiosVideoCheckForVbe() > [IntelFrameworkModulePkg/Csm/BiosThunk/VideoDxe/BiosVideo.c] > - LegacyBiosInt86() > [IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c] > - LegacyBiosFarCall86() > [IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c] > - DhcpSendMessage() > [MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c] > - PxeBcDiscvBootService() > [MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcDhcp.c] > - DevPathFromTextSAS() > [MdePkg/Library/UefiDevicePathLib/DevicePathFromText.c] > - GetDns4ServerFromDhcp4() [NetworkPkg/DnsDxe/DnsDhcp.c] > - Ip6ProcessRouterAdvertise() [NetworkPkg/Ip6Dxe/Ip6Nd.c] > - PxeBcDhcp4Discover() [NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c] > - OhciSetTDField() > [QuarkSocPkg/QuarkSouthCluster/Usb/Ohci/Dxe/OhciUrb.c] > - OhciSetTDField() > [QuarkSocPkg/QuarkSouthCluster/Usb/Ohci/Pei/OhciUrb.c] > - "MiscBiosVendor" > [Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscBiosVendorData.c] >=20 > It's obvious that C code frequently takes advantage of new spec releases = / > bit definitions *before* the corresponding Reserved fields are updated in > the header files. That's an extremely bad practice, but it is still fact.= If we > keep those field names, but change the fields' meanings, code will silent= ly > break. We should make the breakage explicit, by *not* recycling names. Ju= st > delete the old names, and introduce brand new ones. >=20 > To be clear, this refers to patches #8 and #14; I'm ready to ack the rest= of the > patches. >=20 > Thanks > Laszlo >=20 > > > > Cc: Michael D Kinney > > Cc: Ruiyu Ni > > Cc: Laszlo Ersek > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong > > > > > > Eric Dong (14): > > UefiCpuPkg/Include/Register/Msr: Update reference spec info. > > UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h: Add new MSR file > > for goldmont plus microarchitecture. > > UefiCpuPkg/Include/Register/Msr/SilvermontMsr.h: Add new MSR. > > UefiCpuPkg/Include/Register/Msr/*.h: Add new MSR. > > UefiCpuPkg/Include/Register/Msr/XeonPhiMsr.h: Add new MSR. > > UefiCpuPkg/Include/Register/Msr/SkylakeMsr.h: Add new MSRs. > > UefiCpuPkg/Include/Register/ArchitecturalMsr.h: Add new MSR. > > UefiCpuPkg/Include/Register/ArchitecturalMsr.h: Change structure > > definition. > > UefiCpuPkg/Include/Register/Msr/Core2Msr.h: Remove old MSR. > > UefiCpuPkg/Include/Register/Msr/P6Msr.h: Remove old MSR. > > UefiCpuPkg/Include/Register/Msr/CoreMsr.h: Remove old MSR. > > UefiCpuPkg/Include/Register/Msr/SkylakeMsr.h: Add new MSR name > and > > keep old one. > > UefiCpuPkg/Include/Register/Msr/GoldmontMsr.h: Add new MSR name > and > > keep old one. > > UefiCpuPkg/Include/Register/Msr/XeonPhiMsr.h: Change structure > > definition. > > > > UefiCpuPkg/Include/Register/ArchitecturalMsr.h | 130 +- > > UefiCpuPkg/Include/Register/Msr.h | 7 +- > > UefiCpuPkg/Include/Register/Msr/AtomMsr.h | 28 +- > > UefiCpuPkg/Include/Register/Msr/BroadwellMsr.h | 62 +- > > UefiCpuPkg/Include/Register/Msr/Core2Msr.h | 102 +- > > UefiCpuPkg/Include/Register/Msr/CoreMsr.h | 74 +- > > UefiCpuPkg/Include/Register/Msr/GoldmontMsr.h | 88 +- > > UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h | 272 ++++ > > UefiCpuPkg/Include/Register/Msr/HaswellEMsr.h | 62 +- > > UefiCpuPkg/Include/Register/Msr/HaswellMsr.h | 34 +- > > UefiCpuPkg/Include/Register/Msr/IvyBridgeMsr.h | 8 +- > > UefiCpuPkg/Include/Register/Msr/NehalemMsr.h | 52 +- > > UefiCpuPkg/Include/Register/Msr/P6Msr.h | 60 +- > > UefiCpuPkg/Include/Register/Msr/Pentium4Msr.h | 202 +-- > > UefiCpuPkg/Include/Register/Msr/PentiumMMsr.h | 22 +- > > UefiCpuPkg/Include/Register/Msr/PentiumMsr.h | 12 +- > > UefiCpuPkg/Include/Register/Msr/SandyBridgeMsr.h | 49 +- > > UefiCpuPkg/Include/Register/Msr/SilvermontMsr.h | 100 +- > > UefiCpuPkg/Include/Register/Msr/SkylakeMsr.h | 1602 > ++++++++++++++++++++- > > UefiCpuPkg/Include/Register/Msr/Xeon5600Msr.h | 8 +- > > UefiCpuPkg/Include/Register/Msr/XeonDMsr.h | 84 +- > > UefiCpuPkg/Include/Register/Msr/XeonE7Msr.h | 6 +- > > UefiCpuPkg/Include/Register/Msr/XeonPhiMsr.h | 326 ++++- > > 23 files changed, 2813 insertions(+), 577 deletions(-) create mode > > 100644 UefiCpuPkg/Include/Register/Msr/GoldmontPlusMsr.h > > >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel