Adding Jordan, Ard, Liming, Mike; comment at the bottom: On 09/18/18 11:04, Jian J Wang wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1186 > > This patch uses SetJump() to get the stack pointer from esp/rsp > register to replace local variable way, which was marked by static > code checker as an unsafe way. > > Cc: Dandan Bi <dandan...@intel.com> > Cc: Hao A Wu <hao.a...@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > --- > UefiCpuPkg/CpuMpPei/CpuMpPei.h | 8 ++++++++ > UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 +++++++-- > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPei.h > index d097a66aa8..fe61f5e3bc 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h > +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h > @@ -35,6 +35,14 @@ > > extern EFI_PEI_PPI_DESCRIPTOR mPeiCpuMpPpiDesc; > > +#if defined (MDE_CPU_IA32) > +#define CPU_STACK_POINTER(Context) ((Context).Esp) > +#elif defined (MDE_CPU_X64) > +#define CPU_STACK_POINTER(Context) ((Context).Rsp) > +#else > +#error CPU type not supported! > +#endif > + > /** > This service retrieves the number of logical processor in the platform > and the number of those logical processors that are enabled on this boot. > diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c > index c7e0822452..997c20c26e 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c > +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c > @@ -517,9 +517,14 @@ GetStackBase ( > IN OUT VOID *Buffer > ) > { > - EFI_PHYSICAL_ADDRESS StackBase; > + EFI_PHYSICAL_ADDRESS StackBase; > + BASE_LIBRARY_JUMP_BUFFER Context; > > - StackBase = (EFI_PHYSICAL_ADDRESS)(UINTN)&StackBase; > + // > + // Retrieve stack pointer from current processor context. > + // > + SetJump (&Context); > + StackBase = (EFI_PHYSICAL_ADDRESS)CPU_STACK_POINTER (Context); > StackBase += BASE_4KB; > StackBase &= ~((EFI_PHYSICAL_ADDRESS)BASE_4KB - 1); > StackBase -= PcdGet32(PcdCpuApStackSize); >
I think using SetJump() for this purpose, in contexts where library constructors have run, is a good idea. What I like less is that we are open-coding this trick here, in CpuMpPei. Getting the stack pointer in C code is frequently necessary, and I would prefer an API addition to MdePkg's BaseLib, implemented for as many architectures as possible. One discussion that I recall about this is the sub-thread at <https://www.mail-archive.com/edk2-devel@lists.01.org/msg32216.html>. If the MdePkg maintainers disagree with the BaseLib API addition, then the patch should still be improved, if possible. Mike said earlier that in C files we like to avoid MDE_CPU_* dependent-code, instead we extract the affected function(s) to architecture-dependent subdirectories, and use [Sources.<ARCH>] sections in the INF files. That suggests files like: - UefiCpuPkg/CpuMpPei/Ia32/GetStackBase.c - UefiCpuPkg/CpuMpPei/X64/GetStackBase.c here. Possibly overkill, yes, but we should be consistent. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
