* Re: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library [not found] ` <734D49CCEBEEF84792F5B80ED585239D5C352F1D@SHSMSX104.ccr.corp.intel.com> @ 2019-11-07 8:14 ` Ethan Tsao 2019-11-08 6:25 ` Ni, Ray 0 siblings, 1 reply; 7+ messages in thread From: Ethan Tsao @ 2019-11-07 8:14 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V Hi Ray, Thanks your good input. I just review all debug message and most print is for error report purpose, such as allocate memory failure,...etc. >From my opinion, this kind debug message is useful for BIOS when unexpected error happen. In normal case, it will not be print. Best Regards, Ethan > -----Original Message----- > From: Ni, Ray <ray.ni@intel.com> > Sent: Thursday, November 7, 2019 3:15 PM > To: Tsao, Ethan <ethan.tsao@intel.com>; edk2-devel@lists.01.org > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library > > Given the patch only moves the code from one place to another, I am ok with > that. > > By the way, is it still valuable to have so many debug messages everywhere in > this library? > If no, can we remove them or at least some of them? > > Debug messages are valuable I agree. But we also need to think about producing > helpful debug messages, not treated by platform developers as noise : ) > > > > -----Original Message----- > > From: Tsao, Ethan <ethan.tsao@intel.com> > > Sent: Thursday, November 7, 2019 11:31 AM > > To: edk2-devel@lists.01.org > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray > > <ray.ni@intel.com> > > Subject: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > Library > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2318 > > > > Establish one copy of Config blocks library class and instance in > > IntelSiliconPkg and remove copies from other silicon packages , like > > KabyLakeSiliconPkg, CoffelakeSiliconPkg. > > > > Signed-off-by: Ethan Tsao <ethan.tsao@intel.com> > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > --- > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlo > > ckLib.c | 146 ----------------------------------------------------------------- > - > > ---------------------------------------------------------------------- > > ---------- > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlo > > ckLib.inf | 29 ----------------------------- > > Silicon/Intel/{KabylakeSiliconPkg => > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | 0 > > Silicon/Intel/{KabylakeSiliconPkg => > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf | 0 > > 4 files changed, 175 deletions(-) > > > > diff --git > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > nfigB > > lockLib.c > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > nfig > > BlockLib.c > > deleted file mode 100644 > > index 369dab97ee..0000000000 > > --- > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > nfigB > > lockLib.c > > +++ /dev/null > > @@ -1,146 +0,0 @@ > > -/** @file > > - Library functions for Config Block management. > > - > > - Copyright (c) 2019 Intel Corporation. All rights reserved. <BR> > > - > > - SPDX-License-Identifier: BSD-2-Clause-Patent -**/ > > - > > -#include <ConfigBlock.h> > > -#include <Library/ConfigBlockLib.h> > > -#include <Library/BaseMemoryLib.h> > > -#include <Library/MemoryAllocationLib.h> -#include > > <Library/DebugLib.h> > > - > > -/** > > - Create config block table > > - > > - @param[in] TotalSize - Max size to be allocated for the Config > > Block Table > > - @param[out] ConfigBlockTableAddress - On return, points to a pointer > > to the beginning of Config Block Table Address > > - > > - @retval EFI_INVALID_PARAMETER - Invalid Parameter > > - @retval EFI_OUT_OF_RESOURCES - Out of resources > > - @retval EFI_SUCCESS - Successfully created Config Block Table at > > ConfigBlockTableAddress > > -**/ > > -EFI_STATUS > > -EFIAPI > > -CreateConfigBlockTable ( > > - IN UINT16 TotalSize, > > - OUT VOID **ConfigBlockTableAddress > > - ) > > -{ > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > - UINT32 ConfigBlkTblHdrSize; > > - > > - ConfigBlkTblHdrSize = (UINT32)(sizeof (CONFIG_BLOCK_TABLE_HEADER)); > > - > > - if (TotalSize <= (ConfigBlkTblHdrSize + sizeof (CONFIG_BLOCK_HEADER))) { > > - DEBUG ((DEBUG_ERROR, "Invalid Parameter\n")); > > - return EFI_INVALID_PARAMETER; > > - } > > - > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER *)AllocateZeroPool > > (TotalSize); > > - if (ConfigBlkTblAddrPtr == NULL) { > > - DEBUG ((DEBUG_ERROR, "Could not allocate memory.\n")); > > - return EFI_OUT_OF_RESOURCES; > > - } > > - ConfigBlkTblAddrPtr->NumberOfBlocks = 0; > > - ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength = TotalSize; > > - ConfigBlkTblAddrPtr->AvailableSize = TotalSize - > > ConfigBlkTblHdrSize; > > - > > - *ConfigBlockTableAddress = (VOID *)ConfigBlkTblAddrPtr; > > - > > - return EFI_SUCCESS; > > -} > > - > > -/** > > - Add config block into config block table structure > > - > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning of > > Config Block Table Address > > - @param[out] ConfigBlockAddress - On return, points to a pointer to > > the beginning of Config Block Address > > - > > - @retval EFI_OUT_OF_RESOURCES - Config Block Table is full and > > cannot add new Config Block or > > - Config Block Offset Table is full and cannot add new Config > > Block. > > - @retval EFI_SUCCESS - Successfully added Config Block > > -**/ > > -EFI_STATUS > > -EFIAPI > > -AddConfigBlock ( > > - IN VOID *ConfigBlockTableAddress, > > - OUT VOID **ConfigBlockAddress > > - ) > > -{ > > - CONFIG_BLOCK *TempConfigBlk; > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > - CONFIG_BLOCK *ConfigBlkAddrPtr; > > - UINT16 ConfigBlkSize; > > - > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > *)ConfigBlockTableAddress; > > - ConfigBlkAddrPtr = (CONFIG_BLOCK *)(*ConfigBlockAddress); > > - ConfigBlkSize = ConfigBlkAddrPtr->Header.GuidHob.Header.HobLength; > > - DEBUG ((DEBUG_INFO, "Config Block GUID: %g / Config Block Size: > > 0x%x bytes\n", &(ConfigBlkAddrPtr->Header.GuidHob.Name), > > ConfigBlkSize)); > > - if ((ConfigBlkSize % 4) != 0) { > > - DEBUG ((DEBUG_ERROR, "Config Block must be multiples of 4 bytes\n")); > > - return EFI_INVALID_PARAMETER; > > - } > > - if (ConfigBlkTblAddrPtr->AvailableSize < ConfigBlkSize) { > > - DEBUG ((DEBUG_ERROR, "Config Block Table is full and cannot add new > > Config Block.\n")); > > - DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes / > > Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr- > > >AvailableSize, ConfigBlkSize)); > > - return EFI_OUT_OF_RESOURCES; > > - } > > - > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + > > (UINTN)(ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength - > > ConfigBlkTblAddrPtr->AvailableSize)); > > - CopyMem (&TempConfigBlk->Header, &ConfigBlkAddrPtr->Header, > > sizeof(CONFIG_BLOCK_HEADER)); > > - > > - ConfigBlkTblAddrPtr->NumberOfBlocks++; > > - ConfigBlkTblAddrPtr->AvailableSize = > > ConfigBlkTblAddrPtr->AvailableSize - ConfigBlkSize; > > - > > - *ConfigBlockAddress = (VOID *) TempConfigBlk; > > - DEBUG ((DEBUG_INFO, "Config Block Address: 0x%x / Available Config > > Block Size: 0x%x bytes\n", (UINT32)(UINTN)*ConfigBlockAddress, > > ConfigBlkTblAddrPtr->AvailableSize)); > > - return EFI_SUCCESS; > > -} > > - > > -/** > > - Retrieve a specific Config Block data by GUID > > - > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning of > > Config Block Table Address > > - @param[in] ConfigBlockGuid - A pointer to the GUID uses to > > search specific Config Block > > - @param[out] ConfigBlockAddress - On return, points to a pointer to > > the beginning of Config Block Address > > - > > - @retval EFI_NOT_FOUND - Could not find the Config Block > > - @retval EFI_SUCCESS - Config Block found and return > > -**/ > > -EFI_STATUS > > -EFIAPI > > -GetConfigBlock ( > > - IN VOID *ConfigBlockTableAddress, > > - IN EFI_GUID *ConfigBlockGuid, > > - OUT VOID **ConfigBlockAddress > > - ) > > -{ > > - UINT16 OffsetIndex; > > - CONFIG_BLOCK *TempConfigBlk; > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > - UINT32 ConfigBlkTblHdrSize; > > - UINT32 ConfigBlkOffset; > > - UINT16 NumOfBlocks; > > - > > - ConfigBlkTblHdrSize = (UINT32)(sizeof (CONFIG_BLOCK_TABLE_HEADER)); > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > *)ConfigBlockTableAddress; > > - NumOfBlocks = ConfigBlkTblAddrPtr->NumberOfBlocks; > > - > > - ConfigBlkOffset = 0; > > - for (OffsetIndex = 0; OffsetIndex < NumOfBlocks; OffsetIndex++) { > > - if ((ConfigBlkTblHdrSize + ConfigBlkOffset) > (ConfigBlkTblAddrPtr- > > >Header.GuidHob.Header.HobLength)) { > > - break; > > - } > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + > > (UINTN)ConfigBlkTblHdrSize + (UINTN)ConfigBlkOffset); > > - if (CompareGuid (&(TempConfigBlk->Header.GuidHob.Name), > > ConfigBlockGuid)) { > > - *ConfigBlockAddress = (VOID *)TempConfigBlk; > > - return EFI_SUCCESS; > > - } > > - ConfigBlkOffset = ConfigBlkOffset + TempConfigBlk- > > >Header.GuidHob.Header.HobLength; > > - } > > - DEBUG ((DEBUG_ERROR, "Could not find the config block.\n")); > > - return EFI_NOT_FOUND; > > -} > > diff --git > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > nfigB > > lockLib.inf > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > nfig > > BlockLib.inf > > deleted file mode 100644 > > index a7def2481d..0000000000 > > --- > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > nfigB > > lockLib.inf > > +++ /dev/null > > @@ -1,29 +0,0 @@ > > -## @file > > -# Component INF file for the BaseConfigBlock library. > > -# > > -# Copyright (c) 2019 Intel Corporation. All rights reserved. <BR> -# > > -# SPDX-License-Identifier: BSD-2-Clause-Patent -# -## > > - > > -[Defines] > > -INF_VERSION = 0x00010017 > > -BASE_NAME = BaseConfigBlockLib > > -FILE_GUID = 1EC07EA8-7808-4e06-9D79-309AE331D2D5 > > -VERSION_STRING = 1.0 > > -MODULE_TYPE = BASE > > -LIBRARY_CLASS = ConfigBlockLib > > - > > - > > -[Packages] > > -MdePkg/MdePkg.dec > > -CoffeelakeSiliconPkg/SiPkg.dec > > - > > -[Sources] > > -BaseConfigBlockLib.c > > - > > -[LibraryClasses] > > -DebugLib > > -BaseMemoryLib > > -MemoryAllocationLib > > diff --git > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConf > > igBlo > > ckLib.c > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigB > > lockLi > > b.c > > similarity index 100% > > rename from > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfig > > Block > > Lib.c > > rename to > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlo > > ckLib.c > > diff --git > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConf > > igBlo > > ckLib.inf > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigB > > lockLi > > b.inf > > similarity index 100% > > rename from > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfig > > Block > > Lib.inf > > rename to > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlo > > ckLib.i > > nf > > -- > > 2.16.2.windows.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library 2019-11-07 8:14 ` [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library Ethan Tsao @ 2019-11-08 6:25 ` Ni, Ray 2019-11-08 7:17 ` Ethan Tsao 0 siblings, 1 reply; 7+ messages in thread From: Ni, Ray @ 2019-11-08 6:25 UTC (permalink / raw) To: Tsao, Ethan, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V Ethan, What if all of the debug message are removed? Library especially very fundamental like ConfigBlock is better to be quite. It's the ConfigBlock API designer's responsibility to ensure the error is returned properly to caller, otherwise it's the API's bug. When the API can return correct status out, it's caller's responsibility to check the status and do proper handling of the errors. With the debug message added in this fundamental library, it may give callers wrong impression that the library itself can handle the error and callers don't need to check the error status. If we search code in MdePkg, quite few debug messages are printed out from library code. Thanks, Ray > -----Original Message----- > From: Tsao, Ethan <ethan.tsao@intel.com> > Sent: Thursday, November 7, 2019 4:14 PM > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library > > Hi Ray, > Thanks your good input. I just review all debug message and most print is for > error report purpose, such as allocate memory failure,...etc. > From my opinion, this kind debug message is useful for BIOS when > unexpected error happen. In normal case, it will not be print. > > Best Regards, > Ethan > > > -----Original Message----- > > From: Ni, Ray <ray.ni@intel.com> > > Sent: Thursday, November 7, 2019 3:15 PM > > To: Tsao, Ethan <ethan.tsao@intel.com>; edk2-devel@lists.01.org > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library > > > > Given the patch only moves the code from one place to another, I am ok > with > > that. > > > > By the way, is it still valuable to have so many debug messages everywhere > in > > this library? > > If no, can we remove them or at least some of them? > > > > Debug messages are valuable I agree. But we also need to think about > producing > > helpful debug messages, not treated by platform developers as noise : ) > > > > > > > -----Original Message----- > > > From: Tsao, Ethan <ethan.tsao@intel.com> > > > Sent: Thursday, November 7, 2019 11:31 AM > > > To: edk2-devel@lists.01.org > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray > > > <ray.ni@intel.com> > > > Subject: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > > Library > > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2318 > > > > > > Establish one copy of Config blocks library class and instance in > > > IntelSiliconPkg and remove copies from other silicon packages , like > > > KabyLakeSiliconPkg, CoffelakeSiliconPkg. > > > > > > Signed-off-by: Ethan Tsao <ethan.tsao@intel.com> > > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> > > > Cc: Ray Ni <ray.ni@intel.com> > > > --- > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlo > > > ckLib.c | 146 ------------------------------------------------------------- > ---- > > - > > > ---------------------------------------------------------------------- > > > ---------- > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlo > > > ckLib.inf | 29 ----------------------------- > > > Silicon/Intel/{KabylakeSiliconPkg => > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | 0 > > > Silicon/Intel/{KabylakeSiliconPkg => > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf | 0 > > > 4 files changed, 175 deletions(-) > > > > > > diff --git > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > nfigB > > > lockLib.c > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > nfig > > > BlockLib.c > > > deleted file mode 100644 > > > index 369dab97ee..0000000000 > > > --- > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > nfigB > > > lockLib.c > > > +++ /dev/null > > > @@ -1,146 +0,0 @@ > > > -/** @file > > > - Library functions for Config Block management. > > > - > > > - Copyright (c) 2019 Intel Corporation. All rights reserved. <BR> > > > - > > > - SPDX-License-Identifier: BSD-2-Clause-Patent -**/ > > > - > > > -#include <ConfigBlock.h> > > > -#include <Library/ConfigBlockLib.h> > > > -#include <Library/BaseMemoryLib.h> > > > -#include <Library/MemoryAllocationLib.h> -#include > > > <Library/DebugLib.h> > > > - > > > -/** > > > - Create config block table > > > - > > > - @param[in] TotalSize - Max size to be allocated for the > Config > > > Block Table > > > - @param[out] ConfigBlockTableAddress - On return, points to a > pointer > > > to the beginning of Config Block Table Address > > > - > > > - @retval EFI_INVALID_PARAMETER - Invalid Parameter > > > - @retval EFI_OUT_OF_RESOURCES - Out of resources > > > - @retval EFI_SUCCESS - Successfully created Config Block Table at > > > ConfigBlockTableAddress > > > -**/ > > > -EFI_STATUS > > > -EFIAPI > > > -CreateConfigBlockTable ( > > > - IN UINT16 TotalSize, > > > - OUT VOID **ConfigBlockTableAddress > > > - ) > > > -{ > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > - UINT32 ConfigBlkTblHdrSize; > > > - > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > (CONFIG_BLOCK_TABLE_HEADER)); > > > - > > > - if (TotalSize <= (ConfigBlkTblHdrSize + sizeof > (CONFIG_BLOCK_HEADER))) { > > > - DEBUG ((DEBUG_ERROR, "Invalid Parameter\n")); > > > - return EFI_INVALID_PARAMETER; > > > - } > > > - > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > *)AllocateZeroPool > > > (TotalSize); > > > - if (ConfigBlkTblAddrPtr == NULL) { > > > - DEBUG ((DEBUG_ERROR, "Could not allocate memory.\n")); > > > - return EFI_OUT_OF_RESOURCES; > > > - } > > > - ConfigBlkTblAddrPtr->NumberOfBlocks = 0; > > > - ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength = TotalSize; > > > - ConfigBlkTblAddrPtr->AvailableSize = TotalSize - > > > ConfigBlkTblHdrSize; > > > - > > > - *ConfigBlockTableAddress = (VOID *)ConfigBlkTblAddrPtr; > > > - > > > - return EFI_SUCCESS; > > > -} > > > - > > > -/** > > > - Add config block into config block table structure > > > - > > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning > of > > > Config Block Table Address > > > - @param[out] ConfigBlockAddress - On return, points to a pointer > to > > > the beginning of Config Block Address > > > - > > > - @retval EFI_OUT_OF_RESOURCES - Config Block Table is full and > > > cannot add new Config Block or > > > - Config Block Offset Table is full and cannot add new > Config > > > Block. > > > - @retval EFI_SUCCESS - Successfully added Config Block > > > -**/ > > > -EFI_STATUS > > > -EFIAPI > > > -AddConfigBlock ( > > > - IN VOID *ConfigBlockTableAddress, > > > - OUT VOID **ConfigBlockAddress > > > - ) > > > -{ > > > - CONFIG_BLOCK *TempConfigBlk; > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > - CONFIG_BLOCK *ConfigBlkAddrPtr; > > > - UINT16 ConfigBlkSize; > > > - > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > *)ConfigBlockTableAddress; > > > - ConfigBlkAddrPtr = (CONFIG_BLOCK *)(*ConfigBlockAddress); > > > - ConfigBlkSize = ConfigBlkAddrPtr->Header.GuidHob.Header.HobLength; > > > - DEBUG ((DEBUG_INFO, "Config Block GUID: %g / Config Block Size: > > > 0x%x bytes\n", &(ConfigBlkAddrPtr->Header.GuidHob.Name), > > > ConfigBlkSize)); > > > - if ((ConfigBlkSize % 4) != 0) { > > > - DEBUG ((DEBUG_ERROR, "Config Block must be multiples of 4 > bytes\n")); > > > - return EFI_INVALID_PARAMETER; > > > - } > > > - if (ConfigBlkTblAddrPtr->AvailableSize < ConfigBlkSize) { > > > - DEBUG ((DEBUG_ERROR, "Config Block Table is full and cannot add > new > > > Config Block.\n")); > > > - DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes / > > > Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr- > > > >AvailableSize, ConfigBlkSize)); > > > - return EFI_OUT_OF_RESOURCES; > > > - } > > > - > > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + > > > (UINTN)(ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength - > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > - CopyMem (&TempConfigBlk->Header, &ConfigBlkAddrPtr->Header, > > > sizeof(CONFIG_BLOCK_HEADER)); > > > - > > > - ConfigBlkTblAddrPtr->NumberOfBlocks++; > > > - ConfigBlkTblAddrPtr->AvailableSize = > > > ConfigBlkTblAddrPtr->AvailableSize - ConfigBlkSize; > > > - > > > - *ConfigBlockAddress = (VOID *) TempConfigBlk; > > > - DEBUG ((DEBUG_INFO, "Config Block Address: 0x%x / Available Config > > > Block Size: 0x%x bytes\n", (UINT32)(UINTN)*ConfigBlockAddress, > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > - return EFI_SUCCESS; > > > -} > > > - > > > -/** > > > - Retrieve a specific Config Block data by GUID > > > - > > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning > of > > > Config Block Table Address > > > - @param[in] ConfigBlockGuid - A pointer to the GUID uses to > > > search specific Config Block > > > - @param[out] ConfigBlockAddress - On return, points to a > pointer to > > > the beginning of Config Block Address > > > - > > > - @retval EFI_NOT_FOUND - Could not find the Config Block > > > - @retval EFI_SUCCESS - Config Block found and return > > > -**/ > > > -EFI_STATUS > > > -EFIAPI > > > -GetConfigBlock ( > > > - IN VOID *ConfigBlockTableAddress, > > > - IN EFI_GUID *ConfigBlockGuid, > > > - OUT VOID **ConfigBlockAddress > > > - ) > > > -{ > > > - UINT16 OffsetIndex; > > > - CONFIG_BLOCK *TempConfigBlk; > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > - UINT32 ConfigBlkTblHdrSize; > > > - UINT32 ConfigBlkOffset; > > > - UINT16 NumOfBlocks; > > > - > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > (CONFIG_BLOCK_TABLE_HEADER)); > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > *)ConfigBlockTableAddress; > > > - NumOfBlocks = ConfigBlkTblAddrPtr->NumberOfBlocks; > > > - > > > - ConfigBlkOffset = 0; > > > - for (OffsetIndex = 0; OffsetIndex < NumOfBlocks; OffsetIndex++) { > > > - if ((ConfigBlkTblHdrSize + ConfigBlkOffset) > (ConfigBlkTblAddrPtr- > > > >Header.GuidHob.Header.HobLength)) { > > > - break; > > > - } > > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + > > > (UINTN)ConfigBlkTblHdrSize + (UINTN)ConfigBlkOffset); > > > - if (CompareGuid (&(TempConfigBlk->Header.GuidHob.Name), > > > ConfigBlockGuid)) { > > > - *ConfigBlockAddress = (VOID *)TempConfigBlk; > > > - return EFI_SUCCESS; > > > - } > > > - ConfigBlkOffset = ConfigBlkOffset + TempConfigBlk- > > > >Header.GuidHob.Header.HobLength; > > > - } > > > - DEBUG ((DEBUG_ERROR, "Could not find the config block.\n")); > > > - return EFI_NOT_FOUND; > > > -} > > > diff --git > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > nfigB > > > lockLib.inf > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > nfig > > > BlockLib.inf > > > deleted file mode 100644 > > > index a7def2481d..0000000000 > > > --- > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > nfigB > > > lockLib.inf > > > +++ /dev/null > > > @@ -1,29 +0,0 @@ > > > -## @file > > > -# Component INF file for the BaseConfigBlock library. > > > -# > > > -# Copyright (c) 2019 Intel Corporation. All rights reserved. <BR> -# > > > -# SPDX-License-Identifier: BSD-2-Clause-Patent -# -## > > > - > > > -[Defines] > > > -INF_VERSION = 0x00010017 > > > -BASE_NAME = BaseConfigBlockLib > > > -FILE_GUID = 1EC07EA8-7808-4e06-9D79-309AE331D2D5 > > > -VERSION_STRING = 1.0 > > > -MODULE_TYPE = BASE > > > -LIBRARY_CLASS = ConfigBlockLib > > > - > > > - > > > -[Packages] > > > -MdePkg/MdePkg.dec > > > -CoffeelakeSiliconPkg/SiPkg.dec > > > - > > > -[Sources] > > > -BaseConfigBlockLib.c > > > - > > > -[LibraryClasses] > > > -DebugLib > > > -BaseMemoryLib > > > -MemoryAllocationLib > > > diff --git > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConf > > > igBlo > > > ckLib.c > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigB > > > lockLi > > > b.c > > > similarity index 100% > > > rename from > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfig > > > Block > > > Lib.c > > > rename to > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlo > > > ckLib.c > > > diff --git > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConf > > > igBlo > > > ckLib.inf > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigB > > > lockLi > > > b.inf > > > similarity index 100% > > > rename from > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfig > > > Block > > > Lib.inf > > > rename to > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlo > > > ckLib.i > > > nf > > > -- > > > 2.16.2.windows.1 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library 2019-11-08 6:25 ` Ni, Ray @ 2019-11-08 7:17 ` Ethan Tsao 2019-11-08 7:39 ` Ethan Tsao 0 siblings, 1 reply; 7+ messages in thread From: Ethan Tsao @ 2019-11-08 7:17 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V Hi Ray, I do agree with you. I will remove all debug message since all error scenario can be cover by return status. Caller can understanding through return status. Best Regards, Ethan > -----Original Message----- > From: Ni, Ray <ray.ni@intel.com> > Sent: Friday, November 8, 2019 2:25 PM > To: Tsao, Ethan <ethan.tsao@intel.com>; devel@edk2.groups.io > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library > > Ethan, > What if all of the debug message are removed? > Library especially very fundamental like ConfigBlock is better to be quite. > It's the ConfigBlock API designer's responsibility to ensure the error is returned > properly to caller, otherwise it's the API's bug. > When the API can return correct status out, it's caller's responsibility to check > the status and do proper handling of the errors. > > With the debug message added in this fundamental library, it may give callers > wrong impression that the library itself can handle the error and callers don't > need to check the error status. > > If we search code in MdePkg, quite few debug messages are printed out from > library code. > > Thanks, > Ray > > > -----Original Message----- > > From: Tsao, Ethan <ethan.tsao@intel.com> > > Sent: Thursday, November 7, 2019 4:14 PM > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > Library > > > > Hi Ray, > > Thanks your good input. I just review all debug message and most print > > is for error report purpose, such as allocate memory failure,...etc. > > From my opinion, this kind debug message is useful for BIOS when > > unexpected error happen. In normal case, it will not be print. > > > > Best Regards, > > Ethan > > > > > -----Original Message----- > > > From: Ni, Ray <ray.ni@intel.com> > > > Sent: Thursday, November 7, 2019 3:15 PM > > > To: Tsao, Ethan <ethan.tsao@intel.com>; edk2-devel@lists.01.org > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > > Library > > > > > > Given the patch only moves the code from one place to another, I am > > > ok > > with > > > that. > > > > > > By the way, is it still valuable to have so many debug messages > > > everywhere > > in > > > this library? > > > If no, can we remove them or at least some of them? > > > > > > Debug messages are valuable I agree. But we also need to think about > > producing > > > helpful debug messages, not treated by platform developers as noise > > > : ) > > > > > > > > > > -----Original Message----- > > > > From: Tsao, Ethan <ethan.tsao@intel.com> > > > > Sent: Thursday, November 7, 2019 11:31 AM > > > > To: edk2-devel@lists.01.org > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray > > > > <ray.ni@intel.com> > > > > Subject: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > > > Library > > > > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2318 > > > > > > > > Establish one copy of Config blocks library class and instance in > > > > IntelSiliconPkg and remove copies from other silicon packages , > > > > like KabyLakeSiliconPkg, CoffelakeSiliconPkg. > > > > > > > > Signed-off-by: Ethan Tsao <ethan.tsao@intel.com> > > > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > --- > > > > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConf > > igBlo > > > > ckLib.c | 146 ------------------------------------------------------------- > > ---- > > > - > > > > ------------------------------------------------------------------ > > > > ---- > > > > ---------- > > > > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConf > > igBlo > > > > ckLib.inf | 29 ----------------------------- > > > > Silicon/Intel/{KabylakeSiliconPkg => > > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | 0 > > > > Silicon/Intel/{KabylakeSiliconPkg => > > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf | 0 > > > > 4 files changed, 175 deletions(-) > > > > > > > > diff --git > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > seCo > > > > nfigB > > > > lockLib.c > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > seCo > > > > nfig > > > > BlockLib.c > > > > deleted file mode 100644 > > > > index 369dab97ee..0000000000 > > > > --- > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > seCo > > > > nfigB > > > > lockLib.c > > > > +++ /dev/null > > > > @@ -1,146 +0,0 @@ > > > > -/** @file > > > > - Library functions for Config Block management. > > > > - > > > > - Copyright (c) 2019 Intel Corporation. All rights reserved. <BR> > > > > - > > > > - SPDX-License-Identifier: BSD-2-Clause-Patent -**/ > > > > - > > > > -#include <ConfigBlock.h> > > > > -#include <Library/ConfigBlockLib.h> -#include > > > > <Library/BaseMemoryLib.h> -#include > > > > <Library/MemoryAllocationLib.h> -#include <Library/DebugLib.h> > > > > - > > > > -/** > > > > - Create config block table > > > > - > > > > - @param[in] TotalSize - Max size to be allocated for the > > Config > > > > Block Table > > > > - @param[out] ConfigBlockTableAddress - On return, points to a > > pointer > > > > to the beginning of Config Block Table Address > > > > - > > > > - @retval EFI_INVALID_PARAMETER - Invalid Parameter > > > > - @retval EFI_OUT_OF_RESOURCES - Out of resources > > > > - @retval EFI_SUCCESS - Successfully created Config Block Table at > > > > ConfigBlockTableAddress > > > > -**/ > > > > -EFI_STATUS > > > > -EFIAPI > > > > -CreateConfigBlockTable ( > > > > - IN UINT16 TotalSize, > > > > - OUT VOID **ConfigBlockTableAddress > > > > - ) > > > > -{ > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > - UINT32 ConfigBlkTblHdrSize; > > > > - > > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > > (CONFIG_BLOCK_TABLE_HEADER)); > > > > - > > > > - if (TotalSize <= (ConfigBlkTblHdrSize + sizeof > > (CONFIG_BLOCK_HEADER))) { > > > > - DEBUG ((DEBUG_ERROR, "Invalid Parameter\n")); > > > > - return EFI_INVALID_PARAMETER; > > > > - } > > > > - > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > *)AllocateZeroPool > > > > (TotalSize); > > > > - if (ConfigBlkTblAddrPtr == NULL) { > > > > - DEBUG ((DEBUG_ERROR, "Could not allocate memory.\n")); > > > > - return EFI_OUT_OF_RESOURCES; > > > > - } > > > > - ConfigBlkTblAddrPtr->NumberOfBlocks = 0; > > > > - ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength = > > > > TotalSize; > > > > - ConfigBlkTblAddrPtr->AvailableSize = TotalSize - > > > > ConfigBlkTblHdrSize; > > > > - > > > > - *ConfigBlockTableAddress = (VOID *)ConfigBlkTblAddrPtr; > > > > - > > > > - return EFI_SUCCESS; > > > > -} > > > > - > > > > -/** > > > > - Add config block into config block table structure > > > > - > > > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning > > of > > > > Config Block Table Address > > > > - @param[out] ConfigBlockAddress - On return, points to a pointer > > to > > > > the beginning of Config Block Address > > > > - > > > > - @retval EFI_OUT_OF_RESOURCES - Config Block Table is full and > > > > cannot add new Config Block or > > > > - Config Block Offset Table is full and cannot add new > > Config > > > > Block. > > > > - @retval EFI_SUCCESS - Successfully added Config Block > > > > -**/ > > > > -EFI_STATUS > > > > -EFIAPI > > > > -AddConfigBlock ( > > > > - IN VOID *ConfigBlockTableAddress, > > > > - OUT VOID **ConfigBlockAddress > > > > - ) > > > > -{ > > > > - CONFIG_BLOCK *TempConfigBlk; > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > - CONFIG_BLOCK *ConfigBlkAddrPtr; > > > > - UINT16 ConfigBlkSize; > > > > - > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > *)ConfigBlockTableAddress; > > > > - ConfigBlkAddrPtr = (CONFIG_BLOCK *)(*ConfigBlockAddress); > > > > - ConfigBlkSize = > > > > ConfigBlkAddrPtr->Header.GuidHob.Header.HobLength; > > > > - DEBUG ((DEBUG_INFO, "Config Block GUID: %g / Config Block Size: > > > > 0x%x bytes\n", &(ConfigBlkAddrPtr->Header.GuidHob.Name), > > > > ConfigBlkSize)); > > > > - if ((ConfigBlkSize % 4) != 0) { > > > > - DEBUG ((DEBUG_ERROR, "Config Block must be multiples of 4 > > bytes\n")); > > > > - return EFI_INVALID_PARAMETER; > > > > - } > > > > - if (ConfigBlkTblAddrPtr->AvailableSize < ConfigBlkSize) { > > > > - DEBUG ((DEBUG_ERROR, "Config Block Table is full and cannot add > > new > > > > Config Block.\n")); > > > > - DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes / > > > > Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr- > > > > >AvailableSize, ConfigBlkSize)); > > > > - return EFI_OUT_OF_RESOURCES; > > > > - } > > > > - > > > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + > > > > (UINTN)(ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength - > > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > > - CopyMem (&TempConfigBlk->Header, &ConfigBlkAddrPtr->Header, > > > > sizeof(CONFIG_BLOCK_HEADER)); > > > > - > > > > - ConfigBlkTblAddrPtr->NumberOfBlocks++; > > > > - ConfigBlkTblAddrPtr->AvailableSize = > > > > ConfigBlkTblAddrPtr->AvailableSize - ConfigBlkSize; > > > > - > > > > - *ConfigBlockAddress = (VOID *) TempConfigBlk; > > > > - DEBUG ((DEBUG_INFO, "Config Block Address: 0x%x / Available > > > > Config Block Size: 0x%x bytes\n", > > > > (UINT32)(UINTN)*ConfigBlockAddress, > > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > > - return EFI_SUCCESS; > > > > -} > > > > - > > > > -/** > > > > - Retrieve a specific Config Block data by GUID > > > > - > > > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning > > of > > > > Config Block Table Address > > > > - @param[in] ConfigBlockGuid - A pointer to the GUID uses to > > > > search specific Config Block > > > > - @param[out] ConfigBlockAddress - On return, points to a > > pointer to > > > > the beginning of Config Block Address > > > > - > > > > - @retval EFI_NOT_FOUND - Could not find the Config Block > > > > - @retval EFI_SUCCESS - Config Block found and return > > > > -**/ > > > > -EFI_STATUS > > > > -EFIAPI > > > > -GetConfigBlock ( > > > > - IN VOID *ConfigBlockTableAddress, > > > > - IN EFI_GUID *ConfigBlockGuid, > > > > - OUT VOID **ConfigBlockAddress > > > > - ) > > > > -{ > > > > - UINT16 OffsetIndex; > > > > - CONFIG_BLOCK *TempConfigBlk; > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > - UINT32 ConfigBlkTblHdrSize; > > > > - UINT32 ConfigBlkOffset; > > > > - UINT16 NumOfBlocks; > > > > - > > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > > (CONFIG_BLOCK_TABLE_HEADER)); > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > *)ConfigBlockTableAddress; > > > > - NumOfBlocks = ConfigBlkTblAddrPtr->NumberOfBlocks; > > > > - > > > > - ConfigBlkOffset = 0; > > > > - for (OffsetIndex = 0; OffsetIndex < NumOfBlocks; OffsetIndex++) { > > > > - if ((ConfigBlkTblHdrSize + ConfigBlkOffset) > (ConfigBlkTblAddrPtr- > > > > >Header.GuidHob.Header.HobLength)) { > > > > - break; > > > > - } > > > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + > > > > (UINTN)ConfigBlkTblHdrSize + (UINTN)ConfigBlkOffset); > > > > - if (CompareGuid (&(TempConfigBlk->Header.GuidHob.Name), > > > > ConfigBlockGuid)) { > > > > - *ConfigBlockAddress = (VOID *)TempConfigBlk; > > > > - return EFI_SUCCESS; > > > > - } > > > > - ConfigBlkOffset = ConfigBlkOffset + TempConfigBlk- > > > > >Header.GuidHob.Header.HobLength; > > > > - } > > > > - DEBUG ((DEBUG_ERROR, "Could not find the config block.\n")); > > > > - return EFI_NOT_FOUND; > > > > -} > > > > diff --git > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > seCo > > > > nfigB > > > > lockLib.inf > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > seCo > > > > nfig > > > > BlockLib.inf > > > > deleted file mode 100644 > > > > index a7def2481d..0000000000 > > > > --- > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > seCo > > > > nfigB > > > > lockLib.inf > > > > +++ /dev/null > > > > @@ -1,29 +0,0 @@ > > > > -## @file > > > > -# Component INF file for the BaseConfigBlock library. > > > > -# > > > > -# Copyright (c) 2019 Intel Corporation. All rights reserved. <BR> > > > > -# -# SPDX-License-Identifier: BSD-2-Clause-Patent -# -## > > > > - > > > > -[Defines] > > > > -INF_VERSION = 0x00010017 > > > > -BASE_NAME = BaseConfigBlockLib > > > > -FILE_GUID = 1EC07EA8-7808-4e06-9D79-309AE331D2D5 > > > > -VERSION_STRING = 1.0 > > > > -MODULE_TYPE = BASE > > > > -LIBRARY_CLASS = ConfigBlockLib > > > > - > > > > - > > > > -[Packages] > > > > -MdePkg/MdePkg.dec > > > > -CoffeelakeSiliconPkg/SiPkg.dec > > > > - > > > > -[Sources] > > > > -BaseConfigBlockLib.c > > > > - > > > > -[LibraryClasses] > > > > -DebugLib > > > > -BaseMemoryLib > > > > -MemoryAllocationLib > > > > diff --git > > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Base > > > > Conf > > > > igBlo > > > > ckLib.c > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseCon > > > > figB > > > > lockLi > > > > b.c > > > > similarity index 100% > > > > rename from > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > > nfig > > > > Block > > > > Lib.c > > > > rename to > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfi > > > > gBlo > > > > ckLib.c > > > > diff --git > > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Base > > > > Conf > > > > igBlo > > > > ckLib.inf > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseCon > > > > figB > > > > lockLi > > > > b.inf > > > > similarity index 100% > > > > rename from > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > > nfig > > > > Block > > > > Lib.inf > > > > rename to > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfi > > > > gBlo > > > > ckLib.i > > > > nf > > > > -- > > > > 2.16.2.windows.1 > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library 2019-11-08 7:17 ` Ethan Tsao @ 2019-11-08 7:39 ` Ethan Tsao 2019-11-08 8:59 ` Ni, Ray 0 siblings, 1 reply; 7+ messages in thread From: Ethan Tsao @ 2019-11-08 7:39 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V Hi Ray, There is one debug message print available and requested Config block size while Config Block table is not enough to add new. Caller only know size is not enough by return status. With this debug message, Caller can know how many size need to enlarge. What do you think to keep this debug message? DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes / Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr->AvailableSize, ConfigBlkSize)); Best Regards, Ethan > -----Original Message----- > From: Tsao, Ethan > Sent: Friday, November 8, 2019 3:17 PM > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library > > Hi Ray, > I do agree with you. I will remove all debug message since all error scenario can > be cover by return status. > Caller can understanding through return status. > > Best Regards, > Ethan > > > -----Original Message----- > > From: Ni, Ray <ray.ni@intel.com> > > Sent: Friday, November 8, 2019 2:25 PM > > To: Tsao, Ethan <ethan.tsao@intel.com>; devel@edk2.groups.io > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > Library > > > > Ethan, > > What if all of the debug message are removed? > > Library especially very fundamental like ConfigBlock is better to be quite. > > It's the ConfigBlock API designer's responsibility to ensure the error > > is returned properly to caller, otherwise it's the API's bug. > > When the API can return correct status out, it's caller's > > responsibility to check the status and do proper handling of the errors. > > > > With the debug message added in this fundamental library, it may give > > callers wrong impression that the library itself can handle the error > > and callers don't need to check the error status. > > > > If we search code in MdePkg, quite few debug messages are printed out > > from library code. > > > > Thanks, > > Ray > > > > > -----Original Message----- > > > From: Tsao, Ethan <ethan.tsao@intel.com> > > > Sent: Thursday, November 7, 2019 4:14 PM > > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > > Library > > > > > > Hi Ray, > > > Thanks your good input. I just review all debug message and most > > > print is for error report purpose, such as allocate memory failure,...etc. > > > From my opinion, this kind debug message is useful for BIOS when > > > unexpected error happen. In normal case, it will not be print. > > > > > > Best Regards, > > > Ethan > > > > > > > -----Original Message----- > > > > From: Ni, Ray <ray.ni@intel.com> > > > > Sent: Thursday, November 7, 2019 3:15 PM > > > > To: Tsao, Ethan <ethan.tsao@intel.com>; edk2-devel@lists.01.org > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add > > > > BaseConfigBlockLib Library > > > > > > > > Given the patch only moves the code from one place to another, I > > > > am ok > > > with > > > > that. > > > > > > > > By the way, is it still valuable to have so many debug messages > > > > everywhere > > > in > > > > this library? > > > > If no, can we remove them or at least some of them? > > > > > > > > Debug messages are valuable I agree. But we also need to think > > > > about > > > producing > > > > helpful debug messages, not treated by platform developers as > > > > noise > > > > : ) > > > > > > > > > > > > > -----Original Message----- > > > > > From: Tsao, Ethan <ethan.tsao@intel.com> > > > > > Sent: Thursday, November 7, 2019 11:31 AM > > > > > To: edk2-devel@lists.01.org > > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, > > > > > Ray <ray.ni@intel.com> > > > > > Subject: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > > > > Library > > > > > > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2318 > > > > > > > > > > Establish one copy of Config blocks library class and instance > > > > > in IntelSiliconPkg and remove copies from other silicon packages > > > > > , like KabyLakeSiliconPkg, CoffelakeSiliconPkg. > > > > > > > > > > Signed-off-by: Ethan Tsao <ethan.tsao@intel.com> > > > > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > --- > > > > > > > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > nf > > > igBlo > > > > > ckLib.c | 146 ----------------------------------------------------------- > -- > > > ---- > > > > - > > > > > ---------------------------------------------------------------- > > > > > -- > > > > > ---- > > > > > ---------- > > > > > > > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseCo > > > nf > > > igBlo > > > > > ckLib.inf | 29 ----------------------------- > > > > > Silicon/Intel/{KabylakeSiliconPkg => > > > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | 0 > > > > > Silicon/Intel/{KabylakeSiliconPkg => > > > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf | 0 > > > > > 4 files changed, 175 deletions(-) > > > > > > > > > > diff --git > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > Ba > > > > > seCo > > > > > nfigB > > > > > lockLib.c > > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > Ba > > > > > seCo > > > > > nfig > > > > > BlockLib.c > > > > > deleted file mode 100644 > > > > > index 369dab97ee..0000000000 > > > > > --- > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > Ba > > > > > seCo > > > > > nfigB > > > > > lockLib.c > > > > > +++ /dev/null > > > > > @@ -1,146 +0,0 @@ > > > > > -/** @file > > > > > - Library functions for Config Block management. > > > > > - > > > > > - Copyright (c) 2019 Intel Corporation. All rights reserved. > > > > > <BR> > > > > > - > > > > > - SPDX-License-Identifier: BSD-2-Clause-Patent -**/ > > > > > - > > > > > -#include <ConfigBlock.h> > > > > > -#include <Library/ConfigBlockLib.h> -#include > > > > > <Library/BaseMemoryLib.h> -#include > > > > > <Library/MemoryAllocationLib.h> -#include <Library/DebugLib.h> > > > > > - > > > > > -/** > > > > > - Create config block table > > > > > - > > > > > - @param[in] TotalSize - Max size to be allocated for the > > > Config > > > > > Block Table > > > > > - @param[out] ConfigBlockTableAddress - On return, points to a > > > pointer > > > > > to the beginning of Config Block Table Address > > > > > - > > > > > - @retval EFI_INVALID_PARAMETER - Invalid Parameter > > > > > - @retval EFI_OUT_OF_RESOURCES - Out of resources > > > > > - @retval EFI_SUCCESS - Successfully created Config Block Table at > > > > > ConfigBlockTableAddress > > > > > -**/ > > > > > -EFI_STATUS > > > > > -EFIAPI > > > > > -CreateConfigBlockTable ( > > > > > - IN UINT16 TotalSize, > > > > > - OUT VOID **ConfigBlockTableAddress > > > > > - ) > > > > > -{ > > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > > - UINT32 ConfigBlkTblHdrSize; > > > > > - > > > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > > > (CONFIG_BLOCK_TABLE_HEADER)); > > > > > - > > > > > - if (TotalSize <= (ConfigBlkTblHdrSize + sizeof > > > (CONFIG_BLOCK_HEADER))) { > > > > > - DEBUG ((DEBUG_ERROR, "Invalid Parameter\n")); > > > > > - return EFI_INVALID_PARAMETER; > > > > > - } > > > > > - > > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > *)AllocateZeroPool > > > > > (TotalSize); > > > > > - if (ConfigBlkTblAddrPtr == NULL) { > > > > > - DEBUG ((DEBUG_ERROR, "Could not allocate memory.\n")); > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > - } > > > > > - ConfigBlkTblAddrPtr->NumberOfBlocks = 0; > > > > > - ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength = > > > > > TotalSize; > > > > > - ConfigBlkTblAddrPtr->AvailableSize = TotalSize - > > > > > ConfigBlkTblHdrSize; > > > > > - > > > > > - *ConfigBlockTableAddress = (VOID *)ConfigBlkTblAddrPtr; > > > > > - > > > > > - return EFI_SUCCESS; > > > > > -} > > > > > - > > > > > -/** > > > > > - Add config block into config block table structure > > > > > - > > > > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning > > > of > > > > > Config Block Table Address > > > > > - @param[out] ConfigBlockAddress - On return, points to a > pointer > > > to > > > > > the beginning of Config Block Address > > > > > - > > > > > - @retval EFI_OUT_OF_RESOURCES - Config Block Table is full and > > > > > cannot add new Config Block or > > > > > - Config Block Offset Table is full and cannot add new > > > Config > > > > > Block. > > > > > - @retval EFI_SUCCESS - Successfully added Config Block > > > > > -**/ > > > > > -EFI_STATUS > > > > > -EFIAPI > > > > > -AddConfigBlock ( > > > > > - IN VOID *ConfigBlockTableAddress, > > > > > - OUT VOID **ConfigBlockAddress > > > > > - ) > > > > > -{ > > > > > - CONFIG_BLOCK *TempConfigBlk; > > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > > - CONFIG_BLOCK *ConfigBlkAddrPtr; > > > > > - UINT16 ConfigBlkSize; > > > > > - > > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > > *)ConfigBlockTableAddress; > > > > > - ConfigBlkAddrPtr = (CONFIG_BLOCK *)(*ConfigBlockAddress); > > > > > - ConfigBlkSize = > > > > > ConfigBlkAddrPtr->Header.GuidHob.Header.HobLength; > > > > > - DEBUG ((DEBUG_INFO, "Config Block GUID: %g / Config Block Size: > > > > > 0x%x bytes\n", &(ConfigBlkAddrPtr->Header.GuidHob.Name), > > > > > ConfigBlkSize)); > > > > > - if ((ConfigBlkSize % 4) != 0) { > > > > > - DEBUG ((DEBUG_ERROR, "Config Block must be multiples of 4 > > > bytes\n")); > > > > > - return EFI_INVALID_PARAMETER; > > > > > - } > > > > > - if (ConfigBlkTblAddrPtr->AvailableSize < ConfigBlkSize) { > > > > > - DEBUG ((DEBUG_ERROR, "Config Block Table is full and cannot add > > > new > > > > > Config Block.\n")); > > > > > - DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes / > > > > > Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr- > > > > > >AvailableSize, ConfigBlkSize)); > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > - } > > > > > - > > > > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + > > > > > (UINTN)(ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength - > > > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > > > - CopyMem (&TempConfigBlk->Header, &ConfigBlkAddrPtr->Header, > > > > > sizeof(CONFIG_BLOCK_HEADER)); > > > > > - > > > > > - ConfigBlkTblAddrPtr->NumberOfBlocks++; > > > > > - ConfigBlkTblAddrPtr->AvailableSize = > > > > > ConfigBlkTblAddrPtr->AvailableSize - ConfigBlkSize; > > > > > - > > > > > - *ConfigBlockAddress = (VOID *) TempConfigBlk; > > > > > - DEBUG ((DEBUG_INFO, "Config Block Address: 0x%x / Available > > > > > Config Block Size: 0x%x bytes\n", > > > > > (UINT32)(UINTN)*ConfigBlockAddress, > > > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > > > - return EFI_SUCCESS; > > > > > -} > > > > > - > > > > > -/** > > > > > - Retrieve a specific Config Block data by GUID > > > > > - > > > > > - @param[in] ConfigBlockTableAddress - A pointer to the beginning > > > of > > > > > Config Block Table Address > > > > > - @param[in] ConfigBlockGuid - A pointer to the GUID uses to > > > > > search specific Config Block > > > > > - @param[out] ConfigBlockAddress - On return, points to a > > > pointer to > > > > > the beginning of Config Block Address > > > > > - > > > > > - @retval EFI_NOT_FOUND - Could not find the Config Block > > > > > - @retval EFI_SUCCESS - Config Block found and return > > > > > -**/ > > > > > -EFI_STATUS > > > > > -EFIAPI > > > > > -GetConfigBlock ( > > > > > - IN VOID *ConfigBlockTableAddress, > > > > > - IN EFI_GUID *ConfigBlockGuid, > > > > > - OUT VOID **ConfigBlockAddress > > > > > - ) > > > > > -{ > > > > > - UINT16 OffsetIndex; > > > > > - CONFIG_BLOCK *TempConfigBlk; > > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > > - UINT32 ConfigBlkTblHdrSize; > > > > > - UINT32 ConfigBlkOffset; > > > > > - UINT16 NumOfBlocks; > > > > > - > > > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > > > (CONFIG_BLOCK_TABLE_HEADER)); > > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > > *)ConfigBlockTableAddress; > > > > > - NumOfBlocks = ConfigBlkTblAddrPtr->NumberOfBlocks; > > > > > - > > > > > - ConfigBlkOffset = 0; > > > > > - for (OffsetIndex = 0; OffsetIndex < NumOfBlocks; OffsetIndex++) { > > > > > - if ((ConfigBlkTblHdrSize + ConfigBlkOffset) > (ConfigBlkTblAddrPtr- > > > > > >Header.GuidHob.Header.HobLength)) { > > > > > - break; > > > > > - } > > > > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + > > > > > (UINTN)ConfigBlkTblHdrSize + (UINTN)ConfigBlkOffset); > > > > > - if (CompareGuid (&(TempConfigBlk->Header.GuidHob.Name), > > > > > ConfigBlockGuid)) { > > > > > - *ConfigBlockAddress = (VOID *)TempConfigBlk; > > > > > - return EFI_SUCCESS; > > > > > - } > > > > > - ConfigBlkOffset = ConfigBlkOffset + TempConfigBlk- > > > > > >Header.GuidHob.Header.HobLength; > > > > > - } > > > > > - DEBUG ((DEBUG_ERROR, "Could not find the config block.\n")); > > > > > - return EFI_NOT_FOUND; > > > > > -} > > > > > diff --git > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > Ba > > > > > seCo > > > > > nfigB > > > > > lockLib.inf > > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > Ba > > > > > seCo > > > > > nfig > > > > > BlockLib.inf > > > > > deleted file mode 100644 > > > > > index a7def2481d..0000000000 > > > > > --- > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > Ba > > > > > seCo > > > > > nfigB > > > > > lockLib.inf > > > > > +++ /dev/null > > > > > @@ -1,29 +0,0 @@ > > > > > -## @file > > > > > -# Component INF file for the BaseConfigBlock library. > > > > > -# > > > > > -# Copyright (c) 2019 Intel Corporation. All rights reserved. > > > > > <BR> -# -# SPDX-License-Identifier: BSD-2-Clause-Patent -# -## > > > > > - > > > > > -[Defines] > > > > > -INF_VERSION = 0x00010017 > > > > > -BASE_NAME = BaseConfigBlockLib > > > > > -FILE_GUID = 1EC07EA8-7808-4e06-9D79-309AE331D2D5 > > > > > -VERSION_STRING = 1.0 > > > > > -MODULE_TYPE = BASE > > > > > -LIBRARY_CLASS = ConfigBlockLib > > > > > - > > > > > - > > > > > -[Packages] > > > > > -MdePkg/MdePkg.dec > > > > > -CoffeelakeSiliconPkg/SiPkg.dec > > > > > - > > > > > -[Sources] > > > > > -BaseConfigBlockLib.c > > > > > - > > > > > -[LibraryClasses] > > > > > -DebugLib > > > > > -BaseMemoryLib > > > > > -MemoryAllocationLib > > > > > diff --git > > > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > > se > > > > > Conf > > > > > igBlo > > > > > ckLib.c > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseC > > > > > on > > > > > figB > > > > > lockLi > > > > > b.c > > > > > similarity index 100% > > > > > rename from > > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Base > > > > > Co > > > > > nfig > > > > > Block > > > > > Lib.c > > > > > rename to > > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseCon > > > > > fi > > > > > gBlo > > > > > ckLib.c > > > > > diff --git > > > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > > se > > > > > Conf > > > > > igBlo > > > > > ckLib.inf > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseC > > > > > on > > > > > figB > > > > > lockLi > > > > > b.inf > > > > > similarity index 100% > > > > > rename from > > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Base > > > > > Co > > > > > nfig > > > > > Block > > > > > Lib.inf > > > > > rename to > > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseCon > > > > > fi > > > > > gBlo > > > > > ckLib.i > > > > > nf > > > > > -- > > > > > 2.16.2.windows.1 > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library 2019-11-08 7:39 ` Ethan Tsao @ 2019-11-08 8:59 ` Ni, Ray 2019-11-11 5:52 ` Ethan Tsao 0 siblings, 1 reply; 7+ messages in thread From: Ni, Ray @ 2019-11-08 8:59 UTC (permalink / raw) To: Tsao, Ethan, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V When a API needs enough buffer prepared by caller, there are two styles of APIs: 1. Status FillBuffer (VOID *Buffer, UINTN BufferSize) BufferSize is passed by value so the API has no way to return back the required size. It's ok as long as the caller is able to calculate the proper size based on context. 2. Status FillBuffer (VOID *Buffer, UINTN &BufferSize) BufferSize is passed by pointer so the API can return back the required size. In this case, if API cannot return back the buffer size back to caller, but caller is able to calculate the size before calling the API, it follows the style #1 which is still ok. In general, we cannot design a API to use debug messages tell caller the buffer size. Thanks, Ray > -----Original Message----- > From: Tsao, Ethan <ethan.tsao@intel.com> > Sent: Friday, November 8, 2019 3:40 PM > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library > > Hi Ray, > There is one debug message print available and requested Config block size > while Config Block table is not enough to add new. > Caller only know size is not enough by return status. With this debug > message, Caller can know how many size need to enlarge. > What do you think to keep this debug message? > > DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes / > Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr- > >AvailableSize, ConfigBlkSize)); > > Best Regards, > Ethan > > > -----Original Message----- > > From: Tsao, Ethan > > Sent: Friday, November 8, 2019 3:17 PM > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > Library > > > > Hi Ray, > > I do agree with you. I will remove all debug message since all error > > scenario can be cover by return status. > > Caller can understanding through return status. > > > > Best Regards, > > Ethan > > > > > -----Original Message----- > > > From: Ni, Ray <ray.ni@intel.com> > > > Sent: Friday, November 8, 2019 2:25 PM > > > To: Tsao, Ethan <ethan.tsao@intel.com>; devel@edk2.groups.io > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > > Library > > > > > > Ethan, > > > What if all of the debug message are removed? > > > Library especially very fundamental like ConfigBlock is better to be quite. > > > It's the ConfigBlock API designer's responsibility to ensure the > > > error is returned properly to caller, otherwise it's the API's bug. > > > When the API can return correct status out, it's caller's > > > responsibility to check the status and do proper handling of the errors. > > > > > > With the debug message added in this fundamental library, it may > > > give callers wrong impression that the library itself can handle the > > > error and callers don't need to check the error status. > > > > > > If we search code in MdePkg, quite few debug messages are printed > > > out from library code. > > > > > > Thanks, > > > Ray > > > > > > > -----Original Message----- > > > > From: Tsao, Ethan <ethan.tsao@intel.com> > > > > Sent: Thursday, November 7, 2019 4:14 PM > > > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add > > > > BaseConfigBlockLib Library > > > > > > > > Hi Ray, > > > > Thanks your good input. I just review all debug message and most > > > > print is for error report purpose, such as allocate memory failure,...etc. > > > > From my opinion, this kind debug message is useful for BIOS when > > > > unexpected error happen. In normal case, it will not be print. > > > > > > > > Best Regards, > > > > Ethan > > > > > > > > > -----Original Message----- > > > > > From: Ni, Ray <ray.ni@intel.com> > > > > > Sent: Thursday, November 7, 2019 3:15 PM > > > > > To: Tsao, Ethan <ethan.tsao@intel.com>; edk2-devel@lists.01.org > > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add > > > > > BaseConfigBlockLib Library > > > > > > > > > > Given the patch only moves the code from one place to another, I > > > > > am ok > > > > with > > > > > that. > > > > > > > > > > By the way, is it still valuable to have so many debug messages > > > > > everywhere > > > > in > > > > > this library? > > > > > If no, can we remove them or at least some of them? > > > > > > > > > > Debug messages are valuable I agree. But we also need to think > > > > > about > > > > producing > > > > > helpful debug messages, not treated by platform developers as > > > > > noise > > > > > : ) > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Tsao, Ethan <ethan.tsao@intel.com> > > > > > > Sent: Thursday, November 7, 2019 11:31 AM > > > > > > To: edk2-devel@lists.01.org > > > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, > > > > > > Ray <ray.ni@intel.com> > > > > > > Subject: [PATCH] IntelSiliconPkg/Library:Add > > > > > > BaseConfigBlockLib Library > > > > > > > > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2318 > > > > > > > > > > > > Establish one copy of Config blocks library class and instance > > > > > > in IntelSiliconPkg and remove copies from other silicon > > > > > > packages , like KabyLakeSiliconPkg, CoffelakeSiliconPkg. > > > > > > > > > > > > Signed-off-by: Ethan Tsao <ethan.tsao@intel.com> > > > > > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > > --- > > > > > > > > > > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Base > > > > Co > > > > nf > > > > igBlo > > > > > > ckLib.c | 146 ------------------------------------------------------- > ---- > > -- > > > > ---- > > > > > - > > > > > > -------------------------------------------------------------- > > > > > > -- > > > > > > -- > > > > > > ---- > > > > > > ---------- > > > > > > > > > > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Base > > > > Co > > > > nf > > > > igBlo > > > > > > ckLib.inf | 29 ----------------------------- > > > > > > Silicon/Intel/{KabylakeSiliconPkg => > > > > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | > 0 > > > > > > Silicon/Intel/{KabylakeSiliconPkg => > > > > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf > | 0 > > > > > > 4 files changed, 175 deletions(-) > > > > > > > > > > > > diff --git > > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi > > > > > > b/ > > > > > > Ba > > > > > > seCo > > > > > > nfigB > > > > > > lockLib.c > > > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi > > > > > > b/ > > > > > > Ba > > > > > > seCo > > > > > > nfig > > > > > > BlockLib.c > > > > > > deleted file mode 100644 > > > > > > index 369dab97ee..0000000000 > > > > > > --- > > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi > > > > > > b/ > > > > > > Ba > > > > > > seCo > > > > > > nfigB > > > > > > lockLib.c > > > > > > +++ /dev/null > > > > > > @@ -1,146 +0,0 @@ > > > > > > -/** @file > > > > > > - Library functions for Config Block management. > > > > > > - > > > > > > - Copyright (c) 2019 Intel Corporation. All rights reserved. > > > > > > <BR> > > > > > > - > > > > > > - SPDX-License-Identifier: BSD-2-Clause-Patent -**/ > > > > > > - > > > > > > -#include <ConfigBlock.h> > > > > > > -#include <Library/ConfigBlockLib.h> -#include > > > > > > <Library/BaseMemoryLib.h> -#include > > > > > > <Library/MemoryAllocationLib.h> -#include <Library/DebugLib.h> > > > > > > - > > > > > > -/** > > > > > > - Create config block table > > > > > > - > > > > > > - @param[in] TotalSize - Max size to be allocated for the > > > > Config > > > > > > Block Table > > > > > > - @param[out] ConfigBlockTableAddress - On return, points to > a > > > > pointer > > > > > > to the beginning of Config Block Table Address > > > > > > - > > > > > > - @retval EFI_INVALID_PARAMETER - Invalid Parameter > > > > > > - @retval EFI_OUT_OF_RESOURCES - Out of resources > > > > > > - @retval EFI_SUCCESS - Successfully created Config Block > Table at > > > > > > ConfigBlockTableAddress > > > > > > -**/ > > > > > > -EFI_STATUS > > > > > > -EFIAPI > > > > > > -CreateConfigBlockTable ( > > > > > > - IN UINT16 TotalSize, > > > > > > - OUT VOID **ConfigBlockTableAddress > > > > > > - ) > > > > > > -{ > > > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > > > - UINT32 ConfigBlkTblHdrSize; > > > > > > - > > > > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > > > > (CONFIG_BLOCK_TABLE_HEADER)); > > > > > > - > > > > > > - if (TotalSize <= (ConfigBlkTblHdrSize + sizeof > > > > (CONFIG_BLOCK_HEADER))) { > > > > > > - DEBUG ((DEBUG_ERROR, "Invalid Parameter\n")); > > > > > > - return EFI_INVALID_PARAMETER; > > > > > > - } > > > > > > - > > > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > *)AllocateZeroPool > > > > > > (TotalSize); > > > > > > - if (ConfigBlkTblAddrPtr == NULL) { > > > > > > - DEBUG ((DEBUG_ERROR, "Could not allocate memory.\n")); > > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > > - } > > > > > > - ConfigBlkTblAddrPtr->NumberOfBlocks = 0; > > > > > > - ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength = > > > > > > TotalSize; > > > > > > - ConfigBlkTblAddrPtr->AvailableSize = TotalSize - > > > > > > ConfigBlkTblHdrSize; > > > > > > - > > > > > > - *ConfigBlockTableAddress = (VOID *)ConfigBlkTblAddrPtr; > > > > > > - > > > > > > - return EFI_SUCCESS; > > > > > > -} > > > > > > - > > > > > > -/** > > > > > > - Add config block into config block table structure > > > > > > - > > > > > > - @param[in] ConfigBlockTableAddress - A pointer to the > beginning > > > > of > > > > > > Config Block Table Address > > > > > > - @param[out] ConfigBlockAddress - On return, points to a > > pointer > > > > to > > > > > > the beginning of Config Block Address > > > > > > - > > > > > > - @retval EFI_OUT_OF_RESOURCES - Config Block Table is full > > > > > > and cannot add new Config Block or > > > > > > - Config Block Offset Table is full and cannot add > new > > > > Config > > > > > > Block. > > > > > > - @retval EFI_SUCCESS - Successfully added Config Block > > > > > > -**/ > > > > > > -EFI_STATUS > > > > > > -EFIAPI > > > > > > -AddConfigBlock ( > > > > > > - IN VOID *ConfigBlockTableAddress, > > > > > > - OUT VOID **ConfigBlockAddress > > > > > > - ) > > > > > > -{ > > > > > > - CONFIG_BLOCK *TempConfigBlk; > > > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > > > - CONFIG_BLOCK *ConfigBlkAddrPtr; > > > > > > - UINT16 ConfigBlkSize; > > > > > > - > > > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > > > *)ConfigBlockTableAddress; > > > > > > - ConfigBlkAddrPtr = (CONFIG_BLOCK *)(*ConfigBlockAddress); > > > > > > - ConfigBlkSize = > > > > > > ConfigBlkAddrPtr->Header.GuidHob.Header.HobLength; > > > > > > - DEBUG ((DEBUG_INFO, "Config Block GUID: %g / Config Block Size: > > > > > > 0x%x bytes\n", &(ConfigBlkAddrPtr->Header.GuidHob.Name), > > > > > > ConfigBlkSize)); > > > > > > - if ((ConfigBlkSize % 4) != 0) { > > > > > > - DEBUG ((DEBUG_ERROR, "Config Block must be multiples of 4 > > > > bytes\n")); > > > > > > - return EFI_INVALID_PARAMETER; > > > > > > - } > > > > > > - if (ConfigBlkTblAddrPtr->AvailableSize < ConfigBlkSize) { > > > > > > - DEBUG ((DEBUG_ERROR, "Config Block Table is full and cannot > add > > > > new > > > > > > Config Block.\n")); > > > > > > - DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes > / > > > > > > Requested Config Block Size: 0x%x bytes\n", > > > > > > ConfigBlkTblAddrPtr- > > > > > > >AvailableSize, ConfigBlkSize)); > > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > > - } > > > > > > - > > > > > > - TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr > > > > > > + (UINTN)(ConfigBlkTblAddrPtr- > >Header.GuidHob.Header.HobLength > > > > > > - > > > > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > > > > - CopyMem (&TempConfigBlk->Header, &ConfigBlkAddrPtr- > >Header, > > > > > > sizeof(CONFIG_BLOCK_HEADER)); > > > > > > - > > > > > > - ConfigBlkTblAddrPtr->NumberOfBlocks++; > > > > > > - ConfigBlkTblAddrPtr->AvailableSize = > > > > > > ConfigBlkTblAddrPtr->AvailableSize - ConfigBlkSize; > > > > > > - > > > > > > - *ConfigBlockAddress = (VOID *) TempConfigBlk; > > > > > > - DEBUG ((DEBUG_INFO, "Config Block Address: 0x%x / Available > > > > > > Config Block Size: 0x%x bytes\n", > > > > > > (UINT32)(UINTN)*ConfigBlockAddress, > > > > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > > > > - return EFI_SUCCESS; > > > > > > -} > > > > > > - > > > > > > -/** > > > > > > - Retrieve a specific Config Block data by GUID > > > > > > - > > > > > > - @param[in] ConfigBlockTableAddress - A pointer to the > beginning > > > > of > > > > > > Config Block Table Address > > > > > > - @param[in] ConfigBlockGuid - A pointer to the GUID > uses to > > > > > > search specific Config Block > > > > > > - @param[out] ConfigBlockAddress - On return, points to a > > > > pointer to > > > > > > the beginning of Config Block Address > > > > > > - > > > > > > - @retval EFI_NOT_FOUND - Could not find the Config Block > > > > > > - @retval EFI_SUCCESS - Config Block found and return > > > > > > -**/ > > > > > > -EFI_STATUS > > > > > > -EFIAPI > > > > > > -GetConfigBlock ( > > > > > > - IN VOID *ConfigBlockTableAddress, > > > > > > - IN EFI_GUID *ConfigBlockGuid, > > > > > > - OUT VOID **ConfigBlockAddress > > > > > > - ) > > > > > > -{ > > > > > > - UINT16 OffsetIndex; > > > > > > - CONFIG_BLOCK *TempConfigBlk; > > > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > > > - UINT32 ConfigBlkTblHdrSize; > > > > > > - UINT32 ConfigBlkOffset; > > > > > > - UINT16 NumOfBlocks; > > > > > > - > > > > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > > > > (CONFIG_BLOCK_TABLE_HEADER)); > > > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > > > *)ConfigBlockTableAddress; > > > > > > - NumOfBlocks = ConfigBlkTblAddrPtr->NumberOfBlocks; > > > > > > - > > > > > > - ConfigBlkOffset = 0; > > > > > > - for (OffsetIndex = 0; OffsetIndex < NumOfBlocks; OffsetIndex++) > { > > > > > > - if ((ConfigBlkTblHdrSize + ConfigBlkOffset) > > (ConfigBlkTblAddrPtr- > > > > > > >Header.GuidHob.Header.HobLength)) { > > > > > > - break; > > > > > > - } > > > > > > - TempConfigBlk = (CONFIG_BLOCK > *)((UINTN)ConfigBlkTblAddrPtr + > > > > > > (UINTN)ConfigBlkTblHdrSize + (UINTN)ConfigBlkOffset); > > > > > > - if (CompareGuid (&(TempConfigBlk->Header.GuidHob.Name), > > > > > > ConfigBlockGuid)) { > > > > > > - *ConfigBlockAddress = (VOID *)TempConfigBlk; > > > > > > - return EFI_SUCCESS; > > > > > > - } > > > > > > - ConfigBlkOffset = ConfigBlkOffset + TempConfigBlk- > > > > > > >Header.GuidHob.Header.HobLength; > > > > > > - } > > > > > > - DEBUG ((DEBUG_ERROR, "Could not find the config > > > > > > block.\n")); > > > > > > - return EFI_NOT_FOUND; > > > > > > -} > > > > > > diff --git > > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi > > > > > > b/ > > > > > > Ba > > > > > > seCo > > > > > > nfigB > > > > > > lockLib.inf > > > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi > > > > > > b/ > > > > > > Ba > > > > > > seCo > > > > > > nfig > > > > > > BlockLib.inf > > > > > > deleted file mode 100644 > > > > > > index a7def2481d..0000000000 > > > > > > --- > > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi > > > > > > b/ > > > > > > Ba > > > > > > seCo > > > > > > nfigB > > > > > > lockLib.inf > > > > > > +++ /dev/null > > > > > > @@ -1,29 +0,0 @@ > > > > > > -## @file > > > > > > -# Component INF file for the BaseConfigBlock library. > > > > > > -# > > > > > > -# Copyright (c) 2019 Intel Corporation. All rights reserved. > > > > > > <BR> -# -# SPDX-License-Identifier: BSD-2-Clause-Patent -# -## > > > > > > - > > > > > > -[Defines] > > > > > > -INF_VERSION = 0x00010017 > > > > > > -BASE_NAME = BaseConfigBlockLib -FILE_GUID = > > > > > > 1EC07EA8-7808-4e06-9D79-309AE331D2D5 > > > > > > -VERSION_STRING = 1.0 > > > > > > -MODULE_TYPE = BASE > > > > > > -LIBRARY_CLASS = ConfigBlockLib > > > > > > - > > > > > > - > > > > > > -[Packages] > > > > > > -MdePkg/MdePkg.dec > > > > > > -CoffeelakeSiliconPkg/SiPkg.dec > > > > > > - > > > > > > -[Sources] > > > > > > -BaseConfigBlockLib.c > > > > > > - > > > > > > -[LibraryClasses] > > > > > > -DebugLib > > > > > > -BaseMemoryLib > > > > > > -MemoryAllocationLib > > > > > > diff --git > > > > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > > Ba > > > > > > se > > > > > > Conf > > > > > > igBlo > > > > > > ckLib.c > > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/Bas > > > > > > eC > > > > > > on > > > > > > figB > > > > > > lockLi > > > > > > b.c > > > > > > similarity index 100% > > > > > > rename from > > > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > > > se > > > > > > Co > > > > > > nfig > > > > > > Block > > > > > > Lib.c > > > > > > rename to > > > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseC > > > > > > on > > > > > > fi > > > > > > gBlo > > > > > > ckLib.c > > > > > > diff --git > > > > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > > Ba > > > > > > se > > > > > > Conf > > > > > > igBlo > > > > > > ckLib.inf > > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/Bas > > > > > > eC > > > > > > on > > > > > > figB > > > > > > lockLi > > > > > > b.inf > > > > > > similarity index 100% > > > > > > rename from > > > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > > > se > > > > > > Co > > > > > > nfig > > > > > > Block > > > > > > Lib.inf > > > > > > rename to > > > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseC > > > > > > on > > > > > > fi > > > > > > gBlo > > > > > > ckLib.i > > > > > > nf > > > > > > -- > > > > > > 2.16.2.windows.1 > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library 2019-11-08 8:59 ` Ni, Ray @ 2019-11-11 5:52 ` Ethan Tsao 0 siblings, 0 replies; 7+ messages in thread From: Ethan Tsao @ 2019-11-11 5:52 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V Hi Ray, Thanks your input. I will remove this debug message as well. Best Regards, Ethan > -----Original Message----- > From: Ni, Ray <ray.ni@intel.com> > Sent: Friday, November 8, 2019 4:59 PM > To: Tsao, Ethan <ethan.tsao@intel.com>; devel@edk2.groups.io > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library > > When a API needs enough buffer prepared by caller, there are two styles of APIs: > 1. Status FillBuffer (VOID *Buffer, UINTN BufferSize) > BufferSize is passed by value so the API has no way to return back the > required size. > It's ok as long as the caller is able to calculate the proper size based on > context. > > 2. Status FillBuffer (VOID *Buffer, UINTN &BufferSize) > BufferSize is passed by pointer so the API can return back the required size. > > In this case, if API cannot return back the buffer size back to caller, but caller is > able to calculate the size before calling the API, it follows the style #1 which is > still ok. > > In general, we cannot design a API to use debug messages tell caller the buffer > size. > > Thanks, > Ray > > > -----Original Message----- > > From: Tsao, Ethan <ethan.tsao@intel.com> > > Sent: Friday, November 8, 2019 3:40 PM > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > Library > > > > Hi Ray, > > There is one debug message print available and requested Config block > > size while Config Block table is not enough to add new. > > Caller only know size is not enough by return status. With this debug > > message, Caller can know how many size need to enlarge. > > What do you think to keep this debug message? > > > > DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes / > > Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr- > > >AvailableSize, ConfigBlkSize)); > > > > Best Regards, > > Ethan > > > > > -----Original Message----- > > > From: Tsao, Ethan > > > Sent: Friday, November 8, 2019 3:17 PM > > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > > Library > > > > > > Hi Ray, > > > I do agree with you. I will remove all debug message since all error > > > scenario can be cover by return status. > > > Caller can understanding through return status. > > > > > > Best Regards, > > > Ethan > > > > > > > -----Original Message----- > > > > From: Ni, Ray <ray.ni@intel.com> > > > > Sent: Friday, November 8, 2019 2:25 PM > > > > To: Tsao, Ethan <ethan.tsao@intel.com>; devel@edk2.groups.io > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add > > > > BaseConfigBlockLib Library > > > > > > > > Ethan, > > > > What if all of the debug message are removed? > > > > Library especially very fundamental like ConfigBlock is better to be quite. > > > > It's the ConfigBlock API designer's responsibility to ensure the > > > > error is returned properly to caller, otherwise it's the API's bug. > > > > When the API can return correct status out, it's caller's > > > > responsibility to check the status and do proper handling of the errors. > > > > > > > > With the debug message added in this fundamental library, it may > > > > give callers wrong impression that the library itself can handle > > > > the error and callers don't need to check the error status. > > > > > > > > If we search code in MdePkg, quite few debug messages are printed > > > > out from library code. > > > > > > > > Thanks, > > > > Ray > > > > > > > > > -----Original Message----- > > > > > From: Tsao, Ethan <ethan.tsao@intel.com> > > > > > Sent: Thursday, November 7, 2019 4:14 PM > > > > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io > > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add > > > > > BaseConfigBlockLib Library > > > > > > > > > > Hi Ray, > > > > > Thanks your good input. I just review all debug message and most > > > > > print is for error report purpose, such as allocate memory failure,...etc. > > > > > From my opinion, this kind debug message is useful for BIOS when > > > > > unexpected error happen. In normal case, it will not be print. > > > > > > > > > > Best Regards, > > > > > Ethan > > > > > > > > > > > -----Original Message----- > > > > > > From: Ni, Ray <ray.ni@intel.com> > > > > > > Sent: Thursday, November 7, 2019 3:15 PM > > > > > > To: Tsao, Ethan <ethan.tsao@intel.com>; > > > > > > edk2-devel@lists.01.org > > > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> > > > > > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add > > > > > > BaseConfigBlockLib Library > > > > > > > > > > > > Given the patch only moves the code from one place to another, > > > > > > I am ok > > > > > with > > > > > > that. > > > > > > > > > > > > By the way, is it still valuable to have so many debug > > > > > > messages everywhere > > > > > in > > > > > > this library? > > > > > > If no, can we remove them or at least some of them? > > > > > > > > > > > > Debug messages are valuable I agree. But we also need to think > > > > > > about > > > > > producing > > > > > > helpful debug messages, not treated by platform developers as > > > > > > noise > > > > > > : ) > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Tsao, Ethan <ethan.tsao@intel.com> > > > > > > > Sent: Thursday, November 7, 2019 11:31 AM > > > > > > > To: edk2-devel@lists.01.org > > > > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; > > > > > > > Ni, Ray <ray.ni@intel.com> > > > > > > > Subject: [PATCH] IntelSiliconPkg/Library:Add > > > > > > > BaseConfigBlockLib Library > > > > > > > > > > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2318 > > > > > > > > > > > > > > Establish one copy of Config blocks library class and > > > > > > > instance in IntelSiliconPkg and remove copies from other > > > > > > > silicon packages , like KabyLakeSiliconPkg, CoffelakeSiliconPkg. > > > > > > > > > > > > > > Signed-off-by: Ethan Tsao <ethan.tsao@intel.com> > > > > > > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> > > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > > > --- > > > > > > > > > > > > > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > > se > > > > > Co > > > > > nf > > > > > igBlo > > > > > > > ckLib.c | 146 ------------------------------------------------------ > - > > ---- > > > -- > > > > > ---- > > > > > > - > > > > > > > ------------------------------------------------------------ > > > > > > > -- > > > > > > > -- > > > > > > > -- > > > > > > > ---- > > > > > > > ---------- > > > > > > > > > > > > > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Ba > > > > > se > > > > > Co > > > > > nf > > > > > igBlo > > > > > > > ckLib.inf | 29 ----------------------------- > > > > > > > Silicon/Intel/{KabylakeSiliconPkg => > > > > > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | > > 0 > > > > > > > Silicon/Intel/{KabylakeSiliconPkg => > > > > > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockL > > > > > > > ib.inf > > | 0 > > > > > > > 4 files changed, 175 deletions(-) > > > > > > > > > > > > > > diff --git > > > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlock > > > > > > > Li > > > > > > > b/ > > > > > > > Ba > > > > > > > seCo > > > > > > > nfigB > > > > > > > lockLib.c > > > > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlock > > > > > > > Li > > > > > > > b/ > > > > > > > Ba > > > > > > > seCo > > > > > > > nfig > > > > > > > BlockLib.c > > > > > > > deleted file mode 100644 > > > > > > > index 369dab97ee..0000000000 > > > > > > > --- > > > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlock > > > > > > > Li > > > > > > > b/ > > > > > > > Ba > > > > > > > seCo > > > > > > > nfigB > > > > > > > lockLib.c > > > > > > > +++ /dev/null > > > > > > > @@ -1,146 +0,0 @@ > > > > > > > -/** @file > > > > > > > - Library functions for Config Block management. > > > > > > > - > > > > > > > - Copyright (c) 2019 Intel Corporation. All rights reserved. > > > > > > > <BR> > > > > > > > - > > > > > > > - SPDX-License-Identifier: BSD-2-Clause-Patent -**/ > > > > > > > - > > > > > > > -#include <ConfigBlock.h> > > > > > > > -#include <Library/ConfigBlockLib.h> -#include > > > > > > > <Library/BaseMemoryLib.h> -#include > > > > > > > <Library/MemoryAllocationLib.h> -#include > > > > > > > <Library/DebugLib.h> > > > > > > > - > > > > > > > -/** > > > > > > > - Create config block table > > > > > > > - > > > > > > > - @param[in] TotalSize - Max size to be allocated for the > > > > > Config > > > > > > > Block Table > > > > > > > - @param[out] ConfigBlockTableAddress - On return, points to > > a > > > > > pointer > > > > > > > to the beginning of Config Block Table Address > > > > > > > - > > > > > > > - @retval EFI_INVALID_PARAMETER - Invalid Parameter > > > > > > > - @retval EFI_OUT_OF_RESOURCES - Out of resources > > > > > > > - @retval EFI_SUCCESS - Successfully created Config Block > > Table at > > > > > > > ConfigBlockTableAddress > > > > > > > -**/ > > > > > > > -EFI_STATUS > > > > > > > -EFIAPI > > > > > > > -CreateConfigBlockTable ( > > > > > > > - IN UINT16 TotalSize, > > > > > > > - OUT VOID **ConfigBlockTableAddress > > > > > > > - ) > > > > > > > -{ > > > > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > > > > - UINT32 ConfigBlkTblHdrSize; > > > > > > > - > > > > > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > > > > > (CONFIG_BLOCK_TABLE_HEADER)); > > > > > > > - > > > > > > > - if (TotalSize <= (ConfigBlkTblHdrSize + sizeof > > > > > (CONFIG_BLOCK_HEADER))) { > > > > > > > - DEBUG ((DEBUG_ERROR, "Invalid Parameter\n")); > > > > > > > - return EFI_INVALID_PARAMETER; > > > > > > > - } > > > > > > > - > > > > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > > *)AllocateZeroPool > > > > > > > (TotalSize); > > > > > > > - if (ConfigBlkTblAddrPtr == NULL) { > > > > > > > - DEBUG ((DEBUG_ERROR, "Could not allocate memory.\n")); > > > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > > > - } > > > > > > > - ConfigBlkTblAddrPtr->NumberOfBlocks = 0; > > > > > > > - ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength = > > > > > > > TotalSize; > > > > > > > - ConfigBlkTblAddrPtr->AvailableSize = TotalSize - > > > > > > > ConfigBlkTblHdrSize; > > > > > > > - > > > > > > > - *ConfigBlockTableAddress = (VOID *)ConfigBlkTblAddrPtr; > > > > > > > - > > > > > > > - return EFI_SUCCESS; > > > > > > > -} > > > > > > > - > > > > > > > -/** > > > > > > > - Add config block into config block table structure > > > > > > > - > > > > > > > - @param[in] ConfigBlockTableAddress - A pointer to the > > beginning > > > > > of > > > > > > > Config Block Table Address > > > > > > > - @param[out] ConfigBlockAddress - On return, points to a > > > pointer > > > > > to > > > > > > > the beginning of Config Block Address > > > > > > > - > > > > > > > - @retval EFI_OUT_OF_RESOURCES - Config Block Table is full > > > > > > > and cannot add new Config Block or > > > > > > > - Config Block Offset Table is full and cannot add > > new > > > > > Config > > > > > > > Block. > > > > > > > - @retval EFI_SUCCESS - Successfully added Config Block > > > > > > > -**/ > > > > > > > -EFI_STATUS > > > > > > > -EFIAPI > > > > > > > -AddConfigBlock ( > > > > > > > - IN VOID *ConfigBlockTableAddress, > > > > > > > - OUT VOID **ConfigBlockAddress > > > > > > > - ) > > > > > > > -{ > > > > > > > - CONFIG_BLOCK *TempConfigBlk; > > > > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > > > > - CONFIG_BLOCK *ConfigBlkAddrPtr; > > > > > > > - UINT16 ConfigBlkSize; > > > > > > > - > > > > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > > > > *)ConfigBlockTableAddress; > > > > > > > - ConfigBlkAddrPtr = (CONFIG_BLOCK *)(*ConfigBlockAddress); > > > > > > > - ConfigBlkSize = > > > > > > > ConfigBlkAddrPtr->Header.GuidHob.Header.HobLength; > > > > > > > - DEBUG ((DEBUG_INFO, "Config Block GUID: %g / Config Block Size: > > > > > > > 0x%x bytes\n", &(ConfigBlkAddrPtr->Header.GuidHob.Name), > > > > > > > ConfigBlkSize)); > > > > > > > - if ((ConfigBlkSize % 4) != 0) { > > > > > > > - DEBUG ((DEBUG_ERROR, "Config Block must be multiples of 4 > > > > > bytes\n")); > > > > > > > - return EFI_INVALID_PARAMETER; > > > > > > > - } > > > > > > > - if (ConfigBlkTblAddrPtr->AvailableSize < ConfigBlkSize) { > > > > > > > - DEBUG ((DEBUG_ERROR, "Config Block Table is full and cannot > > add > > > > > new > > > > > > > Config Block.\n")); > > > > > > > - DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes > > / > > > > > > > Requested Config Block Size: 0x%x bytes\n", > > > > > > > ConfigBlkTblAddrPtr- > > > > > > > >AvailableSize, ConfigBlkSize)); > > > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > > > - } > > > > > > > - > > > > > > > - TempConfigBlk = (CONFIG_BLOCK > > > > > > > *)((UINTN)ConfigBlkTblAddrPtr > > > > > > > + (UINTN)(ConfigBlkTblAddrPtr- > > >Header.GuidHob.Header.HobLength > > > > > > > - > > > > > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > > > > > - CopyMem (&TempConfigBlk->Header, &ConfigBlkAddrPtr- > > >Header, > > > > > > > sizeof(CONFIG_BLOCK_HEADER)); > > > > > > > - > > > > > > > - ConfigBlkTblAddrPtr->NumberOfBlocks++; > > > > > > > - ConfigBlkTblAddrPtr->AvailableSize = > > > > > > > ConfigBlkTblAddrPtr->AvailableSize - ConfigBlkSize; > > > > > > > - > > > > > > > - *ConfigBlockAddress = (VOID *) TempConfigBlk; > > > > > > > - DEBUG ((DEBUG_INFO, "Config Block Address: 0x%x / > > > > > > > Available Config Block Size: 0x%x bytes\n", > > > > > > > (UINT32)(UINTN)*ConfigBlockAddress, > > > > > > > ConfigBlkTblAddrPtr->AvailableSize)); > > > > > > > - return EFI_SUCCESS; > > > > > > > -} > > > > > > > - > > > > > > > -/** > > > > > > > - Retrieve a specific Config Block data by GUID > > > > > > > - > > > > > > > - @param[in] ConfigBlockTableAddress - A pointer to the > > beginning > > > > > of > > > > > > > Config Block Table Address > > > > > > > - @param[in] ConfigBlockGuid - A pointer to the GUID > > uses to > > > > > > > search specific Config Block > > > > > > > - @param[out] ConfigBlockAddress - On return, points to a > > > > > pointer to > > > > > > > the beginning of Config Block Address > > > > > > > - > > > > > > > - @retval EFI_NOT_FOUND - Could not find the Config Block > > > > > > > - @retval EFI_SUCCESS - Config Block found and return > > > > > > > -**/ > > > > > > > -EFI_STATUS > > > > > > > -EFIAPI > > > > > > > -GetConfigBlock ( > > > > > > > - IN VOID *ConfigBlockTableAddress, > > > > > > > - IN EFI_GUID *ConfigBlockGuid, > > > > > > > - OUT VOID **ConfigBlockAddress > > > > > > > - ) > > > > > > > -{ > > > > > > > - UINT16 OffsetIndex; > > > > > > > - CONFIG_BLOCK *TempConfigBlk; > > > > > > > - CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr; > > > > > > > - UINT32 ConfigBlkTblHdrSize; > > > > > > > - UINT32 ConfigBlkOffset; > > > > > > > - UINT16 NumOfBlocks; > > > > > > > - > > > > > > > - ConfigBlkTblHdrSize = (UINT32)(sizeof > > > > > (CONFIG_BLOCK_TABLE_HEADER)); > > > > > > > - ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER > > > > > > > *)ConfigBlockTableAddress; > > > > > > > - NumOfBlocks = ConfigBlkTblAddrPtr->NumberOfBlocks; > > > > > > > - > > > > > > > - ConfigBlkOffset = 0; > > > > > > > - for (OffsetIndex = 0; OffsetIndex < NumOfBlocks; > > > > > > > OffsetIndex++) > > { > > > > > > > - if ((ConfigBlkTblHdrSize + ConfigBlkOffset) > > > (ConfigBlkTblAddrPtr- > > > > > > > >Header.GuidHob.Header.HobLength)) { > > > > > > > - break; > > > > > > > - } > > > > > > > - TempConfigBlk = (CONFIG_BLOCK > > *)((UINTN)ConfigBlkTblAddrPtr + > > > > > > > (UINTN)ConfigBlkTblHdrSize + (UINTN)ConfigBlkOffset); > > > > > > > - if (CompareGuid (&(TempConfigBlk->Header.GuidHob.Name), > > > > > > > ConfigBlockGuid)) { > > > > > > > - *ConfigBlockAddress = (VOID *)TempConfigBlk; > > > > > > > - return EFI_SUCCESS; > > > > > > > - } > > > > > > > - ConfigBlkOffset = ConfigBlkOffset + TempConfigBlk- > > > > > > > >Header.GuidHob.Header.HobLength; > > > > > > > - } > > > > > > > - DEBUG ((DEBUG_ERROR, "Could not find the config > > > > > > > block.\n")); > > > > > > > - return EFI_NOT_FOUND; > > > > > > > -} > > > > > > > diff --git > > > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlock > > > > > > > Li > > > > > > > b/ > > > > > > > Ba > > > > > > > seCo > > > > > > > nfigB > > > > > > > lockLib.inf > > > > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlock > > > > > > > Li > > > > > > > b/ > > > > > > > Ba > > > > > > > seCo > > > > > > > nfig > > > > > > > BlockLib.inf > > > > > > > deleted file mode 100644 > > > > > > > index a7def2481d..0000000000 > > > > > > > --- > > > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlock > > > > > > > Li > > > > > > > b/ > > > > > > > Ba > > > > > > > seCo > > > > > > > nfigB > > > > > > > lockLib.inf > > > > > > > +++ /dev/null > > > > > > > @@ -1,29 +0,0 @@ > > > > > > > -## @file > > > > > > > -# Component INF file for the BaseConfigBlock library. > > > > > > > -# > > > > > > > -# Copyright (c) 2019 Intel Corporation. All rights reserved. > > > > > > > <BR> -# -# SPDX-License-Identifier: BSD-2-Clause-Patent -# > > > > > > > -## > > > > > > > - > > > > > > > -[Defines] > > > > > > > -INF_VERSION = 0x00010017 > > > > > > > -BASE_NAME = BaseConfigBlockLib -FILE_GUID = > > > > > > > 1EC07EA8-7808-4e06-9D79-309AE331D2D5 > > > > > > > -VERSION_STRING = 1.0 > > > > > > > -MODULE_TYPE = BASE > > > > > > > -LIBRARY_CLASS = ConfigBlockLib > > > > > > > - > > > > > > > - > > > > > > > -[Packages] > > > > > > > -MdePkg/MdePkg.dec > > > > > > > -CoffeelakeSiliconPkg/SiPkg.dec > > > > > > > - > > > > > > > -[Sources] > > > > > > > -BaseConfigBlockLib.c > > > > > > > - > > > > > > > -[LibraryClasses] > > > > > > > -DebugLib > > > > > > > -BaseMemoryLib > > > > > > > -MemoryAllocationLib > > > > > > > diff --git > > > > > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLi > > > > > > > b/ > > > > > > > Ba > > > > > > > se > > > > > > > Conf > > > > > > > igBlo > > > > > > > ckLib.c > > > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/B > > > > > > > as > > > > > > > eC > > > > > > > on > > > > > > > figB > > > > > > > lockLi > > > > > > > b.c > > > > > > > similarity index 100% > > > > > > > rename from > > > > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > > > Ba > > > > > > > se > > > > > > > Co > > > > > > > nfig > > > > > > > Block > > > > > > > Lib.c > > > > > > > rename to > > > > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/Bas > > > > > > > eC > > > > > > > on > > > > > > > fi > > > > > > > gBlo > > > > > > > ckLib.c > > > > > > > diff --git > > > > > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLi > > > > > > > b/ > > > > > > > Ba > > > > > > > se > > > > > > > Conf > > > > > > > igBlo > > > > > > > ckLib.inf > > > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/B > > > > > > > as > > > > > > > eC > > > > > > > on > > > > > > > figB > > > > > > > lockLi > > > > > > > b.inf > > > > > > > similarity index 100% > > > > > > > rename from > > > > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/ > > > > > > > Ba > > > > > > > se > > > > > > > Co > > > > > > > nfig > > > > > > > Block > > > > > > > Lib.inf > > > > > > > rename to > > > > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/Bas > > > > > > > eC > > > > > > > on > > > > > > > fi > > > > > > > gBlo > > > > > > > ckLib.i > > > > > > > nf > > > > > > > -- > > > > > > > 2.16.2.windows.1 > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library
@ 2019-11-07 4:36 ethan.tsao
0 siblings, 0 replies; 7+ messages in thread
From: ethan.tsao @ 2019-11-07 4:36 UTC (permalink / raw)
To: devel; +Cc: Sai Chaganty, Ray Ni
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2318
Establish one copy of Config blocks library class and instance
in IntelSiliconPkg and remove copies from other silicon packages
, like KabyLakeSiliconPkg, CoffelakeSiliconPkg.
Signed-off-by: Ethan Tsao <ethan.tsao@intel.com>
Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
---
Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | 146 --------------------------------------------------------------------------------------------------------------------------------------------------
Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf | 29 -----------------------------
Silicon/Intel/{KabylakeSiliconPkg => IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | 0
Silicon/Intel/{KabylakeSiliconPkg => IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf | 0
4 files changed, 175 deletions(-)
diff --git a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.c b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.c
deleted file mode 100644
index 369dab97ee..0000000000
--- a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.c
+++ /dev/null
@@ -1,146 +0,0 @@
-/** @file
- Library functions for Config Block management.
-
- Copyright (c) 2019 Intel Corporation. All rights reserved. <BR>
-
- SPDX-License-Identifier: BSD-2-Clause-Patent
-**/
-
-#include <ConfigBlock.h>
-#include <Library/ConfigBlockLib.h>
-#include <Library/BaseMemoryLib.h>
-#include <Library/MemoryAllocationLib.h>
-#include <Library/DebugLib.h>
-
-/**
- Create config block table
-
- @param[in] TotalSize - Max size to be allocated for the Config Block Table
- @param[out] ConfigBlockTableAddress - On return, points to a pointer to the beginning of Config Block Table Address
-
- @retval EFI_INVALID_PARAMETER - Invalid Parameter
- @retval EFI_OUT_OF_RESOURCES - Out of resources
- @retval EFI_SUCCESS - Successfully created Config Block Table at ConfigBlockTableAddress
-**/
-EFI_STATUS
-EFIAPI
-CreateConfigBlockTable (
- IN UINT16 TotalSize,
- OUT VOID **ConfigBlockTableAddress
- )
-{
- CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr;
- UINT32 ConfigBlkTblHdrSize;
-
- ConfigBlkTblHdrSize = (UINT32)(sizeof (CONFIG_BLOCK_TABLE_HEADER));
-
- if (TotalSize <= (ConfigBlkTblHdrSize + sizeof (CONFIG_BLOCK_HEADER))) {
- DEBUG ((DEBUG_ERROR, "Invalid Parameter\n"));
- return EFI_INVALID_PARAMETER;
- }
-
- ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER *)AllocateZeroPool (TotalSize);
- if (ConfigBlkTblAddrPtr == NULL) {
- DEBUG ((DEBUG_ERROR, "Could not allocate memory.\n"));
- return EFI_OUT_OF_RESOURCES;
- }
- ConfigBlkTblAddrPtr->NumberOfBlocks = 0;
- ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength = TotalSize;
- ConfigBlkTblAddrPtr->AvailableSize = TotalSize - ConfigBlkTblHdrSize;
-
- *ConfigBlockTableAddress = (VOID *)ConfigBlkTblAddrPtr;
-
- return EFI_SUCCESS;
-}
-
-/**
- Add config block into config block table structure
-
- @param[in] ConfigBlockTableAddress - A pointer to the beginning of Config Block Table Address
- @param[out] ConfigBlockAddress - On return, points to a pointer to the beginning of Config Block Address
-
- @retval EFI_OUT_OF_RESOURCES - Config Block Table is full and cannot add new Config Block or
- Config Block Offset Table is full and cannot add new Config Block.
- @retval EFI_SUCCESS - Successfully added Config Block
-**/
-EFI_STATUS
-EFIAPI
-AddConfigBlock (
- IN VOID *ConfigBlockTableAddress,
- OUT VOID **ConfigBlockAddress
- )
-{
- CONFIG_BLOCK *TempConfigBlk;
- CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr;
- CONFIG_BLOCK *ConfigBlkAddrPtr;
- UINT16 ConfigBlkSize;
-
- ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER *)ConfigBlockTableAddress;
- ConfigBlkAddrPtr = (CONFIG_BLOCK *)(*ConfigBlockAddress);
- ConfigBlkSize = ConfigBlkAddrPtr->Header.GuidHob.Header.HobLength;
- DEBUG ((DEBUG_INFO, "Config Block GUID: %g / Config Block Size: 0x%x bytes\n", &(ConfigBlkAddrPtr->Header.GuidHob.Name), ConfigBlkSize));
- if ((ConfigBlkSize % 4) != 0) {
- DEBUG ((DEBUG_ERROR, "Config Block must be multiples of 4 bytes\n"));
- return EFI_INVALID_PARAMETER;
- }
- if (ConfigBlkTblAddrPtr->AvailableSize < ConfigBlkSize) {
- DEBUG ((DEBUG_ERROR, "Config Block Table is full and cannot add new Config Block.\n"));
- DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes / Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr->AvailableSize, ConfigBlkSize));
- return EFI_OUT_OF_RESOURCES;
- }
-
- TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + (UINTN)(ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength - ConfigBlkTblAddrPtr->AvailableSize));
- CopyMem (&TempConfigBlk->Header, &ConfigBlkAddrPtr->Header, sizeof(CONFIG_BLOCK_HEADER));
-
- ConfigBlkTblAddrPtr->NumberOfBlocks++;
- ConfigBlkTblAddrPtr->AvailableSize = ConfigBlkTblAddrPtr->AvailableSize - ConfigBlkSize;
-
- *ConfigBlockAddress = (VOID *) TempConfigBlk;
- DEBUG ((DEBUG_INFO, "Config Block Address: 0x%x / Available Config Block Size: 0x%x bytes\n", (UINT32)(UINTN)*ConfigBlockAddress, ConfigBlkTblAddrPtr->AvailableSize));
- return EFI_SUCCESS;
-}
-
-/**
- Retrieve a specific Config Block data by GUID
-
- @param[in] ConfigBlockTableAddress - A pointer to the beginning of Config Block Table Address
- @param[in] ConfigBlockGuid - A pointer to the GUID uses to search specific Config Block
- @param[out] ConfigBlockAddress - On return, points to a pointer to the beginning of Config Block Address
-
- @retval EFI_NOT_FOUND - Could not find the Config Block
- @retval EFI_SUCCESS - Config Block found and return
-**/
-EFI_STATUS
-EFIAPI
-GetConfigBlock (
- IN VOID *ConfigBlockTableAddress,
- IN EFI_GUID *ConfigBlockGuid,
- OUT VOID **ConfigBlockAddress
- )
-{
- UINT16 OffsetIndex;
- CONFIG_BLOCK *TempConfigBlk;
- CONFIG_BLOCK_TABLE_HEADER *ConfigBlkTblAddrPtr;
- UINT32 ConfigBlkTblHdrSize;
- UINT32 ConfigBlkOffset;
- UINT16 NumOfBlocks;
-
- ConfigBlkTblHdrSize = (UINT32)(sizeof (CONFIG_BLOCK_TABLE_HEADER));
- ConfigBlkTblAddrPtr = (CONFIG_BLOCK_TABLE_HEADER *)ConfigBlockTableAddress;
- NumOfBlocks = ConfigBlkTblAddrPtr->NumberOfBlocks;
-
- ConfigBlkOffset = 0;
- for (OffsetIndex = 0; OffsetIndex < NumOfBlocks; OffsetIndex++) {
- if ((ConfigBlkTblHdrSize + ConfigBlkOffset) > (ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength)) {
- break;
- }
- TempConfigBlk = (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + (UINTN)ConfigBlkTblHdrSize + (UINTN)ConfigBlkOffset);
- if (CompareGuid (&(TempConfigBlk->Header.GuidHob.Name), ConfigBlockGuid)) {
- *ConfigBlockAddress = (VOID *)TempConfigBlk;
- return EFI_SUCCESS;
- }
- ConfigBlkOffset = ConfigBlkOffset + TempConfigBlk->Header.GuidHob.Header.HobLength;
- }
- DEBUG ((DEBUG_ERROR, "Could not find the config block.\n"));
- return EFI_NOT_FOUND;
-}
diff --git a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf
deleted file mode 100644
index a7def2481d..0000000000
--- a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf
+++ /dev/null
@@ -1,29 +0,0 @@
-## @file
-# Component INF file for the BaseConfigBlock library.
-#
-# Copyright (c) 2019 Intel Corporation. All rights reserved. <BR>
-#
-# SPDX-License-Identifier: BSD-2-Clause-Patent
-#
-##
-
-[Defines]
-INF_VERSION = 0x00010017
-BASE_NAME = BaseConfigBlockLib
-FILE_GUID = 1EC07EA8-7808-4e06-9D79-309AE331D2D5
-VERSION_STRING = 1.0
-MODULE_TYPE = BASE
-LIBRARY_CLASS = ConfigBlockLib
-
-
-[Packages]
-MdePkg/MdePkg.dec
-CoffeelakeSiliconPkg/SiPkg.dec
-
-[Sources]
-BaseConfigBlockLib.c
-
-[LibraryClasses]
-DebugLib
-BaseMemoryLib
-MemoryAllocationLib
diff --git a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.c b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.c
similarity index 100%
rename from Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.c
rename to Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.c
diff --git a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf
similarity index 100%
rename from Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf
rename to Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 7+ messages in threadend of thread, other threads:[~2019-11-11 5:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20191107033058.180-1-ethan.tsao@intel.com>
[not found] ` <734D49CCEBEEF84792F5B80ED585239D5C352F1D@SHSMSX104.ccr.corp.intel.com>
2019-11-07 8:14 ` [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library Ethan Tsao
2019-11-08 6:25 ` Ni, Ray
2019-11-08 7:17 ` Ethan Tsao
2019-11-08 7:39 ` Ethan Tsao
2019-11-08 8:59 ` Ni, Ray
2019-11-11 5:52 ` Ethan Tsao
2019-11-07 4:36 ethan.tsao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox