> >> v1: > >> - Thread: https://edk2.groups.io/g/devel/message/97748 > >> > >> Change-Id: Iec7bf25166bfeefb44a202285465a35b5debbce4 > > > > (1) Please don't include this in upstream patches. > >
I will resubmit the series patches according your below comments. > >> 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". > > Agree, thanks Laszlo, I will add more description to explain the hob usage and design detailed. > > (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. Agree, I will separate the patch to the new series patches for the producer-consumer scenario. > > > > (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> > > Good catch for the missed lib instance. Thanks Laszlo. > > (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 > > ? > That's depends on which patch merged first, then the later patch need consider the case as the new design. > Cc'ing Abdul. > > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98424): https://edk2.groups.io/g/devel/message/98424 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] -=-=-=-=-=-=-=-=-=-=-=-
