From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 448B821106F08 for ; Tue, 18 Sep 2018 02:01:49 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C693D30842AE; Tue, 18 Sep 2018 09:01:48 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-209.rdu2.redhat.com [10.10.120.209]) by smtp.corp.redhat.com (Postfix) with ESMTP id 81ADA89227; Tue, 18 Sep 2018 09:01:47 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Michael D Kinney , Ruiyu Ni References: <20180918014330.28336-1-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <2a7532fe-4fcf-ddb7-95d0-acfd9b4657cf@redhat.com> Date: Tue, 18 Sep 2018 11:01:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180918014330.28336-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Tue, 18 Sep 2018 09:01:48 +0000 (UTC) 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: Tue, 18 Sep 2018 09:01:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. This sounds good. Macros and structure names are easy to grep. > 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 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, but then reintroduces the field in place of the Reserved3 field. IMO, this is risky: if code exists (possibly out-of-tree) that used to refer 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 will likely still *compile*, but it will not work. 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 ever before) should be introduced. 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. 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: - 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] 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 silently break. We should make the breakage explicit, by *not* recycling names. Just delete the old names, and introduce brand new ones. To be clear, this refers to patches #8 and #14; I'm ready to ack the rest of the patches. Thanks Laszlo > > 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 >