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 <[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
