Re: [PATCH] MinPlatformPkg: Add PcdFlashMicrocodeOffset

["Chiu, Chasel" <chasel.chiu@intel.com>]

Please see my comments below inline.

Thanks,
Chasel


> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, April 8, 2021 4:07 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Dong, Eric <eric.dong@intel.com>
> Subject: [PATCH] MinPlatformPkg: Add PcdFlashMicrocodeOffset
> 
> Add PcdFlashMicrocodeOffset in MinPlatformPkg.dec and update
> SecFspWrapperPlatformSecLib library to use the microcode location PCDs
> defined in MinPlatformPkg.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> ---
>  .../SecFspWrapperPlatformSecLib.inf                       | 8 ++++----
>  .../Library/SecFspWrapperPlatformSecLib/SecRamInitData.c  | 6 +++---
>  Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec          | 3 ++-
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatform
> SecLib/SecFspWrapperPlatformSecLib.inf
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatform
> SecLib/SecFspWrapperPlatformSecLib.inf
> index 4f3fa9fa34..68ce5d81cd 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatform
> SecLib/SecFspWrapperPlatformSecLib.inf
> +++ b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> +++ formSecLib/SecFspWrapperPlatformSecLib.inf
> @@ -1,7 +1,7 @@
>  ## @file #  Provide FSP wrapper platform sec related function. #-#  Copyright (c)
> 2017 - 2019, Intel Corporation. All rights reserved.<BR>+#  Copyright (c) 2017 -
> 2021, Intel Corporation. All rights reserved.<BR> # # SPDX-License-Identifier:
> BSD-2-Clause-Patent #@@ -88,9 +88,9 @@
>    gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable           ##
> CONSUMES  [FixedPcd]-
> gIntelFsp2WrapperTokenSpaceGuid.PcdCpuMicrocodePatchAddress         ##
> CONSUMES-
> gIntelFsp2WrapperTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize      ##
> CONSUMES-  gIntelFsp2WrapperTokenSpaceGuid.PcdFlashMicrocodeOffset
> ## CONSUMES+  gMinPlatformPkgTokenSpaceGuid.PcdFlashFvMicrocodeBase
> ## CONSUMES+  gMinPlatformPkgTokenSpaceGuid.PcdFlashFvMicrocodeSize
> ## CONSUMES+  gMinPlatformPkgTokenSpaceGuid.PcdFlashMicrocodeOffset
> ## CONSUMES   gIntelFsp2WrapperTokenSpaceGuid.PcdFlashCodeCacheAddress
> ## CONSUMES   gIntelFsp2WrapperTokenSpaceGuid.PcdFlashCodeCacheSize
> ## CONSUMES   gIntelFsp2WrapperTokenSpaceGuid.PcdFspmBaseAddress
> ## CONSUMESdiff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatform
> SecLib/SecRamInitData.c
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatform
> SecLib/SecRamInitData.c
> index b356327b4c..b4e10cca1f 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatform
> SecLib/SecRamInitData.c
> +++ b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> +++ formSecLib/SecRamInitData.c
> @@ -1,7 +1,7 @@
>  /** @file   Provide TempRamInitParams data. -Copyright (c) 2017, Intel
> Corporation. All rights reserved.<BR>+Copyright (c) 2017 - 2021, Intel
> Corporation. All rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-
> Patent  **/@@ -24,8 +24,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED CONST
> FSPT_UPD_CORE_DATA FsptUpdDataPtr = {
>      }   },   {-    ((UINT32)FixedPcdGet64 (PcdCpuMicrocodePatchAddress) +
> FixedPcdGet32 (PcdFlashMicrocodeOffset)),-    ((UINT32)FixedPcdGet64
> (PcdCpuMicrocodePatchRegionSize) - FixedPcdGet32
> (PcdFlashMicrocodeOffset)),+    FixedPcdGet32 (PcdFlashFvMicrocodeBase) +
> FixedPcdGet32 (PcdFlashMicrocodeOffset),+    FixedPcdGet32
> (PcdFlashFvMicrocodeSize) - FixedPcdGet32 (PcdFlashMicrocodeOffset),     0,
> // Set CodeRegionBase as 0, so that caching will be 4GB-(CodeRegionSize >
> LLCSize ? LLCSize : CodeRegionSize) will be used.     FixedPcdGet32
> (PcdFlashCodeCacheSize),     { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00,diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> index 2b246cf0ac..1c9ae665a6 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> @@ -6,7 +6,7 @@
>  # INF files to generate AutoGen.c and AutoGen.h files # for the build
> infrastructure. #-# Copyright (c) 2017 - 2020, Intel Corporation. All rights
> reserved.<BR>+# Copyright (c) 2017 - 2021, Intel Corporation. All rights
> reserved.<BR> # # SPDX-License-Identifier: BSD-2-Clause-Patent #@@ -165,6
> +165,7 @@
> 
> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvMicrocodeBase|0xFFE60000|UINT
> 32|0x30000004
> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvMicrocodeSize|0x000A0000|UINT
> 32|0x30000005
> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvMicrocodeOffset|0x00660000|UI
> NT32|0x30000006+
> gMinPlatformPkgTokenSpaceGuid.PcdFlashMicrocodeOffset|0x60|UINT32|0x3
> 0000007

1. Maybe we can align with IntelFsp2WrapperPkg which have this PCD default value as 0x90 for typical cases?
2. Since we already have MicrocodeOffset PCD there, to prevent from confusing, how about renaming it to something like PcdFlashMicrocodeBypassBytes?
3. Please add comments to describe this particular PCD meaning, because it is different from others (flash map definition PCD)

> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvPreMemoryBase|0x00000000|UIN
> T32|0x20000004
> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvPreMemorySize|0x00000000|UIN
> T32|0x20000005--
> 2.27.0.windows.1