On 1/12/23 13:37, Laszlo Ersek wrote: > On 1/11/23 09:35, Wu, Jiaxin wrote: >> Mainly changes as below: >> 1. Add Smm Base HOB, which is used to store the information of >> Smm Relocated SmBase array for each Processors; >> 2. Combine 2 SMIs (gcSmmInitTemplate & gcSmiHandlerTemplate) into one >> (gcSmiHandlerTemplate), the new SMI handler needs to run to 2 paths: one >> to SmmCpuFeaturesInitializeProcessor(), the other to SMM Core Entry Point. >> 3. Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init >> before normal SMI sources happen. >> 4. Call SmmCpuFeaturesInitializeProcessor() in parallel. >> >> v2: >> - Refine the coding style >> - Rename hob to gSmmBaseHobGuid >> - Update SmmInitHandler() to handle the SMM relocation >> - Correct the S3 for SMM relocation >> >> v1: >> - Thread: https://edk2.groups.io/g/devel/message/97748 >> >> Change-Id: Iec7bf25166bfeefb44a202285465a35b5debbce4 > > (1) Please don't include this in upstream patches. > >> Cc: Eric Dong <eric.d...@intel.com> >> Cc: Ray Ni <ray...@intel.com> >> Cc: Zeng Star <star.z...@intel.com> >> Signed-off-by: Jiaxin Wu <jiaxin...@intel.com> > > (2) This patch is for UefiCpuPkg, but Gerd has not been CC'd, as far as > I can tell. CC'ing him now. (Please refer to commit 0aca5901e344, > "Maintainers.txt: designate Gerd Hoffmann as UefiCpuPkg reviewer", > 2023-01-06). > >> --- >> UefiCpuPkg/Include/Guid/SmmBaseHob.h | 36 +++++ >> .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 2 + >> .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c | 24 +++- >> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 + >> .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 1 + >> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 1 - >> .../StandaloneMmCpuFeaturesLib.inf | 4 + >> UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 39 +++++- >> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 25 +++- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 149 >> ++++++++++++++++----- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 21 ++- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 + >> UefiCpuPkg/UefiCpuPkg.dec | 3 + >> 13 files changed, 261 insertions(+), 49 deletions(-) >> create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h > > (3) The patch extends the interface between PiSmmCpuDxeSmm and multipe > SmmCpuFeaturesLib instances. There is no concise and complete design > description, either in the commit message, or in a TianoCore bugzilla > ticket (no reference in your commit message), or in the new header file > "SmmBaseHob.h". > > (4) The commit modifies multiple modules at once. In a producer-consumer > scenario (which is usually characteristic of HOBs), we tend to extend > the producer first (if there are multiple producers, then each in > separation), and then the consumers. Usually the consumers are expected > to keep compatibility with the lack of the HOB, if possible. > > The compatibility aspects are not explained, and the modifications are > squashed together in a single patch. Unacceptable. > > (5) OvmfPkg includes its own SmmCpuFeaturesLib instance, but there's not > a peep about OvmfPkg in the patch (code or commit message), not even an > explanation why OvmfPkg is supposed to be unaffected. Unacceptable. > > Nacked-by: Laszlo Ersek <ler...@redhat.com> >
(6) BTW, does this patch conflict with (or at least require coordination with): [edk2-devel] [PATCH v2 0/6] Adds AmdSmmCpuFeaturesLib https://edk2.groups.io/g/devel/message/98270 ? Cc'ing Abdul. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98363): https://edk2.groups.io/g/devel/message/98363 Mute This Topic: https://groups.io/mt/96196283/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
