First of all, the patch didn't aim to change the other part of the boot flow except PlatformRecovery.
Local variable PlatformRecovery is controlled by OsIndications variable. When the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY is set, the firmware should try to platform specific recovery. But that doesn't mean the platform must support the specific recovery. i.e. local PlatformRecovery is controlling the boot flow and the Pcd just indicated whether the platform support recovery or not. So, on my opinion, I don't agree with your change. Default Platform Recovery refer to the short file path to boot the OS. If the firmware supports platform recovery, then *short file path* option would be one part of the PlatformRecovery#### in case there are no other boot options. If the firmware doesn't support platform recovery, we still need this default boot thru a short file path and it should not depend on the PlatformRecovery#### variable for the compatibility thinking. Thanks, Zhichao > -----Original Message----- > From: Wang, Sunny (HPS SW) [mailto:sunnyw...@hpe.com] > Sent: Tuesday, October 15, 2019 4:53 PM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao....@intel.com>; Ni, Ray > <ray...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; > Zeng, Star <star.z...@intel.com>; Gao, Liming <liming....@intel.com>; > Sean Brogan <sean.bro...@microsoft.com>; Michael Turner > <michael.tur...@microsoft.com>; Bret Barkelew > <bret.barke...@microsoft.com>; Li, Walon <walon...@hpe.com>; Wei, Kent > (HPS SW) <kent....@hpe.com>; Wang, Sunny (HPS SW) > <sunnyw...@hpe.com> > Subject: Use a pcd to control PlatformRecovery > > Hi Zhichao and Ray, > > I have some questions about this code change. Sorry for being late to bring > my questions here. > > For now, the code block for iterating the PlatformRecovery#### variables is > controlled by OsIndications variable. However, it looks to me like that the > PlatformRecovery#### should still be attempted for the case of that > processing of BootOrder does NOT result in success (according to section 3.4 > in UEFI 2.8). In other words, I think we should check PCD > "PcdPlatformRecoverySupport" instead of Local variable "PlatformRecovery" > (from OsIndications variable) like the code below. What do you guys think? If > you need a meeting or short talk to discuss this, feel free to let me know. > > if (!BootSuccess) { > - if (PlatformRecovery) { > + if (PcdGetBool (PcdPlatformRecoverySupport)) { > LoadOptions = 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); > } > > > In addition, it looks like EDK2 don't have code to process OsRecovery#### > variables. Do we need to create a Bug on TianoCore Bugzilla system? > > Moreover, I saw that both of you had a discussion about "Default > PlatformRecovery", but I can't figure out the connection between the > discussion and the final code change. Isn't the "Default PlatformRecovery" > part of the Platform Recovery feature? At this moment, we don't have OS > recovery support, so I think that NO platform recovery support can be > identified as NO boot option recovery support. For this case, shouldn't we > use PCD "PcdPlatformRecoverySupport" to control "Default > PlatformRecovery" as well? For the next step, I think we need to get further > clarification from USWG to either not tie "Default Boot Behavior" with > PlatformRecovery or update the description to the following: > - If system firmware supports Platform recovery as described in Section > 3.4.2, system firmware must include a PlatformRecovery#### variable > specifying a short-form File Path Media Device Path.... > > Regards, > Sunny Wang > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49046): https://edk2.groups.io/g/devel/message/49046 Mute This Topic: https://groups.io/mt/34543609/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
