Drop this patch and refine the format according Laszlo's comment. Please help review the series patches @ https://edk2.groups.io/g/devel/message/98446 https://edk2.groups.io/g/devel/message/98447 https://edk2.groups.io/g/devel/message/98448 https://edk2.groups.io/g/devel/message/98449 https://edk2.groups.io/g/devel/message/98450
Thanks, Jiaxin > -----Original Message----- > From: Wu, Jiaxin > Sent: Friday, January 13, 2023 11:05 AM > To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Zeng, Star > <star.z...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Abdul Lateef > Attar <abdat...@amd.com> > Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg: Support SMM Relocated > SmBase handling > > > >> 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 (#98452): https://edk2.groups.io/g/devel/message/98452 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] -=-=-=-=-=-=-=-=-=-=-=-
