UefiCpuPkg/MpInitLib: Fix incorrect Guard page setup for APs

[edk2] [PATCH] UefiCpuPkg/MpInitLib: Fix incorrect Guard page setup for APs

Posted by Jian J Wang 7 years ago

AP has its own stack for code execution. If PcdCpuStackGuard is enabled,
the page at the bottom of stack of AP will be disabled (NOT PRESENT) to
monitor the stack overflow issue. This requires PcdCpuApStackSize to be
set with value more than one page of memory.

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/DxeMpInitLib.inf |  1 +
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 34 ++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 805641b516..e7b9eb4462 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -73,4 +73,5 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ## CONSUMES
 
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 479f8189f6..40c1bf407a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -17,6 +17,7 @@
 #include <Library/UefiLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/DebugAgentLib.h>
+#include <Library/DxeServicesTableLib.h>
 
 #include <Protocol/Timer.h>
 
@@ -288,9 +289,12 @@ InitMpGlobalData (
   IN CPU_MP_DATA               *CpuMpData
   )
 {
-  EFI_STATUS                 Status;
-  EFI_PHYSICAL_ADDRESS       Address;
-  UINTN                      ApSafeBufferSize;
+  EFI_STATUS                          Status;
+  EFI_PHYSICAL_ADDRESS                Address;
+  UINTN                               ApSafeBufferSize;
+  UINTN                               Index;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR     MemDesc;
+  UINTN                               StackBase;
 
   SaveCpuMpData (CpuMpData);
 
@@ -301,6 +305,30 @@ InitMpGlobalData (
     return;
   }
 
+  if (PcdGetBool (PcdCpuStackGuard)) {
+    //
+    // One extra page at the bottom of the stack is needed for Guard page.
+    //
+    if (CpuMpData->CpuApStackSize <= EFI_PAGE_SIZE) {
+      DEBUG ((DEBUG_ERROR, "PcdCpuApStackSize is not big enough for Stack Guard!\n"));
+      ASSERT (FALSE);
+    }
+
+    for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
+      StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
+
+      Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc);
+      ASSERT_EFI_ERROR (Status);
+
+      Status = gDS->SetMemorySpaceAttributes (
+                      StackBase,
+                      EFI_PAGES_TO_SIZE (1),
+                      MemDesc.Attributes | EFI_MEMORY_RP
+                      );
+      ASSERT_EFI_ERROR (Status);
+    }
+  }
+
   //
   // Avoid APs access invalid buffer data which allocated by BootServices,
   // so we will allocate reserved data for AP loop code. We also need to
-- 
2.15.1.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: Fix incorrect Guard page setup for APs

Posted by Wang, Jian J 7 years ago

Hi,

Anyone has any comments on this?

