Re: [Patch 00/14] Update MSR definitions.

["Dong, Eric" <eric.dong@intel.com>]

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 <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch 00/14] Update MSR definitions.
> 
> 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.
> 

Thanks for your suggestion. I think it's a good idea to use totally new ReservedX 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.
> 
> 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 <michael.d.kinney@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> >
> >
> > 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
> >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel