From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mx.groups.io with SMTP id smtpd.web12.233.1571115574520389314 for ; Mon, 14 Oct 2019 21:59:34 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.65, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Oct 2019 21:59:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,297,1566889200"; d="scan'208";a="195175627" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga007.fm.intel.com with ESMTP; 14 Oct 2019 21:59:33 -0700 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 14 Oct 2019 21:59:32 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 14 Oct 2019 21:59:32 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.166]) by shsmsx102.ccr.corp.intel.com ([169.254.2.176]) with mapi id 14.03.0439.000; Tue, 15 Oct 2019 12:59:29 +0800 From: "Ni, Ray" To: "Kubacki, Michael A" , "devel@edk2.groups.io" CC: "Chaganty, Rangasai V" Subject: Re: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API Thread-Topic: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: Reduce library API Thread-Index: AQHVgtYIOTkrtkYpnka2pkFh7v6JB6da6TAggAALuWCAAC6TEA== Date: Tue, 15 Oct 2019 04:59:28 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C32317E@SHSMSX104.ccr.corp.intel.com> References: <20191014212616.18376-1-michael.a.kubacki@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C322E81@SHSMSX104.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 differen= t modules. In another word, I think you can keep the PEI version as a base library ins= tances and all modules can use that instance. Thanks, Ray > -----Original Message----- > From: Kubacki, Michael A > Sent: Tuesday, October 15, 2019 10:22 AM > To: Ni, Ray ; devel@edk2.groups.io > Cc: Chaganty, Rangasai V > Subject: RE: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLib: > Reduce library API >=20 > The two library instances work within the constraints of PEI, DXE, Runtim= e > DXE, and SMM. >=20 > I cannot find how a single instance can support all these environments (i= n as > clean a manner) as is done with the two instances. >=20 > Relevant constraints: > * PEI: Cannot write to global variable > * Runtime DXE / SMM: Boot Services are not available outside module entry > point >=20 > 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 availab= le > and store the value in a global variable so it can be accessed in Runtime= DXE > and SMM. >=20 > Two separate instances resolve these requirements quite naturally. >=20 > How would you propose meeting the same level of support with one > instance? >=20 > Thanks, > Michael >=20 > > -----Original Message----- > > From: Ni, Ray > > Sent: Monday, October 14, 2019 6:30 PM > > To: Kubacki, Michael A ; > > devel@edk2.groups.io > > Cc: Chaganty, Rangasai V > > Subject: RE: [edk2-platforms][PATCH V1 1/1] IntelSiliconPkg/BootMediaLi= b: > > Reduce library API > > > > Reviewed-by: Ray Ni > > > > 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 > > > Sent: Tuesday, October 15, 2019 5:26 AM > > > To: devel@edk2.groups.io > > > Cc: Chaganty, Rangasai V ; Ni, Ray > > > > > > 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 > > > Cc: Ray Ni > > > Signed-off-by: Michael Kubacki > > > --- > > > > > > > > > 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 =3D 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 de= vice 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 de= vice > 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 phase= s. > > > - > > > -Copyright (c) 2019, Intel Corporation. All rights reserved.
> > > -SPDX-License-Identifier: BSD-2-Clause-Patent > > > - > > > -**/ > > > - > > > -#include > > > -#include > > > -#include > > > - > > > -/** > > > - 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 de= vice or > > the > > > boot media is unknown > > > -**/ > > > -BOOLEAN > > > -EFIAPI > > > -FirmwareBootMediaIsSpi ( > > > - VOID > > > - ) > > > -{ > > > - EFI_STATUS Status; > > > - FW_BOOT_MEDIA_TYPE BootMedia; > > > - > > > - Status =3D GetFirmwareBootMediaType (&BootMedia); > > > - if (EFI_ERROR (Status) || BootMedia !=3D 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 de= vice > or > > > the boot media is unknown > > > -**/ > > > -BOOLEAN > > > -EFIAPI > > > -FirmwareBootMediaIsUfs ( > > > - VOID > > > - ) > > > -{ > > > - EFI_STATUS Status; > > > - FW_BOOT_MEDIA_TYPE BootMedia; > > > - > > > - Status =3D GetFirmwareBootMediaType (&BootMedia); > > > - if (EFI_ERROR (Status) || BootMedia !=3D 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 =3D GetFirmwareBootMediaType (&BootMedia); > > > - if (EFI_ERROR (Status) || BootMedia !=3D 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 =3D GetFirmwareBootMediaType (&BootMedia); > > > - if (EFI_ERROR (Status) || BootMedia !=3D FwBootMediaNvme) { > > > - return FALSE; > > > - } else { > > > - return TRUE; > > > - } > > > -} > > > -- > > > 2.16.2.windows.1 > > >=20