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

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

Please ignore my 2nd question.
I saw " AddMemoryRangeHob (BASE_1MB, LowerMemorySize - TsegSize);" when
PcdSmmSmramRequire is TRUE.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, April 25, 2023 9:49 PM
> To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/5] SimicsOpenBoardPkg: Build
> gEfiSmmSmramMemoryGuid Hob in S3 path
> 
> 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
> 
> 
> 
> 
>