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

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

When a API needs enough buffer prepared by caller, there are two styles of APIs:
1. Status FillBuffer (VOID *Buffer, UINTN BufferSize)
     BufferSize is passed by value so the API has no way to return back the required size.
     It's ok as long as the caller is able to calculate the proper size based on context.

2. Status FillBuffer (VOID *Buffer, UINTN &BufferSize)
     BufferSize is passed by pointer so the API can return back the required size.

In this case, if API cannot return back the buffer size back to caller, but caller is able
to calculate the size before calling the API, it follows the style #1 which is still ok.

In general, we cannot design a API to use debug messages tell caller the buffer size.

Thanks,
Ray

> -----Original Message-----
> From: Tsao, Ethan <ethan.tsao@intel.com>
> Sent: Friday, November 8, 2019 3:40 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,
> There is one debug message print available and requested Config block size
> while Config Block table is not enough to add new.
> Caller only know size is not enough by return status. With this debug
> message, Caller can know how many size need to enlarge.
> What do you think to keep this debug message?
> 
> DEBUG ((DEBUG_ERROR, "Available Config Block Size: 0x%x bytes /
> Requested Config Block Size: 0x%x bytes\n", ConfigBlkTblAddrPtr-
> >AvailableSize, ConfigBlkSize));
> 
> Best Regards,
> Ethan
> 
> > -----Original Message-----
> > From: Tsao, Ethan
> > Sent: Friday, November 8, 2019 3:17 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,
> > I do agree with you. I will remove all debug message since all error
> > scenario can be cover by return status.
> > Caller can understanding through return status.
> >
> > Best Regards,
> > Ethan
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Friday, November 8, 2019 2:25 PM
> > > To: Tsao, Ethan <ethan.tsao@intel.com>; devel@edk2.groups.io
> > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> > > Subject: RE: [PATCH] IntelSiliconPkg/Library:Add BaseConfigBlockLib
> > > Library
> > >
> > > 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/Base
> > > > Co
> > > > nf
> > > > igBlo
> > > > > > ckLib.c                      | 146 -------------------------------------------------------
> ----
> > --
> > > > ----
> > > > > -
> > > > > > --------------------------------------------------------------
> > > > > > --
> > > > > > --
> > > > > > ----
> > > > > > ----------
> > > > > >
> > > > > >
> > > > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLib/Base
> > > > Co
> > > > nf
> > > > igBlo
> > > > > > 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/BaseConfigBlockLi
> > > > > > b/
> > > > > > Ba
> > > > > > seCo
> > > > > > nfigB
> > > > > > lockLib.c
> > > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi
> > > > > > b/
> > > > > > Ba
> > > > > > seCo
> > > > > > nfig
> > > > > > BlockLib.c
> > > > > > deleted file mode 100644
> > > > > > index 369dab97ee..0000000000
> > > > > > ---
> > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi
> > > > > > b/
> > > > > > Ba
> > > > > > seCo
> > > > > > 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/BaseConfigBlockLi
> > > > > > b/
> > > > > > Ba
> > > > > > seCo
> > > > > > nfigB
> > > > > > lockLib.inf
> > > > > > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi
> > > > > > b/
> > > > > > Ba
> > > > > > seCo
> > > > > > nfig
> > > > > > BlockLib.inf
> > > > > > deleted file mode 100644
> > > > > > index a7def2481d..0000000000
> > > > > > ---
> > > > > > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseConfigBlockLi
> > > > > > b/
> > > > > > Ba
> > > > > > seCo
> > > > > > 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/
> > > > > > Ba
> > > > > > se
> > > > > > Conf
> > > > > > igBlo
> > > > > > ckLib.c
> > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/Bas
> > > > > > eC
> > > > > > on
> > > > > > figB
> > > > > > lockLi
> > > > > > b.c
> > > > > > similarity index 100%
> > > > > > rename from
> > > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Ba
> > > > > > se
> > > > > > Co
> > > > > > nfig
> > > > > > Block
> > > > > > Lib.c
> > > > > > rename to
> > > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseC
> > > > > > on
> > > > > > fi
> > > > > > gBlo
> > > > > > ckLib.c
> > > > > > diff --git
> > > > > > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/
> > > > > > Ba
> > > > > > se
> > > > > > Conf
> > > > > > igBlo
> > > > > > ckLib.inf
> > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/Bas
> > > > > > eC
> > > > > > on
> > > > > > figB
> > > > > > lockLi
> > > > > > b.inf
> > > > > > similarity index 100%
> > > > > > rename from
> > > > > > Silicon/Intel/KabylakeSiliconPkg/Library/BaseConfigBlockLib/Ba
> > > > > > se
> > > > > > Co
> > > > > > nfig
> > > > > > Block
> > > > > > Lib.inf
> > > > > > rename to
> > > > > > Silicon/Intel/IntelSiliconPkg/Library/BaseConfigBlockLib/BaseC
> > > > > > on
> > > > > > fi
> > > > > > gBlo
> > > > > > ckLib.i
> > > > > > nf
> > > > > > --
> > > > > > 2.16.2.windows.1
> > > > >
> > > >
> > >
>