Re: [PATCH 1/5] SimicsOpenBoardPkg: Build gEfiSmmSmramMemoryGuid Hob in S3 path

["Ni, Ray" <ray.ni@intel.com>]

Zhiguang,
Can you please keep the comments that explain why below 1MB memory resource
should be added for QSP platform?

Another question not related to your changes:
  why "AddMemoryRangeHob (BASE_1MB, LowerMemorySize);" is only called when
  PcdSmmSmramRequire is FALSE?

Thanks,
Ray

> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Tuesday, April 25, 2023 3:03 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH 1/5] SimicsOpenBoardPkg: Build
> gEfiSmmSmramMemoryGuid Hob in S3 path
> 
> gEfiSmmSmramMemoryGuid Hob is needed for SmmRelocation feature
> even for S3 path. So in MemDetect.c, remove specical code path
> for S3 about creating gEfiSmmSmramMemoryGuid Hob and adding some
> memory descriptor, which does no harm in S3 path.
> 
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  .../SimicsOpenBoardPkg/SimicsPei/MemDetect.c  | 107 +++++++-----------
>  1 file changed, 42 insertions(+), 65 deletions(-)
> 
> diff --git a/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> b/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> index 127afffc00..d80ac1d213 100644
> --- a/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> +++ b/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> @@ -405,79 +405,56 @@ QemuInitializeRam (
>    LowerMemorySize = GetSystemMemorySizeBelow4gb ();
>    UpperMemorySize = GetSystemMemorySizeAbove4gb ();
> 
> -  if (mBootMode == BOOT_ON_S3_RESUME) {
> -    //
> -    // Create the following memory HOB as an exception on the S3 boot path.
> -    //
> -    // Normally we'd create memory HOBs only on the normal boot path.
> However,
> -    // CpuMpPei specifically needs such a low-memory HOB on the S3 path as
> -    // well, for "borrowing" a subset of it temporarily, for the AP startup
> -    // vector.
> -    //
> -    // CpuMpPei saves the original contents of the borrowed area in
> permanent
> -    // PEI RAM, in a backup buffer allocated with the normal PEI services.
> -    // CpuMpPei restores the original contents ("returns" the borrowed area)
> at
> -    // End-of-PEI. End-of-PEI in turn is emitted by S3Resume2Pei before
> -    // transferring control to the OS's wakeup vector in the FACS.
> -    //
> -    // We expect any other PEIMs that "borrow" memory similarly to
> CpuMpPei to
> -    // restore the original contents. Furthermore, we expect all such PEIMs
> -    // (CpuMpPei included) to claim the borrowed areas by producing
> memory
> -    // allocation HOBs, and to honor preexistent memory allocation HOBs
> when
> -    // looking for an area to borrow.
> -    //
> -    AddMemoryRangeHob (0, BASE_512KB + BASE_128KB);
> -  } else {
> -    //
> -    // Create memory HOBs
> -    //
> -    AddMemoryRangeHob (0, BASE_512KB + BASE_128KB);
> +  //
> +  // Create memory HOBs
> +  //
> +  AddMemoryRangeHob (0, BASE_512KB + BASE_128KB);
> 
> -    if (FeaturePcdGet (PcdSmmSmramRequire)) {
> -      UINT32 TsegSize;
> +  if (FeaturePcdGet (PcdSmmSmramRequire)) {
> +    UINT32 TsegSize;
> 
> -      TsegSize = mX58TsegMbytes * SIZE_1MB;
> -      AddMemoryRangeHob (BASE_1MB, LowerMemorySize - TsegSize);
> -      AddReservedMemoryBaseSizeHob (LowerMemorySize - TsegSize,
> TsegSize,
> -        TRUE);
> +    TsegSize = mX58TsegMbytes * SIZE_1MB;
> +    AddMemoryRangeHob (BASE_1MB, LowerMemorySize - TsegSize);
> +    AddReservedMemoryBaseSizeHob (LowerMemorySize - TsegSize,
> TsegSize,
> +      TRUE);
> 
> -	  BufferSize = sizeof(EFI_SMRAM_HOB_DESCRIPTOR_BLOCK);
> -	  SmramRanges = 1;
> +    BufferSize = sizeof(EFI_SMRAM_HOB_DESCRIPTOR_BLOCK);
> +    SmramRanges = 1;
> 
> -      Hob.Raw = BuildGuidHob(
> -          &gEfiSmmSmramMemoryGuid,
> -          BufferSize
> -      );
> -      ASSERT(Hob.Raw);
> -
> -      SmramHobDescriptorBlock = (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK
> *)(Hob.Raw);
> -      SmramHobDescriptorBlock->NumberOfSmmReservedRegions =
> SmramRanges;
> -
> -      SmramIndex = 0;
> -      for (Index = 0; Index < SmramRanges; Index++) {
> -        //
> -        // This is an SMRAM range, create an SMRAM descriptor
> -        //
> -        SmramHobDescriptorBlock->Descriptor[SmramIndex].PhysicalStart =
> LowerMemorySize - TsegSize;
> -        SmramHobDescriptorBlock->Descriptor[SmramIndex].CpuStart =
> LowerMemorySize - TsegSize;
> -        SmramHobDescriptorBlock->Descriptor[SmramIndex].PhysicalSize =
> TsegSize;
> -        SmramHobDescriptorBlock->Descriptor[SmramIndex].RegionState =
> EFI_SMRAM_CLOSED | EFI_CACHEABLE;
> -        SmramIndex++;
> -      }
> +    Hob.Raw = BuildGuidHob(
> +        &gEfiSmmSmramMemoryGuid,
> +        BufferSize
> +    );
> +    ASSERT(Hob.Raw);
> 
> -    } else {
> -      AddMemoryRangeHob (BASE_1MB, LowerMemorySize);
> -    }
> +    SmramHobDescriptorBlock = (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK
> *)(Hob.Raw);
> +    SmramHobDescriptorBlock->NumberOfSmmReservedRegions =
> SmramRanges;
> 
> -    //
> -    // If QEMU presents an E820 map, then create memory HOBs for
> the >=4GB RAM
> -    // entries. Otherwise, create a single memory HOB with the flat >=4GB
> -    // memory size read from the CMOS.
> -    //
> -    if (UpperMemorySize != 0) {
> -      AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
> +    SmramIndex = 0;
> +    for (Index = 0; Index < SmramRanges; Index++) {
> +      //
> +      // This is an SMRAM range, create an SMRAM descriptor
> +      //
> +      SmramHobDescriptorBlock->Descriptor[SmramIndex].PhysicalStart =
> LowerMemorySize - TsegSize;
> +      SmramHobDescriptorBlock->Descriptor[SmramIndex].CpuStart =
> LowerMemorySize - TsegSize;
> +      SmramHobDescriptorBlock->Descriptor[SmramIndex].PhysicalSize =
> TsegSize;
> +      SmramHobDescriptorBlock->Descriptor[SmramIndex].RegionState =
> EFI_SMRAM_CLOSED | EFI_CACHEABLE;
> +      SmramIndex++;
>      }
> +
> +  } else {
> +    AddMemoryRangeHob (BASE_1MB, LowerMemorySize);
>    }
> +
> +  //
> +  // If QEMU presents an E820 map, then create memory HOBs for
> the >=4GB RAM
> +  // entries. Otherwise, create a single memory HOB with the flat >=4GB
> +  // memory size read from the CMOS.
> +  //
> +  if (UpperMemorySize != 0) {
> +    AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
> +  }
> +
>  }
> 
>  /**
> --
> 2.31.1.windows.1