UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
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
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
(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
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
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
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
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
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
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.