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

["Ethan Tsao" <ethan.tsao@intel.com>]

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
>