Re: [edk2-devel] Use a pcd to control PlatformRecovery

["Ni, Ray" <ray.ni@intel.com>]

Sunny,
For the OS recovery question, I had the code change in https://github.com/niruiyu/edk2/tree/bds/osrecovery.

Due to there is no OS support for OS recovery yet, the code was not pushed.

Do you see any needs of the OS recovery?

Thanks,
Ray

> -----Original Message-----
> From: Wang, Sunny (HPS SW) <sunnywang@hpe.com>
> Sent: Wednesday, October 16, 2019 3:42 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent
> (HPS SW) <kent.wei@hpe.com>; Spottswood, Jason
> <jason.spottswood@hpe.com>; Wang, Sunny (HPS SW)
> <sunnywang@hpe.com>
> Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
> 
> Thanks for checking this, Zhichao and Ray.
> I just sent a patch to fix it with the subject " [edk2-devel] [PATCH]
> MdeModulePkg/BdsDxe: Make PlatformRecovery work regardless of
> OsIndications"
> 
> Regards,
> Sunny Wang
> 
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Gao, Zhichao
> Sent: Wednesday, October 16, 2019 1:57 PM
> To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.groups.io;
> Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent
> (HPS SW) <kent.wei@hpe.com>
> Subject: Re: [edk2-devel] Use a pcd to control PlatformRecovery
> Importance: High
> 
> 
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Wednesday, October 16, 2019 10:09 AM
> > To: 'Wang, Sunny (HPS SW)' <sunnywang@hpe.com>;
> devel@edk2.groups.io;
> > Ni, Ray <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@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>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> Kent
> > (HPS SW) <kent.wei@hpe.com>
> > Subject: RE: Use a pcd to control PlatformRecovery
> >
> > 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.
> 
> After discuss with Sunny and Ray, and refer to the spec section 3.4.1 and
> 3.4.2, the OsRecovery and PlatformRecovery should always be operated
> regardless of the value of OsIndication variable if fail to boot the BootOrder. I
> am wrong. We should change to use the PcdPlatformRecoverySupport to
> control the PlatformRecovery. Please help to send a patch to fix it. Thanks a
> lot.
> 
> >
> > 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:sunnywang@hpe.com]
> > > Sent: Tuesday, October 15, 2019 4:53 PM
> > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Ni,
> > > Ray <ray.ni@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@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>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> > Kent
> > > (HPS SW) <kent.wei@hpe.com>; Wang, Sunny (HPS SW)
> > <sunnywang@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?
> 
> OsRecovery#### doesn't have an implement yet, we should co-work with
> the OS vendor to define the operation. For now, there is no requirement.
> 
> > >
> > > 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....
> 
> As I said above the 'Default Platform Recovery' refer to the short-form file
> path boot. When the PcdPlatformRecoverySupport is TRUE, it is part of
> PlatformRecovery. Ohterwise, it is not part of platform recovery and we still
> need to support it because of the capability issue. And that isn't conflict with
> the spec.
> 
> Thanks,
> Zhichao
> 
> > >
> > > Regards,
> > > Sunny Wang
> > >
> 
> 
>