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

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

Sean,
Do you have any comments from MS side?

Thanks,
Ray


> -----Original Message-----
> From: Wang, Sunny (HPS SW) <sunnywang@hpe.com>
> Sent: Monday, October 21, 2019 8:45 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Gao, Zhichao
> <zhichao.gao@intel.com>; pjones@redhat.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
> 
> Sorry for the delay and thanks for checking my question, Ray.
> 
> Since the OS recovery and platform recovery options were added to UEFI
> specification at the same time, I thought we will also implement it and push
> the code regardless of the OS support.
> 
> No, I didn't see a real need of using the OS recovery option. If my memory
> serves me well, I have only seen a need of using "One-Time" OS recovery,
> but it can be done by creating, adjusting, and removing a boot option in boot
> order.
> However, I can still imagine that the OS recovery option is needed for the use
> of "persistent" OS recovery. Therefore, if we don't have any concern about
> adding OS recovery option support, I think it is still better to push the code
> because the UEFI specification mentioned it. Add Peter. He may know more
> information about this.
> 
> By the way, I think if no one works on the task below, there will be no OS or
> OS tool supporting it. :)
>    - https://github.com/rhboot/efibootmgr/issues/41
> 
> 
> Regards,
> Sunny Wang
> 
> -----Original Message-----
> From: Ni, Ray [mailto:ray.ni@intel.com]
> Sent: Wednesday, October 16, 2019 4:43 PM
> To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.groups.io;
> Gao, Zhichao <zhichao.gao@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>
> Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
> Importance: High
> 
> 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
> > > >
> >
> > 
> >
>