* [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
@ 2019-10-14 21:26 Kubacki, Michael A
2019-10-15 1:29 ` Ni, Ray
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Kubacki, Michael A @ 2019-10-14 21:26 UTC (permalink / raw)
To: devel; +Cc: Sai Chaganty, Ray Ni
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/DxeSmmFirmwareBootMediaLib.inf | 1 -
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf | 1 -
Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h | 48 ---------
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.c | 109 --------------------
4 files changed, 159 deletions(-)
diff --git a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
index 83ed5f04af..7e10b5f7a7 100644
--- a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
+++ b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
@@ -27,7 +27,6 @@
#
[Sources]
- FirmwareBootMediaLib.c
DxeSmmFirmwareBootMediaLib.c
[Packages]
diff --git a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
index 063c4027d3..ff1da31387 100644
--- a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
+++ b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
@@ -22,7 +22,6 @@
LIBRARY_CLASS = FirmwareBootMediaLib
[Sources]
- FirmwareBootMediaLib.c
PeiFirmwareBootMediaLib.c
[Packages]
diff --git a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
index aca9593a84..b36ebacf30 100644
--- a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
+++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.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/FirmwareBootMediaLib.c b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.c
deleted file mode 100644
index 11a14d172d..0000000000
--- a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-14 21:26 [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API Kubacki, Michael A
@ 2019-10-15 1:29 ` Ni, Ray
2019-10-15 2:21 ` Kubacki, Michael A
2019-10-15 17:20 ` Chaganty, Rangasai V
2019-10-17 7:25 ` [edk2-devel] " Nate DeSimone
2 siblings, 1 reply; 11+ messages in thread
From: Ni, Ray @ 2019-10-15 1:29 UTC (permalink / raw)
To: Kubacki, Michael A, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V
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/PeiFirmwareB
> ootMediaLib.inf | 1 -
> Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
> | 48 ---------
>
> Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBoo
> tMediaLib.c | 109 --------------------
> 4 files changed, 159 deletions(-)
>
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFir
> mwareBootMediaLib.inf
> b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFir
> mwareBootMediaLib.inf
> index 83ed5f04af..7e10b5f7a7 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFir
> 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/PeiFirmwar
> eBootMediaLib.inf
> b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwar
> eBootMediaLib.inf
> index 063c4027d3..ff1da31387 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwar
> eBootMediaLib.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFir
> +++ mwareBootMediaLib.inf
> @@ -22,7 +22,6 @@
> LIBRARY_CLASS = FirmwareBootMediaLib
>
> [Sources]
> - FirmwareBootMediaLib.c
> PeiFirmwareBootMediaLib.c
>
> [Packages]
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
> b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
> index aca9593a84..b36ebacf30 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
> +++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib
> +++ .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/FirmwareB
> ootMediaLib.c
> b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareB
> ootMediaLib.c
> deleted file mode 100644
> index 11a14d172d..0000000000
> ---
> a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareB
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-15 1:29 ` Ni, Ray
@ 2019-10-15 2:21 ` Kubacki, Michael A
2019-10-15 4:59 ` Ni, Ray
0 siblings, 1 reply; 11+ messages in thread
From: Kubacki, Michael A @ 2019-10-15 2:21 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V
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/PeiFirmwareB
> > ootMediaLib.inf | 1 -
> > Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
> > | 48 ---------
> >
> >
> Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBoo
> > 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/PeiFirmw
> > ar
> > eBootMediaLib.inf
> > b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmw
> > ar
> > eBootMediaLib.inf
> > index 063c4027d3..ff1da31387 100644
> > ---
> > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmw
> > ar
> > eBootMediaLib.inf
> > +++ b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiF
> > +++ ir
> > +++ mwareBootMediaLib.inf
> > @@ -22,7 +22,6 @@
> > LIBRARY_CLASS = FirmwareBootMediaLib
> >
> > [Sources]
> > - FirmwareBootMediaLib.c
> > PeiFirmwareBootMediaLib.c
> >
> > [Packages]
> > diff --git
> > a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
> > b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
> > index aca9593a84..b36ebacf30 100644
> > ---
> > a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
> > +++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaL
> > +++ 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/Firmware
> > B
> > ootMediaLib.c
> > b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firmware
> > B
> > ootMediaLib.c
> > deleted file mode 100644
> > index 11a14d172d..0000000000
> > ---
> > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firmware
> > 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-15 2:21 ` Kubacki, Michael A
@ 2019-10-15 4:59 ` Ni, Ray
2019-10-15 16:55 ` Kubacki, Michael A
0 siblings, 1 reply; 11+ messages in thread
From: Ni, Ray @ 2019-10-15 4:59 UTC (permalink / raw)
To: Kubacki, Michael A, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V
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/PeiFirmwar
> > eB
> > > ootMediaLib.inf | 1 -
> > >
> > > Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
> > > | 48 ---------
> > >
> > >
> > Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBo
> > 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/PeiFir
> > > mw
> > > ar
> > > eBootMediaLib.inf
> > > b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFir
> > > mw
> > > ar
> > > eBootMediaLib.inf
> > > index 063c4027d3..ff1da31387 100644
> > > ---
> > > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFir
> > > 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/FirmwareBootMediaLib
> > > .h
> > > b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib
> > > .h
> > > index aca9593a84..b36ebacf30 100644
> > > ---
> > > a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib
> > > .h
> > > +++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMedi
> > > +++ 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/Firmwa
> > > re
> > > B
> > > ootMediaLib.c
> > > b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firmwa
> > > re
> > > B
> > > ootMediaLib.c
> > > deleted file mode 100644
> > > index 11a14d172d..0000000000
> > > ---
> > > a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firmwa
> > > 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
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-15 4:59 ` Ni, Ray
@ 2019-10-15 16:55 ` Kubacki, Michael A
2019-10-16 6:30 ` Ni, Ray
0 siblings, 1 reply; 11+ messages in thread
From: Kubacki, Michael A @ 2019-10-15 16:55 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V
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
> > >
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-14 21:26 [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API Kubacki, Michael A
2019-10-15 1:29 ` Ni, Ray
@ 2019-10-15 17:20 ` Chaganty, Rangasai V
2019-10-17 7:25 ` [edk2-devel] " Nate DeSimone
2 siblings, 0 replies; 11+ messages in thread
From: Chaganty, Rangasai V @ 2019-10-15 17:20 UTC (permalink / raw)
To: Kubacki, Michael A, devel@edk2.groups.io; +Cc: Ni, Ray
Thanks for reducing the redundant APIs.
Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>
-----Original Message-----
From: Kubacki, Michael A
Sent: Monday, October 14, 2019 2:26 PM
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/DxeSmmFirmwareBootMediaLib.inf | 1 -
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf | 1 -
Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h | 48 ---------
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.c | 109 --------------------
4 files changed, 159 deletions(-)
diff --git a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
index 83ed5f04af..7e10b5f7a7 100644
--- a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.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/PeiFirmwareBootMediaLib.inf b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
index 063c4027d3..ff1da31387 100644
--- a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
+++ b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFir
+++ mwareBootMediaLib.inf
@@ -22,7 +22,6 @@
LIBRARY_CLASS = FirmwareBootMediaLib
[Sources]
- FirmwareBootMediaLib.c
PeiFirmwareBootMediaLib.c
[Packages]
diff --git a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
index aca9593a84..b36ebacf30 100644
--- a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
+++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib
+++ .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/FirmwareBootMediaLib.c b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.c
deleted file mode 100644
index 11a14d172d..0000000000
--- a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-15 16:55 ` Kubacki, Michael A
@ 2019-10-16 6:30 ` Ni, Ray
2019-10-16 17:38 ` Kubacki, Michael A
2019-10-17 7:26 ` [edk2-devel] " Nate DeSimone
0 siblings, 2 replies; 11+ messages in thread
From: Ni, Ray @ 2019-10-16 6:30 UTC (permalink / raw)
To: Kubacki, Michael A, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V
> 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/PeiFir
> > > > mw
> > > > ar
> > > > eB
> > > > > ootMediaLib.inf | 1 -
> > > > >
> > > > > Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaL
> > > > > ib
> > > > > .h
> > > > > | 48 ---------
> > > > >
> > > > >
> > > > Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/Firmwa
> > > > 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/PeiDxeSmmBootMediaLi
> > > > > +++ 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/FirmwareBootMedi
> > > > > aL
> > > > > ib
> > > > > .h
> > > > > b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMedi
> > > > > aL
> > > > > ib
> > > > > .h
> > > > > index aca9593a84..b36ebacf30 100644
> > > > > ---
> > > > > a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMedi
> > > > > aL
> > > > > ib
> > > > > .h
> > > > > +++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBoot
> > > > > +++ 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
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-16 6:30 ` Ni, Ray
@ 2019-10-16 17:38 ` Kubacki, Michael A
2019-10-17 7:26 ` [edk2-devel] " Nate DeSimone
1 sibling, 0 replies; 11+ messages in thread
From: Kubacki, Michael A @ 2019-10-16 17:38 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Chaganty, Rangasai V
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
> > > > >
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-14 21:26 [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API Kubacki, Michael A
2019-10-15 1:29 ` Ni, Ray
2019-10-15 17:20 ` Chaganty, Rangasai V
@ 2019-10-17 7:25 ` Nate DeSimone
2 siblings, 0 replies; 11+ messages in thread
From: Nate DeSimone @ 2019-10-17 7:25 UTC (permalink / raw)
To: devel@edk2.groups.io, Kubacki, Michael A; +Cc: Chaganty, Rangasai V, Ni, Ray
Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kubacki, Michael A
Sent: Monday, October 14, 2019 2:26 PM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: [edk2-devel] [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/DxeSmmFirmwareBootMediaLib.inf | 1 -
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf | 1 -
Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h | 48 ---------
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.c | 109 --------------------
4 files changed, 159 deletions(-)
diff --git a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
index 83ed5f04af..7e10b5f7a7 100644
--- a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.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/PeiFirmwareBootMediaLib.inf b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
index 063c4027d3..ff1da31387 100644
--- a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
+++ b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFir
+++ mwareBootMediaLib.inf
@@ -22,7 +22,6 @@
LIBRARY_CLASS = FirmwareBootMediaLib
[Sources]
- FirmwareBootMediaLib.c
PeiFirmwareBootMediaLib.c
[Packages]
diff --git a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
index aca9593a84..b36ebacf30 100644
--- a/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
+++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib
+++ .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/FirmwareBootMediaLib.c b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.c
deleted file mode 100644
index 11a14d172d..0000000000
--- a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-16 6:30 ` Ni, Ray
2019-10-16 17:38 ` Kubacki, Michael A
@ 2019-10-17 7:26 ` Nate DeSimone
2019-10-17 7:37 ` Ni, Ray
1 sibling, 1 reply; 11+ messages in thread
From: Nate DeSimone @ 2019-10-17 7:26 UTC (permalink / raw)
To: devel@edk2.groups.io, Ni, Ray, Kubacki, Michael A; +Cc: Chaganty, Rangasai V
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.kubacki@intel.com>; devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@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.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
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API
2019-10-17 7:26 ` [edk2-devel] " Nate DeSimone
@ 2019-10-17 7:37 ` Ni, Ray
0 siblings, 0 replies; 11+ messages in thread
From: Ni, Ray @ 2019-10-17 7:37 UTC (permalink / raw)
To: Desimone, Nathaniel L, devel@edk2.groups.io, Kubacki, Michael A
Cc: Chaganty, Rangasai V
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.desimone@intel.com>
> Sent: Thursday, October 17, 2019 3:27 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kubacki, Michael A
> <michael.a.kubacki@intel.com>
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@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.kubacki@intel.com>;
> devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@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.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
> > > > >
> > > >
> > >
> >
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-17 7:37 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-14 21:26 [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API Kubacki, Michael A
2019-10-15 1:29 ` Ni, Ray
2019-10-15 2:21 ` Kubacki, Michael A
2019-10-15 4:59 ` Ni, Ray
2019-10-15 16:55 ` Kubacki, Michael A
2019-10-16 6:30 ` Ni, Ray
2019-10-16 17:38 ` Kubacki, Michael A
2019-10-17 7:26 ` [edk2-devel] " Nate DeSimone
2019-10-17 7:37 ` Ni, Ray
2019-10-15 17:20 ` Chaganty, Rangasai V
2019-10-17 7:25 ` [edk2-devel] " Nate DeSimone
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox