Re: [edk2-devel] [PATCH 2/5] SimicsOpenBoardPkg: Move AcpiVariableGuid hob to MemDetect

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

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Zhiguang Liu
> 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: [edk2-devel] [PATCH 2/5] SimicsOpenBoardPkg: Move
> AcpiVariableGuid hob to MemDetect
> 
> Currently, MemDetect create gEfiSmmSmramMemoryGuid Hob containing
> one
> descriptor, which should be updated later, when AcpiVariableGuid hob
> use some buffer from SmRam. However, the Hob doesn't get updated, and
> this is a bug.
> 
> Move the logic creating AcpiVariableGuid hob from PEIM SmmAccessPei.inf
> to MemDetect, so that in the same file, it has both knowleage about
> the smmram and the acpi data structure. So it can create the
> gEfiSmmSmramMemoryGuid Hob containing two descriptors.
> 
> 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  | 36 +++++++++++--------
>  .../SimicsPei/SimicsPei.inf                   |  1 +
>  .../SimicsX58SktPkg/Smm/Access/SmmAccessPei.c |  8 -----
>  .../Smm/Access/SmmAccessPei.inf               |  3 --
>  4 files changed, 22 insertions(+), 26 deletions(-)
> 
> diff --git a/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> b/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> index d80ac1d213..13ee415f40 100644
> --- a/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> +++ b/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/MemDetect.c
> @@ -391,11 +391,10 @@ QemuInitializeRam (
>    UINT64                               LowerMemorySize;
>    UINT64                               UpperMemorySize;
>    UINTN                                 BufferSize;
> -  UINT8                                 SmramIndex;
>    UINT8                                 SmramRanges;
>    EFI_PEI_HOB_POINTERS                  Hob;
>    EFI_SMRAM_HOB_DESCRIPTOR_BLOCK        *SmramHobDescriptorBlock;
> -  UINT8                                 Index;
> +  VOID                                  *GuidHob;
> 
>    DEBUG ((EFI_D_INFO, "%a called\n", __FUNCTION__));
> 
> @@ -418,8 +417,8 @@ QemuInitializeRam (
>      AddReservedMemoryBaseSizeHob (LowerMemorySize - TsegSize,
> TsegSize,
>        TRUE);
> 
> -    BufferSize = sizeof(EFI_SMRAM_HOB_DESCRIPTOR_BLOCK);
> -    SmramRanges = 1;
> +    SmramRanges = 2;
> +    BufferSize = sizeof(EFI_SMRAM_HOB_DESCRIPTOR_BLOCK) +
> (SmramRanges - 1) * sizeof(EFI_SMRAM_DESCRIPTOR);
> 
>      Hob.Raw = BuildGuidHob(
>          &gEfiSmmSmramMemoryGuid,
> @@ -430,18 +429,25 @@ QemuInitializeRam (
>      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++;
> -    }
> +    //
> +    // Create first SMRAM descriptor, which contains data structures used in
> S3 resume.
> +    // One page is enough for the data structure
> +    //
> +    SmramHobDescriptorBlock->Descriptor[0].PhysicalStart =
> LowerMemorySize - TsegSize;
> +    SmramHobDescriptorBlock->Descriptor[0].CpuStart = LowerMemorySize -
> TsegSize;
> +    SmramHobDescriptorBlock->Descriptor[0].PhysicalSize = EFI_PAGE_SIZE;
> +    SmramHobDescriptorBlock->Descriptor[0].RegionState =
> EFI_SMRAM_CLOSED | EFI_CACHEABLE | EFI_ALLOCATED;
> +    GuidHob = BuildGuidHob (&gEfiAcpiVariableGuid,
> sizeof(EFI_SMRAM_DESCRIPTOR));
> +    ASSERT (GuidHob != NULL);
> +    CopyMem (GuidHob, &SmramHobDescriptorBlock->Descriptor[0],
> sizeof(EFI_SMRAM_DESCRIPTOR));
> 
> +    //
> +    // Create second SMRAM descriptor, which is free and will be used by
> SMM foundation.
> +    //
> +    SmramHobDescriptorBlock->Descriptor[1].PhysicalStart =
> SmramHobDescriptorBlock->Descriptor[0].PhysicalStart + EFI_PAGE_SIZE;
> +    SmramHobDescriptorBlock->Descriptor[1].CpuStart =
> SmramHobDescriptorBlock->Descriptor[0].CpuStart + EFI_PAGE_SIZE;
> +    SmramHobDescriptorBlock->Descriptor[1].PhysicalSize = TsegSize -
> EFI_PAGE_SIZE;
> +    SmramHobDescriptorBlock->Descriptor[1].RegionState =
> EFI_SMRAM_CLOSED | EFI_CACHEABLE;
>    } else {
>      AddMemoryRangeHob (BASE_1MB, LowerMemorySize);
>    }
> diff --git a/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/SimicsPei.inf
> b/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/SimicsPei.inf
> index 710fa680be..618ad4075f 100644
> --- a/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/SimicsPei.inf
> +++ b/Platform/Intel/SimicsOpenBoardPkg/SimicsPei/SimicsPei.inf
> @@ -40,6 +40,7 @@
>  [Guids]
>    gEfiMemoryTypeInformationGuid
>    gEfiSmmSmramMemoryGuid              ## CONSUMES
> +  gEfiAcpiVariableGuid
> 
>  [LibraryClasses]
>    BaseLib
> diff --git a/Silicon/Intel/SimicsX58SktPkg/Smm/Access/SmmAccessPei.c
> b/Silicon/Intel/SimicsX58SktPkg/Smm/Access/SmmAccessPei.c
> index c54026b4d1..d489cc7513 100644
> --- a/Silicon/Intel/SimicsX58SktPkg/Smm/Access/SmmAccessPei.c
> +++ b/Silicon/Intel/SimicsX58SktPkg/Smm/Access/SmmAccessPei.c
> @@ -241,7 +241,6 @@ SmmAccessPeiEntryPoint (
>    EFI_STATUS           Status;
>    UINTN                SmramMapSize;
>    EFI_SMRAM_DESCRIPTOR SmramMap[DescIdxCount];
> -  VOID                 *GuidHob;
> 
>    //
>    // This module should only be included if SMRAM support is required.
> @@ -322,13 +321,6 @@ SmmAccessPeiEntryPoint (
>    }
>    DEBUG_CODE_END ();
> 
> -  GuidHob = BuildGuidHob (&gEfiAcpiVariableGuid, sizeof
> SmramMap[DescIdxSmmS3ResumeState]);
> -  if (GuidHob == NULL) {
> -    return EFI_OUT_OF_RESOURCES;
> -  }
> -
> -  CopyMem (GuidHob, &SmramMap[DescIdxSmmS3ResumeState], sizeof
> SmramMap[DescIdxSmmS3ResumeState]);
> -
>    //
>    // We're done. The next step should succeed, but even if it fails, we can't
>    // roll back the above BuildGuidHob() allocation, because PEI doesn't
> support
> diff --git a/Silicon/Intel/SimicsX58SktPkg/Smm/Access/SmmAccessPei.inf
> b/Silicon/Intel/SimicsX58SktPkg/Smm/Access/SmmAccessPei.inf
> index 2b6b14f437..3c71e64fe9 100644
> --- a/Silicon/Intel/SimicsX58SktPkg/Smm/Access/SmmAccessPei.inf
> +++ b/Silicon/Intel/SimicsX58SktPkg/Smm/Access/SmmAccessPei.inf
> @@ -38,9 +38,6 @@
>    SimicsX58SktPkg/SktPkg.dec
>    SimicsIch10Pkg/Ich10Pkg.dec
> 
> -[Guids]
> -  gEfiAcpiVariableGuid
> -
>  [LibraryClasses]
>    BaseLib
>    BaseMemoryLib
> --
> 2.31.1.windows.1
> 
> 
> 
> 
>