From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: ray.ni@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Thu, 25 Apr 2019 22:55:36 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Apr 2019 22:55:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,396,1549958400"; d="scan'208";a="145867528" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 25 Apr 2019 22:55:35 -0700 Received: from fmsmsx153.amr.corp.intel.com (10.18.125.6) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 25 Apr 2019 22:55:34 -0700 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by FMSMSX153.amr.corp.intel.com (10.18.125.6) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 25 Apr 2019 22:55:34 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.92]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.139]) with mapi id 14.03.0415.000; Fri, 26 Apr 2019 13:55:31 +0800 From: "Ni, Ray" To: "Gao, Zhichao" , "'devel@edk2.groups.io'" CC: Bret Barkelew , "Wang, Jian J" , "Zeng, Star" , "Gao, Liming" , Sean Brogan , "Michael Turner" Subject: Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit Thread-Topic: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit Thread-Index: AQHU6Rfyc/ibt9jRSkeXH7mvjl+IP6YznECAgAAAkcCAAHgFgIAAmmXggAAAx2CAGLOXAIAAsyhA Date: Fri, 26 Apr 2019 05:55:31 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C105782@SHSMSX104.ccr.corp.intel.com> References: <20190402054945.9712-1-zhichao.gao@intel.com> <20190402054945.9712-3-zhichao.gao@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C0DBCB4@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C0DBCD8@SHSMSX104.ccr.corp.intel.com> <3CE959C139B4C44DBEA1810E3AA6F9000B7C1237@SHSMSX101.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C0DD161@SHSMSX104.ccr.corp.intel.com> <3CE959C139B4C44DBEA1810E3AA6F9000B7C4589@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <3CE959C139B4C44DBEA1810E3AA6F9000B7C4589@SHSMSX101.ccr.corp.intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMmU4YzYzMjMtODlmOC00N2JlLWIxNDUtZmJmMWQzYzZiZjM1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoicTNoXC9TeTVmb2V3YklzQmZqRHJkcWhKa1M5MVVWS2V1RjBUQ2trUnVsUFZHSUF4SDh6dGN1UlZwV2N6OWNGeWcifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action 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 > -----Original Message----- > From: Gao, Zhichao > Sent: Thursday, April 25, 2019 8:13 PM > To: Ni, Ray ; 'devel@edk2.groups.io' > > Cc: Bret Barkelew ; Wang, Jian J > ; Zeng, Star ; Gao, Liming > ; Sean Brogan ; > Michael Turner > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > indications bit >=20 > Hi Ray, >=20 > Refer to the spec 2.8 section 3.4.3: > If system firmware supports boot option recovery as described in Section = 3.4, > system firmware must include a PlatformRecovery#### variable specifying a > short-form File Path Media Device Path (see Section 3.1.2) containing the > platform default file path for removable media (see Table 15). >=20 > Can we regard the "PlatformRecovery####" variable as optional? If so, I t= hink > it is fine to let the platform to control its behavior. Can you explain a bit more? What's the meaning of "let platform control beh= avior"? >=20 > Thanks, > Zhichao >=20 > > -----Original Message----- > > From: Ni, Ray > > Sent: Wednesday, April 10, 2019 10:00 AM > > To: Gao, Zhichao ; 'devel@edk2.groups.io' > > > > Cc: Bret Barkelew ; Wang, Jian J > > ; Zeng, Star ; Gao, Liming > > ; Sean Brogan ; > > Michael Turner > > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > > indications bit > > > > Sorry I didn't realize the impact in the beginning. > > > > > -----Original Message----- > > > From: Ni, Ray > > > Sent: Wednesday, April 10, 2019 10:00 AM > > > To: Gao, Zhichao ; 'devel@edk2.groups.io' > > > > > > Cc: Bret Barkelew ; Wang, Jian J > > > ; Zeng, Star ; Gao, > > > Liming ; Sean Brogan > > > ; Michael Turner > > > > > > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > > > indications bit > > > > > > The PlatformRecovery#### can be set by PlatformBootManagerLib. > > > We cannot execute these if the feature is not supported. > > > > > > Wait a sec, can I get clarification on the PCD's meaning a bit more: > > > Why do we need such a configuration flag? > > > Turning it off will break the functionality to boot default boot > > > option like EFI\BOOT\BOOTIA32.EFI EFI\BOOT\BOOTx64.EFI ... > > > > > > Is that expected? > > > If it's not, we still need to support booting from these boot > > > options even the PCD is OFF. > > > > > > Thanks, > > > Ray > > > > > > > -----Original Message----- > > > > From: Gao, Zhichao > > > > Sent: Wednesday, April 10, 2019 8:45 AM > > > > To: Ni, Ray ; 'devel@edk2.groups.io' > > > > > > > > Cc: Bret Barkelew ; Wang, Jian J > > > > ; Zeng, Star ; Gao, > > > > Liming ; Sean Brogan > > > > ; Michael Turner > > > > > > > > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > > > > indications bit > > > > > > > > This section can work fine even through the L"PlatformRecovery####" > > > > is not > > > set. > > > > But if we do not support it, it should be better not to run it. > > > > Thanks for your comments, I will update it later. > > > > > > > > Thanks, > > > > Zhichao > > > > > > > > > -----Original Message----- > > > > > From: Ni, Ray > > > > > Sent: Tuesday, April 9, 2019 5:32 PM > > > > > To: Gao, Zhichao ; 'devel@edk2.groups.io' > > > > > > > > > > Cc: Bret Barkelew ; Wang, Jian J > > > > > ; Zeng, Star ; Gao, > > > > > Liming ; Sean Brogan > > > > > ; Michael Turner > > > > > > > > > > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set > > > > > OS indications bit > > > > > > > > > > Resend to groups.io. > > > > > > > > > > > -----Original Message----- > > > > > > From: Ni, Ray > > > > > > Sent: Tuesday, April 9, 2019 5:31 PM > > > > > > To: Gao, Zhichao ; > > > > > > edk2-devel@lists.01.org > > > > > > Cc: Bret Barkelew ; Wang, Jian J > > > > > > ; Zeng, Star ; > > > > > > Gao, Liming ; Sean Brogan > > > > > > ; Michael Turner > > > > > > > > > > > > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set > > > > > > OS indications bit > > > > > > > > > > > > Zhichao, > > > > > > In the very bottom of BdsEntry(), below code is always > > > > > > executed no matter the PCD is true or false. > > > > > > I don't think that's the right behavior. > > > > > > > > > > > > if (!BootSuccess) { > > > > > > LoadOptions =3D EfiBootManagerGetLoadOptions > > > > > > (&LoadOptionCount, LoadOptionTypePlatformRecovery); > > > > > > ProcessLoadOptions (LoadOptions, LoadOptionCount); > > > > > > EfiBootManagerFreeLoadOptions (LoadOptions, > LoadOptionCount); > > > > > > } > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Gao, Zhichao > > > > > > > Sent: Tuesday, April 2, 2019 1:50 PM > > > > > > > To: edk2-devel@lists.01.org > > > > > > > Cc: Bret Barkelew ; Wang, Jian > > > > > > > J ; Ni, Ray ; Zeng, > > > > > > > Star ; Gao, Liming > > > > > > > ; Sean Brogan > > > > > > > ; Michael Turner > > > > > > > > > > > > > > Subject: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set > > > > > > > OS indications bit > > > > > > > > > > > > > > From: Bret Barkelew > > > > > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1678 > > > > > > > > > > > > > > Use the pcd PcdPlatformRecoverySupport to control whether to > > > > > > > set the PlatformRecovery#### variable and whether to set the > > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY bit of > variable > > > > > > > "OsIndicationsSupported". > > > > > > > > > > > > > > Cc: Jian J Wang > > > > > > > Cc: Ray Ni > > > > > > > Cc: Star Zeng > > > > > > > Cc: Liming Gao > > > > > > > Cc: Sean Brogan > > > > > > > Cc: Michael Turner > > > > > > > Cc: Bret Barkelew > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > > > Signed-off-by: Zhichao Gao > > > > > > > --- > > > > > > > MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 1 + > > > > > > > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 74 > > > > > > > +++++++++++++------ > > > > > -- > > > > > > -- > > > > > > > - > > > > > > > 2 files changed, 41 insertions(+), 34 deletions(-) > > > > > > > > > > > > > > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf > > > > > > > b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf > > > > > > > index 82eb8aafc6..9caabbce7f 100644 > > > > > > > --- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf > > > > > > > +++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf > > > > > > > @@ -101,6 +101,7 @@ > > > > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand > > > > > > > ## CONSUMES > > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable > > > > > ## > > > > > > > SOMETIMES_CONSUMES > > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed > > ## > > > > > > > CONSUMES > > > > > > > + > > gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport > > > > > > > ## CONSUMES > > > > > > > > > > > > > > [Depex] > > > > > > > TRUE > > > > > > > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c > > > > > > > b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c > > > > > > > index 8946d79ab2..ade77adb7d 100644 > > > > > > > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c > > > > > > > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c > > > > > > > @@ -552,10 +552,14 @@ BdsFormalizeOSIndicationVariable ( > > > > > > > // > > > > > > > Status =3D EfiBootManagerGetBootManagerMenu > > (&BootManagerMenu); > > > > > > > if (Status !=3D EFI_NOT_FOUND) { > > > > > > > - OsIndicationSupport =3D EFI_OS_INDICATIONS_BOOT_TO_FW_UI > > | > > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > > > + OsIndicationSupport =3D > EFI_OS_INDICATIONS_BOOT_TO_FW_UI; > > > > > > > EfiBootManagerFreeLoadOption (&BootManagerMenu); > > > > > > > } else { > > > > > > > - OsIndicationSupport =3D > > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > > > + OsIndicationSupport =3D 0; } > > > > > > > + > > > > > > > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > > > > > > > + OsIndicationSupport |=3D > > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > > > } > > > > > > > > > > > > > > Status =3D gRT->SetVariable ( @@ -769,41 +773,43 @@ > > > > > > > BdsEntry ( > > > > > > > // > > > > > > > InitializeLanguage (TRUE); > > > > > > > > > > > > > > - // > > > > > > > - // System firmware must include a PlatformRecovery#### > > > > > > > variable specifying > > > > > > > - // a short-form File Path Media Device Path containing > > > > > > > the platform default > > > > > > > - // file path for removable media > > > > > > > - // > > > > > > > - FilePath =3D FileDevicePath (NULL, > > > > > > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > > > > - Status =3D EfiBootManagerInitializeLoadOption ( > > > > > > > - &LoadOption, > > > > > > > - LoadOptionNumberUnassigned, > > > > > > > - LoadOptionTypePlatformRecovery, > > > > > > > - LOAD_OPTION_ACTIVE, > > > > > > > - L"Default PlatformRecovery", > > > > > > > - FilePath, > > > > > > > - NULL, > > > > > > > - 0 > > > > > > > - ); > > > > > > > - ASSERT_EFI_ERROR (Status); > > > > > > > - LoadOptions =3D EfiBootManagerGetLoadOptions > > > > > > > (&LoadOptionCount, LoadOptionTypePlatformRecovery); > > > > > > > - if (EfiBootManagerFindLoadOption (&LoadOption, > > > > > > > LoadOptions, > > > > > > > LoadOptionCount) =3D=3D -1) { > > > > > > > - for (Index =3D 0; Index < LoadOptionCount; Index++) { > > > > > > > - // > > > > > > > - // The PlatformRecovery#### options are sorted by > > OptionNumber. > > > > > > > - // Find the the smallest unused number as the new > > OptionNumber. > > > > > > > - // > > > > > > > - if (LoadOptions[Index].OptionNumber !=3D Index) { > > > > > > > - break; > > > > > > > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > > > > > > > + // > > > > > > > + // System firmware must include a PlatformRecovery#### > > > > > > > + variable > > > > > > > specifying > > > > > > > + // a short-form File Path Media Device Path containing > > > > > > > + the platform > > > > > > > default > > > > > > > + // file path for removable media > > > > > > > + // > > > > > > > + FilePath =3D FileDevicePath (NULL, > > > > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > > > > + Status =3D EfiBootManagerInitializeLoadOption ( > > > > > > > + &LoadOption, > > > > > > > + LoadOptionNumberUnassigned, > > > > > > > + LoadOptionTypePlatformRecovery, > > > > > > > + LOAD_OPTION_ACTIVE, > > > > > > > + L"Default PlatformRecovery", > > > > > > > + FilePath, > > > > > > > + NULL, > > > > > > > + 0 > > > > > > > + ); > > > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > > + LoadOptions =3D EfiBootManagerGetLoadOptions > > > > > > > + (&LoadOptionCount, > > > > > > > LoadOptionTypePlatformRecovery); > > > > > > > + if (EfiBootManagerFindLoadOption (&LoadOption, > > > > > > > + LoadOptions, > > > > > > > LoadOptionCount) =3D=3D -1) { > > > > > > > + for (Index =3D 0; Index < LoadOptionCount; Index++) { > > > > > > > + // > > > > > > > + // The PlatformRecovery#### options are sorted by > > OptionNumber. > > > > > > > + // Find the the smallest unused number as the new > > OptionNumber. > > > > > > > + // > > > > > > > + if (LoadOptions[Index].OptionNumber !=3D Index) { > > > > > > > + break; > > > > > > > + } > > > > > > > } > > > > > > > + LoadOption.OptionNumber =3D Index; > > > > > > > + Status =3D EfiBootManagerLoadOptionToVariable > (&LoadOption); > > > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > > } > > > > > > > - LoadOption.OptionNumber =3D Index; > > > > > > > - Status =3D EfiBootManagerLoadOptionToVariable (&LoadOpti= on); > > > > > > > - ASSERT_EFI_ERROR (Status); > > > > > > > + EfiBootManagerFreeLoadOption (&LoadOption); > > > > > > > + FreePool (FilePath); > > > > > > > + EfiBootManagerFreeLoadOptions (LoadOptions, > > > > > > > + LoadOptionCount); > > > > > > > } > > > > > > > - EfiBootManagerFreeLoadOption (&LoadOption); > > > > > > > - FreePool (FilePath); > > > > > > > - EfiBootManagerFreeLoadOptions (LoadOptions, > > > > > > > LoadOptionCount); > > > > > > > > > > > > > > // > > > > > > > // Report Status Code to indicate connecting drivers will > > > > > > > happen > > > > > > > -- > > > > > > > 2.21.0.windows.1