From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web10.6522.1573194324721178179 for ; Thu, 07 Nov 2019 22:25:24 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Nov 2019 22:25:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,280,1569308400"; d="scan'208";a="231796200" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by FMSMGA003.fm.intel.com with ESMTP; 07 Nov 2019 22:25:24 -0800 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 7 Nov 2019 22:25:23 -0800 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 7 Nov 2019 22:25:23 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.127]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.225]) with mapi id 14.03.0439.000; Fri, 8 Nov 2019 14:25:20 +0800 From: "Ni, Ray" To: "Tsao, Ethan" , "devel@edk2.groups.io" CC: "Chaganty, Rangasai V" Subject: Re: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library Thread-Topic: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library Thread-Index: AQHVlRvJrFzkKZUq1EqIXhOFe7+FQKd/SHjggAATvACAAXHBIA== Date: Fri, 8 Nov 2019 06:25:19 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C354EC3@SHSMSX104.ccr.corp.intel.com> References: <20191107033058.180-1-ethan.tsao@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C352F1D@SHSMSX104.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 r= eturned 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 calle= rs wrong impression that the library itself can handle the error and callers don't n= eed 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 > Sent: Thursday, November 7, 2019 4:14 PM > To: Ni, Ray ; devel@edk2.groups.io > Cc: Chaganty, Rangasai V > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Libra= ry >=20 > 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. >=20 > Best Regards, > Ethan >=20 > > -----Original Message----- > > From: Ni, Ray > > Sent: Thursday, November 7, 2019 3:15 PM > > To: Tsao, Ethan ; edk2-devel@lists.01.org > > Cc: Chaganty, Rangasai V > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Lib= rary > > > > 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 everywh= ere > 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 > > > Sent: Thursday, November 7, 2019 11:31 AM > > > To: edk2-devel@lists.01.org > > > Cc: Chaganty, Rangasai V ; Ni, Ray > > > > > > Subject: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib > > > Library > > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2318 > > > > > > 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 > > > Cc: Sai Chaganty > > > Cc: Ray Ni > > > --- > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigB= lo > > > ckLib.c | 146 ----------------------------------= --------------------------- > ---- > > - > > > ---------------------------------------------------------------------= - > > > ---------- > > > > > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfigB= lo > > > ckLib.inf | 29 ----------------------------- > > > Silicon/Intel/{KabylakeSiliconPkg =3D> > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.c | = 0 > > > Silicon/Intel/{KabylakeSiliconPkg =3D> > > > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf | = 0 > > > 4 files changed, 175 deletions(-) > > > > > > diff --git > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseC= o > > > nfigB > > > lockLib.c > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseC= o > > > nfig > > > BlockLib.c > > > deleted file mode 100644 > > > index 369dab97ee..0000000000 > > > --- > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseC= o > > > nfigB > > > lockLib.c > > > +++ /dev/null > > > @@ -1,146 +0,0 @@ > > > -/** @file > > > - Library functions for Config Block management. > > > - > > > - Copyright (c) 2019 Intel Corporation. All rights reserved.
> > > - > > > - SPDX-License-Identifier: BSD-2-Clause-Patent -**/ > > > - > > > -#include > > > -#include > > > -#include > > > -#include -#include > > > > > > - > > > -/** > > > - Create config block table > > > - > > > - @param[in] TotalSize - Max size to be alloc= ated 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 =3D (UINT32)(sizeof > (CONFIG_BLOCK_TABLE_HEADER)); > > > - > > > - if (TotalSize <=3D (ConfigBlkTblHdrSize + sizeof > (CONFIG_BLOCK_HEADER))) { > > > - DEBUG ((DEBUG_ERROR, "Invalid Parameter\n")); > > > - return EFI_INVALID_PARAMETER; > > > - } > > > - > > > - ConfigBlkTblAddrPtr =3D (CONFIG_BLOCK_TABLE_HEADER > *)AllocateZeroPool > > > (TotalSize); > > > - if (ConfigBlkTblAddrPtr =3D=3D NULL) { > > > - DEBUG ((DEBUG_ERROR, "Could not allocate memory.\n")); > > > - return EFI_OUT_OF_RESOURCES; > > > - } > > > - ConfigBlkTblAddrPtr->NumberOfBlocks =3D 0; > > > - ConfigBlkTblAddrPtr->Header.GuidHob.Header.HobLength =3D TotalSize= ; > > > - ConfigBlkTblAddrPtr->AvailableSize =3D TotalSize - > > > ConfigBlkTblHdrSize; > > > - > > > - *ConfigBlockTableAddress =3D (VOID *)ConfigBlkTblAddrPtr; > > > - > > > - return EFI_SUCCESS; > > > -} > > > - > > > -/** > > > - Add config block into config block table structure > > > - > > > - @param[in] ConfigBlockTableAddress - A pointer to the beg= inning > 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 a= nd 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 =3D (CONFIG_BLOCK_TABLE_HEADER > > > *)ConfigBlockTableAddress; > > > - ConfigBlkAddrPtr =3D (CONFIG_BLOCK *)(*ConfigBlockAddress); > > > - ConfigBlkSize =3D ConfigBlkAddrPtr->Header.GuidHob.Header.HobLengt= h; > > > - DEBUG ((DEBUG_INFO, "Config Block GUID: %g / Config Block Size: > > > 0x%x bytes\n", &(ConfigBlkAddrPtr->Header.GuidHob.Name), > > > ConfigBlkSize)); > > > - if ((ConfigBlkSize % 4) !=3D 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 =3D (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 =3D > > > ConfigBlkTblAddrPtr->AvailableSize - ConfigBlkSize; > > > - > > > - *ConfigBlockAddress =3D (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 be= ginning > of > > > Config Block Table Address > > > - @param[in] ConfigBlockGuid - A pointer to the GU= ID uses to > > > search specific Config Block > > > - @param[out] ConfigBlockAddress - On return, points t= o 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 =3D (UINT32)(sizeof > (CONFIG_BLOCK_TABLE_HEADER)); > > > - ConfigBlkTblAddrPtr =3D (CONFIG_BLOCK_TABLE_HEADER > > > *)ConfigBlockTableAddress; > > > - NumOfBlocks =3D ConfigBlkTblAddrPtr->NumberOfBlocks; > > > - > > > - ConfigBlkOffset =3D 0; > > > - for (OffsetIndex =3D 0; OffsetIndex < NumOfBlocks; OffsetIndex++) = { > > > - if ((ConfigBlkTblHdrSize + ConfigBlkOffset) > (ConfigBlkTblAddrP= tr- > > > >Header.GuidHob.Header.HobLength)) { > > > - break; > > > - } > > > - TempConfigBlk =3D (CONFIG_BLOCK *)((UINTN)ConfigBlkTblAddrPtr + > > > (UINTN)ConfigBlkTblHdrSize + (UINTN)ConfigBlkOffset); > > > - if (CompareGuid (&(TempConfigBlk->Header.GuidHob.Name), > > > ConfigBlockGuid)) { > > > - *ConfigBlockAddress =3D (VOID *)TempConfigBlk; > > > - return EFI_SUCCESS; > > > - } > > > - ConfigBlkOffset =3D 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/BaseC= o > > > nfigB > > > lockLib.inf > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseC= o > > > nfig > > > BlockLib.inf > > > deleted file mode 100644 > > > index a7def2481d..0000000000 > > > --- > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/BaseC= o > > > 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.
-# > > > -# SPDX-License-Identifier: BSD-2-Clause-Patent -# -## > > > - > > > -[Defines] > > > -INF_VERSION =3D 0x00010017 > > > -BASE_NAME =3D BaseConfigBlockLib > > > -FILE_GUID =3D 1EC07EA8-7808-4e06-9D79-309AE331D2D5 > > > -VERSION_STRING =3D 1.0 > > > -MODULE_TYPE =3D BASE > > > -LIBRARY_CLASS =3D ConfigBlockLib > > > - > > > - > > > -[Packages] > > > -MdePkg/MdePkg.dec > > > -CoffeelakeSiliconPkg/SiPkg.dec > > > - > > > -[Sources] > > > -BaseConfigBlockLib.c > > > - > > > -[LibraryClasses] > > > -DebugLib > > > -BaseMemoryLib > > > -MemoryAllocationLib > > > diff --git > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseCon= f > > > igBlo > > > ckLib.c > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfig= B > > > lockLi > > > b.c > > > similarity index 100% > > > rename from > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfi= g > > > Block > > > Lib.c > > > rename to > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBl= o > > > ckLib.c > > > diff --git > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseCon= f > > > igBlo > > > ckLib.inf > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfig= B > > > lockLi > > > b.inf > > > similarity index 100% > > > rename from > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfi= g > > > Block > > > Lib.inf > > > rename to > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBl= o > > > ckLib.i > > > nf > > > -- > > > 2.16.2.windows.1 > > >=20