* [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard @ 2017-12-29 8:36 Jian J Wang 2018-01-03 7:05 ` Wang, Jian J 2018-01-03 17:33 ` Laszlo Ersek 0 siblings, 2 replies; 19+ messages in thread From: Jian J Wang @ 2017-12-29 8:36 UTC (permalink / raw) To: edk2-devel; +Cc: Jiewen Yao, Eric Dong, Laszlo Ersek The reason is that DXE part initialization will reuse the stack allocated at PEI phase, if MP was initialized before. Some code added to check this situation and use stack base address saved in HOB passed from PEI. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang <jian.j.wang@intel.com> --- UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c index 40c1bf407a..05484c9ff3 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c @@ -295,6 +295,7 @@ InitMpGlobalData ( UINTN Index; EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; UINTN StackBase; + CPU_INFO_IN_HOB *CpuInfoInHob; SaveCpuMpData (CpuMpData); @@ -314,9 +315,18 @@ InitMpGlobalData ( ASSERT (FALSE); } - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { - StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize; + // + // DXE will reuse stack allocated for APs at PEI phase if it's available. + // Let's check it here. + // + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob; + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { + StackBase = CpuInfoInHob->ApTopOfStack; + } else { + StackBase = CpuMpData->Buffer; + } + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); ASSERT_EFI_ERROR (Status); @@ -326,6 +336,9 @@ InitMpGlobalData ( MemDesc.Attributes | EFI_MEMORY_RP ); ASSERT_EFI_ERROR (Status); + + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", StackBase, Index)); + StackBase += CpuMpData->CpuApStackSize; } } -- 2.15.1.windows.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2017-12-29 8:36 [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Jian J Wang @ 2018-01-03 7:05 ` Wang, Jian J 2018-01-03 17:33 ` Laszlo Ersek 1 sibling, 0 replies; 19+ messages in thread From: Wang, Jian J @ 2018-01-03 7:05 UTC (permalink / raw) To: Yao, Jiewen Cc: Laszlo Ersek, Dong, Eric, Wang, Jian J, edk2-devel@lists.01.org Jiewen, Please take a look at this patch. Regards, Jian > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian J > Wang > Sent: Friday, December 29, 2017 4:37 PM > To: edk2-devel@lists.01.org > Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; > Dong, Eric <eric.dong@intel.com> > Subject: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as > Stack Guard > > The reason is that DXE part initialization will reuse the stack allocated > at PEI phase, if MP was initialized before. Some code added to check this > situation and use stack base address saved in HOB passed from PEI. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > --- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index 40c1bf407a..05484c9ff3 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -295,6 +295,7 @@ InitMpGlobalData ( > UINTN Index; > EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > UINTN StackBase; > + CPU_INFO_IN_HOB *CpuInfoInHob; > > SaveCpuMpData (CpuMpData); > > @@ -314,9 +315,18 @@ InitMpGlobalData ( > ASSERT (FALSE); > } > > - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > - StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize; > + // > + // DXE will reuse stack allocated for APs at PEI phase if it's available. > + // Let's check it here. > + // > + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob; > + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { > + StackBase = CpuInfoInHob->ApTopOfStack; > + } else { > + StackBase = CpuMpData->Buffer; > + } > > + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); > ASSERT_EFI_ERROR (Status); > > @@ -326,6 +336,9 @@ InitMpGlobalData ( > MemDesc.Attributes | EFI_MEMORY_RP > ); > ASSERT_EFI_ERROR (Status); > + > + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", StackBase, > Index)); > + StackBase += CpuMpData->CpuApStackSize; > } > } > > -- > 2.15.1.windows.2 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2017-12-29 8:36 [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Jian J Wang 2018-01-03 7:05 ` Wang, Jian J @ 2018-01-03 17:33 ` Laszlo Ersek 2018-01-04 0:41 ` Wang, Jian J 1 sibling, 1 reply; 19+ messages in thread From: Laszlo Ersek @ 2018-01-03 17:33 UTC (permalink / raw) To: Jian J Wang, edk2-devel; +Cc: Jiewen Yao, Eric Dong, Jeff Fan (CC Jeff) Sorry about the delay. I have some light comments below; I expect at least a few of them to be incorrect :) On 12/29/17 09:36, Jian J Wang wrote: > The reason is that DXE part initialization will reuse the stack allocated > at PEI phase, if MP was initialized before. Some code added to check this > situation and use stack base address saved in HOB passed from PEI. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > --- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index 40c1bf407a..05484c9ff3 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -295,6 +295,7 @@ InitMpGlobalData ( > UINTN Index; > EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > UINTN StackBase; > + CPU_INFO_IN_HOB *CpuInfoInHob; > > SaveCpuMpData (CpuMpData); > > @@ -314,9 +315,18 @@ InitMpGlobalData ( > ASSERT (FALSE); > } > > - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > - StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize; > + // > + // DXE will reuse stack allocated for APs at PEI phase if it's available. > + // Let's check it here. > + // > + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob; > + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { > + StackBase = CpuInfoInHob->ApTopOfStack; > + } else { > + StackBase = CpuMpData->Buffer; > + } So, if the HOB is not found, then StackBase is set okay. However, I'm unsure about the other case. The CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack (highest address, and the stack grows down); however the loop below *increments* StackBase. Given the incrementing nature of the loop, shouldn't we first calculate the actual base (= lowest address) from the CPU_INFO_IN_HOB.ApTopOfStack field? Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any given processor #N, we should not calculate the stack base as CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData->CpuApStackSize instead we should calculate the stack base as something like: CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData->CpuApStackSize See - the InitializeApData() function, - and its call site in the ApWakeupFunction() function. (To my surprise, I personally modified InitializeApData() earlier, in commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses", 2016-11-17) -- I've totally forgotten about that by now!) What do you think? > > + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); > ASSERT_EFI_ERROR (Status); > > @@ -326,6 +336,9 @@ InitMpGlobalData ( > MemDesc.Attributes | EFI_MEMORY_RP > ); > ASSERT_EFI_ERROR (Status); > + > + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", StackBase, Index)); StackBase has type UINTN, and so it should not be printed with %x. It should be cast to (UINT64), and then printed with %Lx. Similarly, Index has type UINTN. It should not be printed with %d. It should be cast to (UINT64) and printed with %Lu. > + StackBase += CpuMpData->CpuApStackSize; Again, I don't think the simple increment applies when the CpuMpData->CpuInfoInHob array exists. > } > } > > Thanks, Laszlo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-03 17:33 ` Laszlo Ersek @ 2018-01-04 0:41 ` Wang, Jian J 2018-01-04 1:09 ` Wang, Jian J 0 siblings, 1 reply; 19+ messages in thread From: Wang, Jian J @ 2018-01-04 0:41 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Jeff Fan Laszlo, I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack was assigned to CpuMpData->Buffer in MpInitLibInitialize() (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); but in (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize; (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack); Since InitMpGlobalData() is called just after first situation, my patch is correct. I think the problem here is that ApTopOfStack initialized at line 1501 is not correct. Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, January 04, 2018 1:33 AM > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; > Jeff Fan <vanjeff_919@hotmail.com> > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > (CC Jeff) > > Sorry about the delay. > > I have some light comments below; I expect at least a few of them to be > incorrect :) > > On 12/29/17 09:36, Jian J Wang wrote: > > The reason is that DXE part initialization will reuse the stack allocated > > at PEI phase, if MP was initialized before. Some code added to check this > > situation and use stack base address saved in HOB passed from PEI. > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > > --- > > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > index 40c1bf407a..05484c9ff3 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > @@ -295,6 +295,7 @@ InitMpGlobalData ( > > UINTN Index; > > EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > > UINTN StackBase; > > + CPU_INFO_IN_HOB *CpuInfoInHob; > > > > SaveCpuMpData (CpuMpData); > > > > @@ -314,9 +315,18 @@ InitMpGlobalData ( > > ASSERT (FALSE); > > } > > > > - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > > - StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize; > > + // > > + // DXE will reuse stack allocated for APs at PEI phase if it's available. > > + // Let's check it here. > > + // > > + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- > >CpuInfoInHob; > > + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { > > + StackBase = CpuInfoInHob->ApTopOfStack; > > + } else { > > + StackBase = CpuMpData->Buffer; > > + } > > So, if the HOB is not found, then StackBase is set okay. > > However, I'm unsure about the other case. The > CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack > (highest address, and the stack grows down); however the loop below > *increments* StackBase. Given the incrementing nature of the loop, > shouldn't we first calculate the actual base (= lowest address) from the > CPU_INFO_IN_HOB.ApTopOfStack field? > > Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field > points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any > given processor #N, we should not calculate the stack base as > > CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- > >CpuApStackSize > > instead we should calculate the stack base as something like: > > CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData->CpuApStackSize > > See > - the InitializeApData() function, > - and its call site in the ApWakeupFunction() function. > > (To my surprise, I personally modified InitializeApData() earlier, in > commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack > addresses", 2016-11-17) -- I've totally forgotten about that by now!) > > What do you think? > > > > > + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > > Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); > > ASSERT_EFI_ERROR (Status); > > > > @@ -326,6 +336,9 @@ InitMpGlobalData ( > > MemDesc.Attributes | EFI_MEMORY_RP > > ); > > ASSERT_EFI_ERROR (Status); > > + > > + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", > StackBase, Index)); > > StackBase has type UINTN, and so it should not be printed with %x. It > should be cast to (UINT64), and then printed with %Lx. > > Similarly, Index has type UINTN. It should not be printed with %d. It > should be cast to (UINT64) and printed with %Lu. > > > > + StackBase += CpuMpData->CpuApStackSize; > > Again, I don't think the simple increment applies when the > CpuMpData->CpuInfoInHob array exists. > > > } > > } > > > > > > Thanks, > Laszlo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-04 0:41 ` Wang, Jian J @ 2018-01-04 1:09 ` Wang, Jian J 2018-01-04 1:45 ` Wang, Jian J 2018-01-04 12:18 ` Laszlo Ersek 0 siblings, 2 replies; 19+ messages in thread From: Wang, Jian J @ 2018-01-04 1:09 UTC (permalink / raw) To: Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org Cc: Yao, Jiewen, Dong, Eric Laszlo, More explanations: [UefiCpuPkg\Library\MpInitLib\MpLib.c] According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized to the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized (line 598). Although my calculation is correct, I think it'd be better to use AP's ApTopOfStack directly. From this perspective, you're right. Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't need it anyway. Regards, Jian > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, > Jian J > Sent: Thursday, January 04, 2018 8:42 AM > To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > Laszlo, > > I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack > was assigned to CpuMpData->Buffer in MpInitLibInitialize() > > (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); > > but in > > (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * > CpuMpData->CpuApStackSize; > (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, > ApTopOfStack); > > Since InitMpGlobalData() is called just after first situation, my patch is correct. > > I think the problem here is that ApTopOfStack initialized at line 1501 is not > correct. > > > Regards, > Jian > > > > -----Original Message----- > > From: Laszlo Ersek [mailto:lersek@redhat.com] > > Sent: Thursday, January 04, 2018 1:33 AM > > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; > > Jeff Fan <vanjeff_919@hotmail.com> > > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > > as Stack Guard > > > > (CC Jeff) > > > > Sorry about the delay. > > > > I have some light comments below; I expect at least a few of them to be > > incorrect :) > > > > On 12/29/17 09:36, Jian J Wang wrote: > > > The reason is that DXE part initialization will reuse the stack allocated > > > at PEI phase, if MP was initialized before. Some code added to check this > > > situation and use stack base address saved in HOB passed from PEI. > > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > > Cc: Eric Dong <eric.dong@intel.com> > > > Cc: Laszlo Ersek <lersek@redhat.com> > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > > > --- > > > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- > > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > index 40c1bf407a..05484c9ff3 100644 > > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > @@ -295,6 +295,7 @@ InitMpGlobalData ( > > > UINTN Index; > > > EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > > > UINTN StackBase; > > > + CPU_INFO_IN_HOB *CpuInfoInHob; > > > > > > SaveCpuMpData (CpuMpData); > > > > > > @@ -314,9 +315,18 @@ InitMpGlobalData ( > > > ASSERT (FALSE); > > > } > > > > > > - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > > > - StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize; > > > + // > > > + // DXE will reuse stack allocated for APs at PEI phase if it's available. > > > + // Let's check it here. > > > + // > > > + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- > > >CpuInfoInHob; > > > + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { > > > + StackBase = CpuInfoInHob->ApTopOfStack; > > > + } else { > > > + StackBase = CpuMpData->Buffer; > > > + } > > > > So, if the HOB is not found, then StackBase is set okay. > > > > However, I'm unsure about the other case. The > > CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack > > (highest address, and the stack grows down); however the loop below > > *increments* StackBase. Given the incrementing nature of the loop, > > shouldn't we first calculate the actual base (= lowest address) from the > > CPU_INFO_IN_HOB.ApTopOfStack field? > > > > Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field > > points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any > > given processor #N, we should not calculate the stack base as > > > > CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- > > >CpuApStackSize > > > > instead we should calculate the stack base as something like: > > > > CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData->CpuApStackSize > > > > See > > - the InitializeApData() function, > > - and its call site in the ApWakeupFunction() function. > > > > (To my surprise, I personally modified InitializeApData() earlier, in > > commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack > > addresses", 2016-11-17) -- I've totally forgotten about that by now!) > > > > What do you think? > > > > > > > > + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > > > Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); > > > ASSERT_EFI_ERROR (Status); > > > > > > @@ -326,6 +336,9 @@ InitMpGlobalData ( > > > MemDesc.Attributes | EFI_MEMORY_RP > > > ); > > > ASSERT_EFI_ERROR (Status); > > > + > > > + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", > > StackBase, Index)); > > > > StackBase has type UINTN, and so it should not be printed with %x. It > > should be cast to (UINT64), and then printed with %Lx. > > > > Similarly, Index has type UINTN. It should not be printed with %d. It > > should be cast to (UINT64) and printed with %Lu. > > > > > > > + StackBase += CpuMpData->CpuApStackSize; > > > > Again, I don't think the simple increment applies when the > > CpuMpData->CpuInfoInHob array exists. > > > > > } > > > } > > > > > > > > > > Thanks, > > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-04 1:09 ` Wang, Jian J @ 2018-01-04 1:45 ` Wang, Jian J 2018-01-04 12:21 ` Laszlo Ersek 2018-01-04 12:18 ` Laszlo Ersek 1 sibling, 1 reply; 19+ messages in thread From: Wang, Jian J @ 2018-01-04 1:45 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack correctly because the MP service supports BSP/AP exchange. So I think the line 1501 should be changed to InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); instead of InitializeApData (CpuMpData, 0, 0, NULL); Regards, Jian > -----Original Message----- > From: Wang, Jian J > Sent: Thursday, January 04, 2018 9:09 AM > To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; > edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > Laszlo, > > More explanations: > > [UefiCpuPkg\Library\MpInitLib\MpLib.c] > According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized > to > the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized > (line 598). Although my calculation is correct, I think it'd be better to use AP's > ApTopOfStack directly. From this perspective, you're right. > > Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't > need > it anyway. > > Regards, > Jian > > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Wang, > > Jian J > > Sent: Thursday, January 04, 2018 8:42 AM > > To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > > as Stack Guard > > > > Laszlo, > > > > I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack > > was assigned to CpuMpData->Buffer in MpInitLibInitialize() > > > > (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); > > > > but in > > > > (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * > > CpuMpData->CpuApStackSize; > > (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, > > ApTopOfStack); > > > > Since InitMpGlobalData() is called just after first situation, my patch is correct. > > > > I think the problem here is that ApTopOfStack initialized at line 1501 is not > > correct. > > > > > > Regards, > > Jian > > > > > > > -----Original Message----- > > > From: Laszlo Ersek [mailto:lersek@redhat.com] > > > Sent: Thursday, January 04, 2018 1:33 AM > > > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; > > > Jeff Fan <vanjeff_919@hotmail.com> > > > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address > set > > > as Stack Guard > > > > > > (CC Jeff) > > > > > > Sorry about the delay. > > > > > > I have some light comments below; I expect at least a few of them to be > > > incorrect :) > > > > > > On 12/29/17 09:36, Jian J Wang wrote: > > > > The reason is that DXE part initialization will reuse the stack allocated > > > > at PEI phase, if MP was initialized before. Some code added to check this > > > > situation and use stack base address saved in HOB passed from PEI. > > > > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > > > Cc: Eric Dong <eric.dong@intel.com> > > > > Cc: Laszlo Ersek <lersek@redhat.com> > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > > > > --- > > > > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- > > > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > > index 40c1bf407a..05484c9ff3 100644 > > > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > > @@ -295,6 +295,7 @@ InitMpGlobalData ( > > > > UINTN Index; > > > > EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > > > > UINTN StackBase; > > > > + CPU_INFO_IN_HOB *CpuInfoInHob; > > > > > > > > SaveCpuMpData (CpuMpData); > > > > > > > > @@ -314,9 +315,18 @@ InitMpGlobalData ( > > > > ASSERT (FALSE); > > > > } > > > > > > > > - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > > > > - StackBase = CpuMpData->Buffer + Index * CpuMpData- > >CpuApStackSize; > > > > + // > > > > + // DXE will reuse stack allocated for APs at PEI phase if it's available. > > > > + // Let's check it here. > > > > + // > > > > + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- > > > >CpuInfoInHob; > > > > + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { > > > > + StackBase = CpuInfoInHob->ApTopOfStack; > > > > + } else { > > > > + StackBase = CpuMpData->Buffer; > > > > + } > > > > > > So, if the HOB is not found, then StackBase is set okay. > > > > > > However, I'm unsure about the other case. The > > > CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack > > > (highest address, and the stack grows down); however the loop below > > > *increments* StackBase. Given the incrementing nature of the loop, > > > shouldn't we first calculate the actual base (= lowest address) from the > > > CPU_INFO_IN_HOB.ApTopOfStack field? > > > > > > Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field > > > points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any > > > given processor #N, we should not calculate the stack base as > > > > > > CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- > > > >CpuApStackSize > > > > > > instead we should calculate the stack base as something like: > > > > > > CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- > >CpuApStackSize > > > > > > See > > > - the InitializeApData() function, > > > - and its call site in the ApWakeupFunction() function. > > > > > > (To my surprise, I personally modified InitializeApData() earlier, in > > > commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack > > > addresses", 2016-11-17) -- I've totally forgotten about that by now!) > > > > > > What do you think? > > > > > > > > > > > + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > > > > Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); > > > > ASSERT_EFI_ERROR (Status); > > > > > > > > @@ -326,6 +336,9 @@ InitMpGlobalData ( > > > > MemDesc.Attributes | EFI_MEMORY_RP > > > > ); > > > > ASSERT_EFI_ERROR (Status); > > > > + > > > > + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", > > > StackBase, Index)); > > > > > > StackBase has type UINTN, and so it should not be printed with %x. It > > > should be cast to (UINT64), and then printed with %Lx. > > > > > > Similarly, Index has type UINTN. It should not be printed with %d. It > > > should be cast to (UINT64) and printed with %Lu. > > > > > > > > > > + StackBase += CpuMpData->CpuApStackSize; > > > > > > Again, I don't think the simple increment applies when the > > > CpuMpData->CpuInfoInHob array exists. > > > > > > > } > > > > } > > > > > > > > > > > > > > Thanks, > > > Laszlo > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-04 1:45 ` Wang, Jian J @ 2018-01-04 12:21 ` Laszlo Ersek 2018-01-05 0:52 ` Wang, Jian J 2018-01-05 1:40 ` 答复: " Fan Jeff 0 siblings, 2 replies; 19+ messages in thread From: Laszlo Ersek @ 2018-01-04 12:21 UTC (permalink / raw) To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; >> edk2-devel@lists.01.org >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; >>>> Jeff Fan <vanjeff_919@hotmail.com> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack allocated >>>>> at PEI phase, if MP was initialized before. Some code added to check this >>>>> situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>>>> Cc: Eric Dong <eric.dong@intel.com> >>>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-04 12:21 ` Laszlo Ersek @ 2018-01-05 0:52 ` Wang, Jian J 2018-01-05 1:40 ` 答复: " Fan Jeff 1 sibling, 0 replies; 19+ messages in thread From: Wang, Jian J @ 2018-01-05 0:52 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric I'm not familiar with BSP/AP switch code either. I just happen to see code comments mentioned switching BSP/AP. So I think it would be better to initialize cpu 0 the same way as others. No matter what, as what the field name implies, ApTopOfStack should never be used to store stack base address. I'll try to find if any test cases for the BSP/AP switch were developed before. If non, I think it's not easy for me to write one. If it's OK, I prefer to check in current fix and test what you concern later. I think nobody uses this feature. I'll send v3 as what you suggested. Thanks a lot for all your comments. Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, January 04, 2018 8:22 PM > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > On 01/04/18 02:45, Wang, Jian J wrote: > > A correction: although BSP doesn't need it, we still need to initialize its > ApTopOfStack > > correctly because the MP service supports BSP/AP exchange. So I think the line > 1501 > > should be changed to > > > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData- > >CpuApStackSize); > > > > instead of > > > > InitializeApData (CpuMpData, 0, 0, NULL); > > Hmm... Although I don't immediately see all possible consequences of > such a change, it looks like a correct fix. > > Unfortunately, I don't know of any code that actually exercises the > BSP/AP exchange service. I think Intel must have access to some client > code like this, because I vaguely recall some patches over time that > improved BSP/AP exchange. > > If you modify the InitializeApData() call in question like suggested > above, can you perhaps locate that client code, and test the change with it? > > Thanks! > Laszlo > > > >> -----Original Message----- > >> From: Wang, Jian J > >> Sent: Thursday, January 04, 2018 9:09 AM > >> To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek > <lersek@redhat.com>; > >> edk2-devel@lists.01.org > >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address > set > >> as Stack Guard > >> > >> Laszlo, > >> > >> More explanations: > >> > >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] > >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is > initialized > >> to > >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly > initialized > >> (line 598). Although my calculation is correct, I think it'd be better to use AP's > >> ApTopOfStack directly. From this perspective, you're right. > >> > >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't > >> need > >> it anyway. > >> > >> Regards, > >> Jian > >> > >> > >>> -----Original Message----- > >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > >> Wang, > >>> Jian J > >>> Sent: Thursday, January 04, 2018 8:42 AM > >>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org > >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address > set > >>> as Stack Guard > >>> > >>> Laszlo, > >>> > >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack > >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() > >>> > >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); > >>> > >>> but in > >>> > >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * > >>> CpuMpData->CpuApStackSize; > >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, > >>> ApTopOfStack); > >>> > >>> Since InitMpGlobalData() is called just after first situation, my patch is > correct. > >>> > >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not > >>> correct. > >>> > >>> > >>> Regards, > >>> Jian > >>> > >>> > >>>> -----Original Message----- > >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] > >>>> Sent: Thursday, January 04, 2018 1:33 AM > >>>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric > <eric.dong@intel.com>; > >>>> Jeff Fan <vanjeff_919@hotmail.com> > >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address > >> set > >>>> as Stack Guard > >>>> > >>>> (CC Jeff) > >>>> > >>>> Sorry about the delay. > >>>> > >>>> I have some light comments below; I expect at least a few of them to be > >>>> incorrect :) > >>>> > >>>> On 12/29/17 09:36, Jian J Wang wrote: > >>>>> The reason is that DXE part initialization will reuse the stack allocated > >>>>> at PEI phase, if MP was initialized before. Some code added to check this > >>>>> situation and use stack base address saved in HOB passed from PEI. > >>>>> > >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com> > >>>>> Cc: Eric Dong <eric.dong@intel.com> > >>>>> Cc: Laszlo Ersek <lersek@redhat.com> > >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > >>>>> --- > >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- > >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>>> index 40c1bf407a..05484c9ff3 100644 > >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( > >>>>> UINTN Index; > >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > >>>>> UINTN StackBase; > >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; > >>>>> > >>>>> SaveCpuMpData (CpuMpData); > >>>>> > >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( > >>>>> ASSERT (FALSE); > >>>>> } > >>>>> > >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- > >>> CpuApStackSize; > >>>>> + // > >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. > >>>>> + // Let's check it here. > >>>>> + // > >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- > >>>>> CpuInfoInHob; > >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { > >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; > >>>>> + } else { > >>>>> + StackBase = CpuMpData->Buffer; > >>>>> + } > >>>> > >>>> So, if the HOB is not found, then StackBase is set okay. > >>>> > >>>> However, I'm unsure about the other case. The > >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack > >>>> (highest address, and the stack grows down); however the loop below > >>>> *increments* StackBase. Given the incrementing nature of the loop, > >>>> shouldn't we first calculate the actual base (= lowest address) from the > >>>> CPU_INFO_IN_HOB.ApTopOfStack field? > >>>> > >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field > >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any > >>>> given processor #N, we should not calculate the stack base as > >>>> > >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- > >>>>> CpuApStackSize > >>>> > >>>> instead we should calculate the stack base as something like: > >>>> > >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- > >>> CpuApStackSize > >>>> > >>>> See > >>>> - the InitializeApData() function, > >>>> - and its call site in the ApWakeupFunction() function. > >>>> > >>>> (To my surprise, I personally modified InitializeApData() earlier, in > >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack > >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) > >>>> > >>>> What do you think? > >>>> > >>>>> > >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); > >>>>> ASSERT_EFI_ERROR (Status); > >>>>> > >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( > >>>>> MemDesc.Attributes | EFI_MEMORY_RP > >>>>> ); > >>>>> ASSERT_EFI_ERROR (Status); > >>>>> + > >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", > >>>> StackBase, Index)); > >>>> > >>>> StackBase has type UINTN, and so it should not be printed with %x. It > >>>> should be cast to (UINT64), and then printed with %Lx. > >>>> > >>>> Similarly, Index has type UINTN. It should not be printed with %d. It > >>>> should be cast to (UINT64) and printed with %Lu. > >>>> > >>>> > >>>>> + StackBase += CpuMpData->CpuApStackSize; > >>>> > >>>> Again, I don't think the simple increment applies when the > >>>> CpuMpData->CpuInfoInHob array exists. > >>>> > >>>>> } > >>>>> } > >>>>> > >>>>> > >>>> > >>>> Thanks, > >>>> Laszlo > >>> _______________________________________________ > >>> edk2-devel mailing list > >>> edk2-devel@lists.01.org > >>> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-04 12:21 ` Laszlo Ersek 2018-01-05 0:52 ` Wang, Jian J @ 2018-01-05 1:40 ` Fan Jeff 2018-01-05 1:57 ` Wang, Jian J 1 sibling, 1 reply; 19+ messages in thread From: Fan Jeff @ 2018-01-05 1:40 UTC (permalink / raw) To: Laszlo Ersek, Wang, Jian J, edk2-devel@lists.01.org Cc: Yao, Jiewen, Dong, Eric Laszlo, Firstly, SwitchBSP() is one service of MP defined in PI spec. For real case, I think multiple socket system(with different processor stepping) may use this service for purpose. Thanks! Jeff 发件人: Laszlo Ersek<mailto:lersek@redhat.com> 发送时间: 2018年1月4日 20:21 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; >> edk2-devel@lists.01.org >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; >>>> Jeff Fan <vanjeff_919@hotmail.com> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack allocated >>>>> at PEI phase, if MP was initialized before. Some code added to check this >>>>> situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>>>> Cc: Eric Dong <eric.dong@intel.com> >>>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-05 1:40 ` 答复: " Fan Jeff @ 2018-01-05 1:57 ` Wang, Jian J 2018-01-05 2:48 ` 答复: " Fan Jeff 0 siblings, 1 reply; 19+ messages in thread From: Wang, Jian J @ 2018-01-05 1:57 UTC (permalink / raw) To: Fan Jeff, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric Hi Jeff, Do you think the change is OK? Do you know how to test switching BSP? Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 9:40 AM To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Laszlo, Firstly, SwitchBSP() is one service of MP defined in PI spec. For real case, I think multiple socket system(with different processor stepping) may use this service for purpose. Thanks! Jeff 发件人: Laszlo Ersek<mailto:lersek@redhat.com> 发送时间: 2018年1月4日 20:21 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack allocated >>>>> at PEI phase, if MP was initialized before. Some code added to check this >>>>> situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-05 1:57 ` Wang, Jian J @ 2018-01-05 2:48 ` Fan Jeff 2018-01-05 2:49 ` Fan Jeff 0 siblings, 1 reply; 19+ messages in thread From: Fan Jeff @ 2018-01-05 2:48 UTC (permalink / raw) To: Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org Cc: Yao, Jiewen, Dong, Eric You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to know if the switch is successfully. Jeff ________________________________ From: Wang, Jian J <jian.j.wang@intel.com> Sent: Friday, January 5, 2018 9:57:31 AM To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org Cc: Yao, Jiewen; Dong, Eric Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Hi Jeff, Do you think the change is OK? Do you know how to test switching BSP? Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 9:40 AM To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Laszlo, Firstly, SwitchBSP() is one service of MP defined in PI spec. For real case, I think multiple socket system(with different processor stepping) may use this service for purpose. Thanks! Jeff 发件人: Laszlo Ersek<mailto:lersek@redhat.com> 发送时间: 2018年1月4日 20:21 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack allocated >>>>> at PEI phase, if MP was initialized before. Some code added to check this >>>>> situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-05 2:48 ` 答复: " Fan Jeff @ 2018-01-05 2:49 ` Fan Jeff 2018-01-05 2:54 ` Chaganty, Rangasai V 2018-01-05 2:55 ` Wang, Jian J 0 siblings, 2 replies; 19+ messages in thread From: Fan Jeff @ 2018-01-05 2:49 UTC (permalink / raw) To: Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org Cc: Yao, Jiewen, Dong, Eric Sorry, Dump the APICID, not CPUID. Jeff 发件人: Fan Jeff<mailto:vanjeff_919@hotmail.com> 发送时间: 2018年1月5日 10:48 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Laszlo Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to know if the switch is successfully. Jeff From: Wang, Jian J <jian.j.wang@intel.com> Sent: Friday, January 5, 2018 9:57:31 AM To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org Cc: Yao, Jiewen; Dong, Eric Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Hi Jeff, Do you think the change is OK? Do you know how to test switching BSP? Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 9:40 AM To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Laszlo, Firstly, SwitchBSP() is one service of MP defined in PI spec. For real case, I think multiple socket system(with different processor stepping) may use this service for purpose. Thanks! Jeff 发件人: Laszlo Ersek<mailto:lersek@redhat.com> 发送时间: 2018年1月4日 20:21 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack allocated >>>>> at PEI phase, if MP was initialized before. Some code added to check this >>>>> situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-05 2:49 ` Fan Jeff @ 2018-01-05 2:54 ` Chaganty, Rangasai V 2018-01-05 2:56 ` Wang, Jian J 2018-01-05 2:55 ` Wang, Jian J 1 sibling, 1 reply; 19+ messages in thread From: Chaganty, Rangasai V @ 2018-01-05 2:54 UTC (permalink / raw) To: Fan Jeff, Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org Cc: Yao, Jiewen, Dong, Eric APIC_BASE MSR 1BH (BIT8) should tell us if the executing thread is BSP or not. This MSR is defined in SDM. -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fan Jeff Sent: Thursday, January 04, 2018 6:50 PM To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> Subject: [edk2] 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Sorry, Dump the APICID, not CPUID. Jeff 发件人: Fan Jeff<mailto:vanjeff_919@hotmail.com> 发送时间: 2018年1月5日 10:48 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Laszlo Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to know if the switch is successfully. Jeff From: Wang, Jian J <jian.j.wang@intel.com> Sent: Friday, January 5, 2018 9:57:31 AM To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org Cc: Yao, Jiewen; Dong, Eric Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Hi Jeff, Do you think the change is OK? Do you know how to test switching BSP? Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 9:40 AM To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Laszlo, Firstly, SwitchBSP() is one service of MP defined in PI spec. For real case, I think multiple socket system(with different processor stepping) may use this service for purpose. Thanks! Jeff 发件人: Laszlo Ersek<mailto:lersek@redhat.com> 发送时间: 2018年1月4日 20:21 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to > initialize its ApTopOfStack correctly because the MP service supports > BSP/AP exchange. So I think the line 1501 should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + > CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J >> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek >> <lersek@redhat.com<mailto:lersek@redhat.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; >> Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base >> address set as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is >> initialized to the bottom of the stack (line 1501) but AP's >> ApTopOfStack is correctly initialized (line 598). Although my >> calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP >> doesn't need it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>> Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; >>> Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base >>> address set as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that >>> CPU_INFO_IN_HOB.ApTopOfStack was assigned to CpuMpData->Buffer in >>> MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) >>> * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line >>> 1501 is not correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J >>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; >>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>>> Cc: Yao, Jiewen >>>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric >>>> <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base >>>> address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them >>>> to be incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack >>>>> allocated at PEI phase, if MP was initialized before. Some code >>>>> added to check this situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang >>>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the >>>> stack (highest address, and the stack grows down); however the loop >>>> below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) >>>> from the CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob >>>> field points to an *array* of CPU_INFO_IN_HOB structures. >>>> Therefore, for any given processor #N, we should not calculate the >>>> stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, >>>> in commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP >>>> stack addresses", 2016-11-17) -- I've totally forgotten about that >>>> by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. >>>> It should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. >>>> It should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-05 2:54 ` Chaganty, Rangasai V @ 2018-01-05 2:56 ` Wang, Jian J 0 siblings, 0 replies; 19+ messages in thread From: Wang, Jian J @ 2018-01-05 2:56 UTC (permalink / raw) To: Chaganty, Rangasai V, Fan Jeff, Laszlo Ersek, edk2-devel@lists.01.org Cc: Yao, Jiewen, Dong, Eric I see. Thanks a lot! Regards, Jian > -----Original Message----- > From: Chaganty, Rangasai V > Sent: Friday, January 05, 2018 10:55 AM > To: Fan Jeff <vanjeff_919@hotmail.com>; Wang, Jian J > <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2- > devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > Subject: RE: [edk2] 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address > set as Stack Guard > > APIC_BASE MSR 1BH (BIT8) should tell us if the executing thread is BSP or not. > This MSR is defined in SDM. > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fan > Jeff > Sent: Thursday, January 04, 2018 6:50 PM > To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; > edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > Subject: [edk2] 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > Sorry, Dump the APICID, not CPUID. > > Jeff > > 发件人: Fan Jeff<mailto:vanjeff_919@hotmail.com> > 发送时间: 2018年1月5日 10:48 > 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Laszlo > Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2- > devel@lists.01.org> > 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, > Eric<mailto:eric.dong@intel.com> > 主题: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to > know if the switch is successfully. > > Jeff > > > From: Wang, Jian J <jian.j.wang@intel.com> > Sent: Friday, January 5, 2018 9:57:31 AM > To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org > Cc: Yao, Jiewen; Dong, Eric > Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > Hi Jeff, > > Do you think the change is OK? Do you know how to test switching BSP? > > Regards, > Jian > > From: Fan Jeff [mailto:vanjeff_919@hotmail.com] > Sent: Friday, January 05, 2018 9:40 AM > To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; > edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > Laszlo, > > Firstly, SwitchBSP() is one service of MP defined in PI spec. > > For real case, I think multiple socket system(with different processor stepping) > may use this service for purpose. > > Thanks! > Jeff > > 发件人: Laszlo Ersek<mailto:lersek@redhat.com> > 发送时间: 2018年1月4日 20:21 > 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2- > devel@lists.01.org<mailto:edk2-devel@lists.01.org> > 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, > Eric<mailto:eric.dong@intel.com> > 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as > Stack Guard > > On 01/04/18 02:45, Wang, Jian J wrote: > > A correction: although BSP doesn't need it, we still need to > > initialize its ApTopOfStack correctly because the MP service supports > > BSP/AP exchange. So I think the line 1501 should be changed to > > > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + > > CpuMpData->CpuApStackSize); > > > > instead of > > > > InitializeApData (CpuMpData, 0, 0, NULL); > > Hmm... Although I don't immediately see all possible consequences of such a > change, it looks like a correct fix. > > Unfortunately, I don't know of any code that actually exercises the BSP/AP > exchange service. I think Intel must have access to some client code like this, > because I vaguely recall some patches over time that improved BSP/AP > exchange. > > If you modify the InitializeApData() call in question like suggested above, can > you perhaps locate that client code, and test the change with it? > > Thanks! > Laszlo > > > >> -----Original Message----- > >> From: Wang, Jian J > >> Sent: Thursday, January 04, 2018 9:09 AM > >> To: Wang, Jian J > >> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek > >> <lersek@redhat.com<mailto:lersek@redhat.com>>; > >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; > >> Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base > >> address set as Stack Guard > >> > >> Laszlo, > >> > >> More explanations: > >> > >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] > >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is > >> initialized to the bottom of the stack (line 1501) but AP's > >> ApTopOfStack is correctly initialized (line 598). Although my > >> calculation is correct, I think it'd be better to use AP's > >> ApTopOfStack directly. From this perspective, you're right. > >> > >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP > >> doesn't need it anyway. > >> > >> Regards, > >> Jian > >> > >> > >>> -----Original Message----- > >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf > >>> Of > >> Wang, > >>> Jian J > >>> Sent: Thursday, January 04, 2018 8:42 AM > >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; > >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; > >>> Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> > >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base > >>> address set as Stack Guard > >>> > >>> Laszlo, > >>> > >>> I revisited code of MpInitLib. I found that > >>> CPU_INFO_IN_HOB.ApTopOfStack was assigned to CpuMpData->Buffer in > >>> MpInitLibInitialize() > >>> > >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); > >>> > >>> but in > >>> > >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) > >>> * > >>> CpuMpData->CpuApStackSize; > >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, > >>> ApTopOfStack); > >>> > >>> Since InitMpGlobalData() is called just after first situation, my patch is > correct. > >>> > >>> I think the problem here is that ApTopOfStack initialized at line > >>> 1501 is not correct. > >>> > >>> > >>> Regards, > >>> Jian > >>> > >>> > >>>> -----Original Message----- > >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] > >>>> Sent: Thursday, January 04, 2018 1:33 AM > >>>> To: Wang, Jian J > >>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; > >>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > >>>> Cc: Yao, Jiewen > >>>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric > >>>> <eric.dong@intel.com<mailto:eric.dong@intel.com>>; > >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>> > >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base > >>>> address > >> set > >>>> as Stack Guard > >>>> > >>>> (CC Jeff) > >>>> > >>>> Sorry about the delay. > >>>> > >>>> I have some light comments below; I expect at least a few of them > >>>> to be incorrect :) > >>>> > >>>> On 12/29/17 09:36, Jian J Wang wrote: > >>>>> The reason is that DXE part initialization will reuse the stack > >>>>> allocated at PEI phase, if MP was initialized before. Some code > >>>>> added to check this situation and use stack base address saved in HOB > passed from PEI. > >>>>> > >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> > >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> > >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> > >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>>>> Signed-off-by: Jian J Wang > >>>>> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> > >>>>> --- > >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- > >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>>> index 40c1bf407a..05484c9ff3 100644 > >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( > >>>>> UINTN Index; > >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > >>>>> UINTN StackBase; > >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; > >>>>> > >>>>> SaveCpuMpData (CpuMpData); > >>>>> > >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( > >>>>> ASSERT (FALSE); > >>>>> } > >>>>> > >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- > >>> CpuApStackSize; > >>>>> + // > >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. > >>>>> + // Let's check it here. > >>>>> + // > >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- > >>>>> CpuInfoInHob; > >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { > >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; > >>>>> + } else { > >>>>> + StackBase = CpuMpData->Buffer; > >>>>> + } > >>>> > >>>> So, if the HOB is not found, then StackBase is set okay. > >>>> > >>>> However, I'm unsure about the other case. The > >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the > >>>> stack (highest address, and the stack grows down); however the loop > >>>> below > >>>> *increments* StackBase. Given the incrementing nature of the loop, > >>>> shouldn't we first calculate the actual base (= lowest address) > >>>> from the CPU_INFO_IN_HOB.ApTopOfStack field? > >>>> > >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob > >>>> field points to an *array* of CPU_INFO_IN_HOB structures. > >>>> Therefore, for any given processor #N, we should not calculate the > >>>> stack base as > >>>> > >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- > >>>>> CpuApStackSize > >>>> > >>>> instead we should calculate the stack base as something like: > >>>> > >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- > >>> CpuApStackSize > >>>> > >>>> See > >>>> - the InitializeApData() function, > >>>> - and its call site in the ApWakeupFunction() function. > >>>> > >>>> (To my surprise, I personally modified InitializeApData() earlier, > >>>> in commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP > >>>> stack addresses", 2016-11-17) -- I've totally forgotten about that > >>>> by now!) > >>>> > >>>> What do you think? > >>>> > >>>>> > >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); > >>>>> ASSERT_EFI_ERROR (Status); > >>>>> > >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( > >>>>> MemDesc.Attributes | EFI_MEMORY_RP > >>>>> ); > >>>>> ASSERT_EFI_ERROR (Status); > >>>>> + > >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", > >>>> StackBase, Index)); > >>>> > >>>> StackBase has type UINTN, and so it should not be printed with %x. > >>>> It should be cast to (UINT64), and then printed with %Lx. > >>>> > >>>> Similarly, Index has type UINTN. It should not be printed with %d. > >>>> It should be cast to (UINT64) and printed with %Lu. > >>>> > >>>> > >>>>> + StackBase += CpuMpData->CpuApStackSize; > >>>> > >>>> Again, I don't think the simple increment applies when the > >>>> CpuMpData->CpuInfoInHob array exists. > >>>> > >>>>> } > >>>>> } > >>>>> > >>>>> > >>>> > >>>> Thanks, > >>>> Laszlo > >>> _______________________________________________ > >>> edk2-devel mailing list > >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > >>> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > https://lists.01.org/mailman/listinfo/edk2-devel > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-05 2:49 ` Fan Jeff 2018-01-05 2:54 ` Chaganty, Rangasai V @ 2018-01-05 2:55 ` Wang, Jian J 2018-01-05 2:57 ` Yao, Jiewen 1 sibling, 1 reply; 19+ messages in thread From: Wang, Jian J @ 2018-01-05 2:55 UTC (permalink / raw) To: Fan Jeff, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric Got it. Many thanks! Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 10:50 AM To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Sorry, Dump the APICID, not CPUID. Jeff 发件人: Fan Jeff<mailto:vanjeff_919@hotmail.com> 发送时间: 2018年1月5日 10:48 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Laszlo Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to know if the switch is successfully. Jeff From: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> Sent: Friday, January 5, 2018 9:57:31 AM To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen; Dong, Eric Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Hi Jeff, Do you think the change is OK? Do you know how to test switching BSP? Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 9:40 AM To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Laszlo, Firstly, SwitchBSP() is one service of MP defined in PI spec. For real case, I think multiple socket system(with different processor stepping) may use this service for purpose. Thanks! Jeff 发件人: Laszlo Ersek<mailto:lersek@redhat.com> 发送时间: 2018年1月4日 20:21 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack allocated >>>>> at PEI phase, if MP was initialized before. Some code added to check this >>>>> situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-05 2:55 ` Wang, Jian J @ 2018-01-05 2:57 ` Yao, Jiewen 2018-01-05 3:04 ` 答复: " Fan Jeff 0 siblings, 1 reply; 19+ messages in thread From: Yao, Jiewen @ 2018-01-05 2:57 UTC (permalink / raw) To: Wang, Jian J, Fan Jeff, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric Good. I also suggest we run PI-SCT to make sure it is well tested. Thank you Yao Jiewen From: Wang, Jian J Sent: Friday, January 5, 2018 10:56 AM To: Fan Jeff <vanjeff_919@hotmail.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Got it. Many thanks! Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 10:50 AM To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Sorry, Dump the APICID, not CPUID. Jeff 发件人: Fan Jeff<mailto:vanjeff_919@hotmail.com> 发送时间: 2018年1月5日 10:48 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Laszlo Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to know if the switch is successfully. Jeff From: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> Sent: Friday, January 5, 2018 9:57:31 AM To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen; Dong, Eric Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Hi Jeff, Do you think the change is OK? Do you know how to test switching BSP? Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 9:40 AM To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Laszlo, Firstly, SwitchBSP() is one service of MP defined in PI spec. For real case, I think multiple socket system(with different processor stepping) may use this service for purpose. Thanks! Jeff 发件人: Laszlo Ersek<mailto:lersek@redhat.com> 发送时间: 2018年1月4日 20:21 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack allocated >>>>> at PEI phase, if MP was initialized before. Some code added to check this >>>>> situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-05 2:57 ` Yao, Jiewen @ 2018-01-05 3:04 ` Fan Jeff 2018-01-05 3:06 ` Wang, Jian J 0 siblings, 1 reply; 19+ messages in thread From: Fan Jeff @ 2018-01-05 3:04 UTC (permalink / raw) To: Yao, Jiewen, Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org Cc: Dong, Eric Cool. PI SCT is better and easier. Jeff 发件人: Yao, Jiewen<mailto:jiewen.yao@intel.com> 发送时间: 2018年1月5日 10:58 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Fan Jeff<mailto:vanjeff_919@hotmail.com>; Laszlo Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Dong, Eric<mailto:eric.dong@intel.com> 主题: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Good. I also suggest we run PI-SCT to make sure it is well tested. Thank you Yao Jiewen From: Wang, Jian J Sent: Friday, January 5, 2018 10:56 AM To: Fan Jeff <vanjeff_919@hotmail.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Got it. Many thanks! Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 10:50 AM To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Sorry, Dump the APICID, not CPUID. Jeff 发件人: Fan Jeff<mailto:vanjeff_919@hotmail.com> 发送时间: 2018年1月5日 10:48 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Laszlo Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to know if the switch is successfully. Jeff From: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> Sent: Friday, January 5, 2018 9:57:31 AM To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen; Dong, Eric Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Hi Jeff, Do you think the change is OK? Do you know how to test switching BSP? Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 9:40 AM To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Laszlo, Firstly, SwitchBSP() is one service of MP defined in PI spec. For real case, I think multiple socket system(with different processor stepping) may use this service for purpose. Thanks! Jeff 发件人: Laszlo Ersek<mailto:lersek@redhat.com> 发送时间: 2018年1月4日 20:21 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack allocated >>>>> at PEI phase, if MP was initialized before. Some code added to check this >>>>> situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-05 3:04 ` 答复: " Fan Jeff @ 2018-01-05 3:06 ` Wang, Jian J 0 siblings, 0 replies; 19+ messages in thread From: Wang, Jian J @ 2018-01-05 3:06 UTC (permalink / raw) To: Fan Jeff, Yao, Jiewen, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric Glad to know. That saves me time to write my own case:) Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 11:05 AM To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org Cc: Dong, Eric <eric.dong@intel.com> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Cool. PI SCT is better and easier. Jeff 发件人: Yao, Jiewen<mailto:jiewen.yao@intel.com> 发送时间: 2018年1月5日 10:58 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Fan Jeff<mailto:vanjeff_919@hotmail.com>; Laszlo Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Dong, Eric<mailto:eric.dong@intel.com> 主题: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Good. I also suggest we run PI-SCT to make sure it is well tested. Thank you Yao Jiewen From: Wang, Jian J Sent: Friday, January 5, 2018 10:56 AM To: Fan Jeff <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Got it. Many thanks! Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 10:50 AM To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Sorry, Dump the APICID, not CPUID. Jeff 发件人: Fan Jeff<mailto:vanjeff_919@hotmail.com> 发送时间: 2018年1月5日 10:48 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Laszlo Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to know if the switch is successfully. Jeff From: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> Sent: Friday, January 5, 2018 9:57:31 AM To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen; Dong, Eric Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Hi Jeff, Do you think the change is OK? Do you know how to test switching BSP? Regards, Jian From: Fan Jeff [mailto:vanjeff_919@hotmail.com] Sent: Friday, January 05, 2018 9:40 AM To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Laszlo, Firstly, SwitchBSP() is one service of MP defined in PI spec. For real case, I think multiple socket system(with different processor stepping) may use this service for purpose. Thanks! Jeff 发件人: Laszlo Ersek<mailto:lersek@redhat.com> 发送时间: 2018年1月4日 20:21 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong, Eric<mailto:eric.dong@intel.com> 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>> >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> The reason is that DXE part initialization will reuse the stack allocated >>>>> at PEI phase, if MP was initialized before. Some code added to check this >>>>> situation and use stack base address saved in HOB passed from PEI. >>>>> >>>>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>> >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> >>>>> --- >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 100644 >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>>> UINTN Index; >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>>> UINTN StackBase; >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>>> >>>>> SaveCpuMpData (CpuMpData); >>>>> >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard 2018-01-04 1:09 ` Wang, Jian J 2018-01-04 1:45 ` Wang, Jian J @ 2018-01-04 12:18 ` Laszlo Ersek 1 sibling, 0 replies; 19+ messages in thread From: Laszlo Ersek @ 2018-01-04 12:18 UTC (permalink / raw) To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric On 01/04/18 02:09, Wang, Jian J wrote: > Laszlo, > > More explanations: > > [UefiCpuPkg\Library\MpInitLib\MpLib.c] > According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized to > the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized > (line 598). Although my calculation is correct, I think it'd be better to use AP's > ApTopOfStack directly. From this perspective, you're right. Right, after I sent my email, it occurred to me that your calculation could actually match the other calculation that originally populates the CpuInfoInHob[N].ApTopOfStack fields. In other words, the values assigned could be correct. However, I do think / agree that we shouldn't duplicate the calculation, instead we should reuse the pre-computed values. Thanks! Laszlo > Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't need > it anyway. > > Regards, > Jian > > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, >> Jian J >> Sent: Thursday, January 04, 2018 8:42 AM >> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >> >> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >> >> but in >> >> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >> CpuMpData->CpuApStackSize; >> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >> ApTopOfStack); >> >> Since InitMpGlobalData() is called just after first situation, my patch is correct. >> >> I think the problem here is that ApTopOfStack initialized at line 1501 is not >> correct. >> >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>> Sent: Thursday, January 04, 2018 1:33 AM >>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; >>> Jeff Fan <vanjeff_919@hotmail.com> >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> (CC Jeff) >>> >>> Sorry about the delay. >>> >>> I have some light comments below; I expect at least a few of them to be >>> incorrect :) >>> >>> On 12/29/17 09:36, Jian J Wang wrote: >>>> The reason is that DXE part initialization will reuse the stack allocated >>>> at PEI phase, if MP was initialized before. Some code added to check this >>>> situation and use stack base address saved in HOB passed from PEI. >>>> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>>> Cc: Eric Dong <eric.dong@intel.com> >>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com> >>>> --- >>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- >>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> index 40c1bf407a..05484c9ff3 100644 >>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( >>>> UINTN Index; >>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; >>>> UINTN StackBase; >>>> + CPU_INFO_IN_HOB *CpuInfoInHob; >>>> >>>> SaveCpuMpData (CpuMpData); >>>> >>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( >>>> ASSERT (FALSE); >>>> } >>>> >>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize; >>>> + // >>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>> + // Let's check it here. >>>> + // >>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>> CpuInfoInHob; >>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>> + } else { >>>> + StackBase = CpuMpData->Buffer; >>>> + } >>> >>> So, if the HOB is not found, then StackBase is set okay. >>> >>> However, I'm unsure about the other case. The >>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>> (highest address, and the stack grows down); however the loop below >>> *increments* StackBase. Given the incrementing nature of the loop, >>> shouldn't we first calculate the actual base (= lowest address) from the >>> CPU_INFO_IN_HOB.ApTopOfStack field? >>> >>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>> given processor #N, we should not calculate the stack base as >>> >>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>> CpuApStackSize >>> >>> instead we should calculate the stack base as something like: >>> >>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData->CpuApStackSize >>> >>> See >>> - the InitializeApData() function, >>> - and its call site in the ApWakeupFunction() function. >>> >>> (To my surprise, I personally modified InitializeApData() earlier, in >>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>> >>> What do you think? >>> >>>> >>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>> ASSERT_EFI_ERROR (Status); >>>> >>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>> MemDesc.Attributes | EFI_MEMORY_RP >>>> ); >>>> ASSERT_EFI_ERROR (Status); >>>> + >>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>> StackBase, Index)); >>> >>> StackBase has type UINTN, and so it should not be printed with %x. It >>> should be cast to (UINT64), and then printed with %Lx. >>> >>> Similarly, Index has type UINTN. It should not be printed with %d. It >>> should be cast to (UINT64) and printed with %Lu. >>> >>> >>>> + StackBase += CpuMpData->CpuApStackSize; >>> >>> Again, I don't think the simple increment applies when the >>> CpuMpData->CpuInfoInHob array exists. >>> >>>> } >>>> } >>>> >>>> >>> >>> Thanks, >>> Laszlo >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-01-05 3:01 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-29 8:36 [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Jian J Wang 2018-01-03 7:05 ` Wang, Jian J 2018-01-03 17:33 ` Laszlo Ersek 2018-01-04 0:41 ` Wang, Jian J 2018-01-04 1:09 ` Wang, Jian J 2018-01-04 1:45 ` Wang, Jian J 2018-01-04 12:21 ` Laszlo Ersek 2018-01-05 0:52 ` Wang, Jian J 2018-01-05 1:40 ` 答复: " Fan Jeff 2018-01-05 1:57 ` Wang, Jian J 2018-01-05 2:48 ` 答复: " Fan Jeff 2018-01-05 2:49 ` Fan Jeff 2018-01-05 2:54 ` Chaganty, Rangasai V 2018-01-05 2:56 ` Wang, Jian J 2018-01-05 2:55 ` Wang, Jian J 2018-01-05 2:57 ` Yao, Jiewen 2018-01-05 3:04 ` 答复: " Fan Jeff 2018-01-05 3:06 ` Wang, Jian J 2018-01-04 12:18 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox