[PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Jian J Wang]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Laszlo Ersek]

(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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Laszlo Ersek]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Laszlo Ersek]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

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

答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Fan Jeff]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

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

答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Fan Jeff]

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

答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Fan Jeff]

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

Re: 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Chaganty, Rangasai V]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

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

Re: 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Yao, Jiewen]

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

答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Fan Jeff]

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

Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

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