* [PATCH 0/2] Use a pcd to control Platform Recovery behavior @ 2019-04-02 5:49 Zhichao Gao 2019-04-02 5:49 ` [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit Zhichao Gao 2019-04-02 5:49 ` [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set " Zhichao Gao 0 siblings, 2 replies; 10+ messages in thread From: Zhichao Gao @ 2019-04-02 5:49 UTC (permalink / raw) To: edk2-devel Add a pcd PcdPlatformRecoverySupport to control the variable PlatformRecovery#### and the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY bit of variable "OsIndicationsSupported". Bret Barkelew (2): MdeModulePkg: Add a pcd to set the OS indications bit MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit MdeModulePkg/MdeModulePkg.dec | 6 ++ MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 1 + MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 74 +++++++++++++----------- 3 files changed, 47 insertions(+), 34 deletions(-) -- 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit 2019-04-02 5:49 [PATCH 0/2] Use a pcd to control Platform Recovery behavior Zhichao Gao @ 2019-04-02 5:49 ` Zhichao Gao 2019-04-09 9:23 ` [edk2-devel] " Ni, Ray 2019-04-02 5:49 ` [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set " Zhichao Gao 1 sibling, 1 reply; 10+ messages in thread From: Zhichao Gao @ 2019-04-02 5:49 UTC (permalink / raw) To: edk2-devel Cc: Bret Barkelew, Jian J Wang, Ray Ni, Star Zeng, Liming Gao, Sean Brogan, Michael Turner From: Bret Barkelew <Bret.Barkelew@microsoft.com> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678 Add a pcd PcdPlatformRecoverySupport to conditionally set an OS indications bit. Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Michael Turner <Michael.Turner@microsoft.com> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> --- MdeModulePkg/MdeModulePkg.dec | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 43992b7f65..870f49749a 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -1669,6 +1669,12 @@ # @Prompt Reset on memory type information change. gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|TRUE|BOOLEAN|0x00010056 + ## Indicates if the BDS supports Platform Recovery.<BR><BR> + # TRUE - Platform supports Platform Recovery.<BR> + # FALSE - Does not add Boot Manager to boot order.<BR> + # @Prompt Support Platform Recovery. + gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport|TRUE|BOOLEAN|0x00010078 + ## Specify the foreground color for Subtile text in HII Form Browser. The default value is EFI_BLUE. # Only following values defined in UEFI specification are valid:<BR><BR> # 0x00 (EFI_BLACK)<BR> -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit 2019-04-02 5:49 ` [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit Zhichao Gao @ 2019-04-09 9:23 ` Ni, Ray 0 siblings, 0 replies; 10+ messages in thread From: Ni, Ray @ 2019-04-09 9:23 UTC (permalink / raw) To: Gao, Zhichao, devel [-- Attachment #1: Type: text/plain, Size: 40 bytes --] Reviewed-by: Ray Ni <ray.ni@intel.com> [-- Attachment #2: Type: text/html, Size: 99 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit 2019-04-02 5:49 [PATCH 0/2] Use a pcd to control Platform Recovery behavior Zhichao Gao 2019-04-02 5:49 ` [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit Zhichao Gao @ 2019-04-02 5:49 ` Zhichao Gao [not found] ` <734D49CCEBEEF84792F5B80ED585239D5C0DBCB4@SHSMSX104.ccr.corp.intel.com> 1 sibling, 1 reply; 10+ messages in thread From: Zhichao Gao @ 2019-04-02 5:49 UTC (permalink / raw) To: edk2-devel Cc: Bret Barkelew, Jian J Wang, Ray Ni, Star Zeng, Liming Gao, Sean Brogan, Michael Turner From: Bret Barkelew <Bret.Barkelew@microsoft.com> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678 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 <jian.j.wang@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Sean Brogan <sean.brogan@microsoft.com> Cc: Michael Turner <Michael.Turner@microsoft.com> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> --- 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 = EfiBootManagerGetBootManagerMenu (&BootManagerMenu); if (Status != EFI_NOT_FOUND) { - OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI | EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; + OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI; EfiBootManagerFreeLoadOption (&BootManagerMenu); } else { - OsIndicationSupport = EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; + OsIndicationSupport = 0; + } + + if (PcdGetBool (PcdPlatformRecoverySupport)) { + OsIndicationSupport |= EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; } Status = 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 = FileDevicePath (NULL, EFI_REMOVABLE_MEDIA_FILE_NAME); - Status = EfiBootManagerInitializeLoadOption ( - &LoadOption, - LoadOptionNumberUnassigned, - LoadOptionTypePlatformRecovery, - LOAD_OPTION_ACTIVE, - L"Default PlatformRecovery", - FilePath, - NULL, - 0 - ); - ASSERT_EFI_ERROR (Status); - LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery); - if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, LoadOptionCount) == -1) { - for (Index = 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 != 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 = FileDevicePath (NULL, EFI_REMOVABLE_MEDIA_FILE_NAME); + Status = EfiBootManagerInitializeLoadOption ( + &LoadOption, + LoadOptionNumberUnassigned, + LoadOptionTypePlatformRecovery, + LOAD_OPTION_ACTIVE, + L"Default PlatformRecovery", + FilePath, + NULL, + 0 + ); + ASSERT_EFI_ERROR (Status); + LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery); + if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, LoadOptionCount) == -1) { + for (Index = 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 != Index) { + break; + } } + LoadOption.OptionNumber = Index; + Status = EfiBootManagerLoadOptionToVariable (&LoadOption); + ASSERT_EFI_ERROR (Status); } - LoadOption.OptionNumber = Index; - Status = EfiBootManagerLoadOptionToVariable (&LoadOption); - 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <734D49CCEBEEF84792F5B80ED585239D5C0DBCB4@SHSMSX104.ccr.corp.intel.com>]
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit [not found] ` <734D49CCEBEEF84792F5B80ED585239D5C0DBCB4@SHSMSX104.ccr.corp.intel.com> @ 2019-04-09 9:32 ` Ni, Ray 2019-04-10 0:44 ` Gao, Zhichao 0 siblings, 1 reply; 10+ messages in thread From: Ni, Ray @ 2019-04-09 9:32 UTC (permalink / raw) To: Gao, Zhichao, 'devel@edk2.groups.io' Cc: Bret Barkelew, Wang, Jian J, Zeng, Star, Gao, Liming, Sean Brogan, Michael Turner Resend to groups.io. > -----Original Message----- > From: Ni, Ray > Sent: Tuesday, April 9, 2019 5:31 PM > To: Gao, Zhichao <zhichao.gao@intel.com>; edk2-devel@lists.01.org > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > Michael Turner <Michael.Turner@microsoft.com> > 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 = EfiBootManagerGetLoadOptions (&LoadOptionCount, > LoadOptionTypePlatformRecovery); > ProcessLoadOptions (LoadOptions, LoadOptionCount); > EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > } > > > -----Original Message----- > > From: Gao, Zhichao <zhichao.gao@intel.com> > > Sent: Tuesday, April 2, 2019 1:50 PM > > To: edk2-devel@lists.01.org > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star > > <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean Brogan > > <sean.brogan@microsoft.com>; Michael Turner > > <Michael.Turner@microsoft.com> > > Subject: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > > indications bit > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678 > > > > 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 <jian.j.wang@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Cc: Liming Gao <liming.gao@intel.com> > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > --- > > 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 = EfiBootManagerGetBootManagerMenu (&BootManagerMenu); > > if (Status != EFI_NOT_FOUND) { > > - OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI | > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > + OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI; > > EfiBootManagerFreeLoadOption (&BootManagerMenu); > > } else { > > - OsIndicationSupport = > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > + OsIndicationSupport = 0; > > + } > > + > > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > > + OsIndicationSupport |= > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > } > > > > Status = 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 = FileDevicePath (NULL, EFI_REMOVABLE_MEDIA_FILE_NAME); > > - Status = EfiBootManagerInitializeLoadOption ( > > - &LoadOption, > > - LoadOptionNumberUnassigned, > > - LoadOptionTypePlatformRecovery, > > - LOAD_OPTION_ACTIVE, > > - L"Default PlatformRecovery", > > - FilePath, > > - NULL, > > - 0 > > - ); > > - ASSERT_EFI_ERROR (Status); > > - LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > LoadOptionTypePlatformRecovery); > > - if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > LoadOptionCount) == -1) { > > - for (Index = 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 != 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 = FileDevicePath (NULL, EFI_REMOVABLE_MEDIA_FILE_NAME); > > + Status = EfiBootManagerInitializeLoadOption ( > > + &LoadOption, > > + LoadOptionNumberUnassigned, > > + LoadOptionTypePlatformRecovery, > > + LOAD_OPTION_ACTIVE, > > + L"Default PlatformRecovery", > > + FilePath, > > + NULL, > > + 0 > > + ); > > + ASSERT_EFI_ERROR (Status); > > + LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > LoadOptionTypePlatformRecovery); > > + if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > LoadOptionCount) == -1) { > > + for (Index = 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 != Index) { > > + break; > > + } > > } > > + LoadOption.OptionNumber = Index; > > + Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > + ASSERT_EFI_ERROR (Status); > > } > > - LoadOption.OptionNumber = Index; > > - Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > - 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit 2019-04-09 9:32 ` Ni, Ray @ 2019-04-10 0:44 ` Gao, Zhichao 2019-04-10 1:59 ` Ni, Ray 2019-04-10 2:00 ` Ni, Ray 0 siblings, 2 replies; 10+ messages in thread From: Gao, Zhichao @ 2019-04-10 0:44 UTC (permalink / raw) To: Ni, Ray, 'devel@edk2.groups.io' Cc: Bret Barkelew, Wang, Jian J, Zeng, Star, Gao, Liming, Sean Brogan, Michael Turner 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 <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > <devel@edk2.groups.io> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > Michael Turner <Michael.Turner@microsoft.com> > 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 <zhichao.gao@intel.com>; edk2-devel@lists.01.org > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > Michael Turner <Michael.Turner@microsoft.com> > > 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 = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > LoadOptionTypePlatformRecovery); > > ProcessLoadOptions (LoadOptions, LoadOptionCount); > > EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > > } > > > > > -----Original Message----- > > > From: Gao, Zhichao <zhichao.gao@intel.com> > > > Sent: Tuesday, April 2, 2019 1:50 PM > > > To: edk2-devel@lists.01.org > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star > > > <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean > > > Brogan <sean.brogan@microsoft.com>; Michael Turner > > > <Michael.Turner@microsoft.com> > > > Subject: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > > > indications bit > > > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678 > > > > > > 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 <jian.j.wang@intel.com> > > > Cc: Ray Ni <ray.ni@intel.com> > > > Cc: Star Zeng <star.zeng@intel.com> > > > Cc: Liming Gao <liming.gao@intel.com> > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > > --- > > > 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 = EfiBootManagerGetBootManagerMenu (&BootManagerMenu); > > > if (Status != EFI_NOT_FOUND) { > > > - OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI | > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > + OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI; > > > EfiBootManagerFreeLoadOption (&BootManagerMenu); > > > } else { > > > - OsIndicationSupport = > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > + OsIndicationSupport = 0; > > > + } > > > + > > > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > > > + OsIndicationSupport |= > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > } > > > > > > Status = 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 = FileDevicePath (NULL, EFI_REMOVABLE_MEDIA_FILE_NAME); > > > - Status = EfiBootManagerInitializeLoadOption ( > > > - &LoadOption, > > > - LoadOptionNumberUnassigned, > > > - LoadOptionTypePlatformRecovery, > > > - LOAD_OPTION_ACTIVE, > > > - L"Default PlatformRecovery", > > > - FilePath, > > > - NULL, > > > - 0 > > > - ); > > > - ASSERT_EFI_ERROR (Status); > > > - LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > > LoadOptionTypePlatformRecovery); > > > - if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > > LoadOptionCount) == -1) { > > > - for (Index = 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 != 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 = FileDevicePath (NULL, > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > + Status = EfiBootManagerInitializeLoadOption ( > > > + &LoadOption, > > > + LoadOptionNumberUnassigned, > > > + LoadOptionTypePlatformRecovery, > > > + LOAD_OPTION_ACTIVE, > > > + L"Default PlatformRecovery", > > > + FilePath, > > > + NULL, > > > + 0 > > > + ); > > > + ASSERT_EFI_ERROR (Status); > > > + LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > > LoadOptionTypePlatformRecovery); > > > + if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > > LoadOptionCount) == -1) { > > > + for (Index = 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 != Index) { > > > + break; > > > + } > > > } > > > + LoadOption.OptionNumber = Index; > > > + Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > > + ASSERT_EFI_ERROR (Status); > > > } > > > - LoadOption.OptionNumber = Index; > > > - Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > > - 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit 2019-04-10 0:44 ` Gao, Zhichao @ 2019-04-10 1:59 ` Ni, Ray 2019-04-10 2:00 ` Ni, Ray 1 sibling, 0 replies; 10+ messages in thread From: Ni, Ray @ 2019-04-10 1:59 UTC (permalink / raw) To: Gao, Zhichao, 'devel@edk2.groups.io' Cc: Bret Barkelew, Wang, Jian J, Zeng, Star, Gao, Liming, Sean Brogan, Michael Turner 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 <ray.ni@intel.com>; 'devel@edk2.groups.io' <devel@edk2.groups.io> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael > Turner <Michael.Turner@microsoft.com> > 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 <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > > <devel@edk2.groups.io> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > Michael Turner <Michael.Turner@microsoft.com> > > 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 <zhichao.gao@intel.com>; edk2-devel@lists.01.org > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > Liming <liming.gao@intel.com>; Sean Brogan > > > <sean.brogan@microsoft.com>; Michael Turner > > > <Michael.Turner@microsoft.com> > > > 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 = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > > LoadOptionTypePlatformRecovery); > > > ProcessLoadOptions (LoadOptions, LoadOptionCount); > > > EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > > > } > > > > > > > -----Original Message----- > > > > From: Gao, Zhichao <zhichao.gao@intel.com> > > > > Sent: Tuesday, April 2, 2019 1:50 PM > > > > To: edk2-devel@lists.01.org > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star > > > > <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean > > > > Brogan <sean.brogan@microsoft.com>; Michael Turner > > > > <Michael.Turner@microsoft.com> > > > > Subject: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > > > > indications bit > > > > > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678 > > > > > > > > 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 <jian.j.wang@intel.com> > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > Cc: Star Zeng <star.zeng@intel.com> > > > > Cc: Liming Gao <liming.gao@intel.com> > > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > > > --- > > > > 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 = EfiBootManagerGetBootManagerMenu (&BootManagerMenu); > > > > if (Status != EFI_NOT_FOUND) { > > > > - OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI | > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > + OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI; > > > > EfiBootManagerFreeLoadOption (&BootManagerMenu); > > > > } else { > > > > - OsIndicationSupport = > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > + OsIndicationSupport = 0; > > > > + } > > > > + > > > > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > > > > + OsIndicationSupport |= > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > } > > > > > > > > Status = 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 = FileDevicePath (NULL, > > > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > - Status = EfiBootManagerInitializeLoadOption ( > > > > - &LoadOption, > > > > - LoadOptionNumberUnassigned, > > > > - LoadOptionTypePlatformRecovery, > > > > - LOAD_OPTION_ACTIVE, > > > > - L"Default PlatformRecovery", > > > > - FilePath, > > > > - NULL, > > > > - 0 > > > > - ); > > > > - ASSERT_EFI_ERROR (Status); > > > > - LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > > > LoadOptionTypePlatformRecovery); > > > > - if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > > > LoadOptionCount) == -1) { > > > > - for (Index = 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 != 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 = FileDevicePath (NULL, > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > + Status = EfiBootManagerInitializeLoadOption ( > > > > + &LoadOption, > > > > + LoadOptionNumberUnassigned, > > > > + LoadOptionTypePlatformRecovery, > > > > + LOAD_OPTION_ACTIVE, > > > > + L"Default PlatformRecovery", > > > > + FilePath, > > > > + NULL, > > > > + 0 > > > > + ); > > > > + ASSERT_EFI_ERROR (Status); > > > > + LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > > > LoadOptionTypePlatformRecovery); > > > > + if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > > > LoadOptionCount) == -1) { > > > > + for (Index = 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 != Index) { > > > > + break; > > > > + } > > > > } > > > > + LoadOption.OptionNumber = Index; > > > > + Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > > > + ASSERT_EFI_ERROR (Status); > > > > } > > > > - LoadOption.OptionNumber = Index; > > > > - Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > > > - 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit 2019-04-10 0:44 ` Gao, Zhichao 2019-04-10 1:59 ` Ni, Ray @ 2019-04-10 2:00 ` Ni, Ray 2019-04-26 3:12 ` Gao, Zhichao 1 sibling, 1 reply; 10+ messages in thread From: Ni, Ray @ 2019-04-10 2:00 UTC (permalink / raw) To: Gao, Zhichao, 'devel@edk2.groups.io' Cc: Bret Barkelew, Wang, Jian J, Zeng, Star, Gao, Liming, Sean Brogan, Michael Turner 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 <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > <devel@edk2.groups.io> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael > Turner <Michael.Turner@microsoft.com> > 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 <ray.ni@intel.com>; 'devel@edk2.groups.io' > > <devel@edk2.groups.io> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > Michael Turner <Michael.Turner@microsoft.com> > > 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 <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > > > <devel@edk2.groups.io> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > Liming <liming.gao@intel.com>; Sean Brogan > > > <sean.brogan@microsoft.com>; Michael Turner > > > <Michael.Turner@microsoft.com> > > > 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 <zhichao.gao@intel.com>; edk2-devel@lists.01.org > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > > Liming <liming.gao@intel.com>; Sean Brogan > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > <Michael.Turner@microsoft.com> > > > > 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 = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > > > LoadOptionTypePlatformRecovery); > > > > ProcessLoadOptions (LoadOptions, LoadOptionCount); > > > > EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > > > > } > > > > > > > > > -----Original Message----- > > > > > From: Gao, Zhichao <zhichao.gao@intel.com> > > > > > Sent: Tuesday, April 2, 2019 1:50 PM > > > > > To: edk2-devel@lists.01.org > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > > <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star > > > > > <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean > > > > > Brogan <sean.brogan@microsoft.com>; Michael Turner > > > > > <Michael.Turner@microsoft.com> > > > > > Subject: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > > > > > indications bit > > > > > > > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678 > > > > > > > > > > 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 <jian.j.wang@intel.com> > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > Cc: Star Zeng <star.zeng@intel.com> > > > > > Cc: Liming Gao <liming.gao@intel.com> > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > > > > --- > > > > > 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 = EfiBootManagerGetBootManagerMenu (&BootManagerMenu); > > > > > if (Status != EFI_NOT_FOUND) { > > > > > - OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI | > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > + OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI; > > > > > EfiBootManagerFreeLoadOption (&BootManagerMenu); > > > > > } else { > > > > > - OsIndicationSupport = > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > + OsIndicationSupport = 0; > > > > > + } > > > > > + > > > > > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > > > > > + OsIndicationSupport |= > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > } > > > > > > > > > > Status = 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 = FileDevicePath (NULL, > > > > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > > - Status = EfiBootManagerInitializeLoadOption ( > > > > > - &LoadOption, > > > > > - LoadOptionNumberUnassigned, > > > > > - LoadOptionTypePlatformRecovery, > > > > > - LOAD_OPTION_ACTIVE, > > > > > - L"Default PlatformRecovery", > > > > > - FilePath, > > > > > - NULL, > > > > > - 0 > > > > > - ); > > > > > - ASSERT_EFI_ERROR (Status); > > > > > - LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, > > > > > LoadOptionTypePlatformRecovery); > > > > > - if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > > > > LoadOptionCount) == -1) { > > > > > - for (Index = 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 != 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 = FileDevicePath (NULL, > > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > > + Status = EfiBootManagerInitializeLoadOption ( > > > > > + &LoadOption, > > > > > + LoadOptionNumberUnassigned, > > > > > + LoadOptionTypePlatformRecovery, > > > > > + LOAD_OPTION_ACTIVE, > > > > > + L"Default PlatformRecovery", > > > > > + FilePath, > > > > > + NULL, > > > > > + 0 > > > > > + ); > > > > > + ASSERT_EFI_ERROR (Status); > > > > > + LoadOptions = EfiBootManagerGetLoadOptions > > > > > + (&LoadOptionCount, > > > > > LoadOptionTypePlatformRecovery); > > > > > + if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > > > > LoadOptionCount) == -1) { > > > > > + for (Index = 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 != Index) { > > > > > + break; > > > > > + } > > > > > } > > > > > + LoadOption.OptionNumber = Index; > > > > > + Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > > > > + ASSERT_EFI_ERROR (Status); > > > > > } > > > > > - LoadOption.OptionNumber = Index; > > > > > - Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > > > > - 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit 2019-04-10 2:00 ` Ni, Ray @ 2019-04-26 3:12 ` Gao, Zhichao 2019-04-26 5:55 ` Ni, Ray 0 siblings, 1 reply; 10+ messages in thread From: Gao, Zhichao @ 2019-04-26 3:12 UTC (permalink / raw) To: Ni, Ray, 'devel@edk2.groups.io' Cc: Bret Barkelew, Wang, Jian J, Zeng, Star, Gao, Liming, Sean Brogan, Michael Turner Hi Ray, 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). Can we regard the "PlatformRecovery####" variable as optional? If so, I think it is fine to let the platform to control its behavior. Thanks, Zhichao > -----Original Message----- > From: Ni, Ray > Sent: Wednesday, April 10, 2019 10:00 AM > To: Gao, Zhichao <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > <devel@edk2.groups.io> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > Michael Turner <Michael.Turner@microsoft.com> > 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 <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > > <devel@edk2.groups.io> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > Michael Turner <Michael.Turner@microsoft.com> > > 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 <ray.ni@intel.com>; 'devel@edk2.groups.io' > > > <devel@edk2.groups.io> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > Liming <liming.gao@intel.com>; Sean Brogan > > > <sean.brogan@microsoft.com>; Michael Turner > > > <Michael.Turner@microsoft.com> > > > 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 <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > > > > <devel@edk2.groups.io> > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > > Liming <liming.gao@intel.com>; Sean Brogan > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > <Michael.Turner@microsoft.com> > > > > 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 <zhichao.gao@intel.com>; > > > > > edk2-devel@lists.01.org > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > > > Liming <liming.gao@intel.com>; Sean Brogan > > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > > <Michael.Turner@microsoft.com> > > > > > 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 = EfiBootManagerGetLoadOptions > > > > > (&LoadOptionCount, LoadOptionTypePlatformRecovery); > > > > > ProcessLoadOptions (LoadOptions, LoadOptionCount); > > > > > EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > > > > > } > > > > > > > > > > > -----Original Message----- > > > > > > From: Gao, Zhichao <zhichao.gao@intel.com> > > > > > > Sent: Tuesday, April 2, 2019 1:50 PM > > > > > > To: edk2-devel@lists.01.org > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > > > <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, > > > > > > Star <star.zeng@intel.com>; Gao, Liming > > > > > > <liming.gao@intel.com>; Sean Brogan > > > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > > > <Michael.Turner@microsoft.com> > > > > > > Subject: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > > > > > > indications bit > > > > > > > > > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678 > > > > > > > > > > > > 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 <jian.j.wang@intel.com> > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > > Cc: Star Zeng <star.zeng@intel.com> > > > > > > Cc: Liming Gao <liming.gao@intel.com> > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > > > > > --- > > > > > > 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 = EfiBootManagerGetBootManagerMenu > (&BootManagerMenu); > > > > > > if (Status != EFI_NOT_FOUND) { > > > > > > - OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI > | > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > > + OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI; > > > > > > EfiBootManagerFreeLoadOption (&BootManagerMenu); > > > > > > } else { > > > > > > - OsIndicationSupport = > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > > + OsIndicationSupport = 0; > > > > > > + } > > > > > > + > > > > > > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > > > > > > + OsIndicationSupport |= > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > > } > > > > > > > > > > > > Status = 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 = FileDevicePath (NULL, > > > > > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > > > - Status = EfiBootManagerInitializeLoadOption ( > > > > > > - &LoadOption, > > > > > > - LoadOptionNumberUnassigned, > > > > > > - LoadOptionTypePlatformRecovery, > > > > > > - LOAD_OPTION_ACTIVE, > > > > > > - L"Default PlatformRecovery", > > > > > > - FilePath, > > > > > > - NULL, > > > > > > - 0 > > > > > > - ); > > > > > > - ASSERT_EFI_ERROR (Status); > > > > > > - LoadOptions = EfiBootManagerGetLoadOptions > > > > > > (&LoadOptionCount, LoadOptionTypePlatformRecovery); > > > > > > - if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, > > > > > > LoadOptionCount) == -1) { > > > > > > - for (Index = 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 != 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 = FileDevicePath (NULL, > > > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > > > + Status = EfiBootManagerInitializeLoadOption ( > > > > > > + &LoadOption, > > > > > > + LoadOptionNumberUnassigned, > > > > > > + LoadOptionTypePlatformRecovery, > > > > > > + LOAD_OPTION_ACTIVE, > > > > > > + L"Default PlatformRecovery", > > > > > > + FilePath, > > > > > > + NULL, > > > > > > + 0 > > > > > > + ); > > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > + LoadOptions = EfiBootManagerGetLoadOptions > > > > > > + (&LoadOptionCount, > > > > > > LoadOptionTypePlatformRecovery); > > > > > > + if (EfiBootManagerFindLoadOption (&LoadOption, > > > > > > + LoadOptions, > > > > > > LoadOptionCount) == -1) { > > > > > > + for (Index = 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 != Index) { > > > > > > + break; > > > > > > + } > > > > > > } > > > > > > + LoadOption.OptionNumber = Index; > > > > > > + Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > } > > > > > > - LoadOption.OptionNumber = Index; > > > > > > - Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > > > > > - 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit 2019-04-26 3:12 ` Gao, Zhichao @ 2019-04-26 5:55 ` Ni, Ray 0 siblings, 0 replies; 10+ messages in thread From: Ni, Ray @ 2019-04-26 5:55 UTC (permalink / raw) To: Gao, Zhichao, 'devel@edk2.groups.io' Cc: Bret Barkelew, Wang, Jian J, Zeng, Star, Gao, Liming, Sean Brogan, Michael Turner > -----Original Message----- > From: Gao, Zhichao > Sent: Thursday, April 25, 2019 8:13 PM > To: Ni, Ray <ray.ni@intel.com>; 'devel@edk2.groups.io' > <devel@edk2.groups.io> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > Michael Turner <Michael.Turner@microsoft.com> > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS > indications bit > > Hi Ray, > > 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). > > Can we regard the "PlatformRecovery####" variable as optional? If so, I think > 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 behavior"? > > Thanks, > Zhichao > > > -----Original Message----- > > From: Ni, Ray > > Sent: Wednesday, April 10, 2019 10:00 AM > > To: Gao, Zhichao <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > > <devel@edk2.groups.io> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > Michael Turner <Michael.Turner@microsoft.com> > > 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 <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > > > <devel@edk2.groups.io> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > Liming <liming.gao@intel.com>; Sean Brogan > > > <sean.brogan@microsoft.com>; Michael Turner > > > <Michael.Turner@microsoft.com> > > > 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 <ray.ni@intel.com>; 'devel@edk2.groups.io' > > > > <devel@edk2.groups.io> > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > > Liming <liming.gao@intel.com>; Sean Brogan > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > <Michael.Turner@microsoft.com> > > > > 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 <zhichao.gao@intel.com>; 'devel@edk2.groups.io' > > > > > <devel@edk2.groups.io> > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > > > Liming <liming.gao@intel.com>; Sean Brogan > > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > > <Michael.Turner@microsoft.com> > > > > > 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 <zhichao.gao@intel.com>; > > > > > > edk2-devel@lists.01.org > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > > > <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; > > > > > > Gao, Liming <liming.gao@intel.com>; Sean Brogan > > > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > > > <Michael.Turner@microsoft.com> > > > > > > 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 = EfiBootManagerGetLoadOptions > > > > > > (&LoadOptionCount, LoadOptionTypePlatformRecovery); > > > > > > ProcessLoadOptions (LoadOptions, LoadOptionCount); > > > > > > EfiBootManagerFreeLoadOptions (LoadOptions, > LoadOptionCount); > > > > > > } > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Gao, Zhichao <zhichao.gao@intel.com> > > > > > > > Sent: Tuesday, April 2, 2019 1:50 PM > > > > > > > To: edk2-devel@lists.01.org > > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian > > > > > > > J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, > > > > > > > Star <star.zeng@intel.com>; Gao, Liming > > > > > > > <liming.gao@intel.com>; Sean Brogan > > > > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > > > > <Michael.Turner@microsoft.com> > > > > > > > Subject: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set > > > > > > > OS indications bit > > > > > > > > > > > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678 > > > > > > > > > > > > > > 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 <jian.j.wang@intel.com> > > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > > > Cc: Star Zeng <star.zeng@intel.com> > > > > > > > Cc: Liming Gao <liming.gao@intel.com> > > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > > > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> > > > > > > > --- > > > > > > > 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 = EfiBootManagerGetBootManagerMenu > > (&BootManagerMenu); > > > > > > > if (Status != EFI_NOT_FOUND) { > > > > > > > - OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI > > | > > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > > > + OsIndicationSupport = > EFI_OS_INDICATIONS_BOOT_TO_FW_UI; > > > > > > > EfiBootManagerFreeLoadOption (&BootManagerMenu); > > > > > > > } else { > > > > > > > - OsIndicationSupport = > > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > > > + OsIndicationSupport = 0; } > > > > > > > + > > > > > > > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > > > > > > > + OsIndicationSupport |= > > > > > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; > > > > > > > } > > > > > > > > > > > > > > Status = 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 = FileDevicePath (NULL, > > > > > > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > > > > - Status = EfiBootManagerInitializeLoadOption ( > > > > > > > - &LoadOption, > > > > > > > - LoadOptionNumberUnassigned, > > > > > > > - LoadOptionTypePlatformRecovery, > > > > > > > - LOAD_OPTION_ACTIVE, > > > > > > > - L"Default PlatformRecovery", > > > > > > > - FilePath, > > > > > > > - NULL, > > > > > > > - 0 > > > > > > > - ); > > > > > > > - ASSERT_EFI_ERROR (Status); > > > > > > > - LoadOptions = EfiBootManagerGetLoadOptions > > > > > > > (&LoadOptionCount, LoadOptionTypePlatformRecovery); > > > > > > > - if (EfiBootManagerFindLoadOption (&LoadOption, > > > > > > > LoadOptions, > > > > > > > LoadOptionCount) == -1) { > > > > > > > - for (Index = 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 != 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 = FileDevicePath (NULL, > > > > > EFI_REMOVABLE_MEDIA_FILE_NAME); > > > > > > > + Status = EfiBootManagerInitializeLoadOption ( > > > > > > > + &LoadOption, > > > > > > > + LoadOptionNumberUnassigned, > > > > > > > + LoadOptionTypePlatformRecovery, > > > > > > > + LOAD_OPTION_ACTIVE, > > > > > > > + L"Default PlatformRecovery", > > > > > > > + FilePath, > > > > > > > + NULL, > > > > > > > + 0 > > > > > > > + ); > > > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > > + LoadOptions = EfiBootManagerGetLoadOptions > > > > > > > + (&LoadOptionCount, > > > > > > > LoadOptionTypePlatformRecovery); > > > > > > > + if (EfiBootManagerFindLoadOption (&LoadOption, > > > > > > > + LoadOptions, > > > > > > > LoadOptionCount) == -1) { > > > > > > > + for (Index = 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 != Index) { > > > > > > > + break; > > > > > > > + } > > > > > > > } > > > > > > > + LoadOption.OptionNumber = Index; > > > > > > > + Status = EfiBootManagerLoadOptionToVariable > (&LoadOption); > > > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > > } > > > > > > > - LoadOption.OptionNumber = Index; > > > > > > > - Status = EfiBootManagerLoadOptionToVariable (&LoadOption); > > > > > > > - 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-26 5:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-02 5:49 [PATCH 0/2] Use a pcd to control Platform Recovery behavior Zhichao Gao
2019-04-02 5:49 ` [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit Zhichao Gao
2019-04-09 9:23 ` [edk2-devel] " Ni, Ray
2019-04-02 5:49 ` [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set " Zhichao Gao
[not found] ` <734D49CCEBEEF84792F5B80ED585239D5C0DBCB4@SHSMSX104.ccr.corp.intel.com>
2019-04-09 9:32 ` Ni, Ray
2019-04-10 0:44 ` Gao, Zhichao
2019-04-10 1:59 ` Ni, Ray
2019-04-10 2:00 ` Ni, Ray
2019-04-26 3:12 ` Gao, Zhichao
2019-04-26 5:55 ` Ni, Ray
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox