[Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Eric Dong]

V1 changes:
> Current code logic can't confirm CpuS3DataDxe driver start before
> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
> Add implementation for AllocateAcpiCpuData function to remove this
> assumption.

V2 changes:
> Because CpuS3Data memory will be copy to smram at SmmReadToLock point,
> so the memory type no need to be ACPI NVS type, also the address not
> limit to below 4G.
> This change remove the limit of ACPI NVS memory type and below 4G.

Pass OS boot and resume from S3 test.

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

Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
Cc: Fan Jeff <vanjeff_919@hotmail.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 .../DxeRegisterCpuFeaturesLib.c                    |  67 -----------
 .../PeiRegisterCpuFeaturesLib.c                    | 131 ---------------------
 .../RegisterCpuFeaturesLib.c                       |  90 ++++++++++++++
 3 files changed, 90 insertions(+), 198 deletions(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
index 902a339529..1f34a3f489 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
@@ -197,70 +197,3 @@ GetNumberOfProcessor (
   ASSERT_EFI_ERROR (Status);
 }
 
-/**
-  Allocates ACPI NVS memory to save ACPI_CPU_DATA.
-
-  @return  Pointer to allocated ACPI_CPU_DATA.
-**/
-ACPI_CPU_DATA *
-AllocateAcpiCpuData (
-  VOID
-  )
-{
-  //
-  // CpuS3DataDxe will do it.
-  //
-  ASSERT (FALSE);
-  return NULL;
-}
-
-/**
-  Enlarges CPU register table for each processor.
-
-  @param[in, out]  RegisterTable   Pointer processor's CPU register table
-**/
-VOID
-EnlargeRegisterTable (
-  IN OUT CPU_REGISTER_TABLE            *RegisterTable
-  )
-{
-  EFI_STATUS            Status;
-  EFI_PHYSICAL_ADDRESS  Address;
-  UINTN                 AllocatePages;
-
-  Address = BASE_4GB - 1;
-  AllocatePages = RegisterTable->AllocatedSize / EFI_PAGE_SIZE;
-  Status  = gBS->AllocatePages (
-                   AllocateMaxAddress,
-                   EfiACPIMemoryNVS,
-                   AllocatePages + 1,
-                   &Address
-                   );
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // If there are records existing in the register table, then copy its contents
-  // to new region and free the old one.
-  //
-  if (RegisterTable->AllocatedSize > 0) {
-    CopyMem (
-      (VOID *) (UINTN) Address,
-      (VOID *) (UINTN) RegisterTable->RegisterTableEntry,
-      RegisterTable->AllocatedSize
-      );
-    //
-    // RegisterTableEntry is allocated by gBS->AllocatePages() service.
-    // So, gBS->FreePages() service is used to free it.
-    //
-    gBS->FreePages (
-      RegisterTable->RegisterTableEntry,
-      AllocatePages
-      );
-  }
-
-  //
-  // Adjust the allocated size and register table base address.
-  //
-  RegisterTable->AllocatedSize     += EFI_PAGE_SIZE;
-  RegisterTable->RegisterTableEntry = Address;
-}
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c
index 6804eddf65..82fe268812 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c
@@ -257,134 +257,3 @@ GetNumberOfProcessor (
                          );
   ASSERT_EFI_ERROR (Status);
 }
-
-/**
-  Allocates ACPI NVS memory to save ACPI_CPU_DATA.
-
-  @return  Pointer to allocated ACPI_CPU_DATA.
-**/
-ACPI_CPU_DATA *
-AllocateAcpiCpuData (
-  VOID
-  )
-{
-  EFI_STATUS                           Status;
-  EFI_PEI_MP_SERVICES_PPI              *CpuMpPpi;
-  UINTN                                NumberOfCpus;
-  UINTN                                NumberOfEnabledProcessors;
-  ACPI_CPU_DATA                        *AcpiCpuData;
-  EFI_PHYSICAL_ADDRESS                 Address;
-  UINTN                                TableSize;
-  CPU_REGISTER_TABLE                   *RegisterTable;
-  UINTN                                Index;
-  EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
-
-  Status = PeiServicesAllocatePages (
-             EfiACPIMemoryNVS,
-             EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)),
-             &Address
-             );
-  ASSERT_EFI_ERROR (Status);
-  AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) Address;
-  ASSERT (AcpiCpuData != NULL);
-
-  //
-  // Get MP Services Protocol
-  //
-  Status = PeiServicesLocatePpi (
-             &gEfiPeiMpServicesPpiGuid,
-             0,
-             NULL,
-             (VOID **)&CpuMpPpi
-             );
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // Get the number of CPUs
-  //
-  Status = CpuMpPpi->GetNumberOfProcessors (
-                         GetPeiServicesTablePointer (),
-                         CpuMpPpi,
-                         &NumberOfCpus,
-                         &NumberOfEnabledProcessors
-                         );
-  ASSERT_EFI_ERROR (Status);
-  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
-
-  //
-  // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
-  //
-  TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
-  Status = PeiServicesAllocatePages (
-             EfiACPIMemoryNVS,
-             EFI_SIZE_TO_PAGES (TableSize),
-             &Address
-             );
-  ASSERT_EFI_ERROR (Status);
-  RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) Address;
-
-  for (Index = 0; Index < NumberOfCpus; Index++) {
-    Status = CpuMpPpi->GetProcessorInfo (
-                         GetPeiServicesTablePointer (),
-                         CpuMpPpi,
-                         Index,
-                         &ProcessorInfoBuffer
-                         );
-    ASSERT_EFI_ERROR (Status);
-
-    RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
-    RegisterTable[Index].TableLength        = 0;
-    RegisterTable[Index].AllocatedSize      = 0;
-    RegisterTable[Index].RegisterTableEntry = 0;
-
-    RegisterTable[NumberOfCpus + Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
-    RegisterTable[NumberOfCpus + Index].TableLength        = 0;
-    RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;
-    RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
-  }
-  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
-  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
-
-  return AcpiCpuData;
-}
-
-/**
-  Enlarges CPU register table for each processor.
-
-  @param[in, out]  RegisterTable   Pointer processor's CPU register table
-**/
-VOID
-EnlargeRegisterTable (
-  IN OUT CPU_REGISTER_TABLE            *RegisterTable
-  )
-{
-  EFI_STATUS                           Status;
-  EFI_PHYSICAL_ADDRESS                 Address;
-  UINTN                                AllocatePages;
-
-  AllocatePages = RegisterTable->AllocatedSize / EFI_PAGE_SIZE;
-  Status = PeiServicesAllocatePages (
-             EfiACPIMemoryNVS,
-             AllocatePages + 1,
-             &Address
-             );
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // If there are records existing in the register table, then copy its contents
-  // to new region and free the old one.
-  //
-  if (RegisterTable->AllocatedSize > 0) {
-    CopyMem (
-      (VOID *) (UINTN) Address,
-      (VOID *) (UINTN) RegisterTable->RegisterTableEntry,
-      RegisterTable->AllocatedSize
-      );
-  }
-
-  //
-  // Adjust the allocated size and register table base address.
-  //
-  RegisterTable->AllocatedSize += EFI_PAGE_SIZE;
-  RegisterTable->RegisterTableEntry = Address;
-}
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index dd6a82be7a..b87eb280f8 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -488,6 +488,96 @@ RegisterCpuFeature (
   return RETURN_SUCCESS;
 }
 
