-----Original Message-----
From: Oram, Isaac W <isaac.w.o...@intel.com>
Sent: Wednesday, January 12, 2022 7:15 PM
To: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Gao, Liming
<gaolim...@byosoft.com.cn>; Dong, Eric <eric.d...@intel.com>; Tan, Ming
<ming....@intel.com>; Chiu, Chasel <chasel.c...@intel.com>; Bi, Dandan
<dandan...@intel.com>; Shindo, Miki <miki.shi...@intel.com>; Abbas, Mohamed
<mohamed.ab...@intel.com>; KARPAGAVINAYAGAM, MANICKAVASAKAM
<manickavasak...@ami.com>
Subject: RE: [edk2-devel][edk2-platforms][PATCH V1 00/27] Improve feature build
consistency
Thanks Nate.
Comments in line.
Regards,
Isaac
-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>
Sent: Wednesday, January 12, 2022 6:47 PM
To: Oram, Isaac W <isaac.w.o...@intel.com>; devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Gao, Liming
<gaolim...@byosoft.com.cn>; Dong, Eric <eric.d...@intel.com>; Tan, Ming
<ming....@intel.com>; Chiu, Chasel <chasel.c...@intel.com>; Bi, Dandan
<dandan...@intel.com>; Shindo, Miki <miki.shi...@intel.com>; Abbas, Mohamed
<mohamed.ab...@intel.com>; KARPAGAVINAYAGAM, MANICKAVASAKAM
<manickavasak...@ami.com>
Subject: RE: [edk2-devel][edk2-platforms][PATCH V1 00/27] Improve feature build
consistency
Hi Issac,
Thank you for doing this cleanup work. I have some comments for you. I have
provided a summary of my feedback below:
PATCH 01/27
* Features/Intel/Debugging/BeepDebugFeaturePkg/Include/BeepDebugFeature.dsc
Line 69: [Components.IA32] should be changed to [Components.$(PEI_ARCH)] Line
83: [Components.X64] should be changed to [Components.$(DXE_ARCH)]
Note: These comments can also be addressed by restoring the @todo comment
stating that these changes still need to be done (which you deleted.)
[Isaac] $(DXE_ARCH) and $(PEI_ARCH) are not fully functional. It appears that
they are fine in DSC files, even when inside an include. But they are not fine
inside an include in an FDF file. The PreMemory.fdf and PostMemory.fdf do not
work when I try to introduce this change.
As these are not required, ToDo are against coding style and are bad for
comprehensibility, I would prefer to not add such useless comments back in.
Comments should improve the comprehensibility of code and should not distract
from understanding the code.
[Nate] The $(PEI_ARCH)/$(DXE_ARCH) additions are not be necessary in the FDF
files. Adding them to the DSC file should be sufficient. Can you re-test with
just the DSC file change?
PATCH 18/27
* Features/Intel/Debugging/AcpiDebugFeaturePkg/AcpiDebugFeaturePkg.dsc
Line 33: [PcdsFeatureFlag.X64] should be changed to
[PcdsFeatureFlag.$(DXE_ARCH)]
Note: This comment can also be addressed by adding a @todo comment stating that
this change still needs to be done.
[Isaac] See prior response.
PATCH 19/27
* Features/Intel/Debugging/Usb3DebugFeaturePkg/Include/Usb3DebugFeature.dsc
Line 35: Typo here. Usb3DebugPortParamLibo should be Usb3DebugPortParamLib.
[Isaac] Good catch. I guess we don't catch that class of error if the library
class is not used.
* Features/Intel/Debugging/Usb3DebugFeaturePkg/Usb3DebugFeaturePkg.dsc
Line 40: Did you test compilation for the Usb3DebugFeaturePkg? I've generally
run into issues when a components sections does not specify a machine
architecture through some sort of means.
[Isaac] There is no code consuming these libraries. I verified build of 32 and
64 bit modes as well as part of AdvancedFeaturePkg build and a board package
build.
I believe that library classes can be specified by module type and the build
tool builds the right mode for the consuming driver on demand. Basically,
there is no value to specifying architecture for a library.
This does not work with components however. If you leave the architecture
unspecified, you get an error when including the component in an FDF as the
build does not know how to resolve.
[Nate] I bring this up because you added a [Components] section and put this
package's library classes into that [Components] section for the purposes of
running a build test on those library classes even though they are not consumed
by anything. That new [Components] section does not specify a machine
architecture so I'm wondering if the compilation still succeeds.
PATCH 26/27
Since FvAdvanced is post-memory and not covered by the boot guard IBB, I
suspect we should probably also support optional signing of that FV.
[Isaac] I do not know how to act on that suggestion. That seems out of scope
for this change. I restricted my changes to be functionally compatible as I do
not have hardware to test these changes other than minimally.
[Nate] Nevermind. I checked all the OpenBoardPkgs we have and none of them have
FV signing enabled anyway. Ignore this comment.
Thanks,
Nate
-----Original Message-----
From: Oram, Isaac W <isaac.w.o...@intel.com>
Sent: Tuesday, January 11, 2022 6:20 PM
To: devel@edk2.groups.io
Cc: Oram, Isaac W <isaac.w.o...@intel.com>; Chaganty, Rangasai V
<rangasai.v.chaga...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Dong,
Eric <eric.d...@intel.com>; Tan, Ming <ming....@intel.com>; Desimone, Nathaniel
L <nathaniel.l.desim...@intel.com>; Chiu, Chasel <chasel.c...@intel.com>; Bi,
Dandan <dandan...@intel.com>; Shindo, Miki <miki.shi...@intel.com>; Abbas,
Mohamed <mohamed.ab...@intel.com>; KARPAGAVINAYAGAM, MANICKAVASAKAM
<manickavasak...@ami.com>
Subject: [edk2-devel][edk2-platforms][PATCH V1 00/27] Improve feature build
consistency
This series addresses inconsistencies in feature implementation and use. Some
inconsistencies are just conventions of the feature design/template/convention.
Some are inconsistency with feature design intent that negatively affect the
usability of the features and the amount of work required from board porting
engineers.
Some features were missing feature enable flags.
Some features had non-functional standalone builds.
Many features were implemented to include common core build content in their
feature include files.
Updated some of the Readme content.
Added AdvancedFeaturePkg.fdf to build all feature content to support verifying
no build time issues between features.
Removed duplicate and unused content from build files.
Modified the TemplateFeaturePkg to use the common MinPlatform include content.
Removed all instances where features were relative to Features/Intel and made
them relative to the package roots.
This does mean PACKAGES_PATH may need to be extended for all the feature
domains. Debugging, PowerManagement, etc.
However, it should enable packaging tools to function properly as the relative
paths violate spec.
Use of the common MinPlatformPkg build includes does increase the build time
for each individual feature in standalone build modes. It does not negatively
impact board or AdvancedFeaturePkg builds as the common content is only built
once. Part of MinPlatform arch intent is to reduce cognitive complexity, so the
simpler build is more valuable than fast build time.
Cc: Sai Chaganty <mailto:rangasai.v.chaga...@intel.com>
Cc: Liming Gao <mailto:gaolim...@byosoft.com.cn>
Cc: Eric Dong <mailto:eric.d...@intel.com>
Cc: Ming Tan <mailto:ming....@intel.com>
Cc: Nate DeSimone <mailto:nathaniel.l.desim...@intel.com>
Cc: Chasel Chiu <mailto:chasel.c...@intel.com>
Cc: Dandan Bi <mailto:dandan...@intel.com>
Cc: Miki Shindo <mailto:miki.shi...@intel.com>
Cc: Mohamed Abbas <mailto:mohamed.ab...@intel.com>
Cc: Manickavasakam Karpagavinayagam <mailto:manickavasak...@ami.com>
Signed-off-by: Isaac Oram <mailto:isaac.w.o...@intel.com>
Isaac Oram (27):
BeepDebugFeaturePkg: Use MinPlatformPkg build include files
BeepDebugFeaturePkg: Fix all relative package paths
AcpiDebugFeaturePkg: Fix all relative package paths
IpmiFeaturePkg: Fix all relative package paths
IpmiFeaturePkg: Fix build errors
S3FeaturePkg: Fix all relative package paths
S3FeaturePkg: Use MinPlatformPkg build include files
SmbiosFeaturePkg: Fix all relative package paths
SmbiosFeaturePkg: Use MinPlatformPkg build include files
UserAuthFeaturePkg: Fix all relative package paths
UserAuthFeaturePkg: Use MinPlatformPkg build include files
VirtualKeyboardFeaturePkg: Fix all relative package paths
VirtualKeyboardFeaturePkg: Use MinPlatformPkg build include files
VirtualKeyboardFeaturePkg: Add feature enable PCD
NetworkFeaturePkg: Use MinPlatformPkg build include files
LogoFeaturePkg: Use MinPlatformPkg build include files
PostCodeDebugFeaturePkg: Complete as an advanced feature
AcpiDebugFeaturePkg: Use MinPlatformPkg build include files
Usb3DebugFeaturePkg: Align with feature design guidelines
SpcrFeaturePkg: Use MinPlatform build include files
TemplateFeaturePkg: Use MinPlatform build include files
AdvancedFeaturePkg: Fix all relative package paths
AdvancedFeaturePkg: Add missing features
MinPlatformPkg/Build: Add an include file for the common SPI FV info
WhitleyOpenBoardPkg/Build: Use common SPI FV Header include
AdvancedFeaturePkg/Build: Add FDF to create FV for all features
WhitleyOpenBoardPkg/Build: Enable Features/Intel features
Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.dsc
| 67 +++++-
Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.fdf
| 49 +++++
Features/Intel/AdvancedFeaturePkg/Include/AdvancedFeatures.dsc
| 49 +++--
Features/Intel/AdvancedFeaturePkg/Include/AdvancedFeaturesPcd.dsc
| 64 +++++-
Features/Intel/AdvancedFeaturePkg/Include/PostMemory.fdf
| 49 +++--
Features/Intel/AdvancedFeaturePkg/Include/PreMemory.fdf
| 49 +++--
Features/Intel/Debugging/AcpiDebugFeaturePkg/AcpiDebugDxeSmm/AcpiDebugDxe.inf
| 2 +-
Features/Intel/Debugging/AcpiDebugFeaturePkg/AcpiDebugDxeSmm/AcpiDebugSmm.inf
| 2 +-
Features/Intel/Debugging/AcpiDebugFeaturePkg/AcpiDebugFeaturePkg.dec
| 2 +-
Features/Intel/Debugging/AcpiDebugFeaturePkg/AcpiDebugFeaturePkg.dsc
| 21 ++
Features/Intel/Debugging/AcpiDebugFeaturePkg/Include/AcpiDebugFeature.dsc
| 74 +------
Features/Intel/Debugging/AcpiDebugFeaturePkg/Include/PostMemory.fdf
| 4 +-
Features/Intel/Debugging/BeepDebugFeaturePkg/BeepDebugFeaturePkg.dec
| 7 +-
Features/Intel/Debugging/BeepDebugFeaturePkg/BeepDebugFeaturePkg.dsc
| 28 +++
Features/Intel/Debugging/BeepDebugFeaturePkg/Include/BeepDebugFeature.dsc
| 222 ++++++-------------
Features/Intel/Debugging/BeepDebugFeaturePkg/Include/Library/BeepLib.h
| 6 +-
Features/Intel/Debugging/BeepDebugFeaturePkg/Include/PostMemory.fdf
| 14 ++
Features/Intel/Debugging/BeepDebugFeaturePkg/Include/PreMemory.fdf
| 13 ++
Features/Intel/Debugging/BeepDebugFeaturePkg/Library/BeepStatusCodeHandlerLib/PeiBeepStatusCodeHandlerLib.inf
| 5 +-
Features/Intel/Debugging/BeepDebugFeaturePkg/Library/BeepStatusCodeHandlerLib/RuntimeDxeBeepStatusCodeHandlerLib.inf
| 3 -
Features/Intel/Debugging/BeepDebugFeaturePkg/Library/BeepStatusCodeHandlerLib/SmmBeepStatusCodeHandlerLib.inf
| 3 -
Features/Intel/Debugging/BeepDebugFeaturePkg/Readme.md
| 91 +++++---
Features/Intel/Debugging/PostCodeDebugFeaturePkg/Include/PostCodeDebugFeature.dsc
| 231 +++++---------------
Features/Intel/Debugging/PostCodeDebugFeaturePkg/Include/PostMemory.fdf
| 14 ++
Features/Intel/Debugging/PostCodeDebugFeaturePkg/Include/PreMemory.fdf
| 13 ++
Features/Intel/Debugging/PostCodeDebugFeaturePkg/Library/PostCodeStatusCodeHandlerLib/PeiPostCodeStatusCodeHandlerLib.inf
| 2 +-
Features/Intel/Debugging/PostCodeDebugFeaturePkg/PostCodeDebugFeaturePkg.dec
| 11 +
Features/Intel/Debugging/PostCodeDebugFeaturePkg/PostCodeDebugFeaturePkg.dsc
| 30 +++
Features/Intel/Debugging/PostCodeDebugFeaturePkg/Readme.md
| 31 ++-
Features/Intel/Debugging/Usb3DebugFeaturePkg/Include/Usb3DebugFeature.dsc
| 131 ++---------
Features/Intel/Debugging/Usb3DebugFeaturePkg/Readme.md
| 50 +++--
Features/Intel/Debugging/Usb3DebugFeaturePkg/Usb3DebugFeaturePkg.dec
| 14 +-
Features/Intel/Debugging/Usb3DebugFeaturePkg/Usb3DebugFeaturePkg.dsc
| 18 ++
Features/Intel/Network/NetworkFeaturePkg/Include/NetworkFeature.dsc
| 89 +-------
Features/Intel/Network/NetworkFeaturePkg/NetworkFeaturePkg.dsc
| 18 ++
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/BmcAcpi/BmcAcpi.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/BmcElog/BmcElog.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Frb/FrbDxe.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Frb/FrbPei.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/IpmiFeature.dsc
| 90 ++------
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/PostMemory.fdf
| 16 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/PreMemory.fdf
| 6 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dsc
| 18 ++
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFru/IpmiFru.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiInit/DxeIpmiInit.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiInit/PeiIpmiInit.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLib/IpmiBaseLib.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLibNull/IpmiBaseLibNull.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiCommandLib/IpmiCommandLib.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiPlatformHookLibNull/IpmiPlatformHookLibNull.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/PeiIpmiBaseLib/PeiIpmiBaseLib.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/SmmIpmiBaseLib/SmmIpmiBaseLib.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/OsWdt/OsWdt.inf
| 2 +-
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/SolStatus/SolStatus.inf
| 2 +-
Features/Intel/OutOfBandManagement/SpcrFeaturePkg/Include/Library/SpcrDeviceLib.h
| 2 +-
Features/Intel/OutOfBandManagement/SpcrFeaturePkg/Include/PostMemory.fdf
| 13 ++
Features/Intel/OutOfBandManagement/SpcrFeaturePkg/Include/PreMemory.fdf
| 11 +
Features/Intel/OutOfBandManagement/SpcrFeaturePkg/Include/SpcrFeature.dsc
| 62 ------
Features/Intel/OutOfBandManagement/SpcrFeaturePkg/Readme.md
| 12 +-
Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrFeaturePkg.dec
| 6 +
Features/Intel/OutOfBandManagement/SpcrFeaturePkg/SpcrFeaturePkg.dsc
| 18 ++
Features/Intel/PowerManagement/S3FeaturePkg/Include/PreMemory.fdf
| 2 +-
Features/Intel/PowerManagement/S3FeaturePkg/Include/S3Feature.dsc
| 74 +------
Features/Intel/PowerManagement/S3FeaturePkg/S3FeaturePkg.dsc
| 18 ++
Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.inf
| 2 +-
Features/Intel/Readme.md
| 49 +++--
Features/Intel/SystemInformation/SmbiosFeaturePkg/Include/PostMemory.fdf
| 2 +-
Features/Intel/SystemInformation/SmbiosFeaturePkg/Include/SmbiosFeature.dsc
| 54 +----
Features/Intel/SystemInformation/SmbiosFeaturePkg/SmbiosBasicDxe/SmbiosBasicDxe.inf
| 2 +-
Features/Intel/SystemInformation/SmbiosFeaturePkg/SmbiosFeaturePkg.dec
| 10 +-
Features/Intel/SystemInformation/SmbiosFeaturePkg/SmbiosFeaturePkg.dsc
| 18 ++
Features/Intel/TemplateFeaturePkg/Include/TemplateFeature.dsc
| 2 +-
Features/Intel/TemplateFeaturePkg/TemplateFeaturePkg.dsc
| 18 ++
Features/Intel/UserInterface/LogoFeaturePkg/Include/LogoFeature.dsc
| 69 +-----
Features/Intel/UserInterface/LogoFeaturePkg/LogoFeaturePkg.dec
| 2 -
Features/Intel/UserInterface/LogoFeaturePkg/LogoFeaturePkg.dsc
| 38 +++-
Features/Intel/UserInterface/UserAuthFeaturePkg/Include/PostMemory.fdf
| 6 +-
Features/Intel/UserInterface/UserAuthFeaturePkg/Include/UserAuthFeature.dsc
| 92 +-------
Features/Intel/UserInterface/UserAuthFeaturePkg/Library/PlatformPasswordLibNull/PlatformPasswordLibNull.inf
| 2 +-
Features/Intel/UserInterface/UserAuthFeaturePkg/Library/UserPasswordLib/UserPasswordLib.inf
| 2 +-
Features/Intel/UserInterface/UserAuthFeaturePkg/Library/UserPasswordUiLib/UserPasswordUiLib.inf
| 2 +-
Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthFeaturePkg.dsc
| 18 ++
Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthentication2Dxe.inf
| 2 +-
Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationDxe.inf
| 2 +-
Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationSmm.inf
| 2 +-
Features/Intel/UserInterface/VirtualKeyboardFeaturePkg/Include/PostMemory.fdf
| 2 +-
Features/Intel/UserInterface/VirtualKeyboardFeaturePkg/Include/VirtualKeyboardFeature.dsc
| 64 +-----
Features/Intel/UserInterface/VirtualKeyboardFeaturePkg/VirtualKeyboardFeaturePkg.dec
| 7 +-
Features/Intel/UserInterface/VirtualKeyboardFeaturePkg/VirtualKeyboardFeaturePkg.dsc
| 18 ++
Platform/Intel/{WhitleyOpenBoardPkg =>
MinPlatformPkg}/Include/Fdf/CommonSpiFvHeaderInfo.fdf
| 2 +-
Platform/Intel/WhitleyOpenBoardPkg/JunctionCity/PlatformPkg.fdf
| 48 ++--
Platform/Intel/WhitleyOpenBoardPkg/PlatformPkg.dsc
| 44 ++++
Platform/Intel/WhitleyOpenBoardPkg/PlatformPkg.fdf
| 54 ++---
96 files changed, 1159 insertions(+), 1334 deletions(-) create mode 100644
Features/Intel/AdvancedFeaturePkg/AdvancedFeaturePkg.fdf
create mode 100644
Features/Intel/Debugging/BeepDebugFeaturePkg/Include/PostMemory.fdf
create mode 100644
Features/Intel/Debugging/BeepDebugFeaturePkg/Include/PreMemory.fdf
create mode 100644
Features/Intel/Debugging/PostCodeDebugFeaturePkg/Include/PostMemory.fdf
create mode 100644
Features/Intel/Debugging/PostCodeDebugFeaturePkg/Include/PreMemory.fdf
create mode 100644
Features/Intel/OutOfBandManagement/SpcrFeaturePkg/Include/PostMemory.fdf
create mode 100644
Features/Intel/OutOfBandManagement/SpcrFeaturePkg/Include/PreMemory.fdf
rename Platform/Intel/{WhitleyOpenBoardPkg =>
MinPlatformPkg}/Include/Fdf/CommonSpiFvHeaderInfo.fdf (88%)
--
2.27.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85650): https://edk2.groups.io/g/devel/message/85650
Mute This Topic: https://groups.io/mt/88365326/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
