Re: [Patch] MdeModulePkg PCD: Add DynamicEx PcdVpdBaseAddress64 for non SPI platform

["Zeng, Star" <star.zeng@intel.com>]

On 2018/11/22 22:32, Liming Gao wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1356
> Current PcdVpdBaseAddress is 32bit static Pcd. NON SPI platform needs to
> configure it as Dynamic PCD. Emulator platform (such as NT32) may set its
> value to 64bit address.
> To meet with this usage, 64bit DynamicEx PcdVpdBaseAddress64 is introduced.
> If its value is not zero, it will be used.
> If its value is zero, static PcdVpdBaseAddress will be used.
> When NON SPI platform enables VPD PCD, they need to set PcdVpdBaseAddress64.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>

Two minor comments at below, with them handled, Reviewed-by: Star Zeng 
<star.zeng@intel.com>


> ---
>   MdeModulePkg/Universal/PCD/Dxe/Pcd.c     | 16 ++++++++++++++++
>   MdeModulePkg/Universal/PCD/Dxe/Service.c |  3 ++-
>   MdeModulePkg/Universal/PCD/Pei/Service.c | 15 ++++++++++++++-
>   MdeModulePkg/MdeModulePkg.dec            |  8 ++++++++
>   MdeModulePkg/Universal/PCD/Dxe/Pcd.inf   |  3 ++-
>   MdeModulePkg/Universal/PCD/Dxe/Service.h |  2 ++
>   MdeModulePkg/Universal/PCD/Pei/Pcd.inf   |  3 ++-
>   7 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/PCD/Dxe/Pcd.c b/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
> index f977c7f18e..4e38a3844a 100644
> --- a/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
> +++ b/MdeModulePkg/Universal/PCD/Dxe/Pcd.c
> @@ -109,6 +109,7 @@ EFI_GET_PCD_INFO_PROTOCOL  mEfiGetPcdInfoInstance = {
>   };
>   
>   EFI_HANDLE mPcdHandle = NULL;
> +UINTN      mVpdBaseAddress = 0;
>   
>   /**
>     Main entry for PCD DXE driver.
> @@ -175,6 +176,21 @@ PcdDxeInit (
>       &Registration
>       );
>   
> +  //
> +  // Cache VpdBaseAddress in entry point for the following usage.
> +  //
> +
> +  //
> +  // PcdVpdBaseAddress64 is DynamicEx PCD only. So, DxePcdGet64Ex() is used to get its value.
> +  //
> +  mVpdBaseAddress = (UINTN) DxePcdGet64Ex (&gEfiMdeModulePkgTokenSpaceGuid, PcdToken (PcdVpdBaseAddress64));
> +  if (mVpdBaseAddress == 0) {
> +    //
> +    // PcdVpdBaseAddress64 is not set, get value from PcdVpdBaseAddress.
> +    //
> +    mVpdBaseAddress = (UINTN) PcdGet32 (PcdVpdBaseAddress);
> +  }
> +
>     return Status;
>   }
>   
> diff --git a/MdeModulePkg/Universal/PCD/Dxe/Service.c b/MdeModulePkg/Universal/PCD/Dxe/Service.c
> index 0517152366..4b44153e13 100644
> --- a/MdeModulePkg/Universal/PCD/Dxe/Service.c
> +++ b/MdeModulePkg/Universal/PCD/Dxe/Service.c
> @@ -435,7 +435,8 @@ GetWorker (
>     switch (LocalTokenNumber & PCD_TYPE_ALL_SET) {
>       case PCD_TYPE_VPD:
>         VpdHead = (VPD_HEAD *) ((UINT8 *) PcdDb + Offset);
> -      RetPtr = (VOID *) ((UINTN) PcdGet32 (PcdVpdBaseAddress) + VpdHead->Offset);
> +      ASSERT (mVpdBaseAddress != 0);
> +      RetPtr = (VOID *) (mVpdBaseAddress + VpdHead->Offset);
>   
>         break;
>   
> diff --git a/MdeModulePkg/Universal/PCD/Pei/Service.c b/MdeModulePkg/Universal/PCD/Pei/Service.c
> index bb4b52baf3..e1613e8af8 100644
> --- a/MdeModulePkg/Universal/PCD/Pei/Service.c
> +++ b/MdeModulePkg/Universal/PCD/Pei/Service.c
> @@ -861,6 +861,7 @@ GetWorker (
>     UINT32              LocalTokenNumber;
>     UINT32              LocalTokenCount;
>     UINT8               *VaraiableDefaultBuffer;
> +  UINTN               VpdBaseAddress;
>   
>     //
>     // TokenNumber Zero is reserved as PCD_INVALID_TOKEN_NUMBER.
> @@ -889,7 +890,19 @@ GetWorker (
>       {
>         VPD_HEAD *VpdHead;
>         VpdHead = (VPD_HEAD *) ((UINT8 *)PeiPcdDb + Offset);
> -      return (VOID *) ((UINTN) PcdGet32 (PcdVpdBaseAddress) + VpdHead->Offset);
> +
> +      //
> +      // PcdVpdBaseAddress64 is DynamicEx PCD only. So, PeiPcdGet64Ex() is used to get its value.
> +      //
> +      VpdBaseAddress = (UINTN) PeiPcdGet64Ex (&gEfiMdeModulePkgTokenSpaceGuid, PcdToken (PcdVpdBaseAddress64));
> +      if (VpdBaseAddress == 0) {
> +        //
> +        // PcdVpdBaseAddress64 is not set, get value from PcdVpdBaseAddress.
> +        //
> +        VpdBaseAddress = (UINTN) PcdGet32 (PcdVpdBaseAddress);
> +      }
> +      ASSERT (VpdBaseAddress != 0);
> +      return (VOID *)(VpdBaseAddress + VpdHead->Offset);
>       }
>   
>       case PCD_TYPE_HII|PCD_TYPE_STRING:
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 0abacc1a90..f46a4094ca 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -2022,5 +2022,13 @@
>     # @Prompt NV Storage Default Value Buffer
>     gEfiMdeModulePkgTokenSpaceGuid.PcdNvStoreDefaultValueBuffer|{0x0}|VOID*|0x00030005
>   
> +  ## VPD type PCD allows a developer to point to an absolute physical address PcdVpdBaseAddress

1. PcdVpdBaseAddress should be PcdVpdBaseAddress64, right?

> +  #  to store PCD value. It will be DynamicExDefault only.
> +  #  It is used to set VPD region base address. So, it can't be DynamicExVpd PCD. Its value is
> +  #  required to be accessed in PcdDxe driver entry point. So, its value must be set in PEI phase.
> +  #  It can't depend on EFI variable service, and can't be DynamicExHii PCD.
> +  # @Prompt 64bit VPD base address.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress64|0x0|UINT64|0x00030006

Please update MdeModulePkg.uni accordingly for this new PCD also.


Thanks,
Star

> +
>   [UserExtensions.TianoCore."ExtraFiles"]
>     MdeModulePkgExtra.uni
> diff --git a/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf b/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> index 1f41a316bd..4ba78e46a3 100644
> --- a/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> +++ b/MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> @@ -73,7 +73,7 @@
>   #
>   #      c) OEM specificed storage area:
>   #         - The PCD value is stored in OEM specified area which base address is
> -#           specified by a FixedAtBuild PCD setting - PcdVpdBaseAddress.
> +#           specified by PCD setting - PcdVpdBaseAddress64 or PcdVpdBaseAddress.
>   #         - The area is read only for PEI and DXE phase.
>   #         - [PcdsDynamicVpd] is used as section name for this type PCD in platform
>   #           DSC file. [PcdsDynamicExVpd] is for dynamicex type PCD.
> @@ -344,6 +344,7 @@
>   
>   [Pcd]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress      ## SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress64    ## SOMETIMES_CONSUMES
>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNvStoreDefaultId ## SOMETIMES_CONSUMES
>   
>   [Depex]
> diff --git a/MdeModulePkg/Universal/PCD/Dxe/Service.h b/MdeModulePkg/Universal/PCD/Dxe/Service.h
> index ddd5fa471e..3055e30cd1 100644
> --- a/MdeModulePkg/Universal/PCD/Dxe/Service.h
> +++ b/MdeModulePkg/Universal/PCD/Dxe/Service.h
> @@ -48,6 +48,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>     #error "Please make sure the version of PCD DXE Service and the generated PCD DXE Database match."
>   #endif
>   
> +extern UINTN     mVpdBaseAddress;
> +
>   /**
>     Retrieve additional information associated with a PCD token in the default token space.
>   
> diff --git a/MdeModulePkg/Universal/PCD/Pei/Pcd.inf b/MdeModulePkg/Universal/PCD/Pei/Pcd.inf
> index 6e28fce8fa..63b125d48a 100644
> --- a/MdeModulePkg/Universal/PCD/Pei/Pcd.inf
> +++ b/MdeModulePkg/Universal/PCD/Pei/Pcd.inf
> @@ -72,7 +72,7 @@
>   #
>   #      c) OEM specificed storage area:
>   #         - The PCD value is stored in OEM specified area which base address is
> -#           specified by a FixedAtBuild PCD setting - PcdVpdBaseAddress.
> +#           specified by PCD setting - PcdVpdBaseAddress64 or PcdVpdBaseAddress.
>   #         - The area is read only for PEI and DXE phase.
>   #         - [PcdsDynamicVpd] is used as section name for this type PCD in platform
>   #           DSC file. [PcdsDynamicExVpd] is for dynamicex type PCD.
> @@ -344,6 +344,7 @@
>   
>   [Pcd]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress ## SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress64 ## SOMETIMES_CONSUMES
>     gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPcdCallBackNumberPerPcdEntry ## SOMETIMES_CONSUMES
>     gEfiMdeModulePkgTokenSpaceGuid.PcdNvStoreDefaultValueBuffer ## SOMETIMES_CONSUMES
>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNvStoreDefaultId       ## CONSUMES
>