+/**
+  Allocates ACPI NVS memory to save ACPI_CPU_DATA.
+
+  @return  Pointer to allocated ACPI_CPU_DATA.
+**/
+ACPI_CPU_DATA *
+AllocateAcpiCpuData (
+  VOID
+  )
+{
+  EFI_STATUS                           Status;
+  UINTN                                NumberOfCpus;
+  UINTN                                NumberOfEnabledProcessors;
+  ACPI_CPU_DATA                        *AcpiCpuData;
+  UINTN                                TableSize;
+  CPU_REGISTER_TABLE                   *RegisterTable;
+  UINTN                                Index;
+  EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
+
+  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
+  ASSERT (AcpiCpuData != NULL);
+
+  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
+  AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus;
+
+  //
+  // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
+  //  
+  TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
+  RegisterTable  = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
+  ASSERT (RegisterTable != NULL);
+
+  for (Index = 0; Index < NumberOfCpus; Index++) {
+    Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
+    ASSERT_EFI_ERROR (Status);
+
+    RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
+    RegisterTable[Index].TableLength        = 0;
+    RegisterTable[Index].AllocatedSize      = 0;
+    RegisterTable[Index].RegisterTableEntry = 0;
+
+    RegisterTable[NumberOfCpus + Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
+    RegisterTable[NumberOfCpus + Index].TableLength        = 0;
+    RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;
+    RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
+  }
+  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN) RegisterTable;
+  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN) (RegisterTable + NumberOfCpus);
+
+  return AcpiCpuData;
+}
+
+/**
+  Enlarges CPU register table for each processor.
+
+  @param[in, out]  RegisterTable   Pointer processor's CPU register table
+**/
+VOID
+EnlargeRegisterTable (
+  IN OUT CPU_REGISTER_TABLE            *RegisterTable
+  )
+{
+  EFI_PHYSICAL_ADDRESS  Address;
+  UINTN                 UsedPages;
+
+  UsedPages = RegisterTable->AllocatedSize / EFI_PAGE_SIZE;
+  Address  = (UINTN) AllocatePages (UsedPages + 1);
+  ASSERT (Address != 0);
+
+  //
+  // If there are records existing in the register table, then copy its contents
+  // to new region and free the old one.
+  //
+  if (RegisterTable->AllocatedSize > 0) {
+    CopyMem (
+      (VOID *) (UINTN) Address,
+      (VOID *) (UINTN) RegisterTable->RegisterTableEntry,
+      RegisterTable->AllocatedSize
+      );
+
+    FreePages ((VOID *) (UINTN) RegisterTable->RegisterTableEntry, UsedPages);
+  }
+
+  //
+  // Adjust the allocated size and register table base address.
+  //
+  RegisterTable->AllocatedSize     += EFI_PAGE_SIZE;
+  RegisterTable->RegisterTableEntry = Address;
+}
+
 /**
   Add an entry in specified register table.
 
-- 
2.15.0.windows.1

[Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

[Eric Dong]

Because CpuS3Data memory will be copy to smram at SmmReadToLock point,
so the memory type no need to be ACPI NVS type, also the address not
limit to below 4G.
This change remove the limit of ACPI NVS memory type and below 4G.

Pass OS boot and resume from S3 test.

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

Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
Cc: Fan Jeff <vanjeff_919@hotmail.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c      | 60 +++++++-------------------------
 UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 +
 2 files changed, 14 insertions(+), 47 deletions(-)

diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
index dccb406b8d..d8eb8c976f 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
@@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/DebugLib.h>
 #include <Library/MtrrLib.h>
+#include <Library/MemoryAllocationLib.h>
 
 #include <Protocol/MpService.h>
 #include <Guid/EventGroup.h>
@@ -45,42 +46,6 @@ typedef struct {
   IA32_DESCRIPTOR           IdtrProfile;
 } ACPI_CPU_DATA_EX;
 
-/**
-  Allocate EfiACPIMemoryNVS below 4G memory address.
-
-  This function allocates EfiACPIMemoryNVS below 4G memory address.
-
-  @param[in] Size   Size of memory to allocate.
-
-  @return       Allocated address for output.
-
-**/
-VOID *
-AllocateAcpiNvsMemoryBelow4G (
-  IN UINTN  Size
-  )
-{
-  EFI_PHYSICAL_ADDRESS  Address;
-  EFI_STATUS            Status;
-  VOID                  *Buffer;
-
-  Address = BASE_4GB - 1;
-  Status  = gBS->AllocatePages (
-                   AllocateMaxAddress,
-                   EfiACPIMemoryNVS,
-                   EFI_SIZE_TO_PAGES (Size),
-                   &Address
-                   );
-  if (EFI_ERROR (Status)) {
-    return NULL;
-  }
-
-  Buffer = (VOID *)(UINTN)Address;
-  ZeroMem (Buffer, Size);
-
-  return Buffer;
-}
-
 /**
   Callback function executed when the EndOfDxe event group is signaled.
 
@@ -150,7 +115,6 @@ CpuS3DataInitialize (
   EFI_MP_SERVICES_PROTOCOL   *MpServices;
   UINTN                      NumberOfCpus;
   UINTN                      NumberOfEnabledProcessors;
-  VOID                       *Stack;
   UINTN                      TableSize;
   CPU_REGISTER_TABLE         *RegisterTable;
   UINTN                      Index;
@@ -171,10 +135,7 @@ CpuS3DataInitialize (
   //
   OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
 
-  //
-  // Allocate ACPI NVS memory below 4G memory for use on ACPI S3 resume.
-  //
-  AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof (ACPI_CPU_DATA_EX));
+  AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA_EX)));
   ASSERT (AcpiCpuDataEx != NULL);
   AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
 
@@ -210,11 +171,16 @@ CpuS3DataInitialize (
   AcpiCpuData->MtrrTable    = (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
 
   //
-  // Allocate stack space for all CPUs
+  // Allocate stack space for all CPUs, use ACPI NVS memory type because it will
+  // not copy to smram at Smm ready to lock point.
   //
-  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus * AcpiCpuData->StackSize);
-  ASSERT (Stack != NULL);
-  AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
+  Status  = gBS->AllocatePages (
+                   AllocateAnyPages,
+                   EfiACPIMemoryNVS,
+                   EFI_SIZE_TO_PAGES (NumberOfCpus * AcpiCpuData->StackSize),
+                   &AcpiCpuData->StackAddress
+                   );
+  ASSERT_EFI_ERROR (Status);
 
   //
   // Get the boot processor's GDT and IDT
@@ -227,7 +193,7 @@ CpuS3DataInitialize (
   //
   GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
   IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
-  Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
+  Gdt = AllocatePages (EFI_SIZE_TO_PAGES (GdtSize + IdtSize));
   ASSERT (Gdt != NULL);
   Idt = (VOID *)((UINTN)Gdt + GdtSize);
   CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
@@ -243,7 +209,7 @@ CpuS3DataInitialize (
     // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
     //
     TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
-    RegisterTable = (CPU_REGISTER_TABLE *)AllocateAcpiNvsMemoryBelow4G (TableSize);
+    RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
     ASSERT (RegisterTable != NULL);
 
     for (Index = 0; Index < NumberOfCpus; Index++) {
diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
index 480c98ebcd..c16731529c 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
@@ -51,6 +51,7 @@
   DebugLib
   BaseLib
   MtrrLib
+  MemoryAllocationLib
 
 [Guids]
   gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
-- 
2.15.0.windows.1

Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Laszlo Ersek]

Hi Eric,

On 08/08/18 09:40, Eric Dong wrote:
> V1 changes:
>> Current code logic can't confirm CpuS3DataDxe driver start before
>> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
>> Add implementation for AllocateAcpiCpuData function to remove this
>> assumption.
>
> V2 changes:
>> Because CpuS3Data memory will be copy to smram at SmmReadToLock point,
>> so the memory type no need to be ACPI NVS type, also the address not
>> limit to below 4G.
>> This change remove the limit of ACPI NVS memory type and below 4G.
>
> Pass OS boot and resume from S3 test.
>
> Bugz: https://bugzilla.tianocore.org/show_bug.cgi?id=959
>
> Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Cc: Fan Jeff <vanjeff_919@hotmail.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  .../DxeRegisterCpuFeaturesLib.c                    |  67 -----------
>  .../PeiRegisterCpuFeaturesLib.c                    | 131 ---------------------
>  .../RegisterCpuFeaturesLib.c                       |  90 ++++++++++++++
>  3 files changed, 90 insertions(+), 198 deletions(-)

In order to review this patch, I created the following text files:

- allocate-pei.txt
- allocate-dxe.txt
- allocate-common.txt

- enlarge-pei.txt
- enlarge-dxe.txt
- enlarge-common.txt

This way I can compare the PEI<->DXE variants before the patch, and I
can also compare each of PEI/DXE to the combined version.

For AllocateAcpiCpuData(), I have the following comments:

(1) The common implementation still says "Allocates ACPI NVS memory to
save ACPI_CPU_DATA" in the leading comment block. Please update that to
"boot services data".

(2) The AllocateAcpiCpuData() function declaration should be removed
from the "RegisterCpuFeatures.h" file -- this function is now internal.
(Preferably, the function should be made STATIC as well.)

(3) When diffing "allocate-pei.txt" against "allocate-common.txt", I'm
noticing whitespace changes that don't look justified:

> -  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> +  AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus;

and

> -  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
> -  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
> +  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN) RegisterTable;
> +  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN) (RegisterTable + NumberOfCpus);

Inserting a space character between "(type)" and "expression" in a cast
expression is extremely bad practice in edk2, IMO. As I mentioned
earlier, the binding of this operator is one of the strongest bindings
in the C language, and inserting a space character before the expression
makes a misleading and counter-intuitive impression about that.

If you really want to insert that space, I guess I'll have to live with
that, but I'd like to highlight that it's bad practice, and (to my
knowledge) it isn't required by the edk2 coding style.


About the EnlargeRegisterTable() function:

(4) The function declaration should be removed from
"RegisterCpuFeatures.h" -- the function is now internal. (Preferably, it
should be made STATIC too.)

(5) Some more whitespace in cast expressions that I find questionable,
near AllocatePages() and FreePages() :)


What do you think? Thanks!
Laszlo

Re: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

[Laszlo Ersek]

Hello Eric,

On 08/08/18 09:40, Eric Dong wrote:
> Because CpuS3Data memory will be copy to smram at SmmReadToLock point,
> so the memory type no need to be ACPI NVS type, also the address not
> limit to below 4G.
> This change remove the limit of ACPI NVS memory type and below 4G.
>
> Pass OS boot and resume from S3 test.
>
> Bugz: https://bugzilla.tianocore.org/show_bug.cgi?id=959
>
> Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>

(1) I believe these tags don't apply to this patch -- this patch seems
to be a new idea / addition.


> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Cc: Fan Jeff <vanjeff_919@hotmail.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c      | 60 +++++++-------------------------
>  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 +
>  2 files changed, 14 insertions(+), 47 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index dccb406b8d..d8eb8c976f 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MtrrLib.h>
> +#include <Library/MemoryAllocationLib.h>
>
>  #include <Protocol/MpService.h>
>  #include <Guid/EventGroup.h>
> @@ -45,42 +46,6 @@ typedef struct {
>    IA32_DESCRIPTOR           IdtrProfile;
>  } ACPI_CPU_DATA_EX;
>
> -/**
> -  Allocate EfiACPIMemoryNVS below 4G memory address.
> -
> -  This function allocates EfiACPIMemoryNVS below 4G memory address.
> -
> -  @param[in] Size   Size of memory to allocate.
> -
> -  @return       Allocated address for output.
> -
> -**/
> -VOID *
> -AllocateAcpiNvsMemoryBelow4G (
> -  IN UINTN  Size
> -  )
> -{
> -  EFI_PHYSICAL_ADDRESS  Address;
> -  EFI_STATUS            Status;
> -  VOID                  *Buffer;
> -
> -  Address = BASE_4GB - 1;
> -  Status  = gBS->AllocatePages (
> -                   AllocateMaxAddress,
> -                   EfiACPIMemoryNVS,
> -                   EFI_SIZE_TO_PAGES (Size),
> -                   &Address
> -                   );
> -  if (EFI_ERROR (Status)) {
> -    return NULL;
> -  }
> -
> -  Buffer = (VOID *)(UINTN)Address;
> -  ZeroMem (Buffer, Size);
> -
> -  return Buffer;
> -}
> -
>  /**
>    Callback function executed when the EndOfDxe event group is signaled.
>
> @@ -150,7 +115,6 @@ CpuS3DataInitialize (
>    EFI_MP_SERVICES_PROTOCOL   *MpServices;
>    UINTN                      NumberOfCpus;
>    UINTN                      NumberOfEnabledProcessors;
> -  VOID                       *Stack;
>    UINTN                      TableSize;
>    CPU_REGISTER_TABLE         *RegisterTable;
>    UINTN                      Index;
> @@ -171,10 +135,7 @@ CpuS3DataInitialize (
>    //
>    OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
>
> -  //
> -  // Allocate ACPI NVS memory below 4G memory for use on ACPI S3 resume.
> -  //
> -  AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof (ACPI_CPU_DATA_EX));
> +  AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA_EX)));

(2) In the original AllocateAcpiNvsMemoryBelow4G() function, we have a
ZeroMem() call. For compatibility, I think we should call
AllocateZeroPages() here.

(Previously, the trailing portion of the last page may not have been
zeroed, but it's not a problem to zero more than before.)


(3) The "UefiCpuPkg/Include/AcpiCpuData.h" header states that
ACPI_CPU_DATA (the structure itself) "must be allocated below 4GB from
memory of type EfiACPIMemoryNVS".

If we have determined that this is no longer necessary, then:
- we should first update the documentation in "AcpiCpuData.h" first,
- the commit message should carefully explain why the change is valid.

In particular, my concern is not about the normal boot path. (The normal
boot path is explained for example in the message of commit
92b87f1c8c0b, "OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE",
2015-11-30.) I agree that copying from BootServicesData type memory into
SMRAM is fine, during normal boot.

Instead, my concern is with the S3 resume path, when the data from the
SMRAM buffer is *restored*. I seem to recall a case when the data was
first restored from SMRAM to the original AcpiNVS allocation. If we make
that area BootServicesData now, then the OS will reuse it, and then at
S3 resume, we overwrite OS data.

... After reviewing the GetAcpiCpuData() function in
"UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", I *think* this is a valid change,
because GetAcpiCpuData() never remembers the address of the
ACPI_CPU_DATA structure, or the addresses of its fields.

However, I would like Mike to review this as well (CC'ing him).


(4) Because ACPI_CPU_DATA_EX contains MtrrTable, GdtrProfile and
IdtrProfile too, not just ACPI_CPU_DATA, the above change moves all of
those fields into BootServicesData type memory. In turn, the
EFI_PHYSICAL_ADDRESS fields in ACPI_CPU_DATA that have the same names
will point into BootServicesData type memory.

This conflicts with the requirements that are documented in
"UefiCpuPkg/Include/AcpiCpuData.h" for all three fields: MtrrTable,
GdtrProfile and IdtrProfile.

If we want to change the allocation of these objects (of type
MTRR_SETTINGS, IA32_DESCRIPTOR and IA32_DESCRIPTOR, respectively), then
we should audit their use during S3 resume in the PiSmmCpuDxeSmm driver,
and document them one-by-one.

... Based on GetAcpiCpuData() again, I *think* this is valid change,
functionally speaking, but I'd like Mike to review as well.


>    ASSERT (AcpiCpuDataEx != NULL);
>    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
>
> @@ -210,11 +171,16 @@ CpuS3DataInitialize (
>    AcpiCpuData->MtrrTable    = (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
>
>    //
> -  // Allocate stack space for all CPUs
> +  // Allocate stack space for all CPUs, use ACPI NVS memory type because it will
> +  // not copy to smram at Smm ready to lock point.
>    //
> -  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus * AcpiCpuData->StackSize);
> -  ASSERT (Stack != NULL);
> -  AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
> +  Status  = gBS->AllocatePages (
> +                   AllocateAnyPages,
> +                   EfiACPIMemoryNVS,
> +                   EFI_SIZE_TO_PAGES (NumberOfCpus * AcpiCpuData->StackSize),
> +                   &AcpiCpuData->StackAddress
> +                   );
> +  ASSERT_EFI_ERROR (Status);

(5) The leading comment should be clarified. We should state that during
S3 resume, the stack will only be used as scratch space, i.e. we won't
read anything from it before we write to it, in PiSmmCpuDxeSemm.
Otherwise, it would be a security bug to keep the area in AcpiNVS and
not in SMRAM.


(6) This change requires a separate investigation from the rest of the
patch, so it should be in a separate patch.

That implies we should first change this call site, and then remove
AllocateAcpiNvsMemoryBelow4G() in a final patch.


(7) Why is it OK to lift the 4GB limitation? ... I think it may be valid
indeed, because PrepareApStartupVector() stores StackAddress to
"mExchangeInfo->StackStart" (which has type (VOID*)), and because
"UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm" reads the latter with:

        add  edi, StackStartAddressLocation
        add  rax, qword [edi]
        mov  rsp, rax
        mov  qword [edi], rax

in long-mode code. But, again, this should be documented in the
(separate) patch's commit message.


(8) The change breaks the documentation on "ACPI_CPU_DATA.StackAddress",
in "UefiCpuPkg/Include/AcpiCpuData.h". The documentation should be
updated.


>
>    //
>    // Get the boot processor's GDT and IDT
> @@ -227,7 +193,7 @@ CpuS3DataInitialize (
>    //
>    GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
>    IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> -  Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
> +  Gdt = AllocatePages (EFI_SIZE_TO_PAGES (GdtSize + IdtSize));
>    ASSERT (Gdt != NULL);
>    Idt = (VOID *)((UINTN)Gdt + GdtSize);
>    CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);

(9) This change breaks the requirements in
"UefiCpuPkg/Include/AcpiCpuData.h" that both
"ACPI_CPU_DATA.GdtrProfile->Base" and "ACPI_CPU_DATA.IdtrProfile->Base"
point into AcpiNVS data.

("The buffer for GDT must also be allocated below 4GB from memory of
type EfiACPIMemoryNVS" / "The buffer for IDT must also be allocated
below 4GB from memory of type EfiACPIMemoryNVS".)


(10) And, indeed, this is the bug that I suspected in point (3), with
regard to the S3 resume path. Namely:


Please consider the GetAcpiCpuData() function in
"UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c". This function:

(10a) copies the IDT and GDT *descriptors* into dynamically allocated
SMRAM, pointed-to by the "mAcpiCpuData.GdtrProfile" and
"mAcpiCpuData.IdtrProfile" fields;

(10b) copies the IDT and GDT themselves into dynamically allocated
SMRAM, pointed-to by the "mGdtForAp" and "mIdtForAp" global variables,

(10c) *does not* update the "Base" members of the IDT and GDT
descriptors that are saved in SMRAM in step (10a). Those continue to
point to memory that was allocated by CpuS3DataDxe in the
CpuS3DataInitialize() function.


Now, consider the PrepareApStartupVector() function in
"UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", which runs during S3 resume. That
function:

(10d) copies the IDT and GDT descriptors that were saved in (10a) into
"mExchangeInfo" -- this is just an SMRAM-to-SMRAM copy, however the Base
members of the descriptors point to the original CpuS3DataDxe
allocation,

(10e) the IDT and the GDT are themselves restored from step (10b), i.e.
from the "mGdtForAp" and "mIdtForAp" SMRAM global variables, to the
original CpuS3DataDxe allocation address. The comment says:

>   //
>   // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI NVS memory
>   //

In other words, while the IDT and GDT *descriptors* aren't restored
in-place, the IDT and GDT themselves are.

This means that the above code change causes PiSmmCpuDxeSmm to overwrite
OS memory at S3 resume.


Continuing:

On 08/08/18 09:40, Eric Dong wrote:
> @@ -243,7 +209,7 @@ CpuS3DataInitialize (
>      // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
>      //
>      TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateAcpiNvsMemoryBelow4G (TableSize);
> +    RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
>      ASSERT (RegisterTable != NULL);
>
>      for (Index = 0; Index < NumberOfCpus; Index++) {

(11) This looks valid, beyond breaking the documentation in
"UefiCpuPkg/Include/AcpiCpuData.h", of the members
"PreSmmInitRegisterTable" and "RegisterTable".


> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> index 480c98ebcd..c16731529c 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -51,6 +51,7 @@
>    DebugLib
>    BaseLib
>    MtrrLib
> +  MemoryAllocationLib
>
>  [Guids]
>    gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
>

I feel uncomfortable about this patch. It is very tricky to review, and
if we make mistakes, we could corrupt OS data, or (worse) perhaps even
compromise SMRAM.

And, all the gains that I can imagine are, "save a few 32-bit AcpiNVS
pages". I haven't measured, but it doesn't look like an attractive
trade-off to me. And, platforms that disable S3 via "PcdAcpiS3Enable"
don't allocate this memory anyway.

Furthermore, the memory footprint optimization does not seem related to
the CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency issue that
Marvin reported (and that is tracked in bug #959).

I suggest that we drop this patch.

Thanks,
Laszlo

Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Laszlo Ersek]

On 08/08/18 09:40, Eric Dong wrote:
> V1 changes:
>> Current code logic can't confirm CpuS3DataDxe driver start before
>> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
>> Add implementation for AllocateAcpiCpuData function to remove this
>> assumption.
> 
> V2 changes:
>> Because CpuS3Data memory will be copy to smram at SmmReadToLock point,
>> so the memory type no need to be ACPI NVS type, also the address not
>> limit to below 4G.
>> This change remove the limit of ACPI NVS memory type and below 4G.

I'm returning to this patch (v2 1/2) after my other comments (for v2 2/2).

It seems that allocating ACPI_CPU_DATA in BootServicesData type memory
breaks at least the docs / specs in "UefiCpuPkg/Include/AcpiCpuData.h",
even if we do the BootServicesData allocation in RegisterCpuFeaturesLib
instances (i.e. in CpuFeaturesPei / CpuFeaturesDxe), and not in
CpuS3DataDxe.

With that in mind, should we return to your v1 patch,
"UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData function"?

And, looking back at my question (4) there, where I suggested
AllocatePeiAccessiblePages() -- I'm now thinking that it does not apply,
because the ACPI_CPU_DATA spec in "UefiCpuPkg/Include/AcpiCpuData.h"
requires the 4GB limit.

So, currently I think that your v1 patch is best, just add the
Reported-by and Suggested-by tags, plus the TianoCore BZ reference.

Sorry that I've taken so long to grasp the implications. (Anyway we're
now in the quiet period.)

Thanks,
Laszlo

Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Ni, Ruiyu]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, August 9, 2018 5:29 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib:
> Combine implementation.
> 
> On 08/08/18 09:40, Eric Dong wrote:
> > V1 changes:
> >> Current code logic can't confirm CpuS3DataDxe driver start before
> >> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
> >> Add implementation for AllocateAcpiCpuData function to remove this
> >> assumption.
> >
> > V2 changes:
> >> Because CpuS3Data memory will be copy to smram at SmmReadToLock
> >> point, so the memory type no need to be ACPI NVS type, also the
> >> address not limit to below 4G.
> >> This change remove the limit of ACPI NVS memory type and below 4G.
> 
> I'm returning to this patch (v2 1/2) after my other comments (for v2 2/2).
> 
> It seems that allocating ACPI_CPU_DATA in BootServicesData type memory
> breaks at least the docs / specs in "UefiCpuPkg/Include/AcpiCpuData.h",
> even if we do the BootServicesData allocation in RegisterCpuFeaturesLib
> instances (i.e. in CpuFeaturesPei / CpuFeaturesDxe), and not in
> CpuS3DataDxe.
> 
> With that in mind, should we return to your v1 patch,
> "UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData
> function"?
> 
> And, looking back at my question (4) there, where I suggested
> AllocatePeiAccessiblePages() -- I'm now thinking that it does not apply,
> because the ACPI_CPU_DATA spec in "UefiCpuPkg/Include/AcpiCpuData.h"
> requires the 4GB limit.

I guess Eric forgot to update the comments in AcpiCpuData.h regarding the
4GB/NVS restriction.
We've reviewed the whole producer/consumer code and came to the conclusion
that 4GB/NVS restriction is unnecessary.

> 
> So, currently I think that your v1 patch is best, just add the Reported-by and
> Suggested-by tags, plus the TianoCore BZ reference.
> 
> Sorry that I've taken so long to grasp the implications. (Anyway we're now in
> the quiet period.)
> 
> Thanks,
> Laszlo

Re: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

[Ni, Ruiyu]

> (3) The "UefiCpuPkg/Include/AcpiCpuData.h" header states that
> ACPI_CPU_DATA (the structure itself) "must be allocated below 4GB from
> memory of type EfiACPIMemoryNVS".
> 
> If we have determined that this is no longer necessary, then:
> - we should first update the documentation in "AcpiCpuData.h" first,
> - the commit message should carefully explain why the change is valid.

Indeed. The header file will be updated.

> 
> In particular, my concern is not about the normal boot path. (The normal
> boot path is explained for example in the message of commit 92b87f1c8c0b,
> "OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE",
> 2015-11-30.) I agree that copying from BootServicesData type memory into
> SMRAM is fine, during normal boot.
> 
> Instead, my concern is with the S3 resume path, when the data from the
> SMRAM buffer is *restored*. I seem to recall a case when the data was first
> restored from SMRAM to the original AcpiNVS allocation. If we make that
> area BootServicesData now, then the OS will reuse it, and then at
> S3 resume, we overwrite OS data.
> 
> ... After reviewing the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", I *think* this is a valid change,
> because GetAcpiCpuData() never remembers the address of the
> ACPI_CPU_DATA structure, or the addresses of its fields.

In order to making this changes, we reviewed the whole boot/S3 execution
code path and didn't see any SMRAM to normal memory restore.
We think it's safe to use BS memory. The NVS memory usage was added before
the SMRAM shadow. Now since S3 boot path only uses the SMRAM content, there
is no need to use NVS memory at all.
> 
> However, I would like Mike to review this as well (CC'ing him).
> 
> 
> (4) Because ACPI_CPU_DATA_EX contains MtrrTable, GdtrProfile and
> IdtrProfile too, not just ACPI_CPU_DATA, the above change moves all of
> those fields into BootServicesData type memory. In turn, the
> EFI_PHYSICAL_ADDRESS fields in ACPI_CPU_DATA that have the same
> names will point into BootServicesData type memory.
> 
> This conflicts with the requirements that are documented in
> "UefiCpuPkg/Include/AcpiCpuData.h" for all three fields: MtrrTable,
> GdtrProfile and IdtrProfile.
> 
> If we want to change the allocation of these objects (of type
> MTRR_SETTINGS, IA32_DESCRIPTOR and IA32_DESCRIPTOR, respectively),
> then we should audit their use during S3 resume in the PiSmmCpuDxeSmm
> driver, and document them one-by-one.
> 
> ... Based on GetAcpiCpuData() again, I *think* this is valid change,
> functionally speaking, but I'd like Mike to review as well.
> 
> 
> >    ASSERT (AcpiCpuDataEx != NULL);
> >    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
> >
> > @@ -210,11 +171,16 @@ CpuS3DataInitialize (
> >    AcpiCpuData->MtrrTable    =
> (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
> >
> >    //
> > -  // Allocate stack space for all CPUs
> > +  // Allocate stack space for all CPUs, use ACPI NVS memory type
> > + because it will  // not copy to smram at Smm ready to lock point.
> >    //
> > -  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus *
> > AcpiCpuData->StackSize);
> > -  ASSERT (Stack != NULL);
> > -  AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
> > +  Status  = gBS->AllocatePages (
> > +                   AllocateAnyPages,
> > +                   EfiACPIMemoryNVS,
> > +                   EFI_SIZE_TO_PAGES (NumberOfCpus * AcpiCpuData-
> >StackSize),
> > +                   &AcpiCpuData->StackAddress
> > +                   );
> > +  ASSERT_EFI_ERROR (Status);
> 
> (5) The leading comment should be clarified. We should state that during
> S3 resume, the stack will only be used as scratch space, i.e. we won't read
> anything from it before we write to it, in PiSmmCpuDxeSemm.
> Otherwise, it would be a security bug to keep the area in AcpiNVS and not in
> SMRAM.
> 
> 
> (6) This change requires a separate investigation from the rest of the patch,
> so it should be in a separate patch.
> 
> That implies we should first change this call site, and then remove
> AllocateAcpiNvsMemoryBelow4G() in a final patch.
> 
> 
> (7) Why is it OK to lift the 4GB limitation? ... I think it may be valid indeed,
> because PrepareApStartupVector() stores StackAddress to "mExchangeInfo-
> >StackStart" (which has type (VOID*)), and because
> "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm" reads the latter with:
> 
>         add  edi, StackStartAddressLocation
>         add  rax, qword [edi]
>         mov  rsp, rax
>         mov  qword [edi], rax
> 
> in long-mode code. But, again, this should be documented in the
> (separate) patch's commit message.
> 
> 
> (8) The change breaks the documentation on
> "ACPI_CPU_DATA.StackAddress", in "UefiCpuPkg/Include/AcpiCpuData.h".
> The documentation should be updated.
> 
> 
> >
> >    //
> >    // Get the boot processor's GDT and IDT @@ -227,7 +193,7 @@
> > CpuS3DataInitialize (
> >    //
> >    GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
> >    IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> > -  Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
> > +  Gdt = AllocatePages (EFI_SIZE_TO_PAGES (GdtSize + IdtSize));
> >    ASSERT (Gdt != NULL);
> >    Idt = (VOID *)((UINTN)Gdt + GdtSize);
> >    CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
> 
> (9) This change breaks the requirements in
> "UefiCpuPkg/Include/AcpiCpuData.h" that both
> "ACPI_CPU_DATA.GdtrProfile->Base" and "ACPI_CPU_DATA.IdtrProfile-
> >Base"
> point into AcpiNVS data.
> 
> ("The buffer for GDT must also be allocated below 4GB from memory of type
> EfiACPIMemoryNVS" / "The buffer for IDT must also be allocated below 4GB
> from memory of type EfiACPIMemoryNVS".)
> 
> 
> (10) And, indeed, this is the bug that I suspected in point (3), with regard to
> the S3 resume path. Namely:
> 
> 
> Please consider the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c". This function:
> 
> (10a) copies the IDT and GDT *descriptors* into dynamically allocated
> SMRAM, pointed-to by the "mAcpiCpuData.GdtrProfile" and
> "mAcpiCpuData.IdtrProfile" fields;
> 
> (10b) copies the IDT and GDT themselves into dynamically allocated SMRAM,
> pointed-to by the "mGdtForAp" and "mIdtForAp" global variables,
> 
> (10c) *does not* update the "Base" members of the IDT and GDT descriptors
> that are saved in SMRAM in step (10a). Those continue to point to memory
> that was allocated by CpuS3DataDxe in the
> CpuS3DataInitialize() function.
> 
> 
> Now, consider the PrepareApStartupVector() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", which runs during S3 resume.
> That
> function:
> 
> (10d) copies the IDT and GDT descriptors that were saved in (10a) into
> "mExchangeInfo" -- this is just an SMRAM-to-SMRAM copy, however the
> Base members of the descriptors point to the original CpuS3DataDxe
> allocation,
> 
> (10e) the IDT and the GDT are themselves restored from step (10b), i.e.
> from the "mGdtForAp" and "mIdtForAp" SMRAM global variables, to the
> original CpuS3DataDxe allocation address. The comment says:
> 
> >   //
> >   // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI
> NVS memory
> >   //
> 
> In other words, while the IDT and GDT *descriptors* aren't restored in-place,
> the IDT and GDT themselves are.
> 
> This means that the above code change causes PiSmmCpuDxeSmm to
> overwrite OS memory at S3 resume.
> 
> 
> Continuing:
> 
> On 08/08/18 09:40, Eric Dong wrote:
> > @@ -243,7 +209,7 @@ CpuS3DataInitialize (
> >      // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
> >      //
> >      TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > -    RegisterTable = (CPU_REGISTER_TABLE
> *)AllocateAcpiNvsMemoryBelow4G (TableSize);
> > +    RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> >      ASSERT (RegisterTable != NULL);
> >
> >      for (Index = 0; Index < NumberOfCpus; Index++) {
> 
> (11) This looks valid, beyond breaking the documentation in
> "UefiCpuPkg/Include/AcpiCpuData.h", of the members
> "PreSmmInitRegisterTable" and "RegisterTable".
> 
> 
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > index 480c98ebcd..c16731529c 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > @@ -51,6 +51,7 @@
> >    DebugLib
> >    BaseLib
> >    MtrrLib
> > +  MemoryAllocationLib
> >
> >  [Guids]
> >    gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
> >
> 
> I feel uncomfortable about this patch. It is very tricky to review, and if we
> make mistakes, we could corrupt OS data, or (worse) perhaps even
> compromise SMRAM.
> 
> And, all the gains that I can imagine are, "save a few 32-bit AcpiNVS pages". I
> haven't measured, but it doesn't look like an attractive trade-off to me. And,
> platforms that disable S3 via "PcdAcpiS3Enable"
> don't allocate this memory anyway.
> 
> Furthermore, the memory footprint optimization does not seem related to
> the CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency issue that
> Marvin reported (and that is tracked in bug #959).
> 
> I suggest that we drop this patch.
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Laszlo Ersek]

On 08/09/18 05:18, Ni, Ruiyu wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Thursday, August 9, 2018 5:29 AM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib:
>> Combine implementation.
>>
>> On 08/08/18 09:40, Eric Dong wrote:
>>> V1 changes:
>>>> Current code logic can't confirm CpuS3DataDxe driver start before
>>>> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
>>>> Add implementation for AllocateAcpiCpuData function to remove this
>>>> assumption.
>>>
>>> V2 changes:
>>>> Because CpuS3Data memory will be copy to smram at SmmReadToLock
>>>> point, so the memory type no need to be ACPI NVS type, also the
>>>> address not limit to below 4G.
>>>> This change remove the limit of ACPI NVS memory type and below 4G.
>>
>> I'm returning to this patch (v2 1/2) after my other comments (for v2 2/2).
>>
>> It seems that allocating ACPI_CPU_DATA in BootServicesData type memory
>> breaks at least the docs / specs in "UefiCpuPkg/Include/AcpiCpuData.h",
>> even if we do the BootServicesData allocation in RegisterCpuFeaturesLib
>> instances (i.e. in CpuFeaturesPei / CpuFeaturesDxe), and not in
>> CpuS3DataDxe.
>>
>> With that in mind, should we return to your v1 patch,
>> "UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData
>> function"?
>>
>> And, looking back at my question (4) there, where I suggested
>> AllocatePeiAccessiblePages() -- I'm now thinking that it does not apply,
>> because the ACPI_CPU_DATA spec in "UefiCpuPkg/Include/AcpiCpuData.h"
>> requires the 4GB limit.
> 
> I guess Eric forgot to update the comments in AcpiCpuData.h regarding the
> 4GB/NVS restriction.

If patch #2 is fixed, *and* the "AcpiCpuData.h" documentation is
updated, relaxing the allocation restriction, then this patch set could
be viable, yes. I would still like Mike to confirm as well.

> We've reviewed the whole producer/consumer code and came to the conclusion
> that 4GB/NVS restriction is unnecessary.

I think you both missed the in-place restoration of the GDT and the IDT,
to AcpiNVS memory allocated originally by CpuS3DataDxe. Please re-read
section (10) of my other email carefully:

http://mid.mail-archive.com/f48935e4-96b3-978e-d67c-84a169414ccb@redhat.com

Basically, the GdtrProfile and IdtrProfile fields in ACPI_CPU_DATA are
*doubly* indirect references. The fields themselves point to IDT and GDT
*descriptors*, and those descriptors (the Base fields in them) point to
the tables (IDT and GDT).

Indeed, PiSmmCpuDxeSmm saves everything into SMRAM on the normal boot
path: (a) ACPI_CPU_DATA, (b) the descriptors pointed-to by GdtrProfile
and IdtrProfile, and (c) the tables pointed-to by GdtrProfile->Base and
IdtrProfile->Base.

However, on the S3 resume path, PiSmmCpuDxeSmm does not use everything
from SMRAM. In the PrepareApStartupVector() function, the GDT and the
IDT tables -- at the end of the pointer chain -- are restored to the
*original* AcpiNVS locations (*not* SMRAM), and they are then used from
there.

This is not a security bug, because even if the OS overwrites the
AcpiNVS area between normal boot and S3 resume, PiSmmCpuDxeSmm never
reads that area at S3 before restoring it, from the SMRAM objects
"mGdtForAp" and "mIdtForAp".

It does mean though that the OS must be informed in advance to stay away
from that area, because PiSmmCpuDxeSmm will overwrite it at S3 resume.
This is why that allocation must be kept as AcpiNVS. (Or moved to SMRAM
entirely.)

Laszlo

Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Laszlo Ersek]

On 08/09/18 05:18, Ni, Ruiyu wrote:

> We've reviewed the whole producer/consumer code and came to the conclusion
> that 4GB/NVS restriction is unnecessary.

I'd like to comment on this from a workflow POV as well.

If you perform a detailed review of the code, that's great. However, in
that case, please do take the time to *document* the review *in detail*
in the commit message.

I assume you and Eric may have spent a few hours reviewing the code,
maybe drawing diagrams, using check lists, and so on. Why was none of
that documented in the commit message?

The commit message stated the result of your investigation, and none of
the evidence. In order to review the patch, I had to rebuild the entire
argument from zero, checking the life-cycle of every single field in
ACPI_CPU_DATA, and that took the better part of a day.

My job as a reviewer is to *read* your investigation and verify it, not
to *reconstruct* it from scratch.

Documenting one's findings in detail also helps one root out omissions
or mistakes in one's reasoning. I catch my own errors like this all the
time.

Thanks
Laszlo

Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Dong, Eric]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, August 9, 2018 7:21 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-
> devel@lists.01.org
> Subject: Re: [edk2] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib:
> Combine implementation.
> 
> On 08/09/18 05:18, Ni, Ruiyu wrote:
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: Thursday, August 9, 2018 5:29 AM
> >> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> >> Subject: Re: [edk2] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib:
> >> Combine implementation.
> >>
> >> On 08/08/18 09:40, Eric Dong wrote:
> >>> V1 changes:
> >>>> Current code logic can't confirm CpuS3DataDxe driver start before
> >>>> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
> >>>> Add implementation for AllocateAcpiCpuData function to remove this
> >>>> assumption.
> >>>
> >>> V2 changes:
> >>>> Because CpuS3Data memory will be copy to smram at SmmReadToLock
> >>>> point, so the memory type no need to be ACPI NVS type, also the
> >>>> address not limit to below 4G.
> >>>> This change remove the limit of ACPI NVS memory type and below 4G.
> >>
> >> I'm returning to this patch (v2 1/2) after my other comments (for v2 2/2).
> >>
> >> It seems that allocating ACPI_CPU_DATA in BootServicesData type
> >> memory breaks at least the docs / specs in
> >> "UefiCpuPkg/Include/AcpiCpuData.h",
> >> even if we do the BootServicesData allocation in
> >> RegisterCpuFeaturesLib instances (i.e. in CpuFeaturesPei /
> >> CpuFeaturesDxe), and not in CpuS3DataDxe.
> >>
> >> With that in mind, should we return to your v1 patch,
> >> "UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData
> >> function"?
> >>
> >> And, looking back at my question (4) there, where I suggested
> >> AllocatePeiAccessiblePages() -- I'm now thinking that it does not
> >> apply, because the ACPI_CPU_DATA spec in
> "UefiCpuPkg/Include/AcpiCpuData.h"
> >> requires the 4GB limit.
> >
> > I guess Eric forgot to update the comments in AcpiCpuData.h regarding
> > the 4GB/NVS restriction.
> 
> If patch #2 is fixed, *and* the "AcpiCpuData.h" documentation is updated,
> relaxing the allocation restriction, then this patch set could be viable, yes. I
> would still like Mike to confirm as well.
> 
> > We've reviewed the whole producer/consumer code and came to the
> > conclusion that 4GB/NVS restriction is unnecessary.
> 
> I think you both missed the in-place restoration of the GDT and the IDT, to
> AcpiNVS memory allocated originally by CpuS3DataDxe. Please re-read
> section (10) of my other email carefully:
> 
> http://mid.mail-archive.com/f48935e4-96b3-978e-d67c-
> 84a169414ccb@redhat.com
> 
> Basically, the GdtrProfile and IdtrProfile fields in ACPI_CPU_DATA are
> *doubly* indirect references. The fields themselves point to IDT and GDT
> *descriptors*, and those descriptors (the Base fields in them) point to the
> tables (IDT and GDT).
> 
> Indeed, PiSmmCpuDxeSmm saves everything into SMRAM on the normal boot
> path: (a) ACPI_CPU_DATA, (b) the descriptors pointed-to by GdtrProfile and
> IdtrProfile, and (c) the tables pointed-to by GdtrProfile->Base and
> IdtrProfile->Base.
> 
> However, on the S3 resume path, PiSmmCpuDxeSmm does not use everything
> from SMRAM. In the PrepareApStartupVector() function, the GDT and the IDT
> tables -- at the end of the pointer chain -- are restored to the
> *original* AcpiNVS locations (*not* SMRAM), and they are then used from
> there.
> 

Yes, this code is used to restore data from Smram to AcpiNVS memory.  But I think this is a weird behavior and we should use GDT/IDT in smram. I will submit a separate patch to do this change.

Thanks,
Eric
> This is not a security bug, because even if the OS overwrites the AcpiNVS area
> between normal boot and S3 resume, PiSmmCpuDxeSmm never reads that
> area at S3 before restoring it, from the SMRAM objects "mGdtForAp" and
> "mIdtForAp".
> 
> It does mean though that the OS must be informed in advance to stay away
> from that area, because PiSmmCpuDxeSmm will overwrite it at S3 resume.
> This is why that allocation must be kept as AcpiNVS. (Or moved to SMRAM
> entirely.)
> 



> Laszlo

Re: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

[Dong, Eric]

Hi Laszlo,



> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, August 9, 2018 12:55 AM
> To: Dong, Eric <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change
> Memory Type and address limitation.
> 
> Hello Eric,
> 
> On 08/08/18 09:40, Eric Dong wrote:
> > Because CpuS3Data memory will be copy to smram at SmmReadToLock
> point,
> > so the memory type no need to be ACPI NVS type, also the address not
> > limit to below 4G.
> > This change remove the limit of ACPI NVS memory type and below 4G.
> >
> > Pass OS boot and resume from S3 test.
> >
> > Bugz: https://bugzilla.tianocore.org/show_bug.cgi?id=959
> >
> > Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
> > Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
> 
> (1) I believe these tags don't apply to this patch -- this patch seems to be a
> new idea / addition.
> 

Yes, will remove it in my next version patch.

> 
> > Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
> > Cc: Fan Jeff <vanjeff_919@hotmail.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c      | 60 +++++++-----------------------
> --
> >  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 +
> >  2 files changed, 14 insertions(+), 47 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > index dccb406b8d..d8eb8c976f 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > @@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/DebugLib.h>
> >  #include <Library/MtrrLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> >
> >  #include <Protocol/MpService.h>
> >  #include <Guid/EventGroup.h>
> > @@ -45,42 +46,6 @@ typedef struct {
> >    IA32_DESCRIPTOR           IdtrProfile;
> >  } ACPI_CPU_DATA_EX;
> >
> > -/**
> > -  Allocate EfiACPIMemoryNVS below 4G memory address.
> > -
> > -  This function allocates EfiACPIMemoryNVS below 4G memory address.
> > -
> > -  @param[in] Size   Size of memory to allocate.
> > -
> > -  @return       Allocated address for output.
> > -
> > -**/
> > -VOID *
> > -AllocateAcpiNvsMemoryBelow4G (
> > -  IN UINTN  Size
> > -  )
> > -{
> > -  EFI_PHYSICAL_ADDRESS  Address;
> > -  EFI_STATUS            Status;
> > -  VOID                  *Buffer;
> > -
> > -  Address = BASE_4GB - 1;
> > -  Status  = gBS->AllocatePages (
> > -                   AllocateMaxAddress,
> > -                   EfiACPIMemoryNVS,
> > -                   EFI_SIZE_TO_PAGES (Size),
> > -                   &Address
> > -                   );
> > -  if (EFI_ERROR (Status)) {
> > -    return NULL;
> > -  }
> > -
> > -  Buffer = (VOID *)(UINTN)Address;
> > -  ZeroMem (Buffer, Size);
> > -
> > -  return Buffer;
> > -}
> > -
> >  /**
> >    Callback function executed when the EndOfDxe event group is signaled.
> >
> > @@ -150,7 +115,6 @@ CpuS3DataInitialize (
> >    EFI_MP_SERVICES_PROTOCOL   *MpServices;
> >    UINTN                      NumberOfCpus;
> >    UINTN                      NumberOfEnabledProcessors;
> > -  VOID                       *Stack;
> >    UINTN                      TableSize;
> >    CPU_REGISTER_TABLE         *RegisterTable;
> >    UINTN                      Index;
> > @@ -171,10 +135,7 @@ CpuS3DataInitialize (
> >    //
> >    OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
> > (PcdCpuS3DataAddress);
> >
> > -  //
> > -  // Allocate ACPI NVS memory below 4G memory for use on ACPI S3
> resume.
> > -  //
> > -  AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof
> > (ACPI_CPU_DATA_EX));
> > +  AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> > + (ACPI_CPU_DATA_EX)));
> 
> (2) In the original AllocateAcpiNvsMemoryBelow4G() function, we have a
> ZeroMem() call. For compatibility, I think we should call
> AllocateZeroPages() here.
> 
> (Previously, the trailing portion of the last page may not have been zeroed,
> but it's not a problem to zero more than before.)
> 

Agree, will use AllocateZeroPages.

> 
> (3) The "UefiCpuPkg/Include/AcpiCpuData.h" header states that
> ACPI_CPU_DATA (the structure itself) "must be allocated below 4GB from
> memory of type EfiACPIMemoryNVS".
> 
> If we have determined that this is no longer necessary, then:
> - we should first update the documentation in "AcpiCpuData.h" first,
> - the commit message should carefully explain why the change is valid.
> 

Agree, will do it.

> In particular, my concern is not about the normal boot path. (The normal boot
> path is explained for example in the message of commit 92b87f1c8c0b,
> "OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE",
> 2015-11-30.) I agree that copying from BootServicesData type memory into
> SMRAM is fine, during normal boot.
> 
> Instead, my concern is with the S3 resume path, when the data from the
> SMRAM buffer is *restored*. I seem to recall a case when the data was first
> restored from SMRAM to the original AcpiNVS allocation. 

I searched the code base and not found any code needs to restore the original AcpiNVS memory.

> If we make that
> area BootServicesData now, then the OS will reuse it, and then at
> S3 resume, we overwrite OS data.
> 

> ... After reviewing the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", I *think* this is a valid change,
> because GetAcpiCpuData() never remembers the address of the
> ACPI_CPU_DATA structure, or the addresses of its fields.
> 
> However, I would like Mike to review this as well (CC'ing him).
> 
> 
> (4) Because ACPI_CPU_DATA_EX contains MtrrTable, GdtrProfile and
> IdtrProfile too, not just ACPI_CPU_DATA, the above change moves all of those
> fields into BootServicesData type memory. In turn, the
> EFI_PHYSICAL_ADDRESS fields in ACPI_CPU_DATA that have the same names
> will point into BootServicesData type memory.
> 
> This conflicts with the requirements that are documented in
> "UefiCpuPkg/Include/AcpiCpuData.h" for all three fields: MtrrTable,
> GdtrProfile and IdtrProfile.
> 

I will add a separate patch to remove the memory type and below 4G limitation in the header file.

> If we want to change the allocation of these objects (of type MTRR_SETTINGS,
> IA32_DESCRIPTOR and IA32_DESCRIPTOR, respectively), then we should audit
> their use during S3 resume in the PiSmmCpuDxeSmm driver, and document
> them one-by-one.
> 
> ... Based on GetAcpiCpuData() again, I *think* this is valid change,
> functionally speaking, but I'd like Mike to review as well.
> 
> 
> >    ASSERT (AcpiCpuDataEx != NULL);
> >    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
> >
> > @@ -210,11 +171,16 @@ CpuS3DataInitialize (
> >    AcpiCpuData->MtrrTable    =
> (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
> >
> >    //
> > -  // Allocate stack space for all CPUs
> > +  // Allocate stack space for all CPUs, use ACPI NVS memory type
> > + because it will  // not copy to smram at Smm ready to lock point.
> >    //
> > -  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus *
> > AcpiCpuData->StackSize);
> > -  ASSERT (Stack != NULL);
> > -  AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
> > +  Status  = gBS->AllocatePages (
> > +                   AllocateAnyPages,
> > +                   EfiACPIMemoryNVS,
> > +                   EFI_SIZE_TO_PAGES (NumberOfCpus * AcpiCpuData-
> >StackSize),
> > +                   &AcpiCpuData->StackAddress
> > +                   );
> > +  ASSERT_EFI_ERROR (Status);
> 
> (5) The leading comment should be clarified. We should state that during
> S3 resume, the stack will only be used as scratch space, i.e. we won't read
> anything from it before we write to it, in PiSmmCpuDxeSemm.
> Otherwise, it would be a security bug to keep the area in AcpiNVS and not in
> SMRAM.

Agree, will update the comments.

> 
> 
> (6) This change requires a separate investigation from the rest of the patch,
> so it should be in a separate patch.
> 
> That implies we should first change this call site, and then remove
> AllocateAcpiNvsMemoryBelow4G() in a final patch.

Agree, will create a separate patch for this issue.

> 
> 
> (7) Why is it OK to lift the 4GB limitation? ... I think it may be valid indeed,
> because PrepareApStartupVector() stores StackAddress to "mExchangeInfo-
> >StackStart" (which has type (VOID*)), and because
> "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm" reads the latter with:
> 
>         add  edi, StackStartAddressLocation
>         add  rax, qword [edi]
>         mov  rsp, rax
>         mov  qword [edi], rax
> 
> in long-mode code. But, again, this should be documented in the
> (separate) patch's commit message.
> 
> 
> (8) The change breaks the documentation on "ACPI_CPU_DATA.StackAddress",
> in "UefiCpuPkg/Include/AcpiCpuData.h". The documentation should be
> updated.
> 
> 
> >
> >    //
> >    // Get the boot processor's GDT and IDT @@ -227,7 +193,7 @@
> > CpuS3DataInitialize (
> >    //
> >    GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
> >    IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> > -  Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
> > +  Gdt = AllocatePages (EFI_SIZE_TO_PAGES (GdtSize + IdtSize));
> >    ASSERT (Gdt != NULL);
> >    Idt = (VOID *)((UINTN)Gdt + GdtSize);
> >    CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
> 
> (9) This change breaks the requirements in
> "UefiCpuPkg/Include/AcpiCpuData.h" that both "ACPI_CPU_DATA.GdtrProfile-
> >Base" and "ACPI_CPU_DATA.IdtrProfile->Base"
> point into AcpiNVS data.
> 
> ("The buffer for GDT must also be allocated below 4GB from memory of type
> EfiACPIMemoryNVS" / "The buffer for IDT must also be allocated below 4GB
> from memory of type EfiACPIMemoryNVS".)
> 
> 
> (10) And, indeed, this is the bug that I suspected in point (3), with regard to
> the S3 resume path. Namely:
> 
> 
> Please consider the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c". This function:
> 
> (10a) copies the IDT and GDT *descriptors* into dynamically allocated
> SMRAM, pointed-to by the "mAcpiCpuData.GdtrProfile" and
> "mAcpiCpuData.IdtrProfile" fields;
> 
> (10b) copies the IDT and GDT themselves into dynamically allocated SMRAM,
> pointed-to by the "mGdtForAp" and "mIdtForAp" global variables,
> 
> (10c) *does not* update the "Base" members of the IDT and GDT descriptors
> that are saved in SMRAM in step (10a). Those continue to point to memory
> that was allocated by CpuS3DataDxe in the
> CpuS3DataInitialize() function.

Agree this is a bug, will create a separate patch to fix this issue.

> 
> 
> Now, consider the PrepareApStartupVector() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", which runs during S3 resume.
> That
> function:
> 
> (10d) copies the IDT and GDT descriptors that were saved in (10a) into
> "mExchangeInfo" -- this is just an SMRAM-to-SMRAM copy, however the Base
> members of the descriptors point to the original CpuS3DataDxe allocation,
> 
> (10e) the IDT and the GDT are themselves restored from step (10b), i.e.
> from the "mGdtForAp" and "mIdtForAp" SMRAM global variables, to the
> original CpuS3DataDxe allocation address. The comment says:
> 
> >   //
> >   // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI
> NVS memory
> >   //
> 
> In other words, while the IDT and GDT *descriptors* aren't restored in-place,
> the IDT and GDT themselves are.
> 
> This means that the above code change causes PiSmmCpuDxeSmm to
> overwrite OS memory at S3 resume.
> 
> 
> Continuing:
> 
> On 08/08/18 09:40, Eric Dong wrote:
> > @@ -243,7 +209,7 @@ CpuS3DataInitialize (
> >      // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
> >      //
> >      TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > -    RegisterTable = (CPU_REGISTER_TABLE
> *)AllocateAcpiNvsMemoryBelow4G (TableSize);
> > +    RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> >      ASSERT (RegisterTable != NULL);
> >
> >      for (Index = 0; Index < NumberOfCpus; Index++) {
> 
> (11) This looks valid, beyond breaking the documentation in
> "UefiCpuPkg/Include/AcpiCpuData.h", of the members
> "PreSmmInitRegisterTable" and "RegisterTable".
> 
> 
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > index 480c98ebcd..c16731529c 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > @@ -51,6 +51,7 @@
> >    DebugLib
> >    BaseLib
> >    MtrrLib
> > +  MemoryAllocationLib
> >
> >  [Guids]
> >    gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
> >
> 
> I feel uncomfortable about this patch. It is very tricky to review, and if we
> make mistakes, we could corrupt OS data, or (worse) perhaps even
> compromise SMRAM.
> 
> And, all the gains that I can imagine are, "save a few 32-bit AcpiNVS pages". I
> haven't measured, but it doesn't look like an attractive trade-off to me. And,
> platforms that disable S3 via "PcdAcpiS3Enable"
> don't allocate this memory anyway.
> 
> Furthermore, the memory footprint optimization does not seem related to the
> CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency issue that Marvin
> reported (and that is tracked in bug #959).
> 
> I suggest that we drop this patch.

We have reach the agreement that this is a "Not a correctly used" code bug, and it is caused by not do the related code clean when enable new features.  So I prefer to do this clean up to make code more clean.

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

答复: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

[Fan Jeff]

Hi,



ACPI NVS and 4G limitation are history reason and are necessary currently.  I agree to do such cleanup from header file and implementation.



Long term, could we think of combine CpuFeaturesDxe and CpuS3DataDxe?



Jeff



发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986>应用



________________________________
发件人: edk2-devel <edk2-devel-bounces@lists.01.org> 代表 Dong, Eric <eric.dong@intel.com>
发送时间: Friday, August 10, 2018 11:39:16 AM
收件人: 'Laszlo Ersek'
抄送: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org
主题: Re: [edk2] [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

Hi Laszlo,



> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, August 9, 2018 12:55 AM
> To: Dong, Eric <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change
> Memory Type and address limitation.
>
> Hello Eric,
>
> On 08/08/18 09:40, Eric Dong wrote:
> > Because CpuS3Data memory will be copy to smram at SmmReadToLock
> point,
> > so the memory type no need to be ACPI NVS type, also the address not
> > limit to below 4G.
> > This change remove the limit of ACPI NVS memory type and below 4G.
> >
> > Pass OS boot and resume from S3 test.
> >
> > Bugz: https://bugzilla.tianocore.org/show_bug.cgi?id=959
> >
> > Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
> > Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
>
> (1) I believe these tags don't apply to this patch -- this patch seems to be a
> new idea / addition.
>

Yes, will remove it in my next version patch.

>
> > Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
> > Cc: Fan Jeff <vanjeff_919@hotmail.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c      | 60 +++++++-----------------------
> --
> >  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 +
> >  2 files changed, 14 insertions(+), 47 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > index dccb406b8d..d8eb8c976f 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > @@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/DebugLib.h>
> >  #include <Library/MtrrLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> >
> >  #include <Protocol/MpService.h>
> >  #include <Guid/EventGroup.h>
> > @@ -45,42 +46,6 @@ typedef struct {
> >    IA32_DESCRIPTOR           IdtrProfile;
> >  } ACPI_CPU_DATA_EX;
> >
> > -/**
> > -  Allocate EfiACPIMemoryNVS below 4G memory address.
> > -
> > -  This function allocates EfiACPIMemoryNVS below 4G memory address.
> > -
> > -  @param[in] Size   Size of memory to allocate.
> > -
> > -  @return       Allocated address for output.
> > -
> > -**/
> > -VOID *
> > -AllocateAcpiNvsMemoryBelow4G (
> > -  IN UINTN  Size
> > -  )
> > -{
> > -  EFI_PHYSICAL_ADDRESS  Address;
> > -  EFI_STATUS            Status;
> > -  VOID                  *Buffer;
> > -
> > -  Address = BASE_4GB - 1;
> > -  Status  = gBS->AllocatePages (
> > -                   AllocateMaxAddress,
> > -                   EfiACPIMemoryNVS,
> > -                   EFI_SIZE_TO_PAGES (Size),
> > -                   &Address
> > -                   );
> > -  if (EFI_ERROR (Status)) {
> > -    return NULL;
> > -  }
> > -
> > -  Buffer = (VOID *)(UINTN)Address;
> > -  ZeroMem (Buffer, Size);
> > -
> > -  return Buffer;
> > -}
> > -
> >  /**
> >    Callback function executed when the EndOfDxe event group is signaled.
> >
> > @@ -150,7 +115,6 @@ CpuS3DataInitialize (
> >    EFI_MP_SERVICES_PROTOCOL   *MpServices;
> >    UINTN                      NumberOfCpus;
> >    UINTN                      NumberOfEnabledProcessors;
> > -  VOID                       *Stack;
> >    UINTN                      TableSize;
> >    CPU_REGISTER_TABLE         *RegisterTable;
> >    UINTN                      Index;
> > @@ -171,10 +135,7 @@ CpuS3DataInitialize (
> >    //
> >    OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
> > (PcdCpuS3DataAddress);
> >
> > -  //
> > -  // Allocate ACPI NVS memory below 4G memory for use on ACPI S3
> resume.
> > -  //
> > -  AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof
> > (ACPI_CPU_DATA_EX));
> > +  AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> > + (ACPI_CPU_DATA_EX)));
>
> (2) In the original AllocateAcpiNvsMemoryBelow4G() function, we have a
> ZeroMem() call. For compatibility, I think we should call
> AllocateZeroPages() here.
>
> (Previously, the trailing portion of the last page may not have been zeroed,
> but it's not a problem to zero more than before.)
>

Agree, will use AllocateZeroPages.

>
> (3) The "UefiCpuPkg/Include/AcpiCpuData.h" header states that
> ACPI_CPU_DATA (the structure itself) "must be allocated below 4GB from
> memory of type EfiACPIMemoryNVS".
>
> If we have determined that this is no longer necessary, then:
> - we should first update the documentation in "AcpiCpuData.h" first,
> - the commit message should carefully explain why the change is valid.
>

Agree, will do it.

> In particular, my concern is not about the normal boot path. (The normal boot
> path is explained for example in the message of commit 92b87f1c8c0b,
> "OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE",
> 2015-11-30.) I agree that copying from BootServicesData type memory into
> SMRAM is fine, during normal boot.
>
> Instead, my concern is with the S3 resume path, when the data from the
> SMRAM buffer is *restored*. I seem to recall a case when the data was first
> restored from SMRAM to the original AcpiNVS allocation.

I searched the code base and not found any code needs to restore the original AcpiNVS memory.

> If we make that
> area BootServicesData now, then the OS will reuse it, and then at
> S3 resume, we overwrite OS data.
>

> ... After reviewing the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", I *think* this is a valid change,
> because GetAcpiCpuData() never remembers the address of the
> ACPI_CPU_DATA structure, or the addresses of its fields.
>
> However, I would like Mike to review this as well (CC'ing him).
>
>
> (4) Because ACPI_CPU_DATA_EX contains MtrrTable, GdtrProfile and
> IdtrProfile too, not just ACPI_CPU_DATA, the above change moves all of those
> fields into BootServicesData type memory. In turn, the
> EFI_PHYSICAL_ADDRESS fields in ACPI_CPU_DATA that have the same names
> will point into BootServicesData type memory.
>
> This conflicts with the requirements that are documented in
> "UefiCpuPkg/Include/AcpiCpuData.h" for all three fields: MtrrTable,
> GdtrProfile and IdtrProfile.
>

I will add a separate patch to remove the memory type and below 4G limitation in the header file.

> If we want to change the allocation of these objects (of type MTRR_SETTINGS,
> IA32_DESCRIPTOR and IA32_DESCRIPTOR, respectively), then we should audit
> their use during S3 resume in the PiSmmCpuDxeSmm driver, and document
> them one-by-one.
>
> ... Based on GetAcpiCpuData() again, I *think* this is valid change,
> functionally speaking, but I'd like Mike to review as well.
>
>
> >    ASSERT (AcpiCpuDataEx != NULL);
> >    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
> >
> > @@ -210,11 +171,16 @@ CpuS3DataInitialize (
> >    AcpiCpuData->MtrrTable    =
> (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
> >
> >    //
> > -  // Allocate stack space for all CPUs
> > +  // Allocate stack space for all CPUs, use ACPI NVS memory type
> > + because it will  // not copy to smram at Smm ready to lock point.
> >    //
> > -  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus *
> > AcpiCpuData->StackSize);
> > -  ASSERT (Stack != NULL);
> > -  AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
> > +  Status  = gBS->AllocatePages (
> > +                   AllocateAnyPages,
> > +                   EfiACPIMemoryNVS,
> > +                   EFI_SIZE_TO_PAGES (NumberOfCpus * AcpiCpuData-
> >StackSize),
> > +                   &AcpiCpuData->StackAddress
> > +                   );
> > +  ASSERT_EFI_ERROR (Status);
>
> (5) The leading comment should be clarified. We should state that during
> S3 resume, the stack will only be used as scratch space, i.e. we won't read
> anything from it before we write to it, in PiSmmCpuDxeSemm.
> Otherwise, it would be a security bug to keep the area in AcpiNVS and not in
> SMRAM.

Agree, will update the comments.

>
>
> (6) This change requires a separate investigation from the rest of the patch,
> so it should be in a separate patch.
>
> That implies we should first change this call site, and then remove
> AllocateAcpiNvsMemoryBelow4G() in a final patch.

Agree, will create a separate patch for this issue.

>
>
> (7) Why is it OK to lift the 4GB limitation? ... I think it may be valid indeed,
> because PrepareApStartupVector() stores StackAddress to "mExchangeInfo-
> >StackStart" (which has type (VOID*)), and because
> "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm" reads the latter with:
>
>         add  edi, StackStartAddressLocation
>         add  rax, qword [edi]
>         mov  rsp, rax
>         mov  qword [edi], rax
>
> in long-mode code. But, again, this should be documented in the
> (separate) patch's commit message.
>
>
> (8) The change breaks the documentation on "ACPI_CPU_DATA.StackAddress",
> in "UefiCpuPkg/Include/AcpiCpuData.h". The documentation should be
> updated.
>
>
> >
> >    //
> >    // Get the boot processor's GDT and IDT @@ -227,7 +193,7 @@
> > CpuS3DataInitialize (
> >    //
> >    GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
> >    IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> > -  Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
> > +  Gdt = AllocatePages (EFI_SIZE_TO_PAGES (GdtSize + IdtSize));
> >    ASSERT (Gdt != NULL);
> >    Idt = (VOID *)((UINTN)Gdt + GdtSize);
> >    CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
>
> (9) This change breaks the requirements in
> "UefiCpuPkg/Include/AcpiCpuData.h" that both "ACPI_CPU_DATA.GdtrProfile-
> >Base" and "ACPI_CPU_DATA.IdtrProfile->Base"
> point into AcpiNVS data.
>
> ("The buffer for GDT must also be allocated below 4GB from memory of type
> EfiACPIMemoryNVS" / "The buffer for IDT must also be allocated below 4GB
> from memory of type EfiACPIMemoryNVS".)
>
>
> (10) And, indeed, this is the bug that I suspected in point (3), with regard to
> the S3 resume path. Namely:
>
>
> Please consider the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c". This function:
>
> (10a) copies the IDT and GDT *descriptors* into dynamically allocated
> SMRAM, pointed-to by the "mAcpiCpuData.GdtrProfile" and
> "mAcpiCpuData.IdtrProfile" fields;
>
> (10b) copies the IDT and GDT themselves into dynamically allocated SMRAM,
> pointed-to by the "mGdtForAp" and "mIdtForAp" global variables,
>
> (10c) *does not* update the "Base" members of the IDT and GDT descriptors
> that are saved in SMRAM in step (10a). Those continue to point to memory
> that was allocated by CpuS3DataDxe in the
> CpuS3DataInitialize() function.

Agree this is a bug, will create a separate patch to fix this issue.

>
>
> Now, consider the PrepareApStartupVector() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", which runs during S3 resume.
> That
> function:
>
> (10d) copies the IDT and GDT descriptors that were saved in (10a) into
> "mExchangeInfo" -- this is just an SMRAM-to-SMRAM copy, however the Base
> members of the descriptors point to the original CpuS3DataDxe allocation,
>
> (10e) the IDT and the GDT are themselves restored from step (10b), i.e.
> from the "mGdtForAp" and "mIdtForAp" SMRAM global variables, to the
> original CpuS3DataDxe allocation address. The comment says:
>
> >   //
> >   // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI
> NVS memory
> >   //
>
> In other words, while the IDT and GDT *descriptors* aren't restored in-place,
> the IDT and GDT themselves are.
>
> This means that the above code change causes PiSmmCpuDxeSmm to
> overwrite OS memory at S3 resume.
>
>
> Continuing:
>
> On 08/08/18 09:40, Eric Dong wrote:
> > @@ -243,7 +209,7 @@ CpuS3DataInitialize (
> >      // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
> >      //
> >      TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > -    RegisterTable = (CPU_REGISTER_TABLE
> *)AllocateAcpiNvsMemoryBelow4G (TableSize);
> > +    RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> >      ASSERT (RegisterTable != NULL);
> >
> >      for (Index = 0; Index < NumberOfCpus; Index++) {
>
> (11) This looks valid, beyond breaking the documentation in
> "UefiCpuPkg/Include/AcpiCpuData.h", of the members
> "PreSmmInitRegisterTable" and "RegisterTable".
>
>
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > index 480c98ebcd..c16731529c 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > @@ -51,6 +51,7 @@
> >    DebugLib
> >    BaseLib
> >    MtrrLib
> > +  MemoryAllocationLib
> >
> >  [Guids]
> >    gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
> >
>
> I feel uncomfortable about this patch. It is very tricky to review, and if we
> make mistakes, we could corrupt OS data, or (worse) perhaps even
> compromise SMRAM.
>
> And, all the gains that I can imagine are, "save a few 32-bit AcpiNVS pages". I
> haven't measured, but it doesn't look like an attractive trade-off to me. And,
> platforms that disable S3 via "PcdAcpiS3Enable"
> don't allocate this memory anyway.
>
> Furthermore, the memory footprint optimization does not seem related to the
> CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency issue that Marvin
> reported (and that is tracked in bug #959).
>
> I suggest that we drop this patch.

We have reach the agreement that this is a "Not a correctly used" code bug, and it is caused by not do the related code clean when enable new features.  So I prefer to do this clean up to make code more clean.

>
> 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

答复: 答复: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

[Fan Jeff]

Sorry. Correct some critical wording.



“ACPI NVS and 4G limitation are history reason and are NOT necessary currently.”



Jeff



________________________________
发件人: edk2-devel <edk2-devel-bounces@lists.01.org> 代表 Fan Jeff <vanjeff_919@hotmail.com>
发送时间: Friday, August 10, 2018 12:00:08 PM
收件人: Dong, Eric; 'Laszlo Ersek'
抄送: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org
主题: [edk2] 答复: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

Hi,



ACPI NVS and 4G limitation are history reason and are necessary currently.  I agree to do such cleanup from header file and implementation.



Long term, could we think of combine CpuFeaturesDxe and CpuS3DataDxe?



Jeff



发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986>应用



________________________________
发件人: edk2-devel <edk2-devel-bounces@lists.01.org> 代表 Dong, Eric <eric.dong@intel.com>
发送时间: Friday, August 10, 2018 11:39:16 AM
收件人: 'Laszlo Ersek'
抄送: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org
主题: Re: [edk2] [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

Hi Laszlo,



> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, August 9, 2018 12:55 AM
> To: Dong, Eric <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change
> Memory Type and address limitation.
>
> Hello Eric,
>
> On 08/08/18 09:40, Eric Dong wrote:
> > Because CpuS3Data memory will be copy to smram at SmmReadToLock
> point,
> > so the memory type no need to be ACPI NVS type, also the address not
> > limit to below 4G.
> > This change remove the limit of ACPI NVS memory type and below 4G.
> >
> > Pass OS boot and resume from S3 test.
> >
> > Bugz: https://bugzilla.tianocore.org/show_bug.cgi?id=959
> >
> > Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
> > Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
>
> (1) I believe these tags don't apply to this patch -- this patch seems to be a
> new idea / addition.
>

Yes, will remove it in my next version patch.

>
> > Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
> > Cc: Fan Jeff <vanjeff_919@hotmail.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c      | 60 +++++++-----------------------
> --
> >  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 +
> >  2 files changed, 14 insertions(+), 47 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > index dccb406b8d..d8eb8c976f 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > @@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/DebugLib.h>
> >  #include <Library/MtrrLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> >
> >  #include <Protocol/MpService.h>
> >  #include <Guid/EventGroup.h>
> > @@ -45,42 +46,6 @@ typedef struct {
> >    IA32_DESCRIPTOR           IdtrProfile;
> >  } ACPI_CPU_DATA_EX;
> >
> > -/**
> > -  Allocate EfiACPIMemoryNVS below 4G memory address.
> > -
> > -  This function allocates EfiACPIMemoryNVS below 4G memory address.
> > -
> > -  @param[in] Size   Size of memory to allocate.
> > -
> > -  @return       Allocated address for output.
> > -
> > -**/
> > -VOID *
> > -AllocateAcpiNvsMemoryBelow4G (
> > -  IN UINTN  Size
> > -  )
> > -{
> > -  EFI_PHYSICAL_ADDRESS  Address;
> > -  EFI_STATUS            Status;
> > -  VOID                  *Buffer;
> > -
> > -  Address = BASE_4GB - 1;
> > -  Status  = gBS->AllocatePages (
> > -                   AllocateMaxAddress,
> > -                   EfiACPIMemoryNVS,
> > -                   EFI_SIZE_TO_PAGES (Size),
> > -                   &Address
> > -                   );
> > -  if (EFI_ERROR (Status)) {
> > -    return NULL;
> > -  }
> > -
> > -  Buffer = (VOID *)(UINTN)Address;
> > -  ZeroMem (Buffer, Size);
> > -
> > -  return Buffer;
> > -}
> > -
> >  /**
> >    Callback function executed when the EndOfDxe event group is signaled.
> >
> > @@ -150,7 +115,6 @@ CpuS3DataInitialize (
> >    EFI_MP_SERVICES_PROTOCOL   *MpServices;
> >    UINTN                      NumberOfCpus;
> >    UINTN                      NumberOfEnabledProcessors;
> > -  VOID                       *Stack;
> >    UINTN                      TableSize;
> >    CPU_REGISTER_TABLE         *RegisterTable;
> >    UINTN                      Index;
> > @@ -171,10 +135,7 @@ CpuS3DataInitialize (
> >    //
> >    OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
> > (PcdCpuS3DataAddress);
> >
> > -  //
> > -  // Allocate ACPI NVS memory below 4G memory for use on ACPI S3
> resume.
> > -  //
> > -  AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof
> > (ACPI_CPU_DATA_EX));
> > +  AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> > + (ACPI_CPU_DATA_EX)));
>
> (2) In the original AllocateAcpiNvsMemoryBelow4G() function, we have a
> ZeroMem() call. For compatibility, I think we should call
> AllocateZeroPages() here.
>
> (Previously, the trailing portion of the last page may not have been zeroed,
> but it's not a problem to zero more than before.)
>

Agree, will use AllocateZeroPages.

>
> (3) The "UefiCpuPkg/Include/AcpiCpuData.h" header states that
> ACPI_CPU_DATA (the structure itself) "must be allocated below 4GB from
> memory of type EfiACPIMemoryNVS".
>
> If we have determined that this is no longer necessary, then:
> - we should first update the documentation in "AcpiCpuData.h" first,
> - the commit message should carefully explain why the change is valid.
>

Agree, will do it.

> In particular, my concern is not about the normal boot path. (The normal boot
> path is explained for example in the message of commit 92b87f1c8c0b,
> "OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE",
> 2015-11-30.) I agree that copying from BootServicesData type memory into
> SMRAM is fine, during normal boot.
>
> Instead, my concern is with the S3 resume path, when the data from the
> SMRAM buffer is *restored*. I seem to recall a case when the data was first
> restored from SMRAM to the original AcpiNVS allocation.

I searched the code base and not found any code needs to restore the original AcpiNVS memory.

> If we make that
> area BootServicesData now, then the OS will reuse it, and then at
> S3 resume, we overwrite OS data.
>

> ... After reviewing the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", I *think* this is a valid change,
> because GetAcpiCpuData() never remembers the address of the
> ACPI_CPU_DATA structure, or the addresses of its fields.
>
> However, I would like Mike to review this as well (CC'ing him).
>
>
> (4) Because ACPI_CPU_DATA_EX contains MtrrTable, GdtrProfile and
> IdtrProfile too, not just ACPI_CPU_DATA, the above change moves all of those
> fields into BootServicesData type memory. In turn, the
> EFI_PHYSICAL_ADDRESS fields in ACPI_CPU_DATA that have the same names
> will point into BootServicesData type memory.
>
> This conflicts with the requirements that are documented in
> "UefiCpuPkg/Include/AcpiCpuData.h" for all three fields: MtrrTable,
> GdtrProfile and IdtrProfile.
>

I will add a separate patch to remove the memory type and below 4G limitation in the header file.

> If we want to change the allocation of these objects (of type MTRR_SETTINGS,
> IA32_DESCRIPTOR and IA32_DESCRIPTOR, respectively), then we should audit
> their use during S3 resume in the PiSmmCpuDxeSmm driver, and document
> them one-by-one.
>
> ... Based on GetAcpiCpuData() again, I *think* this is valid change,
> functionally speaking, but I'd like Mike to review as well.
>
>
> >    ASSERT (AcpiCpuDataEx != NULL);
> >    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
> >
> > @@ -210,11 +171,16 @@ CpuS3DataInitialize (
> >    AcpiCpuData->MtrrTable    =
> (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
> >
> >    //
> > -  // Allocate stack space for all CPUs
> > +  // Allocate stack space for all CPUs, use ACPI NVS memory type
> > + because it will  // not copy to smram at Smm ready to lock point.
> >    //
> > -  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus *
> > AcpiCpuData->StackSize);
> > -  ASSERT (Stack != NULL);
> > -  AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
> > +  Status  = gBS->AllocatePages (
> > +                   AllocateAnyPages,
> > +                   EfiACPIMemoryNVS,
> > +                   EFI_SIZE_TO_PAGES (NumberOfCpus * AcpiCpuData-
> >StackSize),
> > +                   &AcpiCpuData->StackAddress
> > +                   );
> > +  ASSERT_EFI_ERROR (Status);
>
> (5) The leading comment should be clarified. We should state that during
> S3 resume, the stack will only be used as scratch space, i.e. we won't read
> anything from it before we write to it, in PiSmmCpuDxeSemm.
> Otherwise, it would be a security bug to keep the area in AcpiNVS and not in
> SMRAM.

Agree, will update the comments.

>
>
> (6) This change requires a separate investigation from the rest of the patch,
> so it should be in a separate patch.
>
> That implies we should first change this call site, and then remove
> AllocateAcpiNvsMemoryBelow4G() in a final patch.

Agree, will create a separate patch for this issue.

>
>
> (7) Why is it OK to lift the 4GB limitation? ... I think it may be valid indeed,
> because PrepareApStartupVector() stores StackAddress to "mExchangeInfo-
> >StackStart" (which has type (VOID*)), and because
> "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm" reads the latter with:
>
>         add  edi, StackStartAddressLocation
>         add  rax, qword [edi]
>         mov  rsp, rax
>         mov  qword [edi], rax
>
> in long-mode code. But, again, this should be documented in the
> (separate) patch's commit message.
>
>
> (8) The change breaks the documentation on "ACPI_CPU_DATA.StackAddress",
> in "UefiCpuPkg/Include/AcpiCpuData.h". The documentation should be
> updated.
>
>
> >
> >    //
> >    // Get the boot processor's GDT and IDT @@ -227,7 +193,7 @@
> > CpuS3DataInitialize (
> >    //
> >    GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
> >    IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> > -  Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
> > +  Gdt = AllocatePages (EFI_SIZE_TO_PAGES (GdtSize + IdtSize));
> >    ASSERT (Gdt != NULL);
> >    Idt = (VOID *)((UINTN)Gdt + GdtSize);
> >    CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
>
> (9) This change breaks the requirements in
> "UefiCpuPkg/Include/AcpiCpuData.h" that both "ACPI_CPU_DATA.GdtrProfile-
> >Base" and "ACPI_CPU_DATA.IdtrProfile->Base"
> point into AcpiNVS data.
>
> ("The buffer for GDT must also be allocated below 4GB from memory of type
> EfiACPIMemoryNVS" / "The buffer for IDT must also be allocated below 4GB
> from memory of type EfiACPIMemoryNVS".)
>
>
> (10) And, indeed, this is the bug that I suspected in point (3), with regard to
> the S3 resume path. Namely:
>
>
> Please consider the GetAcpiCpuData() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c". This function:
>
> (10a) copies the IDT and GDT *descriptors* into dynamically allocated
> SMRAM, pointed-to by the "mAcpiCpuData.GdtrProfile" and
> "mAcpiCpuData.IdtrProfile" fields;
>
> (10b) copies the IDT and GDT themselves into dynamically allocated SMRAM,
> pointed-to by the "mGdtForAp" and "mIdtForAp" global variables,
>
> (10c) *does not* update the "Base" members of the IDT and GDT descriptors
> that are saved in SMRAM in step (10a). Those continue to point to memory
> that was allocated by CpuS3DataDxe in the
> CpuS3DataInitialize() function.

Agree this is a bug, will create a separate patch to fix this issue.

>
>
> Now, consider the PrepareApStartupVector() function in
> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", which runs during S3 resume.
> That
> function:
>
> (10d) copies the IDT and GDT descriptors that were saved in (10a) into
> "mExchangeInfo" -- this is just an SMRAM-to-SMRAM copy, however the Base
> members of the descriptors point to the original CpuS3DataDxe allocation,
>
> (10e) the IDT and the GDT are themselves restored from step (10b), i.e.
> from the "mGdtForAp" and "mIdtForAp" SMRAM global variables, to the
> original CpuS3DataDxe allocation address. The comment says:
>
> >   //
> >   // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI
> NVS memory
> >   //
>
> In other words, while the IDT and GDT *descriptors* aren't restored in-place,
> the IDT and GDT themselves are.
>
> This means that the above code change causes PiSmmCpuDxeSmm to
> overwrite OS memory at S3 resume.
>
>
> Continuing:
>
> On 08/08/18 09:40, Eric Dong wrote:
> > @@ -243,7 +209,7 @@ CpuS3DataInitialize (
> >      // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
> >      //
> >      TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > -    RegisterTable = (CPU_REGISTER_TABLE
> *)AllocateAcpiNvsMemoryBelow4G (TableSize);
> > +    RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
> >      ASSERT (RegisterTable != NULL);
> >
> >      for (Index = 0; Index < NumberOfCpus; Index++) {
>
> (11) This looks valid, beyond breaking the documentation in
> "UefiCpuPkg/Include/AcpiCpuData.h", of the members
> "PreSmmInitRegisterTable" and "RegisterTable".
>
>
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > index 480c98ebcd..c16731529c 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> > @@ -51,6 +51,7 @@
> >    DebugLib
> >    BaseLib
> >    MtrrLib
> > +  MemoryAllocationLib
> >
> >  [Guids]
> >    gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
> >
>
> I feel uncomfortable about this patch. It is very tricky to review, and if we
> make mistakes, we could corrupt OS data, or (worse) perhaps even
> compromise SMRAM.
>
> And, all the gains that I can imagine are, "save a few 32-bit AcpiNVS pages". I
> haven't measured, but it doesn't look like an attractive trade-off to me. And,
> platforms that disable S3 via "PcdAcpiS3Enable"
> don't allocate this memory anyway.
>
> Furthermore, the memory footprint optimization does not seem related to the
> CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency issue that Marvin
> reported (and that is tracked in bug #959).
>
> I suggest that we drop this patch.

We have reach the agreement that this is a "Not a correctly used" code bug, and it is caused by not do the related code clean when enable new features.  So I prefer to do this clean up to make code more clean.

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