Re: [edk2-devel] [edk2/master PATCH DxeIplHandoffLib v1] MdePkg/DxeIplHandoffLibNullLib: Abstract DxeIpl

["Michael D Kinney" <michael.d.kinney@intel.com>]

Abner,

If the ARM/AARCH64/RISCV64 content required by DxeIpl were added to
MdePkg or MdeModulePkg could we remove the need for this new lib class?

What RISCV64 content needs to be added to MdePkg/MdeModulePkg so all
CPUs are supported the same way?

Thanks,

Mike

> -----Original Message-----
> From: Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>
> Sent: Wednesday, March 18, 2020 8:22 PM
> To: devel@edk2.groups.io; Bi, Dandan
> <dandan.bi@intel.com>
> Cc: Leif Lindholm <leif@nuviainc.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Laszlo Ersek
> <lersek@redhat.com>; Schaefer, Daniel (DualStudy)
> <daniel.schaefer@hpe.com>; Chen, Gilbert
> <gilbert.chen@hpe.com>
> Subject: RE: [edk2-devel] [edk2/master PATCH
> DxeIplHandoffLib v1] MdePkg/DxeIplHandoffLibNullLib:
> Abstract DxeIpl
> 
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf Of
> > Dandan Bi
> > Sent: Thursday, March 19, 2020 11:09 AM
> > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW
> Technologist)
> > <abner.chang@hpe.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>; Kinney,
> Michael D
> > <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Ard
> > Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek
> <lersek@redhat.com>;
> > Schaefer, Daniel (DualStudy)
> <daniel.schaefer@hpe.com>; Chen, Gilbert
> > <gilbert.chen@hpe.com>
> > Subject: Re: [edk2-devel] [edk2/master PATCH
> DxeIplHandoffLib v1]
> > MdePkg/DxeIplHandoffLibNullLib: Abstract DxeIpl
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf Of
> > > Abner Chang
> > > Sent: Monday, March 16, 2020 10:17 AM
> > > To: Bi, Dandan <dandan.bi@intel.com>;
> devel@edk2.groups.io
> > > Cc: Leif Lindholm <leif@nuviainc.com>; Kinney,
> Michael D
> > > <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Ard
> > > Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo
> Ersek
> > > <lersek@redhat.com>; Schaefer, Daniel (DualStudy)
> > > <daniel.schaefer@hpe.com>; Chen, Gilbert
> <gilbert.chen@hpe.com>
> > > Subject: Re: [edk2-devel] [edk2/master PATCH
> DxeIplHandoffLib v1]
> > > MdePkg/DxeIplHandoffLibNullLib: Abstract DxeIpl
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Bi, Dandan [mailto:dandan.bi@intel.com]
> > > > Sent: Monday, March 16, 2020 9:31 AM
> > > > To: Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>;
> > > > devel@edk2.groups.io
> > > > Cc: Leif Lindholm <leif@nuviainc.com>; Kinney,
> Michael D
> > > > <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>;
> > > > Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Laszlo Ersek
> > > > <lersek@redhat.com>; Schaefer, Daniel (DualStudy)
> > > > <daniel.schaefer@hpe.com>; Chen, Gilbert
> <gilbert.chen@hpe.com>
> > > > Subject: RE: [edk2/master PATCH DxeIplHandoffLib
> v1]
> > > > MdePkg/DxeIplHandoffLibNullLib: Abstract DxeIpl
> > > >
> > > > Hi Abner,
> > > >
> > > > Some comments as below.
> > > >
> > > > 1. For the patch itself
> > > > a)  it introduces a new library class, so besides
> the instance, we
> > > > also need to add the header file (public
> interface definitions) in
> > > > the include/library directory and define the
> library class in dec file.
> > > > b)  EFIAPI keyword should be added with the
> public API definition (
> > > > HandOffToDxeCore).
> > > Ok.  Will resend the patch.
> > >
> > > >
> > > > 2. This path is just to add an empty instance for
> now.
> > > > Abner, will you also add other instances for
> other Archs and update
> > > > DxeIpl to consume the new library?
> > > The purpose of this change is to abstract arch from
> DxeIpl module
> > > under MdeModulePkg and remove the dependencies with
> arch package
> > from
> > > MdeModulePkg.ci.yaml.
> > > Yes, I already added an instance for RISC-V and
> revised DxeIpl to
> > > consume new lib *only* for RISCV64 arch,  this
> change will be in the
> > > set of RISC-V edk2 patches. The patch you are
> reviewing now is the
> > > prerequisite for RISC-V edk2 port.
> > >
> > > > And for platform, it's incompatible change to use
> new added library
> > > instance.
> > > > So it's better review it in TianoCore Design
> Meeting firstly.
> > > It will stay the same for X86, ARM may have to use
> this NULL instance
> > > and remove the dependencies from
> MdeModulePkg.ci.yaml as well.
> > > However, I can't speak for ARM because the
> dependence with ARM
> > package
> > > in MdeModulePkg has been in MdeModulePkg.ci.yaml
> when edk2 CI was
> > > introduced (my guess, not sure the history).
> RISCV64 is a new arch and
> > > was requested to be decoupled from MdeModulePkg.
> This change don't
> > > bring incompatible issue though. ARM arch can still
> stay the same as
> > > it is in edk2 now.
> > Abner,
> > So you will only and one instance used for RISCV64,
> no instance for other
> > archs.
> Yes.
> 
> > HandOffToDxeCore logic in DxeLoadFunc.c for IA32 and
> X64 will keep same
> > with before, do not leverage new DxeIplHandoffLib
> Then there is no
> > incompatible concern for IA32 and X64, is that
> correct?
> No, no impacts on architectures other than RISC-V.  X86
> doesn't have to change because there is no package
> dependency issues when edk2 CI testing.
> ARM still have problem if they keep the same way,
> however they are good because CI package test has an
> exception for ARM in MdeModulePkg.ci.yaml.
> 
> I will get on TianoCore design meeting tomorrow and
> have a short presentation of this change. FYI.
> 
> >
> > Thanks,
> > Dandan
> > >
> > > >
> > > >
> > > >
> > > > Thanks,
> > > > Dandan
> > > > > -----Original Message-----
> > > > > From: Abner Chang [mailto:abner.chang@hpe.com]
> > > > > Sent: Monday, March 9, 2020 6:28 PM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: abner.chang@hpe.com; Leif Lindholm
> <leif@nuviainc.com>;
> > > > > Kinney, Michael D <michael.d.kinney@intel.com>;
> Gao, Liming
> > > > > <liming.gao@intel.com>; Ard Biesheuvel
> > > > > <ard.biesheuvel@linaro.org>; Laszlo Ersek
> <lersek@redhat.com>; Bi,
> > > > > Dandan <dandan.bi@intel.com>; Daniel Schaefer
> > > > > <daniel.schaefer@hpe.com>; Gilbert Chen
> <gilbert.chen@hpe.com>
> > > > > Subject: [edk2/master PATCH DxeIplHandoffLib
> v1]
> > > > > MdePkg/DxeIplHandoffLibNullLib: Abstract DxeIpl
> > > > >
> > > > > BZ:2583:
> > > > > INVALID URI REMOVED
> > > > > e.org_show-5Fbug.cgi-3Fid-
> > > > 3D2583&d=DwIFAg&c=C5b8zRQO1miGmBeVZ2LFWg&r=_
> > > > >
> > > >
> > >
> >
> SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=YQR7NX_kxz
> 4BPRET
> > > > p5nNLWWOK
> > > > >
> > > >
> > >
> >
> NOimkostEzdrvyvPkA&s=7wyyAOitp2IvMKv19tlpbJxt2m0bn_ZsR4
> R7llYI19c&
> > > > e=
> > > > >
> > > > > Current DxeIpl has bindings for different
> processor architectures,
> > > > > this results in MdeModulePkg has the dependence
> with processor
> > > > > architecture packages such as ArmPkg or
> RiscVPkg. This also leads
> > > > > CI testing to error during package dependency
> check. Provide a
> > > > > default DxeIplHandoff library to abstract
> processor architecture
> > > > > from DxeIpl driver, platform can provide its
> own library instance
> > > > > for the processor
> > > > architecture- specific implementation.
> > > > >
> > > > > Signed-off-by: Abner Chang
> <abner.chang@hpe.com>
> > > > >
> > > > > Cc: Leif Lindholm <leif@nuviainc.com>
> > > > > Cc: Michael D Kinney
> <michael.d.kinney@intel.com>
> > > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > Cc: Dandan Bi <dandan.bi@intel.com>
> > > > > Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
> > > > > Cc: Gilbert Chen <gilbert.chen@hpe.com>
> > > > > ---
> > > > >  MdeModulePkg/MdeModulePkg.dsc
> |  2 ++
> > > > >  .../DxeIplHandoffLibNull.inf
> | 30 +++++++++++++++++
> > > > >  .../DxeIplHandoffLibNull.c
> | 33 +++++++++++++++++++
> > > > >  .../DxeIplHandoffLibNull.uni
> | 14 ++++++++
> > > > >  4 files changed, 79 insertions(+)  create mode
> 100644
> > > > >
> >
> MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHandoff
> LibNull.inf
> > > > >  create mode 100644
> > > > >
> MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHandoff
> LibNull.c
> > > > >  create mode 100644
> > > > >
> >
> MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHandoff
> LibNull.uni
> > > > >
> > > > > diff --git a/MdeModulePkg/MdeModulePkg.dsc
> > > > > b/MdeModulePkg/MdeModulePkg.dsc index
> f7dbb27ce2..6eb922dfaa
> > > > > 100644
> > > > > --- a/MdeModulePkg/MdeModulePkg.dsc
> > > > > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > > > > @@ -3,6 +3,7 @@
> > > > >  # # (C) Copyright 2014 Hewlett-Packard
> Development Company,
> > > > > L.P.<BR> # Copyright (c) 2007 - 2019, Intel
> Corporation. All
> > > > > rights reserved.<BR>+# Copyright (c) 2020,
> Hewlett Packard
> > > > > Enterprise
> > > > Development LP. All rights
> > > > > reserved.<BR> # #    SPDX-License-Identifier:
> BSD-2-Clause-Patent
> > #@@ -
> > > > > 321,6 +322,7 @@
> > > > >
> MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLi
> b.inf
> > > > >
> > > >
> > >
> >
> MdeModulePkg/Library/DisplayUpdateProgressLibGraphics/D
> isplayUpdateP
> > > > > rogressLibGraphics.inf
> > > > >
> > > >
> > >
> >
> MdeModulePkg/Library/DisplayUpdateProgressLibText/Displ
> ayUpdateProgr
> > > > > essLibText.inf+
> > > > >
> >
> MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHandoff
> LibNull.inf
> > > > > MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > > > >
> > > >
> > >
> >
> MdeModulePkg/Application/BootManagerMenuApp/BootManager
> MenuAp
> > > > > p.infdiff --git
> > > > >
> > >
> a/MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHando
> ffLibNull.inf
> > > > >
> > >
> b/MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHando
> ffLibNull.inf
> > > > > new file mode 100644
> > > > > index 0000000000..b7210656b7
> > > > > --- /dev/null
> > > > > +++
> > > > >
> > >
> b/MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHando
> ffLibNull.inf
> > > > > @@ -0,0 +1,30 @@
> > > > > +## @file+#  Null DXE IPL handoff to DXE Core
> Library
> > > > > +instance.+#+#
> > > > > Copyright (c) 2020, Hewlett Packard Enterprise
> Development LP. All
> > > > > rights reserved.<BR>+#+#  SPDX-License-
> Identifier: BSD-2-Clause-
> > > > > Patent+#+##++[Defines]+  INF_VERSION
> = 0x0001001b+
> > > > > BASE_NAME                      =
> DxeIplHandoffLibNull+  MODULE_UNI_FILE
> > > > > = DxeIplHandoffLibNull.uni+  FILE_GUID
> = 5c18812d-3684-
> > 4093-
> > > > > bc75-fc846a595353+  MODULE_TYPE
> = BASE+
> > VERSION_STRING
> > > > > = 1.0+  LIBRARY_CLASS                  =
> DxeIplHandoffLib++#+# The following
> > > > > information is for reference only and not
> required by the build
> > tools.+#+#
> > > > > VALID_ARCHITECTURES           =
> RISCV64+#++[Sources]+
> > > > > DxeIplHandoffLibNull.c++[Packages]+
> MdePkg/MdePkg.dec+diff --git
> > > > >
> >
> a/MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHando
> ffLibNull.c
> > > > >
> >
> b/MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHando
> ffLibNull.c
> > > > > new file mode 100644
> > > > > index 0000000000..c83922cb6e
> > > > > --- /dev/null
> > > > > +++
> > > >
> b/MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHando
> ffLibNull.c
> > > > > @@ -0,0 +1,33 @@
> > > > > +/** @file+  NULL instance of DXE IPL handoff
> to DXE Core
> > > > > +Library.++
> > > > > Copyright (c) 2020, Hewlett Packard Enterprise
> Development LP. All
> > > > > rights reserved.<BR>++  SPDX-License-
> Identifier: BSD-2-Clause-
> > > > > Patent++**/++#include <PiPei.h>+#include
> > > <Library/DebugLib.h>++/**+
> > > > > Transfers control to DxeCore.++   This function
> performs a CPU
> > > architecture
> > > > > specific operations to execute+   the entry
> point of DxeCore with the
> > > > > parameters of HobList.+   It also installs
> EFI_END_OF_PEI_PPI to signal
> > the
> > > > > end of PEI phase.++   @param DxeCoreEntryPoint
> The entry point
> > of
> > > > > DxeCore.+   @param HobList
> The start of HobList passed to
> > > > > DxeCore.++**/+VOID+HandOffToDxeCore (+  IN
> > > EFI_PHYSICAL_ADDRESS
> > > > > DxeCoreEntryPoint,+  IN EFI_PEI_HOB_POINTERS
> HobList+  )+{+
> > DEBUG
> > > > > ((DEBUG_INFO, "No implementation of DXE IPL
> handoff to DXE Core
> > > > > library.\r\n"));+  ASSERT (FALSE);+}+diff --git
> > > > >
> > >
> a/MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHando
> ffLibNull.uni
> > > > >
> > >
> b/MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHando
> ffLibNull.uni
> > > > > new file mode 100644
> > > > > index 0000000000..5a8973d6e5
> > > > > --- /dev/null
> > > > > +++
> > > > >
> > >
> b/MdeModulePkg/Library/DxeIplHandoffLibNull/DxeIplHando
> ffLibNull.uni
> > > > > @@ -0,0 +1,14 @@
> > > > > +// /** @file+// Null DXE IPL handoff to DXE
> Core Library
> > > > > +instance.+//+//
> > > > > Copyright (c) 2020, Hewlett Packard Enterprise
> Development LP. All
> > > > > rights reserved.<BR>+//+// SPDX-License-
> Identifier: BSD-2-Clause-
> > > > Patent+//+//
> > > > > **/+++#string STR_MODULE_ABSTRACT
> #language en-US "Null
> > > DXE
> > > > > IPL handoff to DXE Core Library
> instance."++#string
> > > > > STR_MODULE_DESCRIPTION          #language en-US
> "Null DXE IPL
> > handoff
> > > to
> > > > > DXE Core Library instance."+--
> > > > > 2.25.0
> > >
> > >
> > >
> >
> >
> > 
>