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

["Gao, Liming" <liming.gao@intel.com>]

Good catch. I will update the patch. 

Thanks
Liming
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, November 26, 2018 9:28 AM
> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> Cc: Bi, Dandan <dandan.bi@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [Patch] MdeModulePkg PCD: Add DynamicEx PcdVpdBaseAddress64 for non SPI platform
> 
> 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
> >