Regards,
Jian


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Thursday, December 21, 2017 9:27 AM
> 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 incorrect Guard page setup
> for APs
> 
> AP has its own stack for code execution. If PcdCpuStackGuard is enabled,
> the page at the bottom of stack of AP will be disabled (NOT PRESENT) to
> monitor the stack overflow issue. This requires PcdCpuApStackSize to be
> set with value more than one page of memory.
> 
> 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/DxeMpInitLib.inf |  1 +
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 34
> ++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 805641b516..e7b9eb4462 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -73,4 +73,5 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> CONSUMES
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 479f8189f6..40c1bf407a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -17,6 +17,7 @@
>  #include <Library/UefiLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/DebugAgentLib.h>
> +#include <Library/DxeServicesTableLib.h>
> 
>  #include <Protocol/Timer.h>
> 
> @@ -288,9 +289,12 @@ InitMpGlobalData (
>    IN CPU_MP_DATA               *CpuMpData
>    )
>  {
> -  EFI_STATUS                 Status;
> -  EFI_PHYSICAL_ADDRESS       Address;
> -  UINTN                      ApSafeBufferSize;
> +  EFI_STATUS                          Status;
> +  EFI_PHYSICAL_ADDRESS                Address;
> +  UINTN                               ApSafeBufferSize;
> +  UINTN                               Index;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR     MemDesc;
> +  UINTN                               StackBase;
> 
>    SaveCpuMpData (CpuMpData);
> 
> @@ -301,6 +305,30 @@ InitMpGlobalData (
>      return;
>    }
> 
> +  if (PcdGetBool (PcdCpuStackGuard)) {
> +    //
> +    // One extra page at the bottom of the stack is needed for Guard page.
> +    //
> +    if (CpuMpData->CpuApStackSize <= EFI_PAGE_SIZE) {
> +      DEBUG ((DEBUG_ERROR, "PcdCpuApStackSize is not big enough for Stack
> Guard!\n"));
> +      ASSERT (FALSE);
> +    }
> +
> +    for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
> +      StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
> +
> +      Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc);
> +      ASSERT_EFI_ERROR (Status);
> +
> +      Status = gDS->SetMemorySpaceAttributes (
> +                      StackBase,
> +                      EFI_PAGES_TO_SIZE (1),
> +                      MemDesc.Attributes | EFI_MEMORY_RP
> +                      );
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +  }
> +
>    //
>    // Avoid APs access invalid buffer data which allocated by BootServices,
>    // so we will allocate reserved data for AP loop code. We also need to
> --
> 2.15.1.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: Fix incorrect Guard page setup for APs

Posted by Yao, Jiewen 7 years ago

I do not think we have AP guard page before, right?
So I suggest we change commit message to be: Add missing Guard page setup for APs.

With commit message update, reviewed-by: Jiewen.yao@intel.com


> -----Original Message-----
> From: Wang, Jian J
> Sent: Friday, December 22, 2017 12:26 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: Fix incorrect Guard page
> setup for APs
> 
> Hi,
> 
> Anyone has any comments on this?
> 
> Regards,
> Jian
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian
> J
> > Wang
> > Sent: Thursday, December 21, 2017 9:27 AM
> > 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 incorrect Guard page setup
> > for APs
> >
> > AP has its own stack for code execution. If PcdCpuStackGuard is enabled,
> > the page at the bottom of stack of AP will be disabled (NOT PRESENT) to
> > monitor the stack overflow issue. This requires PcdCpuApStackSize to be
> > set with value more than one page of memory.
> >
> > 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/DxeMpInitLib.inf |  1 +
> >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 34
> > ++++++++++++++++++++++++---
> >  2 files changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > index 805641b516..e7b9eb4462 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > @@ -73,4 +73,5 @@
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize
> ##
> > CONSUMES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode
> ## CONSUMES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate
> ##
> > SOMETIMES_CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> ##
> > CONSUMES
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > index 479f8189f6..40c1bf407a 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > @@ -17,6 +17,7 @@
> >  #include <Library/UefiLib.h>
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/DebugAgentLib.h>
> > +#include <Library/DxeServicesTableLib.h>
> >
> >  #include <Protocol/Timer.h>
> >
> > @@ -288,9 +289,12 @@ InitMpGlobalData (
> >    IN CPU_MP_DATA               *CpuMpData
> >    )
> >  {
> > -  EFI_STATUS                 Status;
> > -  EFI_PHYSICAL_ADDRESS       Address;
> > -  UINTN                      ApSafeBufferSize;
> > +  EFI_STATUS                          Status;
> > +  EFI_PHYSICAL_ADDRESS                Address;
> > +  UINTN                               ApSafeBufferSize;
> > +  UINTN                               Index;
> > +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR     MemDesc;
> > +  UINTN                               StackBase;
> >
> >    SaveCpuMpData (CpuMpData);
> >
> > @@ -301,6 +305,30 @@ InitMpGlobalData (
> >      return;
> >    }
> >
> > +  if (PcdGetBool (PcdCpuStackGuard)) {
> > +    //
> > +    // One extra page at the bottom of the stack is needed for Guard page.
> > +    //
> > +    if (CpuMpData->CpuApStackSize <= EFI_PAGE_SIZE) {
> > +      DEBUG ((DEBUG_ERROR, "PcdCpuApStackSize is not big enough for
> Stack
> > Guard!\n"));
> > +      ASSERT (FALSE);
> > +    }
> > +
> > +    for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
> > +      StackBase = CpuMpData->Buffer + Index *
> CpuMpData->CpuApStackSize;
> > +
> > +      Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc);
> > +      ASSERT_EFI_ERROR (Status);
> > +
> > +      Status = gDS->SetMemorySpaceAttributes (
> > +                      StackBase,
> > +                      EFI_PAGES_TO_SIZE (1),
> > +                      MemDesc.Attributes | EFI_MEMORY_RP
> > +                      );
> > +      ASSERT_EFI_ERROR (Status);
> > +    }
> > +  }
> > +
> >    //
> >    // Avoid APs access invalid buffer data which allocated by BootServices,
> >    // so we will allocate reserved data for AP loop code. We also need to
> > --
> > 2.15.1.windows.2
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: Fix incorrect Guard page setup for APs

Posted by Wang, Jian J 7 years ago

You're right. It will be updated to use more accurate wording. Thanks for catching it.

Regards,
Jian


> -----Original Message-----
> From: Yao, Jiewen
> Sent: Friday, December 22, 2017 1:22 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: Fix incorrect Guard page
> setup for APs
> 
> I do not think we have AP guard page before, right?
> So I suggest we change commit message to be: Add missing Guard page setup
> for APs.
> 
> With commit message update, reviewed-by: Jiewen.yao@intel.com
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Friday, December 22, 2017 12:26 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> > Dong, Eric <eric.dong@intel.com>
> > Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: Fix incorrect Guard page
> > setup for APs
> >
> > Hi,
> >
> > Anyone has any comments on this?
> >
> > Regards,
> > Jian
> >
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jian
> > J
> > > Wang
> > > Sent: Thursday, December 21, 2017 9:27 AM
> > > 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 incorrect Guard page
> setup
> > > for APs
> > >
> > > AP has its own stack for code execution. If PcdCpuStackGuard is enabled,
> > > the page at the bottom of stack of AP will be disabled (NOT PRESENT) to
> > > monitor the stack overflow issue. This requires PcdCpuApStackSize to be
> > > set with value more than one page of memory.
> > >
> > > 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/DxeMpInitLib.inf |  1 +
> > >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 34
> > > ++++++++++++++++++++++++---
> > >  2 files changed, 32 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > index 805641b516..e7b9eb4462 100644
> > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > @@ -73,4 +73,5 @@
> > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize
> > ##
> > > CONSUMES
> > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode
> > ## CONSUMES
> > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate
> > ##
> > > SOMETIMES_CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> > ##
> > > CONSUMES
> > >
> > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > index 479f8189f6..40c1bf407a 100644
> > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > @@ -17,6 +17,7 @@
> > >  #include <Library/UefiLib.h>
> > >  #include <Library/UefiBootServicesTableLib.h>
> > >  #include <Library/DebugAgentLib.h>
> > > +#include <Library/DxeServicesTableLib.h>
> > >
> > >  #include <Protocol/Timer.h>
> > >
> > > @@ -288,9 +289,12 @@ InitMpGlobalData (
> > >    IN CPU_MP_DATA               *CpuMpData
> > >    )
> > >  {
> > > -  EFI_STATUS                 Status;
> > > -  EFI_PHYSICAL_ADDRESS       Address;
> > > -  UINTN                      ApSafeBufferSize;
> > > +  EFI_STATUS                          Status;
> > > +  EFI_PHYSICAL_ADDRESS                Address;
> > > +  UINTN                               ApSafeBufferSize;
> > > +  UINTN                               Index;
> > > +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR     MemDesc;
> > > +  UINTN                               StackBase;
> > >
> > >    SaveCpuMpData (CpuMpData);
> > >
> > > @@ -301,6 +305,30 @@ InitMpGlobalData (
> > >      return;
> > >    }
> > >
> > > +  if (PcdGetBool (PcdCpuStackGuard)) {
> > > +    //
> > > +    // One extra page at the bottom of the stack is needed for Guard page.
> > > +    //
> > > +    if (CpuMpData->CpuApStackSize <= EFI_PAGE_SIZE) {
> > > +      DEBUG ((DEBUG_ERROR, "PcdCpuApStackSize is not big enough for
> > Stack
> > > Guard!\n"));
> > > +      ASSERT (FALSE);
> > > +    }
> > > +
> > > +    for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
> > > +      StackBase = CpuMpData->Buffer + Index *
> > CpuMpData->CpuApStackSize;
> > > +
> > > +      Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc);
> > > +      ASSERT_EFI_ERROR (Status);
> > > +
> > > +      Status = gDS->SetMemorySpaceAttributes (
> > > +                      StackBase,
> > > +                      EFI_PAGES_TO_SIZE (1),
> > > +                      MemDesc.Attributes | EFI_MEMORY_RP
> > > +                      );
> > > +      ASSERT_EFI_ERROR (Status);
> > > +    }
> > > +  }
> > > +
> > >    //
> > >    // Avoid APs access invalid buffer data which allocated by BootServices,
> > >    // so we will allocate reserved data for AP loop code. We also need to
> > > --
> > > 2.15.1.windows.2
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel