Hi Laszlo,
> -----Original Message-----
> From: edk2-devel [mailto:[email protected]] On Behalf Of
> Laszlo Ersek
> Sent: Tuesday, September 18, 2018 5:02 PM
> To: Dong, Eric <[email protected]>; [email protected]
> Cc: Kinney, Michael D <[email protected]>; Ni, Ruiyu
> <[email protected]>
> 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 <[email protected]>
> > Cc: Ruiyu Ni <[email protected]>
> > Cc: Laszlo Ersek <[email protected]>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <[email protected]>
> >
> >
> > 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
> [email protected]
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
