[edk2-devel] [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

[Wu, Jiaxin]

> >> 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.dong at intel.com>
> >> Cc: Ray Ni <ray.ni at intel.com>
> >> Cc: Zeng Star <star.zeng at intel.com>
> >> Signed-off-by: Jiaxin Wu <jiaxin.wu at 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 <lersek at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-