I know the rule. But I saw it used in CpuDxe/CpuGdt.h and I thought it could have exceptions. Anyway, I agree keeping consistency is more important.
Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Wednesday, September 19, 2018 12:42 AM > To: Wang, Jian J <[email protected]>; [email protected] > Cc: Bi, Dandan <[email protected]>; Wu, Hao A <[email protected]>; > Dong, Eric <[email protected]>; Justen, Jordan L > <[email protected]>; > Ard Biesheuvel <[email protected]>; Gao, Liming > <[email protected]>; Kinney, Michael D <[email protected]> > Subject: Re: [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack pointer > > 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 <[email protected]> > > Cc: Hao A Wu <[email protected]> > > Cc: Eric Dong <[email protected]> > > Cc: Laszlo Ersek <[email protected]> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <[email protected]> > > --- > > 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/[email protected]/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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
