Re: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API

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

My concerns with that approach:

1. In general, I believe it is better if the library reads the value once and caches it. The firmware
boot media is fixed after power-on by nature and in some platforms, boot media information 
may be provided to the IBB in a temporary SRAM (or other volatile memory) early in the boot
flow that is temporary (e.g. not accessible after main memory initialization). Here the HOB to
global variable transition in DXE, Runtime DXE, SMM is a transparent mechanism to the library
consumers to get the boot media information regardless of early boot memory properties.

2. Forcing library consumers to cache puts unnecessary burden on a large number of library consumers to:
  1.a. Understand the library implementation (lack of encapsulation).
  1.b. Understand the nuances of their driver type in relation to the library implementation.
  1.c. Perform this evaluation every time the library is used.
  1.d. Implement overhead to manage the data in a global variable when this could automatically be linked by the library.

3. If the HOB is used, it blurs the implementation between the HOB producer phase (PEI) and
HOB consumer phase (DXE).

We have many libraries with phase-specific instances. When it reduces programming mistakes
and eases integration I believe this is beneficial. In this case, I feel the net result for library
consumers is better if they simply manage the instance in the DSC as opposed to modifying
source in drivers on a case-by-case basis dependent on the library implementation.

Thanks,
Michael

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Monday, October 14, 2019 9:59 PM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> Subject: RE: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib:
> Reduce library API
> 
> Mike,
> I don't think the library needs to cache the boot media type for RT and SMM.
> RT and SMM modules can have a module local variable to cache the boot
> media type.
> It simplifies the implementation of this library and also the platform DSC
> because there is no need to choose different library instances for different
> modules.
> 
> In another word, I think you can keep the PEI version as a base library
> instances and all modules can use that instance.
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> > Sent: Tuesday, October 15, 2019 10:22 AM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> > Subject: RE: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib:
> > Reduce library API
> >
> > The two library instances work within the constraints of PEI, DXE,
> > Runtime DXE, and SMM.
> >
> > I cannot find how a single instance can support all these environments
> > (in as clean a manner) as is done with the two instances.
> >
> > Relevant constraints:
> > * PEI: Cannot write to global variable
> > * Runtime DXE / SMM: Boot Services are not available outside module
> > entry point
> >
> > Solution:
> > * PEI instance: Always retrieve the value from a HOB (use heap for R/W).
> > * DXE, Runtime DXE, SMM instance: Retrieve the value from the HOB in
> > the entry point
> > (constructor) when the driver is dispatched and Boot Services are
> > available and store the value in a global variable so it can be
> > accessed in Runtime DXE and SMM.
> >
> > Two separate instances resolve these requirements quite naturally.
> >
> > How would you propose meeting the same level of support with one
> > instance?
> >
> > Thanks,
> > Michael
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Monday, October 14, 2019 6:30 PM
> > > To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> > > devel@edk2.groups.io
> > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> > > Subject: RE: [edk2-platforms][PATCH V1 1/1]
> IntelSiliconPkg/BootMediaLib:
> > > Reduce library API
> > >
> > > Reviewed-by: Ray Ni <ray.ni@intel.com>
> > >
> > > Mike,
> > > Thanks for reducing the API.
> > >
> > > For the other comments I raised (single library instance usable for
> > > PEI and DXE), do you have any comments?
> > >
> > >
> > > > -----Original Message-----
> > > > From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> > > > Sent: Tuesday, October 15, 2019 5:26 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray
> > > > <ray.ni@intel.com>
> > > > Subject: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib:
> > > > Reduce library API
> > > >
> > > > Removes the following functions from FirmwareBootMediaLib.h:
> > > >  * FirmwareBootMediaIsSpi ()
> > > >  * FirmwareBootMediaIsUfs ()
> > > >  * FirmwareBootMediaIsEmmc ()
> > > >  * FirmwareBootMediaIsNvme ()
> > > >
> > > > It is preferred to have a single method to retrieve the firmware
> > > > boot
> > > media.
> > > > To reduce overall maintenance effort over time, the
> > > > FirmwareBootMediaIsXxx () pattern is removed in favor of returning
> > > > the firmware boot media type via GetFirmwareBootMediaType ().
> > > >
> > > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> > > > ---
> > > >
> > > >
> > >
> >
> Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirm
> > > > wareBootMediaLib.inf |   1 -
> > > >
> > > >
> > > Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmw
> > > ar
> > > eB
> > > > ootMediaLib.inf    |   1 -
> > > >
> > > > Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib
> > > > .h
> > > > |  48 ---------
> > > >
> > > >
> > > Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firmware
> > > Bo
> > > o
> > > > tMediaLib.c         | 109 --------------------
> > > >  4 files changed, 159 deletions(-)
> > > >
> > > > diff --git
> > > >
> > >
> >
> a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFi
> > > > r
> > > > mwareBootMediaLib.inf
> > > >
> > >
> >
> b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFi
> > > > r
> > > > mwareBootMediaLib.inf
> > > > index 83ed5f04af..7e10b5f7a7 100644
> > > > ---
> > > >
> > >
> >
> a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFi
> > > > r
> > > > mwareBootMediaLib.inf
> > > > +++
> > > >
> > b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmm
> > > > +++ FirmwareBootMediaLib.inf
> > > > @@ -27,7 +27,6 @@
> > > >  #
> > > >
> > > >  [Sources]
> > > > -  FirmwareBootMediaLib.c
> > > >    DxeSmmFirmwareBootMediaLib.c
> > > >
> > > >  [Packages]
> > > > diff --git
> > > > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiF
> > > > ir
> > > > mw
> > > > ar
> > > > eBootMediaLib.inf
> > > > b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiF
> > > > ir
> > > > mw
> > > > ar
> > > > eBootMediaLib.inf
> > > > index 063c4027d3..ff1da31387 100644
> > > > ---
> > > > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiF
> > > > ir
> > > > mw
> > > > ar
> > > > eBootMediaLib.inf
> > > > +++ b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/
> > > > +++ Pe
> > > > +++ iF
> > > > +++ ir
> > > > +++ mwareBootMediaLib.inf
> > > > @@ -22,7 +22,6 @@
> > > >    LIBRARY_CLASS        = FirmwareBootMediaLib
> > > >
> > > >  [Sources]
> > > > -  FirmwareBootMediaLib.c
> > > >    PeiFirmwareBootMediaLib.c
> > > >
> > > >  [Packages]
> > > > diff --git
> > > > a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaL
> > > > ib
> > > > .h
> > > > b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaL
> > > > ib
> > > > .h
> > > > index aca9593a84..b36ebacf30 100644
> > > > ---
> > > > a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaL
> > > > ib
> > > > .h
> > > > +++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMe
> > > > +++ di
> > > > +++ aL
> > > > +++ ib
> > > > +++ .h
> > > > @@ -55,52 +55,4 @@ FirmwareBootMediaIsKnown (
> > > >    VOID
> > > >    );
> > > >
> > > > -/**
> > > > -  Determines if the platform firmware is booting from SPI.
> > > > -
> > > > -  @retval TRUE        Platform firmware is booting from SPI
> > > > -  @retval FALSE       Platform firmware is booting from a non-SPI device
> or
> > > the
> > > > boot media is unknown
> > > > -**/
> > > > -BOOLEAN
> > > > -EFIAPI
> > > > -FirmwareBootMediaIsSpi (
> > > > -  VOID
> > > > -  );
> > > > -
> > > > -/**
> > > > -  Determines if the platform firmware is booting from UFS.
> > > > -
> > > > -  @retval TRUE        Platform firmware is booting from UFS
> > > > -  @retval FALSE       Platform firmware is booting from a non-UFS device
> > or
> > > > the boot media is unknown
> > > > -**/
> > > > -BOOLEAN
> > > > -EFIAPI
> > > > -FirmwareBootMediaIsUfs (
> > > > -  VOID
> > > > -  );
> > > > -
> > > > -/**
> > > > -  Determines if the platform firmware is booting from eMMC.
> > > > -
> > > > -  @retval TRUE        Platform firmware is booting from eMMC
> > > > -  @retval FALSE       Platform firmware is booting from a non-eMMC
> > device
> > > or
> > > > the boot media is unknown
> > > > -**/
> > > > -BOOLEAN
> > > > -EFIAPI
> > > > -FirmwareBootMediaIsEmmc (
> > > > -  VOID
> > > > -  );
> > > > -
> > > > -/**
> > > > -  Determines if the platform firmware is booting from NVMe.
> > > > -
> > > > -  @retval TRUE        Platform firmware is booting from NVMe.
> > > > -  @retval FALSE       Platform firmware is booting from a non-NVMe
> > device
> > > or
> > > > the boot media is unknown
> > > > -**/
> > > > -BOOLEAN
> > > > -EFIAPI
> > > > -FirmwareBootMediaIsNvme (
> > > > -  VOID
> > > > -  );
> > > > -
> > > >  #endif
> > > > diff --git
> > > > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firm
> > > > wa
> > > > re
> > > > B
> > > > ootMediaLib.c
> > > > b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firm
> > > > wa
> > > > re
> > > > B
> > > > ootMediaLib.c
> > > > deleted file mode 100644
> > > > index 11a14d172d..0000000000
> > > > ---
> > > > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firm
> > > > wa
> > > > re
> > > > B
> > > > ootMediaLib.c
> > > > +++ /dev/null
> > > > @@ -1,109 +0,0 @@
> > > > -/** @file
> > > > -  This library identifies the firmware boot media device.
> > > > -
> > > > -  The firmware boot media device is used to make system
> > > > initialization decisions in the boot flow dependent
> > > > -  upon firmware boot media. Note that the firmware boot media is
> > > > the storage media that the boot firmware is stored on.
> > > > -  It is not the OS storage media which may be stored upon a
> > > > different
> > > > non- volatile storage device.
> > > > -
> > > > -  This file contains library implementation common to all boot phases.
> > > > -
> > > > -Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > > > -SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > -
> > > > -**/
> > > > -
> > > > -#include <Library/BaseLib.h>
> > > > -#include <Library/DebugLib.h>
> > > > -#include <Library/FirmwareBootMediaLib.h>
> > > > -
> > > > -/**
> > > > -  Determines if the platform firmware is booting from SPI.
> > > > -
> > > > -  @retval TRUE        Platform firmware is booting from SPI
> > > > -  @retval FALSE       Platform firmware is booting from a non-SPI device
> or
> > > the
> > > > boot media is unknown
> > > > -**/
> > > > -BOOLEAN
> > > > -EFIAPI
> > > > -FirmwareBootMediaIsSpi (
> > > > -  VOID
> > > > -  )
> > > > -{
> > > > -  EFI_STATUS          Status;
> > > > -  FW_BOOT_MEDIA_TYPE  BootMedia;
> > > > -
> > > > -  Status = GetFirmwareBootMediaType (&BootMedia);
> > > > -  if (EFI_ERROR (Status) || BootMedia != FwBootMediaSpi) {
> > > > -    return FALSE;
> > > > -  } else {
> > > > -    return TRUE;
> > > > -  }
> > > > -}
> > > > -
> > > > -/**
> > > > -  Determines if the platform firmware is booting from UFS.
> > > > -
> > > > -  @retval TRUE        Platform firmware is booting from UFS
> > > > -  @retval FALSE       Platform firmware is booting from a non-UFS device
> > or
> > > > the boot media is unknown
> > > > -**/
> > > > -BOOLEAN
> > > > -EFIAPI
> > > > -FirmwareBootMediaIsUfs (
> > > > -  VOID
> > > > -  )
> > > > -{
> > > > -  EFI_STATUS          Status;
> > > > -  FW_BOOT_MEDIA_TYPE  BootMedia;
> > > > -
> > > > -  Status = GetFirmwareBootMediaType (&BootMedia);
> > > > -  if (EFI_ERROR (Status) || BootMedia != FwBootMediaUfs) {
> > > > -    return FALSE;
> > > > -  } else {
> > > > -    return TRUE;
> > > > -  }
> > > > -}
> > > > -
> > > > -/**
> > > > -  Determines if the platform firmware is booting from eMMC.
> > > > -
> > > > -  @retval TRUE        Platform firmware is booting from eMMC
> > > > -  @retval FALSE       Platform firmware is booting from a non-eMMC
> > device
> > > or
> > > > the boot media is unknown
> > > > -**/
> > > > -BOOLEAN
> > > > -EFIAPI
> > > > -FirmwareBootMediaIsEmmc (
> > > > -  VOID
> > > > -  )
> > > > -{
> > > > -  EFI_STATUS          Status;
> > > > -  FW_BOOT_MEDIA_TYPE  BootMedia;
> > > > -
> > > > -  Status = GetFirmwareBootMediaType (&BootMedia);
> > > > -  if (EFI_ERROR (Status) || BootMedia != FwBootMediaEmmc) {
> > > > -    return FALSE;
> > > > -  } else {
> > > > -    return TRUE;
> > > > -  }
> > > > -}
> > > > -
> > > > -/**
> > > > -  Determines if the platform firmware is booting from NVMe.
> > > > -
> > > > -  @retval TRUE        Platform firmware is booting from NVMe.
> > > > -  @retval FALSE       Platform firmware is booting from a non-NVMe
> > device
> > > or
> > > > the boot media is unknown
> > > > -**/
> > > > -BOOLEAN
> > > > -EFIAPI
> > > > -FirmwareBootMediaIsNvme (
> > > > -  VOID
> > > > -  )
> > > > -{
> > > > -  EFI_STATUS          Status;
> > > > -  FW_BOOT_MEDIA_TYPE  BootMedia;
> > > > -
> > > > -  Status = GetFirmwareBootMediaType (&BootMedia);
> > > > -  if (EFI_ERROR (Status) || BootMedia != FwBootMediaNvme) {
> > > > -    return FALSE;
> > > > -  } else {
> > > > -    return TRUE;
> > > > -  }
> > > > -}
> > > > --
> > > > 2.16.2.windows.1
> > >
> >
>