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...@intel.com> > Sent: Thursday, November 7, 2019 3:15 PM > To: Tsao, Ethan <ethan.t...@intel.com>; edk2-de...@lists.01.org > Cc: Chaganty, Rangasai V <rangasai.v.chaga...@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.t...@intel.com> > > Sent: Thursday, November 7, 2019 11:31 AM > > To: edk2-de...@lists.01.org > > Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Ni, Ray > > <ray...@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.t...@intel.com> > > Cc: Sai Chaganty <rangasai.v.chaga...@intel.com> > > Cc: Ray Ni <ray...@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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#50186): https://edk2.groups.io/g/devel/message/50186 Mute This Topic: https://groups.io/mt/45239214/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
