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

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

Thanks Ray, this was a good discussion. I will leave the library instances as-is for now.

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, October 15, 2019 11: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
> 
> > 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.
> 
> I just feel that having two library instances increases the complexity.
> There were already arguments around EDKII like there are so many instances
> for a library class and people don't know which one is being used.
> But if you insist, I am ok with that. Removing unnecessary APIs resolved most
> of my concerns.
> 
> >
> > 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).
> 
> In fact the value of single library instance I can see is caller doesn't need to
> dig into the details of the library implementation. All they need to know is
> the library gets the info from HOB every time the API is called.
> With the two lib instances, consumers need to be aware of the different
> implementations.
> I am not using this to support my point. Just providing my thought.
> 
> >   1.b. Understand the nuances of their driver type in relation to the
> > library implementation.
> 
> I think having a single instance avoids DSC writers to supply two instances for
> different modules. Might be different from what you think😊
> 
> 
> >   1.c. Perform this evaluation every time the library is used.
> 
> Agree.
> 
> >   1.d. Implement overhead to manage the data in a global variable when
> > this could automatically be linked by the library.
> Some callers may not be aware of the lib implementation details. So they
> may still choose to cache to global variables.
> Some callers may use it only once. Having a lib global variable is also a waste.
> 
> >
> > 3. If the HOB is used, it blurs the implementation between the HOB
> > producer phase (PEI) and HOB consumer phase (DXE).
> 
> Don't understand.
> 
> >
> > 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.
> 
> If a single instance can support the real needs I tend to use single instances.
> It makes the code logic easy to trace. I like the multiple instances idea but
> want to avoid over-using them.
> 
> Again I am just providing what I think. They are not here to support my point.
> I am ok with current 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/PeiF
> > > > > ir
> > > > > mw
> > > > > ar
> > > > > eB
> > > > > > ootMediaLib.inf    |   1 -
> > > > > >
> > > > > > Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMedi
> > > > > > aL
> > > > > > ib
> > > > > > .h
> > > > > > |  48 ---------
> > > > > >
> > > > > >
> > > > > Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firm
> > > > > wa
> > > > > re
> > > > > 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/
> > > > > > Pe
> > > > > > iF
> > > > > > ir
> > > > > > mw
> > > > > > ar
> > > > > > eBootMediaLib.inf
> > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/
> > > > > > Pe
> > > > > > iF
> > > > > > ir
> > > > > > mw
> > > > > > ar
> > > > > > eBootMediaLib.inf
> > > > > > index 063c4027d3..ff1da31387 100644
> > > > > > ---
> > > > > > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/
> > > > > > Pe
> > > > > > iF
> > > > > > ir
> > > > > > mw
> > > > > > ar
> > > > > > eBootMediaLib.inf
> > > > > > +++ b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMedia
> > > > > > +++ Li
> > > > > > +++ b/
> > > > > > +++ 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/FirmwareBootMe
> > > > > > di
> > > > > > aL
> > > > > > ib
> > > > > > .h
> > > > > > b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMe
> > > > > > di
> > > > > > aL
> > > > > > ib
> > > > > > .h
> > > > > > index aca9593a84..b36ebacf30 100644
> > > > > > ---
> > > > > > a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMe
> > > > > > di
> > > > > > aL
> > > > > > ib
> > > > > > .h
> > > > > > +++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBo
> > > > > > +++ ot
> > > > > > +++ Me
> > > > > > +++ 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/
> > > > > > Fi
> > > > > > rm
> > > > > > wa
> > > > > > re
> > > > > > B
> > > > > > ootMediaLib.c
> > > > > > b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/
> > > > > > Fi
> > > > > > rm
> > > > > > wa
> > > > > > re
> > > > > > B
> > > > > > ootMediaLib.c
> > > > > > deleted file mode 100644
> > > > > > index 11a14d172d..0000000000
> > > > > > ---
> > > > > > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/
> > > > > > Fi
> > > > > > rm
> > > > > > 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
> > > > >
> > > >
> > >
> >
>