Laszlo, You mean to add a specific interface to get stack pointer, right? I think it work for me too. But I guess there might be discussions before on such thing. Since it's not there, there might be some reasons not to do it.
Mike and Liming, do you have any comments on this? Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Wednesday, September 26, 2018 4:30 PM > To: Wang, Jian J <[email protected]>; Justen, Jordan L > <[email protected]>; [email protected] > Cc: Wu, Hao A <[email protected]>; Bi, Dandan <[email protected]>; > Dong, Eric <[email protected]>; Kinney, Michael D > <[email protected]> > Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack > pointer > > On 09/26/18 04:18, Wang, Jian J wrote: > > Hi, > > > > Since the patch will introduce "#if defined(...)" macro in code, which > > violates > > edk2 coding style, it's suggested to add exception to static checker. > > > > I'll wait for one or two days in case there's other suggestions. If no > > objection > > then, I'll withdraw this patch and close BZ#1186 as not-fix. > > If we can *selectively* suppress this one warning from the static > checker (saying that "yeah we know what we are doing"), then I agree > WONTFIX is acceptable for the BZ. It's not optimal IMO (I think there > would be value in a generic facility for getting the stack pointer -- > several places in edk2 want to know the stack pointer), but it is > acceptable. (As far as I'm concerned anyway.) > > Thanks > Laszlo > > >> -----Original Message----- > >> From: Laszlo Ersek [mailto:[email protected]] > >> Sent: Wednesday, September 19, 2018 7:17 PM > >> To: Justen, Jordan L <[email protected]>; Wang, Jian J > >> <[email protected]>; [email protected] > >> Cc: Wu, Hao A <[email protected]>; Bi, Dandan <[email protected]>; > >> Dong, Eric <[email protected]>; Kinney, Michael D > >> <[email protected]> > >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get > stack > >> pointer > >> > >> On 09/18/18 20:02, Jordan Justen wrote: > >>> I guess the git config sendemail.from setting did not help your > >>> patches. ?? It still is coming through with a From field of > >>> <[email protected]>. > >>> > >>> Regarding this patch, I suppose it is worth asking if &StackBase in > >>> the old code could possibly be an address not on the stack. I don't > >>> think it is possible, and I'm guessing the C specification would > >>> probably back that up. > >>> > >>> It can be unsafe to get an address of something on the stack and then > >>> refer to that address after the variable is no longer in scope. I > >>> suspect this is what the static checker is noticing. By calling > >>> SetJump, aren't we just doing the same thing, but hiding what we are > >>> doing from the static checker? > >> > >> Yep, we're totally doing that. > >> > >> Thanks, > >> Laszlo > >> > >>> > >>> So, can't we just tell the static checker to ignore the error because > >>> we know what we are doing? > >>> > >>> -Jordan > >>> > >>> On 2018-09-18 02:04:48, 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); > >>>> -- > >>>> 2.16.2.windows.1 > >>>> > >>>> _______________________________________________ > >>>> edk2-devel mailing list > >>>> [email protected] > >>>> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
