From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.93; helo=mga11.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A49D12119307D for ; Sun, 25 Nov 2018 17:28:21 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Nov 2018 17:28:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,280,1539673200"; d="scan'208";a="109225140" Received: from shzintpr01.sh.intel.com (HELO [10.253.24.48]) ([10.239.4.80]) by fmsmga004.fm.intel.com with ESMTP; 25 Nov 2018 17:28:20 -0800 To: Liming Gao , edk2-devel@lists.01.org Cc: Dandan Bi , star.zeng@intel.com References: <20181122143250.29968-1-liming.gao@intel.com> From: "Zeng, Star" Message-ID: Date: Mon, 26 Nov 2018 09:27:49 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181122143250.29968-1-liming.gao@intel.com> Subject: Re: [Patch] MdeModulePkg PCD: Add DynamicEx PcdVpdBaseAddress64 for non SPI platform X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Nov 2018 01:28:21 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Star Zeng > Cc: Jian J Wang > Cc: Dandan Bi Two minor comments at below, with them handled, Reviewed-by: Star Zeng > --- > 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 >