Re: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library

["Ni, Ray" <ray.ni@intel.com>]

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
> >
>