[Patch] MdeModulePkg/PiSmmCore: Control S3 related functionality with flag.

[Patch] MdeModulePkg/PiSmmCore: Control S3 related functionality with flag.

[Eric Dong]

https://bugzilla.tianocore.org/show_bug.cgi?id=1590

Use PcdAcpiS3Enable to control whether need to enable S3 related functionality
in Pi SMM Core.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c   | 70 ++++++++++++++++++++++---------
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf |  1 +
 2 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index d0bc65b564..bd19803c37 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -77,6 +77,12 @@ BOOLEAN  mInLegacyBoot = FALSE;
 //
 BOOLEAN  mDuringS3Resume = FALSE;
 
+//
+// Flag to determine if platform enabled S3.
+// Get the value from PcdAcpiS3Enable.
+//
+BOOLEAN  mAcpiS3Enable = FALSE;
+
 //
 // Table of SMI Handlers that are registered by the SMM Core when it is initialized
 //
@@ -87,6 +93,13 @@ SMM_CORE_SMI_HANDLERS  mSmmCoreSmiHandlers[] = {
   { SmmExitBootServicesHandler, &gEfiEventExitBootServicesGuid,      NULL, FALSE },
   { SmmReadyToBootHandler,      &gEfiEventReadyToBootGuid,           NULL, FALSE },
   { SmmEndOfDxeHandler,         &gEfiEndOfDxeEventGroupGuid,         NULL, TRUE },
+  { NULL,                       NULL,                                NULL, FALSE }
+};
+
+//
+// Table of SMI Handlers that are registered by the SMM Core when it is initialized
+//
+SMM_CORE_SMI_HANDLERS  mSmmCoreS3SmiHandlers[] = {
   { SmmS3SmmInitDoneHandler,    &gEdkiiS3SmmInitDoneGuid,            NULL, FALSE },
   { SmmEndOfS3ResumeHandler,    &gEdkiiEndOfS3ResumeGuid,            NULL, FALSE },
   { NULL,                       NULL,                                NULL, FALSE }
@@ -445,28 +458,30 @@ SmmEndOfDxeHandler (
              NULL
              );
 
-  //
-  // Locate SmmSxDispatch2 protocol.
-  //
-  Status = SmmLocateProtocol (
-             &gEfiSmmSxDispatch2ProtocolGuid,
-             NULL,
-             (VOID **)&SxDispatch
-             );
-  if (!EFI_ERROR (Status) && (SxDispatch != NULL)) {
+  if (mAcpiS3Enable) {
     //
-    // Register a S3 entry callback function to
-    // determine if it will be during S3 resume.
+    // Locate SmmSxDispatch2 protocol.
     //
-    EntryRegisterContext.Type  = SxS3;
-    EntryRegisterContext.Phase = SxEntry;
-    Status = SxDispatch->Register (
-                           SxDispatch,
-                           SmmS3EntryCallBack,
-                           &EntryRegisterContext,
-                           &S3EntryHandle
-                           );
-    ASSERT_EFI_ERROR (Status);
+    Status = SmmLocateProtocol (
+               &gEfiSmmSxDispatch2ProtocolGuid,
+               NULL,
+               (VOID **)&SxDispatch
+               );
+    if (!EFI_ERROR (Status) && (SxDispatch != NULL)) {
+      //
+      // Register a S3 entry callback function to
+      // determine if it will be during S3 resume.
+      //
+      EntryRegisterContext.Type  = SxS3;
+      EntryRegisterContext.Phase = SxEntry;
+      Status = SxDispatch->Register (
+                             SxDispatch,
+                             SmmS3EntryCallBack,
+                             &EntryRegisterContext,
+                             &S3EntryHandle
+                             );
+      ASSERT_EFI_ERROR (Status);
+    }
   }
 
   return EFI_SUCCESS;
@@ -883,6 +898,21 @@ SmmMain (
     ASSERT_EFI_ERROR (Status);
   }
 
+  mAcpiS3Enable = PcdGetBool (PcdAcpiS3Enable);
+  if (mAcpiS3Enable) {
+    //
+    // Register all S3 related SMI Handlers required by the SMM Core
+    //
+    for (Index = 0; mSmmCoreS3SmiHandlers[Index].HandlerType != NULL; Index++) {
+      Status = SmiHandlerRegister (
+                 mSmmCoreS3SmiHandlers[Index].Handler,
+                 mSmmCoreS3SmiHandlers[Index].HandlerType,
+                 &mSmmCoreS3SmiHandlers[Index].DispatchHandle
+                 );
+      ASSERT_EFI_ERROR (Status);
+    }
+  }
+
   RegisterSmramProfileHandler ();
   SmramProfileInstallProtocol ();
 
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
index f3ece22121..9a31cb79d6 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
@@ -101,6 +101,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask               ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                        ## CONSUMES
 
 [Guids]
   gAprioriGuid                                  ## SOMETIMES_CONSUMES   ## File
-- 
2.15.0.windows.1

[Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up Buffer.

[Eric Dong]

https://bugzilla.tianocore.org/show_bug.cgi?id=1538

Current CpuDxe driver "borrows" Wakeup Buffer (through Allocate & free
to get the buffer pointer, backup the buffer data before using it and
restore it after using).  Now this logic met a problem described in
the above BZ because the test tool and the CpuDxe both use the same
memory at the same time.

In order to fix the above issue, CpuDxe changed to allocate the buffer
below 1M instead of borrow it. After investigation, we found below
0x88000 is the possible space which can be used. For now, range
0x60000 ~ 0x88000 used for Legacy OPROMs by LegacyBios driver. And it
tries to allocate these range page(4K size) by page. It just reports
warning message if specific page been used by others already.

Also CpuDxe driver will produce CPU arch protocol and LegacyBios driver
has dependency for this protocol. So CpuDxe driver will start before
LegacyBios driver and CpuDxe driver can allocate that space successful.

With this change, CpuDxe driver will use 0x87000 ~ 0x88000 as wakeup
buffer.

Cc: Ray Ni <ray.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index b2307cbb61..5bc9a47efb 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -76,7 +76,7 @@ SaveCpuMpData (
 }
 
 /**
-  Get available system memory below 1MB by specified size.
+  Get available system memory below 0x88000 by specified size.
 
   @param[in] WakeupBufferSize   Wakeup buffer size required
 
@@ -91,7 +91,19 @@ GetWakeupBuffer (
   EFI_STATUS              Status;
   EFI_PHYSICAL_ADDRESS    StartAddress;
 
-  StartAddress = BASE_1MB;
+  //
+  // Current "Borrow" space mechanism caused potential race condition if both
+  // AP and the original owner use the share space.
+  //
+  // LegacyBios driver tries to allocate 4K pages between 0x60000 ~ 0x88000
+  // space. It will just report an waring message if the page has been allocate
+  // by other drivers.
+  // LagacyBios driver depends on CPU Arch protocol, so it will start after
+  // CpuDxe driver which produce Cpu Arch Protocol and use this library.
+  // So below allocate logic will be trigged before LegacyBios driver and it
+  // will always return success.
+  //
+  StartAddress = BASE_512KB + BASE_32KB;
   Status = gBS->AllocatePages (
                   AllocateMaxAddress,
                   EfiBootServicesData,
@@ -99,17 +111,13 @@ GetWakeupBuffer (
                   &StartAddress
                   );
   ASSERT_EFI_ERROR (Status);
-  if (!EFI_ERROR (Status)) {
-    Status = gBS->FreePages(
-               StartAddress,
-               EFI_SIZE_TO_PAGES (WakeupBufferSize)
-               );
-    ASSERT_EFI_ERROR (Status);
-    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
-                        (UINTN) StartAddress, WakeupBufferSize));
-  } else {
+  if (EFI_ERROR (Status)) {
     StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
   }
+
+  DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
+                      (UINTN) StartAddress, WakeupBufferSize));
+
   return (UINTN) StartAddress;
 }
 
-- 
2.15.0.windows.1

Re: [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up Buffer.

[Zeng, Star]

Just an idea to avoid hard code value 0x88000.

Before EndOfDxe: Allocate buffer in AllocateResetVector(), and free buffer in FreeResetVector().
At EndOfDxe (it is after LegacyBiosDxe driver entry point) callback: Allocate buffer and record it to CpuMpData->WakeupBuffer, and always occupy the buffer, that means no free.
After EndOfDxe: Use CpuMpData->WakeupBuffer.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Eric Dong
Sent: Tuesday, March 5, 2019 10:07 AM
To: edk2-devel@lists.01.org
Subject: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up Buffer.

https://bugzilla.tianocore.org/show_bug.cgi?id=1538

Current CpuDxe driver "borrows" Wakeup Buffer (through Allocate & free to get the buffer pointer, backup the buffer data before using it and restore it after using).  Now this logic met a problem described in the above BZ because the test tool and the CpuDxe both use the same memory at the same time.

In order to fix the above issue, CpuDxe changed to allocate the buffer below 1M instead of borrow it. After investigation, we found below
0x88000 is the possible space which can be used. For now, range
0x60000 ~ 0x88000 used for Legacy OPROMs by LegacyBios driver. And it tries to allocate these range page(4K size) by page. It just reports warning message if specific page been used by others already.

Also CpuDxe driver will produce CPU arch protocol and LegacyBios driver has dependency for this protocol. So CpuDxe driver will start before LegacyBios driver and CpuDxe driver can allocate that space successful.

With this change, CpuDxe driver will use 0x87000 ~ 0x88000 as wakeup buffer.

Cc: Ray Ni <ray.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index b2307cbb61..5bc9a47efb 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -76,7 +76,7 @@ SaveCpuMpData (
 }
 
 /**
-  Get available system memory below 1MB by specified size.
+  Get available system memory below 0x88000 by specified size.
 
   @param[in] WakeupBufferSize   Wakeup buffer size required
 
@@ -91,7 +91,19 @@ GetWakeupBuffer (
   EFI_STATUS              Status;
   EFI_PHYSICAL_ADDRESS    StartAddress;
 
-  StartAddress = BASE_1MB;
+  //
+  // Current "Borrow" space mechanism caused potential race condition 
+ if both  // AP and the original owner use the share space.
+  //
+  // LegacyBios driver tries to allocate 4K pages between 0x60000 ~ 
+ 0x88000  // space. It will just report an waring message if the page 
+ has been allocate  // by other drivers.
+  // LagacyBios driver depends on CPU Arch protocol, so it will start 
+ after  // CpuDxe driver which produce Cpu Arch Protocol and use this library.
+  // So below allocate logic will be trigged before LegacyBios driver 
+ and it  // will always return success.
+  //
+  StartAddress = BASE_512KB + BASE_32KB;
   Status = gBS->AllocatePages (
                   AllocateMaxAddress,
                   EfiBootServicesData,
@@ -99,17 +111,13 @@ GetWakeupBuffer (
                   &StartAddress
                   );
   ASSERT_EFI_ERROR (Status);
-  if (!EFI_ERROR (Status)) {
-    Status = gBS->FreePages(
-               StartAddress,
-               EFI_SIZE_TO_PAGES (WakeupBufferSize)
-               );
-    ASSERT_EFI_ERROR (Status);
-    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
-                        (UINTN) StartAddress, WakeupBufferSize));
-  } else {
+  if (EFI_ERROR (Status)) {
     StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
   }
+
+  DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
+                      (UINTN) StartAddress, WakeupBufferSize));
+
   return (UINTN) StartAddress;
 }
 
--
2.15.0.windows.1

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

Re: [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up Buffer.

[Dong, Eric]

Hi Star,

This logic seems much complicated than mine. Also after CSM retired from EDKII, we will change this code back to only require allocate buffer below 1M. I will add such notes in the code comments.  So I prefer to use my change.

Thanks,
Eric

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, March 5, 2019 3:33 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek (lersek@redhat.com)
> <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for
> Wake up Buffer.
> 
> Just an idea to avoid hard code value 0x88000.
> 
> Before EndOfDxe: Allocate buffer in AllocateResetVector(), and free buffer
> in FreeResetVector().
> At EndOfDxe (it is after LegacyBiosDxe driver entry point) callback: Allocate
> buffer and record it to CpuMpData->WakeupBuffer, and always occupy the
> buffer, that means no free.
> After EndOfDxe: Use CpuMpData->WakeupBuffer.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Eric Dong
> Sent: Tuesday, March 5, 2019 10:07 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for
> Wake up Buffer.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1538
> 
> Current CpuDxe driver "borrows" Wakeup Buffer (through Allocate & free to
> get the buffer pointer, backup the buffer data before using it and restore it
> after using).  Now this logic met a problem described in the above BZ
> because the test tool and the CpuDxe both use the same memory at the
> same time.
> 
> In order to fix the above issue, CpuDxe changed to allocate the buffer below
> 1M instead of borrow it. After investigation, we found below
> 0x88000 is the possible space which can be used. For now, range
> 0x60000 ~ 0x88000 used for Legacy OPROMs by LegacyBios driver. And it tries
> to allocate these range page(4K size) by page. It just reports warning
> message if specific page been used by others already.
> 
> Also CpuDxe driver will produce CPU arch protocol and LegacyBios driver has
> dependency for this protocol. So CpuDxe driver will start before LegacyBios
> driver and CpuDxe driver can allocate that space successful.
> 
> With this change, CpuDxe driver will use 0x87000 ~ 0x88000 as wakeup buffer.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 30 +++++++++++++++++++----
> -------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index b2307cbb61..5bc9a47efb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -76,7 +76,7 @@ SaveCpuMpData (
>  }
> 
>  /**
> -  Get available system memory below 1MB by specified size.
> +  Get available system memory below 0x88000 by specified size.
> 
>    @param[in] WakeupBufferSize   Wakeup buffer size required
> 
> @@ -91,7 +91,19 @@ GetWakeupBuffer (
>    EFI_STATUS              Status;
>    EFI_PHYSICAL_ADDRESS    StartAddress;
> 
> -  StartAddress = BASE_1MB;
> +  //
> +  // Current "Borrow" space mechanism caused potential race condition
> + if both  // AP and the original owner use the share space.
> +  //
> +  // LegacyBios driver tries to allocate 4K pages between 0x60000 ~
> + 0x88000  // space. It will just report an waring message if the page
> + has been allocate  // by other drivers.
> +  // LagacyBios driver depends on CPU Arch protocol, so it will start
> + after  // CpuDxe driver which produce Cpu Arch Protocol and use this
> library.
> +  // So below allocate logic will be trigged before LegacyBios driver
> + and it  // will always return success.
> +  //
> +  StartAddress = BASE_512KB + BASE_32KB;
>    Status = gBS->AllocatePages (
>                    AllocateMaxAddress,
>                    EfiBootServicesData,
> @@ -99,17 +111,13 @@ GetWakeupBuffer (
>                    &StartAddress
>                    );
>    ASSERT_EFI_ERROR (Status);
> -  if (!EFI_ERROR (Status)) {
> -    Status = gBS->FreePages(
> -               StartAddress,
> -               EFI_SIZE_TO_PAGES (WakeupBufferSize)
> -               );
> -    ASSERT_EFI_ERROR (Status);
> -    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
> = %x\n",
> -                        (UINTN) StartAddress, WakeupBufferSize));
> -  } else {
> +  if (EFI_ERROR (Status)) {
>      StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>    }
> +
> +  DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
> = %x\n",
> +                      (UINTN) StartAddress, WakeupBufferSize));
> +
>    return (UINTN) StartAddress;
>  }
> 
> --
> 2.15.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up Buffer.

[Laszlo Ersek]

On 03/07/19 03:53, Dong, Eric wrote:
> Hi Star,
> 
> This logic seems much complicated than mine. Also after CSM retired from EDKII, we will change this code back to only require allocate buffer below 1M. I will add such notes in the code comments.  So I prefer to use my change.

I apologize for my inability to follow the recent developments in this
thread. However, what I recall is that we may not retire the CSM from
edk2 entirely. Instead, if someone volunteers to maintain the CSM under
OvmfPkg for example, then we'll keep it there.

Personally I'm not interested in the CSM, but it would be nice if we
didn't implement logic in UefiCpuPkg that would be, by design,
incompatible with an optional feature that we might carry forward in
OvmfPkg.

Thanks
Laszlo

> Thanks,
> Eric
> 
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Tuesday, March 5, 2019 3:33 PM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek (lersek@redhat.com)
>> <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: RE: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for
>> Wake up Buffer.
>>
>> Just an idea to avoid hard code value 0x88000.
>>
>> Before EndOfDxe: Allocate buffer in AllocateResetVector(), and free buffer
>> in FreeResetVector().
>> At EndOfDxe (it is after LegacyBiosDxe driver entry point) callback: Allocate
>> buffer and record it to CpuMpData->WakeupBuffer, and always occupy the
>> buffer, that means no free.
>> After EndOfDxe: Use CpuMpData->WakeupBuffer.
>>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Eric Dong
>> Sent: Tuesday, March 5, 2019 10:07 AM
>> To: edk2-devel@lists.01.org
>> Subject: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for
>> Wake up Buffer.
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=1538
>>
>> Current CpuDxe driver "borrows" Wakeup Buffer (through Allocate & free to
>> get the buffer pointer, backup the buffer data before using it and restore it
>> after using).  Now this logic met a problem described in the above BZ
>> because the test tool and the CpuDxe both use the same memory at the
>> same time.
>>
>> In order to fix the above issue, CpuDxe changed to allocate the buffer below
>> 1M instead of borrow it. After investigation, we found below
>> 0x88000 is the possible space which can be used. For now, range
>> 0x60000 ~ 0x88000 used for Legacy OPROMs by LegacyBios driver. And it tries
>> to allocate these range page(4K size) by page. It just reports warning
>> message if specific page been used by others already.
>>
>> Also CpuDxe driver will produce CPU arch protocol and LegacyBios driver has
>> dependency for this protocol. So CpuDxe driver will start before LegacyBios
>> driver and CpuDxe driver can allocate that space successful.
>>
>> With this change, CpuDxe driver will use 0x87000 ~ 0x88000 as wakeup buffer.
>>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>> ---
>>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 30 +++++++++++++++++++----
>> -------
>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> index b2307cbb61..5bc9a47efb 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> @@ -76,7 +76,7 @@ SaveCpuMpData (
>>  }
>>
>>  /**
>> -  Get available system memory below 1MB by specified size.
>> +  Get available system memory below 0x88000 by specified size.
>>
>>    @param[in] WakeupBufferSize   Wakeup buffer size required
>>
>> @@ -91,7 +91,19 @@ GetWakeupBuffer (
>>    EFI_STATUS              Status;
>>    EFI_PHYSICAL_ADDRESS    StartAddress;
>>
>> -  StartAddress = BASE_1MB;
>> +  //
>> +  // Current "Borrow" space mechanism caused potential race condition
>> + if both  // AP and the original owner use the share space.
>> +  //
>> +  // LegacyBios driver tries to allocate 4K pages between 0x60000 ~
>> + 0x88000  // space. It will just report an waring message if the page
>> + has been allocate  // by other drivers.
>> +  // LagacyBios driver depends on CPU Arch protocol, so it will start
>> + after  // CpuDxe driver which produce Cpu Arch Protocol and use this
>> library.
>> +  // So below allocate logic will be trigged before LegacyBios driver
>> + and it  // will always return success.
>> +  //
>> +  StartAddress = BASE_512KB + BASE_32KB;
>>    Status = gBS->AllocatePages (
>>                    AllocateMaxAddress,
>>                    EfiBootServicesData,
>> @@ -99,17 +111,13 @@ GetWakeupBuffer (
>>                    &StartAddress
>>                    );
>>    ASSERT_EFI_ERROR (Status);
>> -  if (!EFI_ERROR (Status)) {
>> -    Status = gBS->FreePages(
>> -               StartAddress,
>> -               EFI_SIZE_TO_PAGES (WakeupBufferSize)
>> -               );
>> -    ASSERT_EFI_ERROR (Status);
>> -    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
>> = %x\n",
>> -                        (UINTN) StartAddress, WakeupBufferSize));
>> -  } else {
>> +  if (EFI_ERROR (Status)) {
>>      StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>>    }
>> +
>> +  DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
>> = %x\n",
>> +                      (UINTN) StartAddress, WakeupBufferSize));
>> +
>>    return (UINTN) StartAddress;
>>  }
>>
>> --
>> 2.15.0.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up Buffer.

[Dong, Eric]

Hi Laszlo,

This change is not incompatible with CSM feature. It just make the max address not directly than without CSM solution. 
We just add comments to claim solution can be enhanced  when CSM is retired.

Thanks,
Eric

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, March 8, 2019 1:40 AM
> To: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>;
> edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>; David Woodhouse <dwmw2@infradead.org>
> Subject: Re: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for
> Wake up Buffer.
> 
> On 03/07/19 03:53, Dong, Eric wrote:
> > Hi Star,
> >
> > This logic seems much complicated than mine. Also after CSM retired from
> EDKII, we will change this code back to only require allocate buffer below 1M.
> I will add such notes in the code comments.  So I prefer to use my change.
> 
> I apologize for my inability to follow the recent developments in this thread.
> However, what I recall is that we may not retire the CSM from
> edk2 entirely. Instead, if someone volunteers to maintain the CSM under
> OvmfPkg for example, then we'll keep it there.
> 
> Personally I'm not interested in the CSM, but it would be nice if we didn't
> implement logic in UefiCpuPkg that would be, by design, incompatible with
> an optional feature that we might carry forward in OvmfPkg.
> 
> Thanks
> Laszlo
> 
> > Thanks,
> > Eric
> >
> >> -----Original Message-----
> >> From: Zeng, Star
> >> Sent: Tuesday, March 5, 2019 3:33 PM
> >> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> >> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek (lersek@redhat.com)
> >> <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> >> Subject: RE: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate
> >> buffer for Wake up Buffer.
> >>
> >> Just an idea to avoid hard code value 0x88000.
> >>
> >> Before EndOfDxe: Allocate buffer in AllocateResetVector(), and free
> >> buffer in FreeResetVector().
> >> At EndOfDxe (it is after LegacyBiosDxe driver entry point) callback:
> >> Allocate buffer and record it to CpuMpData->WakeupBuffer, and always
> >> occupy the buffer, that means no free.
> >> After EndOfDxe: Use CpuMpData->WakeupBuffer.
> >>
> >>
> >> Thanks,
> >> Star
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >> Of Eric Dong
> >> Sent: Tuesday, March 5, 2019 10:07 AM
> >> To: edk2-devel@lists.01.org
> >> Subject: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer
> >> for Wake up Buffer.
> >>
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=1538
> >>
> >> Current CpuDxe driver "borrows" Wakeup Buffer (through Allocate &
> >> free to get the buffer pointer, backup the buffer data before using
> >> it and restore it after using).  Now this logic met a problem
> >> described in the above BZ because the test tool and the CpuDxe both
> >> use the same memory at the same time.
> >>
> >> In order to fix the above issue, CpuDxe changed to allocate the
> >> buffer below 1M instead of borrow it. After investigation, we found
> >> below
> >> 0x88000 is the possible space which can be used. For now, range
> >> 0x60000 ~ 0x88000 used for Legacy OPROMs by LegacyBios driver. And it
> >> tries to allocate these range page(4K size) by page. It just reports
> >> warning message if specific page been used by others already.
> >>
> >> Also CpuDxe driver will produce CPU arch protocol and LegacyBios
> >> driver has dependency for this protocol. So CpuDxe driver will start
> >> before LegacyBios driver and CpuDxe driver can allocate that space
> successful.
> >>
> >> With this change, CpuDxe driver will use 0x87000 ~ 0x88000 as wakeup
> buffer.
> >>
> >> Cc: Ray Ni <ray.ni@intel.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Eric Dong <eric.dong@intel.com>
> >> ---
> >>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 30 +++++++++++++++++++-
> ---
> >> -------
> >>  1 file changed, 19 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> >> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> >> index b2307cbb61..5bc9a47efb 100644
> >> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> >> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> >> @@ -76,7 +76,7 @@ SaveCpuMpData (
> >>  }
> >>
> >>  /**
> >> -  Get available system memory below 1MB by specified size.
> >> +  Get available system memory below 0x88000 by specified size.
> >>
> >>    @param[in] WakeupBufferSize   Wakeup buffer size required
> >>
> >> @@ -91,7 +91,19 @@ GetWakeupBuffer (
> >>    EFI_STATUS              Status;
> >>    EFI_PHYSICAL_ADDRESS    StartAddress;
> >>
> >> -  StartAddress = BASE_1MB;
> >> +  //
> >> +  // Current "Borrow" space mechanism caused potential race
> >> + condition if both  // AP and the original owner use the share space.
> >> +  //
> >> +  // LegacyBios driver tries to allocate 4K pages between 0x60000 ~
> >> + 0x88000  // space. It will just report an waring message if the
> >> + page has been allocate  // by other drivers.
> >> +  // LagacyBios driver depends on CPU Arch protocol, so it will
> >> + start after  // CpuDxe driver which produce Cpu Arch Protocol and
> >> + use this
> >> library.
> >> +  // So below allocate logic will be trigged before LegacyBios
> >> + driver and it  // will always return success.
> >> +  //
> >> +  StartAddress = BASE_512KB + BASE_32KB;
> >>    Status = gBS->AllocatePages (
> >>                    AllocateMaxAddress,
> >>                    EfiBootServicesData, @@ -99,17 +111,13 @@
> >> GetWakeupBuffer (
> >>                    &StartAddress
> >>                    );
> >>    ASSERT_EFI_ERROR (Status);
> >> -  if (!EFI_ERROR (Status)) {
> >> -    Status = gBS->FreePages(
> >> -               StartAddress,
> >> -               EFI_SIZE_TO_PAGES (WakeupBufferSize)
> >> -               );
> >> -    ASSERT_EFI_ERROR (Status);
> >> -    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
> >> = %x\n",
> >> -                        (UINTN) StartAddress, WakeupBufferSize));
> >> -  } else {
> >> +  if (EFI_ERROR (Status)) {
> >>      StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
> >>    }
> >> +
> >> +  DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
> >> = %x\n",
> >> +                      (UINTN) StartAddress, WakeupBufferSize));
> >> +
> >>    return (UINTN) StartAddress;
> >>  }
> >>
> >> --
> >> 2.15.0.windows.1
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up Buffer.

[Ni, Ray]

Eric,
I agree with the code change. It fixes the memtest86 problem through
a very simple code change.
But I think we do need a very meaningful comments to describe a such simple
code change.

Comments below:
1. Please make sure each line of commit message doesn't exceed 70.

> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Tuesday, March 5, 2019 10:07 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up
> Buffer.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1538
> 
> Current CpuDxe driver "borrows" Wakeup Buffer (through Allocate & free
> to get the buffer pointer, backup the buffer data before using it and
> restore it after using).  Now this logic met a problem described in
> the above BZ because the test tool and the CpuDxe both use the same
> memory at the same time.

2. Can you describe the issue more clearly in the commit message?
What problem is met? What's the tool?

> 
> In order to fix the above issue, CpuDxe changed to allocate the buffer
> below 1M instead of borrow it. After investigation, we found below
> 0x88000 is the possible space which can be used. 

3. What investigation was made to choose 0x88000?

> For now, range
> 0x60000 ~ 0x88000 used for Legacy OPROMs by LegacyBios driver. And it
> tries to allocate these range page(4K size) by page. It just reports
> warning message if specific page been used by others already.
> 
> Also CpuDxe driver will produce CPU arch protocol and LegacyBios driver
> has dependency for this protocol. So CpuDxe driver will start before
> LegacyBios driver and CpuDxe driver can allocate that space successful.
> 
> With this change, CpuDxe driver will use 0x87000 ~ 0x88000 as wakeup
> buffer.

4. What if someone allocates the exact page? Will CpuDxe driver fail to start?

> 
> Cc: Ray Ni <ray.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 30 +++++++++++++++++++----
> -------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index b2307cbb61..5bc9a47efb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -76,7 +76,7 @@ SaveCpuMpData (
>  }
> 
>  /**
> -  Get available system memory below 1MB by specified size.
> +  Get available system memory below 0x88000 by specified size.
> 
>    @param[in] WakeupBufferSize   Wakeup buffer size required
> 
> @@ -91,7 +91,19 @@ GetWakeupBuffer (
>    EFI_STATUS              Status;
>    EFI_PHYSICAL_ADDRESS    StartAddress;
> 
> -  StartAddress = BASE_1MB;
> +  //
> +  // Current "Borrow" space mechanism caused potential race condition if
> both
> +  // AP and the original owner use the share space.
> +  //

5. future code reader cannot understand what "current borrow mechanism" because
you change the implementation.

> +  // LegacyBios driver tries to allocate 4K pages between 0x60000 ~ 0x88000
> +  // space. It will just report an waring message if the page has been allocate
> +  // by other drivers.

6. The above wording describes the behavior of LegacyBios driver. What's the
relation ship between the LegacyBios driver behavior and the new implementation?

> +  // LagacyBios driver depends on CPU Arch protocol, so it will start after
> +  // CpuDxe driver which produce Cpu Arch Protocol and use this library.
> +  // So below allocate logic will be trigged before LegacyBios driver and it
> +  // will always return success.
> +  //
> +  StartAddress = BASE_512KB + BASE_32KB;

7. If 0x88000 is chosen, why use BASE_512KB + BASE_32KB instead of 0x88000?
Will you remove the hard-code 0x88000 and use BASE_1MB after CSM is EOL?

>    Status = gBS->AllocatePages (
>                    AllocateMaxAddress,
>                    EfiBootServicesData,
> @@ -99,17 +111,13 @@ GetWakeupBuffer (
>                    &StartAddress
>                    );
>    ASSERT_EFI_ERROR (Status);
> -  if (!EFI_ERROR (Status)) {
> -    Status = gBS->FreePages(
> -               StartAddress,
> -               EFI_SIZE_TO_PAGES (WakeupBufferSize)
> -               );
> -    ASSERT_EFI_ERROR (Status);
> -    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
> = %x\n",
> -                        (UINTN) StartAddress, WakeupBufferSize));
> -  } else {
> +  if (EFI_ERROR (Status)) {
>      StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>    }
> +
> +  DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
> = %x\n",
> +                      (UINTN) StartAddress, WakeupBufferSize));
> +
>    return (UINTN) StartAddress;
>  }
> 
> --
> 2.15.0.windows.1

Re: [Patch] MdeModulePkg/PiSmmCore: Control S3 related functionality with flag.

[Wu, Hao A]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Eric Dong
> Sent: Tuesday, March 05, 2019 10:07 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch] MdeModulePkg/PiSmmCore: Control S3 related
> functionality with flag.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1590
> 
> Use PcdAcpiS3Enable to control whether need to enable S3 related
> functionality

The above line does not pass the PatchCheck.py tool. Please help to
re-format it.

Also, please help to update the copyright year for the modified files.

With the above things handled,
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> in Pi SMM Core.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.c   | 70
> ++++++++++++++++++++++---------
>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf |  1 +
>  2 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> index d0bc65b564..bd19803c37 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> @@ -77,6 +77,12 @@ BOOLEAN  mInLegacyBoot = FALSE;
>  //
>  BOOLEAN  mDuringS3Resume = FALSE;
> 
> +//
> +// Flag to determine if platform enabled S3.
> +// Get the value from PcdAcpiS3Enable.
> +//
> +BOOLEAN  mAcpiS3Enable = FALSE;
> +
>  //
>  // Table of SMI Handlers that are registered by the SMM Core when it is
> initialized
>  //
> @@ -87,6 +93,13 @@ SMM_CORE_SMI_HANDLERS
> mSmmCoreSmiHandlers[] = {
>    { SmmExitBootServicesHandler, &gEfiEventExitBootServicesGuid,      NULL,
> FALSE },
>    { SmmReadyToBootHandler,      &gEfiEventReadyToBootGuid,           NULL,
> FALSE },
>    { SmmEndOfDxeHandler,         &gEfiEndOfDxeEventGroupGuid,         NULL,
> TRUE },
> +  { NULL,                       NULL,                                NULL, FALSE }
> +};
> +
> +//
> +// Table of SMI Handlers that are registered by the SMM Core when it is
> initialized
> +//
> +SMM_CORE_SMI_HANDLERS  mSmmCoreS3SmiHandlers[] = {
>    { SmmS3SmmInitDoneHandler,    &gEdkiiS3SmmInitDoneGuid,            NULL,
> FALSE },
>    { SmmEndOfS3ResumeHandler,    &gEdkiiEndOfS3ResumeGuid,            NULL,
> FALSE },
>    { NULL,                       NULL,                                NULL, FALSE }
> @@ -445,28 +458,30 @@ SmmEndOfDxeHandler (
>               NULL
>               );
> 
> -  //
> -  // Locate SmmSxDispatch2 protocol.
> -  //
> -  Status = SmmLocateProtocol (
> -             &gEfiSmmSxDispatch2ProtocolGuid,
> -             NULL,
> -             (VOID **)&SxDispatch
> -             );
> -  if (!EFI_ERROR (Status) && (SxDispatch != NULL)) {
> +  if (mAcpiS3Enable) {
>      //
> -    // Register a S3 entry callback function to
> -    // determine if it will be during S3 resume.
> +    // Locate SmmSxDispatch2 protocol.
>      //
> -    EntryRegisterContext.Type  = SxS3;
> -    EntryRegisterContext.Phase = SxEntry;
> -    Status = SxDispatch->Register (
> -                           SxDispatch,
> -                           SmmS3EntryCallBack,
> -                           &EntryRegisterContext,
> -                           &S3EntryHandle
> -                           );
> -    ASSERT_EFI_ERROR (Status);
> +    Status = SmmLocateProtocol (
> +               &gEfiSmmSxDispatch2ProtocolGuid,
> +               NULL,
> +               (VOID **)&SxDispatch
> +               );
> +    if (!EFI_ERROR (Status) && (SxDispatch != NULL)) {
> +      //
> +      // Register a S3 entry callback function to
> +      // determine if it will be during S3 resume.
> +      //
> +      EntryRegisterContext.Type  = SxS3;
> +      EntryRegisterContext.Phase = SxEntry;
> +      Status = SxDispatch->Register (
> +                             SxDispatch,
> +                             SmmS3EntryCallBack,
> +                             &EntryRegisterContext,
> +                             &S3EntryHandle
> +                             );
> +      ASSERT_EFI_ERROR (Status);
> +    }
>    }
> 
>    return EFI_SUCCESS;
> @@ -883,6 +898,21 @@ SmmMain (
>      ASSERT_EFI_ERROR (Status);
>    }
> 
> +  mAcpiS3Enable = PcdGetBool (PcdAcpiS3Enable);
> +  if (mAcpiS3Enable) {
> +    //
> +    // Register all S3 related SMI Handlers required by the SMM Core
> +    //
> +    for (Index = 0; mSmmCoreS3SmiHandlers[Index].HandlerType != NULL;
> Index++) {
> +      Status = SmiHandlerRegister (
> +                 mSmmCoreS3SmiHandlers[Index].Handler,
> +                 mSmmCoreS3SmiHandlers[Index].HandlerType,
> +                 &mSmmCoreS3SmiHandlers[Index].DispatchHandle
> +                 );
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +  }
> +
>    RegisterSmramProfileHandler ();
>    SmramProfileInstallProtocol ();
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> index f3ece22121..9a31cb79d6 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> @@ -101,6 +101,7 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType                   ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType                   ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                        ##
> CONSUMES
> 
>  [Guids]
>    gAprioriGuid                                  ## SOMETIMES_CONSUMES   ## File
> --
> 2.15.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel