Re: [edk2-devel] [Patch V3] IntelSiliconPkg/Library:Add BaseConfigBlockLib Library

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

This was committed and broke the build for almost all board packages that consume this library:
  * KabylakeOpenBoardPkg/GalagoPro3
  * KabylakeOpenBoardPkg/KabylakeRvp3
  * WhiskeylakeOpenBoardPkg/WhiskeylakeURvp

I am surprised that this patch series was approved with no thought to package DSC file changes
Needed for library consumers. I will submit a patch in the affected board packages in edk2-platforms
but please check impact more carefully in the future.

Thanks,
Michael

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Monday, November 11, 2019 12:00 AM
> To: Tsao, Ethan <ethan.tsao@intel.com>; devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> Subject: Re: [edk2-devel] [Patch V3] IntelSiliconPkg/Library:Add
> BaseConfigBlockLib Library
> 
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> > -----Original Message-----
> > From: Tsao, Ethan <ethan.tsao@intel.com>
> > Sent: Monday, November 11, 2019 2:05 PM
> > To: devel@edk2.groups.io
> > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray
> > <ray.ni@intel.com>
> > Subject: [Patch V3] 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   |  10 ++-
> --
> > -----
> >  Silicon/Intel/{KabylakeSiliconPkg =>
> > IntelSiliconPkg}/Library/BaseConfigBlockLib/BaseConfigBlockLib.inf |   0
> >  4 files changed, 2 insertions(+), 183 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 84%
> > rename from
> > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConfig
> > Block
> > Lib.c
> > rename to
> > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigBlo
> > ckLib.c
> > index b9bd6f3a48..77eb2244c2 100644
> > ---
> > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/BaseConf
> > igBlo
> > ckLib.c
> > +++
> > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseConfigB
> > lockLi
> > b.c
> > @@ -34,13 +34,11 @@ CreateConfigBlockTable (
> >    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; @@ -77,14 +75,11 @@
> > AddConfigBlock (
> >    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;
> >    }
> >
> > @@ -95,7 +90,6 @@ AddConfigBlock (
> >    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;
> >  }
> >
> > @@ -140,6 +134,6 @@ GetConfigBlock (
> >      }
> >      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/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
> 
> 
>