Re: [PATCH v3 2/3] Platform/AMD/MinBoardPkg: Adds SetCacheMtrrLib library

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]

Hi Abdul, one comment below,

> -----Original Message-----
> From: Abdul Lateef Attar <abdattar@amd.com>
> Sent: Thursday, March 23, 2023 2:14 PM
> To: devel@edk2.groups.io
> Cc: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Chang, Abner <Abner.Chang@amd.com>;
> Michael D Kinney <michael.d.kinney@intel.com>
> Subject: [PATCH v3 2/3] Platform/AMD/MinBoardPkg: Adds SetCacheMtrrLib
> library
> 
> Adds SetCacheMtrrLib library for MinBoardPkg, which sets MTRR values for
> PEI phase and also modifies the MTRR value at the end of PEI phase.
> 
> Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  Platform/AMD/MinBoardPkg/MinBoardPkg.dsc      |   9 ++
>  .../SetCacheMtrrLib/SetCacheMtrrLib.inf       |  37 +++++
>  .../Library/SetCacheMtrrLib/SetCacheMtrrLib.c | 133 ++++++++++++++++++
>  3 files changed, 179 insertions(+)
>  create mode 100644
> Platform/AMD/MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.inf
>  create mode 100644
> Platform/AMD/MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.c
> 
> diff --git a/Platform/AMD/MinBoardPkg/MinBoardPkg.dsc
> b/Platform/AMD/MinBoardPkg/MinBoardPkg.dsc
> index 8c120c0649e7..810fac7aa9de 100644
> --- a/Platform/AMD/MinBoardPkg/MinBoardPkg.dsc
> +++ b/Platform/AMD/MinBoardPkg/MinBoardPkg.dsc
> @@ -17,5 +17,14 @@ [Defines]
>    SUPPORTED_ARCHITECTURES     = IA32 | X64
> 
>  [Packages]
> +  MdePkg/MdePkg.dec
>    MinBoardPkg/MinBoardPkg.dec
> +  MinPlatformPkg/MinPlatformPkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses.common.PEIM]
> +
> +SetCacheMtrrLib|MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.i
> nf
> +
> +[Components.IA32]
> +  MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.inf
> 
> diff --git
> a/Platform/AMD/MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.in
> f
> b/Platform/AMD/MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.in
> f
> new file mode 100644
> index 000000000000..add2e71651f9
> --- /dev/null
> +++
> b/Platform/AMD/MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.i
> +++ nf
> @@ -0,0 +1,37 @@
> +## @file
> +# Component information file for Platform SetCacheMtrr Library.
> +# This library implementation is for AMD processor based platforms.
> +#
> +# Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> +reserved.<BR> # # SPDX-License-Identifier: BSD-2-Clause-Patent # ##
> +
> +[Defines]
> +  INF_VERSION                    = 1.29
> +  BASE_NAME                      = PeiSetCacheMtrrLib
> +  FILE_GUID                      = 1E8468E0-5EB4-4088-9B52-BFDC6E4DAE87
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = SetCacheMtrrLib
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  MtrrLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MinPlatformPkg/MinPlatformPkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[Sources]
> +  SetCacheMtrrLib.c
> +
> +[Guids]
We can remove this section if no reference to any GUIDs.
You don't have to send another version of patch to address this comment, just update this file before you pushing it to edk2-platforms.

Reviewed-by: Abner Chang <abner.chang@amd.com>
Thanks
Abner
> +
> +[Pcd]
> +  gMinPlatformPkgTokenSpaceGuid.PcdFlashAreaBaseAddress
> +  gMinPlatformPkgTokenSpaceGuid.PcdFlashAreaSize
> +
> diff --git
> a/Platform/AMD/MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.c
> b/Platform/AMD/MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.c
> new file mode 100644
> index 000000000000..33b774fedbd3
> --- /dev/null
> +++
> b/Platform/AMD/MinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.c
> @@ -0,0 +1,133 @@
> +/** @file
> +
> +SetCacheMtrr library functions.
> +This library implementation is for AMD processor based platforms.
> +
> +Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> +reserved.<BR>
> +
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi.h>
> +#include <PiPei.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MtrrLib.h>
> +
> +/**
> +  This function sets the cache MTRR values for PEI phase.
> +**/
> +VOID
> +EFIAPI
> +SetCacheMtrr (
> +  VOID
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = MtrrSetMemoryAttribute (
> +             0,
> +             0xA0000,
> +             CacheWriteBack
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "Error(%r) in setting CacheWriteBack for 0-0x9FFFF\n",
> +      Status
> +      ));
> +  }
> +
> +  Status = MtrrSetMemoryAttribute (
> +             0xA0000,
> +             0x20000,
> +             CacheUncacheable
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "Error(%r) in setting CacheUncacheable for 0xA0000-0xBFFFF\n",
> +      Status
> +      ));
> +  }
> +
> +  Status = MtrrSetMemoryAttribute (
> +             0xC0000,
> +             0x40000,
> +             CacheWriteProtected
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "Error(%r) in setting CacheWriteProtected for 0xC0000-0xFFFFF\n",
> +      Status
> +      ));
> +  }
> +
> +  Status = MtrrSetMemoryAttribute (
> +             0x100000,
> +             0xAFF00000,
> +             CacheWriteBack
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "Error(%r) in setting CacheWriteBack for 0x100000-0xAFFFFFFF\n",
> +      Status
> +      ));
> +  }
> +
> +  Status = MtrrSetMemoryAttribute (
> +             PcdGet32 (PcdFlashAreaBaseAddress),
> +             PcdGet32 (PcdFlashAreaSize),
> +             CacheWriteProtected
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "Error(%r) in setting CacheWriteProtected for 0x%X-0x%X\n",
> +      Status,
> +      PcdGet32 (PcdFlashAreaBaseAddress),
> +      PcdGet32 (PcdFlashAreaBaseAddress) + PcdGet32 (PcdFlashAreaSize)
> +      ));
> +  }
> +
> +  MtrrDebugPrintAllMtrrs ();
> +  return;
> +}
> +
> +/**
> +  Update MTRR setting in EndOfPei phase.
> +  This function will set the MTRR value as CacheUncacheable
> +  for Flash address.
> +
> +  @retval  EFI_SUCCESS  The function completes successfully.
> +  @retval  Others       Some error occurs.
> +**/
> +EFI_STATUS
> +EFIAPI
> +SetCacheMtrrAfterEndOfPei (
> +  VOID
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = MtrrSetMemoryAttribute (
> +             PcdGet32 (PcdFlashAreaBaseAddress),
> +             PcdGet32 (PcdFlashAreaSize),
> +             CacheUncacheable
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "Error(%r) in setting CacheUncacheable for 0x%X-0x%X\n",
> +      Status,
> +      PcdGet32 (PcdFlashAreaBaseAddress),
> +      PcdGet32 (PcdFlashAreaBaseAddress) + PcdGet32 (PcdFlashAreaSize)
> +      ));
> +  }
> +
> +  MtrrDebugPrintAllMtrrs ();
> +  return EFI_SUCCESS;
> +}
> +
> --
> 2.25.1