Re: [edk2-devel] 回复: [Patch edk2-platforms V2] Intel/BoardModulePkg: sort load option in the first boot

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

Liming,
Using HII PCDs introduces too many software layers that may cause potential bugs if any layer is not proper implemented (Imaging a case when a platform forgets to include the mdeModule.dsc.inc).

Variable used here is very straightforward.

That was also the reason when I redesigned the BDS 10 years ago, I chose not to include the PcdBootState into the MdeModulePkg but left that in platform.

I agree with you that consistency is better. And I suggest we submit a Bugzilla to change all open platforms to use variable directly for first boot detection.

Thanks,
Ray

> -----Original Message-----
> From: gaoliming <gaoliming@byosoft.com.cn>
> Sent: Wednesday, March 17, 2021 11:01 AM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Agyeman, Prince
> <prince.agyeman@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Subject: 回复: [edk2-devel] 回复: [Patch edk2-platforms V2] Intel/BoardModulePkg: sort load option in the first boot
> 
> Zhiguang:
>   This is the common platform usage. I suggest to apply the same solution. My solution is to define this PCD PcdBootState in
> MdeModulePkg.dec, and add MdeModule.dsc.inc file that defines this PCD as DynamicHii PCD, platform DSC includes
> MdeModule.dsc.inc file, platform modules consume this PCD (set/get).
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Zhiguang Liu
> > 发送时间: 2021年3月16日 9:57
> > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn; Ni, Ray
> > <ray.ni@intel.com>
> > 抄送: Dong, Eric <eric.dong@intel.com>; Desimone, Nathaniel L
> > <nathaniel.l.desimone@intel.com>; Agyeman, Prince
> > <prince.agyeman@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> > 主题: Re: [edk2-devel] 回复: [Patch edk2-platforms V2]
> > Intel/BoardModulePkg: sort load option in the first boot
> >
> > Hi Liming,
> >
> > Thanks for the comments. This patch is merged before this comment, but I can
> > still send another patch to modify if needed.
> >
> > However, I think the implement in this patch is more simple.
> > The implement in QuarkPlatformPkg need changes in inf, dec and dsc files,
> > and is not as intuitive as just getting and setting a variable.
> > It may be simpler if the implements can reuse a same DynamicHiiPcd, do you
> > think it is possible?
> > If I misunderstand anything, please correct me.
> >
> > Thanks
> > Zhiguang
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > gaoliming
> > > Sent: Monday, March 15, 2021 9:36 AM
> > > To: Ni, Ray <ray.ni@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> > > devel@edk2.groups.io
> > > Cc: Dong, Eric <eric.dong@intel.com>; Desimone, Nathaniel L
> > > <nathaniel.l.desimone@intel.com>; Agyeman, Prince
> > > <prince.agyeman@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> > > Subject: [edk2-devel] 回复: [Patch edk2-platforms V2]
> > > Intel/BoardModulePkg: sort load option in the first boot
> > >
> > > Zhiguang:
> > >   I see QuarkPlatformPkg uses PCD
> > > gQuarkPlatformTokenSpaceGuid.PcdBootState
> > > to decide whether current boot is the first boot or not.
> > >   This PCD is configured as DynamicHiiPcd, and be set in
> > > Platform\Intel\QuarkPlatformPkg\Library\PlatformBootManagerLib\Platfor
> > > mBootM
> > > anager.c
> > >
> > >   Can you use the same solution in Intel BoardModulePkg?
> > >
> > > Thanks
> > > Liming
> > > > -----邮件原件-----
> > > > 发件人: Ni, Ray <ray.ni@intel.com>
> > > > 发送时间: 2021年3月10日 17:56
> > > > 收件人: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io
> > > > 抄送: Dong, Eric <eric.dong@intel.com>; Liming Gao
> > > > <gaoliming@byosoft.com.cn>; Desimone, Nathaniel L
> > > > <nathaniel.l.desimone@intel.com>; Agyeman, Prince
> > > > <prince.agyeman@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> > > > 主题: RE: [Patch edk2-platforms V2] Intel/BoardModulePkg: sort load
> > > > option in the first boot
> > > >
> > > > 1. DataSIze should be set to sizeof (BOOLEAN) before calling
> > > > GetVariable()
> > > >
> > > > > +      Status = gRT->GetVariable (
> > > > > +                      L"IsFirstBoot",
> > > >
> > > > 2. Can you please define a macro in this C file for IsFirstBoot string?
> > > > e.g.: #define IS_FIRST_BOOT_VAR_NAME L"IsFirstBoot"
> > > >
> > > > > +      if (IsFirstBoot == TRUE) {
> > > >
> > > > 3. Please remove "== TRUE". Just use "If (IsFirstBoot)".
> > > >
> > > > > +                        L"IsFirstBoot",
> > > > 4. Please use the macro defined as above.
> > > >
> > > > >
> > > > > +                        &gEfiCallerIdGuid,
> > > > >
> > > > > +                        EFI_VARIABLE_NON_VOLATILE |
> > > > > EFI_VARIABLE_RUNTIME_ACCESS |
> > > EFI_VARIABLE_BOOTSERVICE_ACCESS,
> > > >
> > > > 5. Please remove "EFI_VARIABLE_RUNTIME_ACCESS".
> > > >
> > > > > +                        1,
> > > > 6. Please use sizeof (BOOLEAN) instead of "1".
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> > 
> >
> 
>