From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: zhichao.gao@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Thu, 13 Jun 2019 00:51:59 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Jun 2019 00:51:59 -0700 X-ExtLoop1: 1 Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga008.fm.intel.com with ESMTP; 13 Jun 2019 00:51:59 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 13 Jun 2019 00:51:59 -0700 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 13 Jun 2019 00:51:58 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.104]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.188]) with mapi id 14.03.0415.000; Thu, 13 Jun 2019 15:51:55 +0800 From: "Gao, Zhichao" To: "Wu, Hao A" , "devel@edk2.groups.io" CC: "Wang, Jian J" , "Ni, Ray" , "Zeng, Star" , "Gao, Liming" , Sean Brogan , Michael Turner , Bret Barkelew Subject: Re: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery Thread-Topic: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery Thread-Index: AQHVGnGeZiLMVfjmKk2YfDMrLWK6W6aZPdnAgAAF70A= Date: Thu, 13 Jun 2019 07:51:54 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B7F51C6@SHSMSX101.ccr.corp.intel.com> References: <20190604010515.14352-1-zhichao.gao@intel.com> <20190604010515.14352-3-zhichao.gao@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: zhichao.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Wu, Hao A > Sent: Thursday, June 13, 2019 3:39 PM > To: devel@edk2.groups.io; Gao, Zhichao > Cc: Wang, Jian J ; Ni, Ray ; Ze= ng, > Star ; Gao, Liming ; Sean > Brogan ; Michael Turner > ; Bret Barkelew > > Subject: RE: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/BdsDxe: Use a pcd > to control PlatformRecovery >=20 > One comment below: >=20 > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Gao, Zhichao > > Sent: Tuesday, June 04, 2019 9:05 AM > > To: devel@edk2.groups.io > > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming; Sean > > Brogan; Michael Turner; Bret Barkelew > > Subject: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/BdsDxe: Use a pcd > to > > control PlatformRecovery > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1678 > > > > Use the PcdPlatformRecoverySupport to control the function of platform > > recovery in BDS. > > First, set the variable's ("OsIndicationsSupported") > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY bit base on the pcd. > > It would affect the variable "OsIndications". > > While the platform does not support the platform recovery, it is > > inappropriate to set a PlatformRecovery#### variable. So skip setting > > the variable. But it should remain the behavior of booting from a > > default file path (such as \EFI\BOOT\BOOTX64.EFI) to be compatible > > with the previous version UEFI spec. > > > > Add memory check before build platform default boot option. If fail to > > allocate memory for the defualt boot file path, put the system into > > dead loop to indicate it is unable to boot. > > > > Cc: Jian J Wang > > Cc: Hao Wu > > Cc: Ray Ni > > Cc: Star Zeng > > Cc: Liming Gao > > Cc: Sean Brogan > > Cc: Michael Turner > > Cc: Bret Barkelew > > Signed-off-by: Zhichao Gao > > --- > > MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 3 +- > > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 71 +++++++++++++++----- > - > > --- > > 2 files changed, 47 insertions(+), 27 deletions(-) > > > > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf > > b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf > > index 6913389d34..7f94ca17df 100644 > > --- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf > > +++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf > > @@ -5,7 +5,7 @@ > > # gEfiBdsArchProtocolGuid. After DxeCore finish dispatching, DxeCore > > will invoke Entry # interface of protocol gEfiBdsArchProtocolGuid, > > then BDS phase is entered. > > # > > -# Copyright (c) 2008 - 2018, Intel Corporation. All rights > > reserved.
> > +# Copyright (c) 2008 - 2019, Intel Corporation. All rights > > +reserved.
> > # SPDX-License-Identifier: BSD-2-Clause-Patent # ## @@ -95,6 +95,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 9d312bd982..8c8a0b5236 100644 > > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c > > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c > > @@ -5,7 +5,7 @@ > > After DxeCore finish DXE phase, gEfiBdsArchProtocolGuid->BdsEntry > > will be invoked > > to enter BDS phase. > > > > -Copyright (c) 2004 - 2018, Intel Corporation. All rights > > reserved.
> > +Copyright (c) 2004 - 2019, Intel Corporation. All rights > > +reserved.
> > (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> > (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> > SPDX-License-Identifier: BSD-2-Clause-Patent @@ -546,10 +546,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 ( > > @@ -662,6 +666,7 @@ BdsEntry ( > > BOOLEAN BootSuccess; > > EFI_DEVICE_PATH_PROTOCOL *FilePath; > > EFI_STATUS BootManagerMenuStatus; > > + EFI_BOOT_MANAGER_LOAD_OPTION PlatformDefaultBootOption; > > > > HotkeyTriggered =3D NULL; > > Status =3D EFI_SUCCESS; > > @@ -763,14 +768,13 @@ 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); > > + if (FilePath =3D=3D NULL) { > > + DEBUG ((DEBUG_ERROR, "Fail to allocate memory for defualt boot > > + file > > path. Unable to boot.\n")); > > + CpuDeadLoop (); > > + } > > Status =3D EfiBootManagerInitializeLoadOption ( > > - &LoadOption, > > + &PlatformDefaultBootOption, > > LoadOptionNumberUnassigned, > > LoadOptionTypePlatformRecovery, > > LOAD_OPTION_ACTIVE, > > @@ -780,24 +784,31 @@ BdsEntry ( > > 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; > > + > > + // > > + // 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 if the platform supports Platform > > Recovery. > > + // > > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > > + LoadOptions =3D EfiBootManagerGetLoadOptions (&LoadOptionCount, > > LoadOptionTypePlatformRecovery); > > + if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > LoadOptionCount) =3D=3D -1) { >=20 >=20 > In the above 'if' statement, 'LoadOption' is not being initialized. > Is this a typo here? Yes. You are right. 'LoadOption' should change to 'PlatformDefaultBootOpti= on'. I would update this patch set with your comments. And ignore my push reque= st before. Thanks, Zhichao >=20 > Best Regards, > Hao Wu >=20 >=20 > > + for (Index =3D 0; Index < LoadOptionCount; Index++) { > > + // > > + // The PlatformRecovery#### options are sorted by OptionNumbe= r. > > + // Find the the smallest unused number as the new OptionNumbe= r. > > + // > > + if (LoadOptions[Index].OptionNumber !=3D Index) { > > + break; > > + } > > } > > + PlatformDefaultBootOption.OptionNumber =3D Index; > > + Status =3D EfiBootManagerLoadOptionToVariable > > (&PlatformDefaultBootOption); > > + ASSERT_EFI_ERROR (Status); > > } > > - LoadOption.OptionNumber =3D Index; > > - Status =3D EfiBootManagerLoadOptionToVariable (&LoadOption); > > - ASSERT_EFI_ERROR (Status); > > + EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > > } > > - EfiBootManagerFreeLoadOption (&LoadOption); > > FreePool (FilePath); > > - EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > > > > // > > // Report Status Code to indicate connecting drivers will happen @@ > > -1043,10 +1054,18 @@ BdsEntry ( > > } > > > > if (!BootSuccess) { > > - LoadOptions =3D EfiBootManagerGetLoadOptions (&LoadOptionCount, > > LoadOptionTypePlatformRecovery); > > - ProcessLoadOptions (LoadOptions, LoadOptionCount); > > - EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > > + if (PlatformRecovery) { > > + LoadOptions =3D EfiBootManagerGetLoadOptions (&LoadOptionCount, > > LoadOptionTypePlatformRecovery); > > + ProcessLoadOptions (LoadOptions, LoadOptionCount); > > + EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > > + } else { > > + // > > + // When platform recovery is not enabled, still boot to > > + platform default > > file path. > > + // > > + EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption); > > + } > > } > > + EfiBootManagerFreeLoadOption (&PlatformDefaultBootOption); > > > > DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n")); > > PlatformBootManagerUnableToBoot (); > > -- > > 2.21.0.windows.1 > > > > > >=20