Nate,
I remember that I've agreed with current implementation.
I agree with you the library helps caller in a certain way. But another problem
we need to solve is how to simplify platform DSC with so many library
instances. I don't strongly prefer to merge the two library instances for this
specific case. But we need to keep in mind that any addition of library APIs,
library instances is actually a burden to platform developers because they need
to make choices. (This is why I like iPhone over Android because Android opens
so many settings to end-users.)
Thanks,
Ray
> -----Original Message-----
> From: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>
> Sent: Thursday, October 17, 2019 3:27 PM
> To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Kubacki, Michael A
> <michael.a.kuba...@intel.com>
> Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>
> Subject: RE: [edk2-devel] [edk2-platforms][PATCH V1 1/1]
> IntelSiliconPkg/BootMediaLib: Reduce library API
>
> Hi Ray,
>
> This really comes down to a philosophical question of how much do we wish
> to shield the user of the BootMediaLib against the nuances of the library's
> underlying implementation.
>
> The primary function of BootMediaLib is retrieval of state. All of the
> functions
> in BootMediaLib are accessors: BootMediaIsSpi(), GetBootMediaType(), etc.
> Since the purpose of the library is the manipulation of stateful data, it
> generally is assumed by most programmers that the data accessors
> themselves are stateless. In simpler words, most programmers expect to be
> able to call the BootMediaIsSpi() function without having to consider what
> the current environmental context is of the system.
>
> In your proposal, the programmer must consider if they are writing PEI, DXE,
> SMM, and RuntimeDXE code when using the BootMediaLib. I consider it very
> confusing if I was required to call BootMediaIsSpi() everywhere in PEI, while
> at the same time I was required to only call BootMediaIsSpi() from the entry
> point for SMM or RuntimeDXE drivers. Michael's solution abstracts away this
> complexity and allows a single programming model to be used everywhere.
>
> I agree with Michael that encapsulation of the environmental state is
> desirable especially for code that is used in the implementation of UEFI
> variable services. And I recommend that the code be merged as-is.
>
> Regards,
> Nate
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, October 15, 2019 11:30 PM
> To: Kubacki, Michael A <michael.a.kuba...@intel.com>;
> devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>
> Subject: Re: [edk2-devel] [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...@intel.com>
> > > Sent: Monday, October 14, 2019 9:59 PM
> > > To: Kubacki, Michael A <michael.a.kuba...@intel.com>;
> > > devel@edk2.groups.io
> > > Cc: Chaganty, Rangasai V <rangasai.v.chaga...@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.kuba...@intel.com>
> > > > Sent: Tuesday, October 15, 2019 10:22 AM
> > > > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io
> > > > Cc: Chaganty, Rangasai V <rangasai.v.chaga...@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...@intel.com>
> > > > > Sent: Monday, October 14, 2019 6:30 PM
> > > > > To: Kubacki, Michael A <michael.a.kuba...@intel.com>;
> > > > > devel@edk2.groups.io
> > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>
> > > > > Subject: RE: [edk2-platforms][PATCH V1 1/1]
> > > IntelSiliconPkg/BootMediaLib:
> > > > > Reduce library API
> > > > >
> > > > > Reviewed-by: Ray Ni <ray...@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.kuba...@intel.com>
> > > > > > Sent: Tuesday, October 15, 2019 5:26 AM
> > > > > > To: devel@edk2.groups.io
> > > > > > Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Ni,
> > > > > > Ray <ray...@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.chaga...@intel.com>
> > > > > > Cc: Ray Ni <ray...@intel.com>
> > > > > > Signed-off-by: Michael Kubacki <michael.a.kuba...@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
> > > > >
> > > >
> > >
> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49172): https://edk2.groups.io/g/devel/message/49172
Mute This Topic: https://groups.io/mt/34538841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
