Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Gao, Liming]

Mike:
  I think build performance is also a key point. I prefer to add this option in 
NOOPT target. After add this option, we can build edk2 packages to detect those 
duplicated issues.

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Kinney, Michael D
>Sent: Friday, May 26, 2017 6:48 AM
>To: Laszlo Ersek ; Ard Biesheuvel
>; Andrew Fish (af...@apple.com)
>; Kinney, Michael D 
>Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff
>
>Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
>duplicate symbol
>
>Laszlo,
>
>The other idea I have is for MSFT tool chains to do the DLINK step twice.  Once
>with /WHOLEARCHIVE set to a .dll that is not used in later steps, but the doing
>the DLINK action detects duplicate symbols.
>
>If the first DLINK step passes, then so a second DLINK step to a .dll without
>/WHOLEARCHIVE set and use this .dll to produce the .efi file that goes into the
>FW image.
>
>This 2 step link process would have the side effect of potentially increasing
>build times, but could be done for specific tool chain families in 
>build_rules.txt.
>
>The first DLINK step could also disable a lot of the optimizations that take
>longer since the goal of this step is only to detect a duplicate symbol.
>
>Best regards,
>
>Mike
>
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Laszlo
>> Ersek
>> Sent: Thursday, May 25, 2017 1:11 PM
>> To: Kinney, Michael D ; Ard Biesheuvel
>> ; Andrew Fish (af...@apple.com)
>
>> Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff
>> 
>> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>Fix
>> duplicate symbol
>>
>> On 05/25/17 21:57, Kinney, Michael D wrote:
>> > Laszlo,
>> >
>> > I have the same concern on final image sizes.  I have done some
>> > evaluation:
>> >
>> > GCC5 OVMF X64 DEBUG without -whole-archive
>> > ==
>> > FV Space Information
>> > SECFV [19%Full] 212992 total, 42000 used, 170992 free
>> > FVMAIN_COMPACT [33%Full] 3440640 total, 1162760 used, 2277880 free
>> > DXEFV [38%Full] 10485760 total, 4024024 used, 6461736 free
>> > PEIFV [19%Full] 917504 total, 180648 used, 736856 free
>> > Total used = 5409432
>> >
>> > GCC5 OVMF X64 DEBUG with -whole-archive
>> > ===
>> > FV Space Information
>> > SECFV [19%Full] 212992 total, 41936 used, 171056 free
>> > FVMAIN_COMPACT [33%Full] 3440640 total, 1158304 used, 2282336 free
>> > DXEFV [38%Full] 10485760 total, 4029656 used, 6456104 free
>> > PEIFV [19%Full] 917504 total, 181352 used, 736152 free
>> > Total used = 5411248
>> >
>> > Total used difference = 1816 bytes larger with -whole-archive
>> >
>> > I was also able to do a MSFT VS2015 build with /WHOLEARCHIVE set
>> > and it also catches the same duplicate symbol error now.
>> >
>> > error C2220: warning treated as error - no 'executable' file generated
>> > warning C4744: 'mMemoryDiscoveredNotifyList' has different type in
>>
>'d:\work\github\tianocore\edk2\mdemodulepkg\core\dxeiplpeim\dxeload.c'
>and
>>
>'d:\work\github\tianocore\edk2\sourceleveldebugpkg\library\debugagent\s
>ecpeidebug
>> agent\secpeidebugagentlib.c': 'struct (12 bytes)' and 'array (12 bytes)'
>> > DxeIpl.lib(DxeLoad.obj) : error LNK2005: _mMemoryDiscoveredNotifyList
>already
>> defined in SecPeiDebugAgentLib.lib(SecPeiDebugAgentLib.obj)
>> >
>>
>d:\work\github\tianocore\edk2\Build\OvmfIa32\DEBUG_VS2015x86\IA32\M
>deModulePkg\Co
>> re\DxeIplPeim\DxeIpl\DEBUG\DxeIpl.dll : fatal error LNK1169: one or more
>multiply
>> defined symbols found
>> >
>> > VS2015 OVMF X64 DEBUG without /WHOLEARCHIVE
>> > ===
>> > FV Space Information
>> > SECFV [22%Full] 212992 total, 48560 used, 164432 free
>> > FVMAIN_COMPACT [33%Full] 3440640 total, 1147464 used, 2293176 free
>> > DXEFV [39%Full] 10485760 total, 4163888 used, 6321872 free
>> > PEIFV [22%Full] 917504 total, 204840 used, 712664 free
>> > Total used = 5564752
>> >
>> >
>> > VS2015 OVMF X64 DEBUG with /WHOLEARCHIVE
>> > ===
>> > FV Space Information
>> > SECFV [23%Full] 212992 total, 50384 used, 162608 free
>> > FVMAIN_COMPACT [33%Full] 3440640 total, 1147424 used, 2293216 free
>> > DXEFV [42%Full] 10485760 total, 4422992 used, 6062768 free
>> > PEIFV [27%Full] 917504 total, 255528 used, 661976 free
>> > Total used = 5875338
>> >
>> > Total used difference = 310586 bytes larger with /WHOLEARCHIVE
>> >
>> > For tool chains that do have size impacts, one option is to
>> > have a "test" build that enables the linker flags to detect
>> > duplicate symbols.  For example the following could be added
>> > to 

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Gao, Liming]

Mike:
  I remember community suggests to use VS /Gw option to remove the global data, 
and then can define GLOBAL_REMOVE_IF_UNREFERENCED as empty or static.  

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Kinney, Michael D
>Sent: Friday, May 26, 2017 6:42 AM
>To: af...@apple.com; Laszlo Ersek ; Kinney, Michael D
>
>Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff
>; Felix Poludov ; Ard Biesheuvel
>
>Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
>duplicate symbol
>
>Andrew,
>
>The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED was
>added referred to __declspec( selectany ) as putting the symbol into its own
>comdat, so it was then available to be optimized away with the use of OPT:REF.
>
>I think it is time to re-evaluate the VS optimizers to see if they can optimize
>away global variables without being decorated with__declspec( selectany ).  If
>we can remove __declspec( selectany ), then we have a path to use STATIC
>properly to hide global variables that are not declared as extern in the 
>library
>class.
>
>I will investigate some more.
>
>Mike
>
>From: af...@apple.com [mailto:af...@apple.com]
>Sent: Thursday, May 25, 2017 2:26 PM
>To: Laszlo Ersek 
>Cc: Ard Biesheuvel ; Wu, Hao A
>; edk2-devel@lists.01.org; Felix Poludov
>; Kinney, Michael D ; Fan,
>Jeff 
>Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
>duplicate symbol
>
>
>On May 25, 2017, at 2:02 PM, Laszlo Ersek
>> wrote:
>
>On 05/25/17 22:37, Andrew Fish wrote:
>
>
>
>On May 25, 2017, at 1:28 PM, Laszlo Ersek
>> wrote:
>
>On 05/25/17 22:11, Ard Biesheuvel wrote:
>
>On 25 May 2017 at 13:06, Kinney, Michael D
>> wrote:
>
>Laszlo and Andrew,
>
>With the information that has been collected on this thread, I
>still think this patch in its original form is a good change
>to resolve the this one specific duplicate symbol issue for all
>tool chains.  'static' can not be mixed with
>GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
>the global variable is the easiest way to remove the duplicate.
>
>GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
>was Felix who reported on this recently?
>
>STATIC is really the only sensible way to deal with this for symbols
>that are only referenced by a single compilation unit.
>
>
>I will continue to work on ways to detect duplicate symbols for
>all tool chains and will enter a Bugzilla issue to for that
>feature.
>
>In addition, the idea of detecting if a library is exporting more
>than the library class defines is another good feature to consider
>and I will enter a Bugzilla issue for that one as well.
>
>If we can find ways to both restrict the symbols exported by a
>library and strip all symbols that are unused, then we can have
>additional Bugzilla issues to perform that clean up on each
>library instance that is exporting more than the library class.
>
>A static library is nothing more than an archive containing a
>collection of object files. Sadly, that implies that we cannot
>distinguish between symbols that may only be referenced by other
>objects in the same static library and symbols that are exported to
>the library client.
>
>Do we know for a fact that, with /OPT:REF, VS does not strip unused
>*static* variables and functions?
>
>I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
>with STATIC in this case would lead to a size increase?
>
>If that's the case, then I'm fine if we go ahead with this patch, I'd
>just like to request that Mike please file some of those BZs, and please
>reference them from the commit message (as the longer term solution),
>before committing the patch.
>
>Clang will warn if you have unused static variables when warnings are cranked
>up.
>
>~/work/Compiler>cat static.c
>static unsigned char gTest[] = { 42 };
>
>static int test ()
>{
> return 1;
>}
>
>int main ()
>{
> return 0;
>}
>~/work/Compiler>clang -Os static.c -Wall
>static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
>static unsigned char gTest[] = { 42 };
>^
>static.c:3:12: warning: unused function 'test' [-Wunused-function]
>static int test ()
>  ^
>2 warnings generated.
>
>Sorry, my question was imprecise.
>
>Assume there is a public library function ("external linkage") that
>calls a static function in the same library instance and uses a static
>variable in the same library instance. Then this library instance is
>linked into a driver, but the driver never actually calls the extern
>function 

[edk2] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] GCC build support.

[zwei4]

Change code which is not compatible with GCC.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: zwei4 
---
 BuildBIOS.sh   |  5 
 Core/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  3 ++-
 Platform/BroxtonPlatformPkg/BiosId.env |  1 +
 Platform/BroxtonPlatformPkg/BuildBios.sh   |  4 ++--
 Platform/BroxtonPlatformPkg/BuildIFWI.sh   |  3 ++-
 .../SmramSaveInfoHandlerSmm.c  |  1 +
 .../Smbios/SmBiosMiscDxe/MiscOemType0x94Function.c |  1 +
 .../Common/Library/PlatformSecLib/Ia32/SecEntry.S  |  1 +
 .../PlatformPostMemPei/PlatformInit.c  |  3 +++
 .../PlatformPostMemPei/PlatformInit.h  |  1 +
 .../PlatformPreMemPei/PlatformInit.h   |  1 +
 .../Common/PlatformSmm/Platform.c  | 12 ++
 .../Common/PlatformSmm/SmmPlatform.h   | 15 +++-
 .../BroxtonPlatformPkg/DefineAtBuildMacros.dsc | 15 
 .../BroxtonPlatformPkg/PlatformDsc/Components.dsc  | 12 ++
 Platform/BroxtonPlatformPkg/PlatformPkg.fdf| 28 --
 .../SouthCluster/ScInit/Smm/ScInitSmm.h|  3 +++
 .../SouthCluster/ScInit/Smm/ScPcieSmm.c|  1 +
 .../SouthCluster/ScSmiDispatcher/Smm/ScSmm.h   |  3 +++
 .../SouthCluster/ScSmiDispatcher/Smm/ScSmmCore.c   |  2 ++
 .../ScSmiDispatcher/Smm/ScSmmPeriodicTimer.c   |  1 +
 .../BroxtonSoC/BroxtonSiPkg/Txe/Heci/Smm/HeciSmm.c | 10 
 .../BroxtonSiPkg/Txe/Heci/Smm/HeciSmmRuntimeDxe.c  |  9 +++
 23 files changed, 113 insertions(+), 22 deletions(-)

diff --git a/BuildBIOS.sh b/BuildBIOS.sh
index 0dece1f77..ff815e9fa 100755
--- a/BuildBIOS.sh
+++ b/BuildBIOS.sh
@@ -11,6 +11,11 @@
 Target_Flag=Release
 if [ "$1" == "Debug" ]; then
   Target_Flag=Debug
+  shift
+fi
+if [ "$1" == "Release" ]; then
+  Target_Flag=Release
+  shift
 fi
 
 echo $Target_Flag
diff --git a/Core/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf 
b/Core/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 9751ba1f0..4dfd82951 100644
--- a/Core/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/Core/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  MP Initialize Library instance for DXE driver.
 #
-#  Copyright (c) 2016, Intel Corporation. All rights reserved.
+#  Copyright (c) 2017, Intel Corporation. All rights reserved.
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD 
License
 #  which accompanies this distribution.  The full text of the license may be 
found at
@@ -56,6 +56,7 @@
   UefiCpuLib
   UefiBootServicesTableLib
   DebugAgentLib
+  SynchronizationLib
 
 [Protocols]
   gEfiTimerArchProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/Platform/BroxtonPlatformPkg/BiosId.env 
b/Platform/BroxtonPlatformPkg/BiosId.env
index 2bf5bada9..18b4e0190 100644
--- a/Platform/BroxtonPlatformPkg/BiosId.env
+++ b/Platform/BroxtonPlatformPkg/BiosId.env
@@ -33,3 +33,4 @@ OEM_ID = X64
 BUILD_TYPE= D
 VERSION_MAJOR = 0064
 VERSION_MINOR = 01
+BOARD_EXT = X64
diff --git a/Platform/BroxtonPlatformPkg/BuildBios.sh 
b/Platform/BroxtonPlatformPkg/BuildBios.sh
index cadffc70d..fb6ac670d 100644
--- a/Platform/BroxtonPlatformPkg/BuildBios.sh
+++ b/Platform/BroxtonPlatformPkg/BuildBios.sh
@@ -317,7 +317,7 @@ cat SpiChunk1.bin IBBL.Fv IBB.Fv SpiChunk2.bin OBB.Fv 
NvStorage.Fv SpiChunk3.bin
 popd
 
 echo
-echo SPI IFWI location: 
$WORKSPACE/Platform/BroxtonPlatformPkg/Common/Tools/Stitch/$BIOS_Name"_GCC".bin
+echo Check if SPI IFWI image is generated at below location:
+echo 
$WORKSPACE/Platform/BroxtonPlatformPkg/Common/Tools/Stitch/$BIOS_Name"_GCC".bin
 echo
-echo  The EDKII BIOS build has successfully completed. 

 echo
diff --git a/Platform/BroxtonPlatformPkg/BuildIFWI.sh 
b/Platform/BroxtonPlatformPkg/BuildIFWI.sh
index bf91b5c18..a319bd3ee 100755
--- a/Platform/BroxtonPlatformPkg/BuildIFWI.sh
+++ b/Platform/BroxtonPlatformPkg/BuildIFWI.sh
@@ -163,6 +163,7 @@ echo "Build_IFWI:  Calling BIOS build Script..."
 sh Platform/BroxtonPlatformPkg/BuildBios.sh $Build_Flags $Platform_Type 
$Build_Target
 
 echo
-echo Finished Building BIOS.
+echo Finished Building Process.
+echo
 
 
diff --git 
a/Platform/BroxtonPlatformPkg/Common/Features/S3/SmramSaveInfoHandlerSmm/SmramSaveInfoHandlerSmm.c
 
b/Platform/BroxtonPlatformPkg/Common/Features/S3/SmramSaveInfoHandlerSmm/SmramSaveInfoHandlerSmm.c
index ef45fdb6b..40655c2f6 100644
--- 
a/Platform/BroxtonPlatformPkg/Common/Features/S3/SmramSaveInfoHandlerSmm/SmramSaveInfoHandlerSmm.c
+++ 
b/Platform/BroxtonPlatformPkg/Common/Features/S3/SmramSaveInfoHandlerSmm/SmramSaveInfoHandlerSmm.c
@@ -45,6 +45,7 @@ CPU_INFO_PROTOCOL *mCpuInfoProtocol;
 
 **/
 EFI_STATUS
+EFIAPI
 SmramSaveInfoHandler (
   IN  EFI_HANDLEDispatchHandle,
   IN  EFI_SMM_SW_REGISTER_CONTEXT 

Re: [edk2] Duet misbehaving.

[Wu, Hao A]

Lyons,

For the steps to create a usb boot disk, please follow the steps mentioned in 
file DuetPkg\ReadMe.txt.
Please help to see if this can work for you.


Best Regards,
Hao Wu


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jd
> Lyons
> Sent: Friday, May 26, 2017 10:15 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Duet misbehaving.
> 
> I’ve built the latest source of EDK2 Duet under Windows 8.1 with VS20015, the
> build went fine, however the CreateUsb.bat does not create a bootable USB
> Drive. I had to manually run GenBootSector and BootSectImage.
> 
> Is there some updated instructions for creating a bootable USB Stick?
> 
> Also, on my system Zotac GF9300(MPC79), it doesn’t ever boot correctly. I get
> the “Welcome to EFI” and the Tiancore loading screen, but then I just get a
> black screen with a curser.
> 
> On an old Intel motherboard, it will boot, but I can’t load the Shell.efi, 
> when I
> choose boot from file, and the shell.efi, it just returns to the Boot 
> Maintenance
> Manager.
> 
> Does anyone have any idea what I’m doing wrong, I need the shell for a project
> I’m working on?
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] Duet misbehaving.

[Jd Lyons]

I’ve built the latest source of EDK2 Duet under Windows 8.1 with VS20015, the 
build went fine, however the CreateUsb.bat does not create a bootable USB 
Drive. I had to manually run GenBootSector and BootSectImage.

Is there some updated instructions for creating a bootable USB Stick?

Also, on my system Zotac GF9300(MPC79), it doesn’t ever boot correctly. I get 
the “Welcome to EFI” and the Tiancore loading screen, but then I just get a 
black screen with a curser.

On an old Intel motherboard, it will boot, but I can’t load the Shell.efi, when 
I choose boot from file, and the shell.efi, it just returns to the Boot 
Maintenance Manager.

Does anyone have any idea what I’m doing wrong, I need the shell for a project 
I’m working on?  
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Kinney, Michael D]

Laszlo,

The other idea I have is for MSFT tool chains to do the DLINK step twice.  Once
with /WHOLEARCHIVE set to a .dll that is not used in later steps, but the doing 
the DLINK action detects duplicate symbols.

If the first DLINK step passes, then so a second DLINK step to a .dll without 
/WHOLEARCHIVE set and use this .dll to produce the .efi file that goes into the 
FW image.

This 2 step link process would have the side effect of potentially increasing 
build times, but could be done for specific tool chain families in 
build_rules.txt.

The first DLINK step could also disable a lot of the optimizations that take 
longer since the goal of this step is only to detect a duplicate symbol.

Best regards,

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Thursday, May 25, 2017 1:11 PM
> To: Kinney, Michael D ; Ard Biesheuvel
> ; Andrew Fish (af...@apple.com) 
> Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff
> 
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> duplicate symbol
> 
> On 05/25/17 21:57, Kinney, Michael D wrote:
> > Laszlo,
> >
> > I have the same concern on final image sizes.  I have done some
> > evaluation:
> >
> > GCC5 OVMF X64 DEBUG without -whole-archive
> > ==
> > FV Space Information
> > SECFV [19%Full] 212992 total, 42000 used, 170992 free
> > FVMAIN_COMPACT [33%Full] 3440640 total, 1162760 used, 2277880 free
> > DXEFV [38%Full] 10485760 total, 4024024 used, 6461736 free
> > PEIFV [19%Full] 917504 total, 180648 used, 736856 free
> > Total used = 5409432
> >
> > GCC5 OVMF X64 DEBUG with -whole-archive
> > ===
> > FV Space Information
> > SECFV [19%Full] 212992 total, 41936 used, 171056 free
> > FVMAIN_COMPACT [33%Full] 3440640 total, 1158304 used, 2282336 free
> > DXEFV [38%Full] 10485760 total, 4029656 used, 6456104 free
> > PEIFV [19%Full] 917504 total, 181352 used, 736152 free
> > Total used = 5411248
> >
> > Total used difference = 1816 bytes larger with -whole-archive
> >
> > I was also able to do a MSFT VS2015 build with /WHOLEARCHIVE set
> > and it also catches the same duplicate symbol error now.
> >
> > error C2220: warning treated as error - no 'executable' file generated
> > warning C4744: 'mMemoryDiscoveredNotifyList' has different type in
> 'd:\work\github\tianocore\edk2\mdemodulepkg\core\dxeiplpeim\dxeload.c' and
> 'd:\work\github\tianocore\edk2\sourceleveldebugpkg\library\debugagent\secpeidebug
> agent\secpeidebugagentlib.c': 'struct (12 bytes)' and 'array (12 bytes)'
> > DxeIpl.lib(DxeLoad.obj) : error LNK2005: _mMemoryDiscoveredNotifyList 
> > already
> defined in SecPeiDebugAgentLib.lib(SecPeiDebugAgentLib.obj)
> >
> d:\work\github\tianocore\edk2\Build\OvmfIa32\DEBUG_VS2015x86\IA32\MdeModulePkg\Co
> re\DxeIplPeim\DxeIpl\DEBUG\DxeIpl.dll : fatal error LNK1169: one or more 
> multiply
> defined symbols found
> >
> > VS2015 OVMF X64 DEBUG without /WHOLEARCHIVE
> > ===
> > FV Space Information
> > SECFV [22%Full] 212992 total, 48560 used, 164432 free
> > FVMAIN_COMPACT [33%Full] 3440640 total, 1147464 used, 2293176 free
> > DXEFV [39%Full] 10485760 total, 4163888 used, 6321872 free
> > PEIFV [22%Full] 917504 total, 204840 used, 712664 free
> > Total used = 5564752
> >
> >
> > VS2015 OVMF X64 DEBUG with /WHOLEARCHIVE
> > ===
> > FV Space Information
> > SECFV [23%Full] 212992 total, 50384 used, 162608 free
> > FVMAIN_COMPACT [33%Full] 3440640 total, 1147424 used, 2293216 free
> > DXEFV [42%Full] 10485760 total, 4422992 used, 6062768 free
> > PEIFV [27%Full] 917504 total, 255528 used, 661976 free
> > Total used = 5875338
> >
> > Total used difference = 310586 bytes larger with /WHOLEARCHIVE
> >
> > For tool chains that do have size impacts, one option is to
> > have a "test" build that enables the linker flags to detect
> > duplicate symbols.  For example the following could be added
> > to a DSC file.  May want to disable GenFds stage when doing
> > this type of build.
> >
> > [BuildOptions]
> > !ifdef $(DETECT_DUPLICATE_SYMBOLS)
> >   MSFT:*_VS2015_*_DLINK_FLAGS = /WHOLEARCHIVE
> > !endif
> 
> Thank you (again) for the research! Looks like the gcc size impact is
> friendly enough to keep --whole-archive enabled at all times (possibly
> due to the --gc-sections flag mentioned by Ard, which we already have
> enabled).
> 
> The VS2015 impact is really large however.
> 
> I was hoping we could add these flags to
> "BaseTools/Conf/tools_def.template", regardless of platform DSC. (If the
> flag is non-default, and/or it's platform-dependent, then it will almost
> never be used, most likely.) But the VS2015 size increase really
> precludes /WHOLEARCHIVE (for the MSFT family) from "tools_def.template".
> 

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Kinney, Michael D]

Andrew,

The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED was added 
referred to __declspec( selectany ) as putting the symbol into its own comdat, 
so it was then available to be optimized away with the use of OPT:REF.

I think it is time to re-evaluate the VS optimizers to see if they can optimize 
away global variables without being decorated with__declspec( selectany ).  If 
we can remove __declspec( selectany ), then we have a path to use STATIC 
properly to hide global variables that are not declared as extern in the 
library class.

I will investigate some more.

Mike

From: af...@apple.com [mailto:af...@apple.com]
Sent: Thursday, May 25, 2017 2:26 PM
To: Laszlo Ersek 
Cc: Ard Biesheuvel ; Wu, Hao A ; 
edk2-devel@lists.01.org; Felix Poludov ; Kinney, Michael D 
; Fan, Jeff 
Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix 
duplicate symbol


On May 25, 2017, at 2:02 PM, Laszlo Ersek 
> wrote:

On 05/25/17 22:37, Andrew Fish wrote:



On May 25, 2017, at 1:28 PM, Laszlo Ersek 
> wrote:

On 05/25/17 22:11, Ard Biesheuvel wrote:

On 25 May 2017 at 13:06, Kinney, Michael D 
> wrote:

Laszlo and Andrew,

With the information that has been collected on this thread, I
still think this patch in its original form is a good change
to resolve the this one specific duplicate symbol issue for all
tool chains.  'static' can not be mixed with
GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
the global variable is the easiest way to remove the duplicate.

GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
was Felix who reported on this recently?

STATIC is really the only sensible way to deal with this for symbols
that are only referenced by a single compilation unit.


I will continue to work on ways to detect duplicate symbols for
all tool chains and will enter a Bugzilla issue to for that
feature.

In addition, the idea of detecting if a library is exporting more
than the library class defines is another good feature to consider
and I will enter a Bugzilla issue for that one as well.

If we can find ways to both restrict the symbols exported by a
library and strip all symbols that are unused, then we can have
additional Bugzilla issues to perform that clean up on each
library instance that is exporting more than the library class.

A static library is nothing more than an archive containing a
collection of object files. Sadly, that implies that we cannot
distinguish between symbols that may only be referenced by other
objects in the same static library and symbols that are exported to
the library client.

Do we know for a fact that, with /OPT:REF, VS does not strip unused
*static* variables and functions?

I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
with STATIC in this case would lead to a size increase?

If that's the case, then I'm fine if we go ahead with this patch, I'd
just like to request that Mike please file some of those BZs, and please
reference them from the commit message (as the longer term solution),
before committing the patch.

Clang will warn if you have unused static variables when warnings are cranked 
up.

~/work/Compiler>cat static.c
static unsigned char gTest[] = { 42 };

static int test ()
{
 return 1;
}

int main ()
{
 return 0;
}
~/work/Compiler>clang -Os static.c -Wall
static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
static unsigned char gTest[] = { 42 };
^
static.c:3:12: warning: unused function 'test' [-Wunused-function]
static int test ()
  ^
2 warnings generated.

Sorry, my question was imprecise.

Assume there is a public library function ("external linkage") that
calls a static function in the same library instance and uses a static
variable in the same library instance. Then this library instance is
linked into a driver, but the driver never actually calls the extern
function -- so the static variable and the static function too become
useless.

In this case, will /OPT:REF remove the static variable and the static
function too?

It seems counter-intuitive to me that an internal-only function or an
internal-only variable has to be declared extern (via
GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
time, if it is never referenced (transitively).


Laszlo,

I agree. The LLVM LTO does not have an issue "doing the right thing". Seems 
like static is also more of a compile time concept vs a link time (global 
optimization) kind of thing?

Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to __declspec( selectany ) I 
would guess maybe it has more to due with supporting old non standard header 
files that can't change without breaking compatibility.

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Andrew Fish]

> On May 25, 2017, at 2:02 PM, Laszlo Ersek  wrote:
> 
> On 05/25/17 22:37, Andrew Fish wrote:
>> 
>>> On May 25, 2017, at 1:28 PM, Laszlo Ersek  wrote:
>>> 
>>> On 05/25/17 22:11, Ard Biesheuvel wrote:
 On 25 May 2017 at 13:06, Kinney, Michael D  
 wrote:
> Laszlo and Andrew,
> 
> With the information that has been collected on this thread, I
> still think this patch in its original form is a good change
> to resolve the this one specific duplicate symbol issue for all
> tool chains.  'static' can not be mixed with
> GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
> the global variable is the easiest way to remove the duplicate.
> 
 
 GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
 was Felix who reported on this recently?
 
 STATIC is really the only sensible way to deal with this for symbols
 that are only referenced by a single compilation unit.
 
> I will continue to work on ways to detect duplicate symbols for
> all tool chains and will enter a Bugzilla issue to for that
> feature.
> 
> In addition, the idea of detecting if a library is exporting more
> than the library class defines is another good feature to consider
> and I will enter a Bugzilla issue for that one as well.
> 
> If we can find ways to both restrict the symbols exported by a
> library and strip all symbols that are unused, then we can have
> additional Bugzilla issues to perform that clean up on each
> library instance that is exporting more than the library class.
> 
 
 A static library is nothing more than an archive containing a
 collection of object files. Sadly, that implies that we cannot
 distinguish between symbols that may only be referenced by other
 objects in the same static library and symbols that are exported to
 the library client.
>>> 
>>> Do we know for a fact that, with /OPT:REF, VS does not strip unused
>>> *static* variables and functions?
>>> 
>>> I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
>>> with STATIC in this case would lead to a size increase?
>>> 
>>> If that's the case, then I'm fine if we go ahead with this patch, I'd
>>> just like to request that Mike please file some of those BZs, and please
>>> reference them from the commit message (as the longer term solution),
>>> before committing the patch.
>>> 
>> 
>> Clang will warn if you have unused static variables when warnings are 
>> cranked up.
>> 
>> ~/work/Compiler>cat static.c
>> static unsigned char gTest[] = { 42 };
>> 
>> static int test ()
>> {
>>  return 1;
>> }
>> 
>> int main ()
>> {
>>  return 0;
>> }
>> ~/work/Compiler>clang -Os static.c -Wall
>> static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
>> static unsigned char gTest[] = { 42 };
>> ^
>> static.c:3:12: warning: unused function 'test' [-Wunused-function]
>> static int test ()
>>   ^
>> 2 warnings generated.
> 
> Sorry, my question was imprecise.
> 
> Assume there is a public library function ("external linkage") that
> calls a static function in the same library instance and uses a static
> variable in the same library instance. Then this library instance is
> linked into a driver, but the driver never actually calls the extern
> function -- so the static variable and the static function too become
> useless.
> 
> In this case, will /OPT:REF remove the static variable and the static
> function too?
> 
> It seems counter-intuitive to me that an internal-only function or an
> internal-only variable has to be declared extern (via
> GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
> time, if it is never referenced (transitively).
> 

Laszlo,

I agree. The LLVM LTO does not have an issue "doing the right thing". Seems 
like static is also more of a compile time concept vs a link time (global 
optimization) kind of thing? 

Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to __declspec( selectany ) I 
would guess maybe it has more to due with supporting old non standard header 
files that can't change without breaking compatibility. 

MSDN on __declspec( selectany ) :
A global data item can normally be initialized only once in an EXE or DLL 
project. selectany can be used in initializing global data defined by headers, 
when the same header appears in more than one source file. selectany is 
available in both the C and C++ compilers.

Thanks,

Andrew Fish


> Thanks
> Laszlo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org 
> https://lists.01.org/mailman/listinfo/edk2-devel 
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] BaseTools/GCC ARM/AARCH64: Force disable PIE

[dann frazier]

On Wed, May 24, 2017 at 03:27:07PM -0700, Ard Biesheuvel wrote:
> On 24 May 2017 at 13:26, dann frazier  wrote:
> > v2:
> > * Replace -no-pie w/ -static for compat with GCC 4.9
> >
> 
> For my understanding, could you elaborate on what goes wrong if you
> omit -static / -no-pie?

Well, nothing... now :) -static/-no-pie was avoiding an issue with GenFw:

   ---
"GenFw" -e SEC -o 
/tmp/edk2/Build/ArmVirtQemu-AARCH64/DEBUG_GCC49/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.efi
 
/tmp/edk2/Build/ArmVirtQemu-AARCH64/DEBUG_GCC49/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
GenFw: ERROR 3000: Invalid
  WriteSections64():
  
/tmp/edk2/Build/ArmVirtQemu-AARCH64/DEBUG_GCC49/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
  AARCH64 small code model requires identical ELF and PE/COFF section
  offsets modulo 4 KB.
[...]
   ---

Upon rebase/retest, I found that this was no longer reproducible.
Bisection shows that the problem went away recently after this commit:

commit 00b00cc57bfe0fca54c904d4dd44a263e243c88b
Author: Ard Biesheuvel 
Date:   Fri May 19 11:47:40 2017 +0100

BaseTools/Scripts: discard .gnu.hash section in GCC builds

-*f*no-pie does still seem to be needed though. Without it, the ARM
build fails with:

   ---
"GenFw" -e DXE_DRIVER -o 
/tmp/edk2/Build/ArmVirtQemu-ARM/DEBUG_GCC49/ARM/MdeModulePkg/Universal/Network/DpcDxe/DpcDxe/DEBUG/DpcDxe.efi
 
/tmp/edk2/Build/ArmVirtQemu-ARM/DEBUG_GCC49/ARM/MdeModulePkg/Universal/Network/DpcDxe/DpcDxe/DEBUG/DpcDxe.dll
GenFw: ERROR 3000: Invalid
  
/tmp/edk2/Build/ArmVirtQemu-ARM/DEBUG_GCC49/ARM/MdeModulePkg/Universal/Network/DpcDxe/DpcDxe/DEBUG/DpcDxe.dll:
 Bad definition for symbol '_GLOBAL_OFFSET_TABLE_'@0x5e94 or unsupported symbol 
type.  For example, absolute and undefined symbols are not supported.
   ---

  -dann
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Laszlo Ersek]

On 05/25/17 22:37, Andrew Fish wrote:
> 
>> On May 25, 2017, at 1:28 PM, Laszlo Ersek  wrote:
>>
>> On 05/25/17 22:11, Ard Biesheuvel wrote:
>>> On 25 May 2017 at 13:06, Kinney, Michael D  
>>> wrote:
 Laszlo and Andrew,

 With the information that has been collected on this thread, I
 still think this patch in its original form is a good change
 to resolve the this one specific duplicate symbol issue for all
 tool chains.  'static' can not be mixed with
 GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
 the global variable is the easiest way to remove the duplicate.

>>>
>>> GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
>>> was Felix who reported on this recently?
>>>
>>> STATIC is really the only sensible way to deal with this for symbols
>>> that are only referenced by a single compilation unit.
>>>
 I will continue to work on ways to detect duplicate symbols for
 all tool chains and will enter a Bugzilla issue to for that
 feature.

 In addition, the idea of detecting if a library is exporting more
 than the library class defines is another good feature to consider
 and I will enter a Bugzilla issue for that one as well.

 If we can find ways to both restrict the symbols exported by a
 library and strip all symbols that are unused, then we can have
 additional Bugzilla issues to perform that clean up on each
 library instance that is exporting more than the library class.

>>>
>>> A static library is nothing more than an archive containing a
>>> collection of object files. Sadly, that implies that we cannot
>>> distinguish between symbols that may only be referenced by other
>>> objects in the same static library and symbols that are exported to
>>> the library client.
>>
>> Do we know for a fact that, with /OPT:REF, VS does not strip unused
>> *static* variables and functions?
>>
>> I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
>> with STATIC in this case would lead to a size increase?
>>
>> If that's the case, then I'm fine if we go ahead with this patch, I'd
>> just like to request that Mike please file some of those BZs, and please
>> reference them from the commit message (as the longer term solution),
>> before committing the patch.
>>
> 
> Clang will warn if you have unused static variables when warnings are cranked 
> up.
> 
> ~/work/Compiler>cat static.c
> static unsigned char gTest[] = { 42 };
> 
> static int test ()
> {
>   return 1;
> }
> 
> int main ()
> {
>   return 0;
> }
> ~/work/Compiler>clang -Os static.c -Wall
> static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
> static unsigned char gTest[] = { 42 };
>  ^
> static.c:3:12: warning: unused function 'test' [-Wunused-function]
> static int test ()
>^
> 2 warnings generated.

Sorry, my question was imprecise.

Assume there is a public library function ("external linkage") that
calls a static function in the same library instance and uses a static
variable in the same library instance. Then this library instance is
linked into a driver, but the driver never actually calls the extern
function -- so the static variable and the static function too become
useless.

In this case, will /OPT:REF remove the static variable and the static
function too?

It seems counter-intuitive to me that an internal-only function or an
internal-only variable has to be declared extern (via
GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
time, if it is never referenced (transitively).

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Andrew Fish]

> On May 25, 2017, at 1:28 PM, Laszlo Ersek  wrote:
> 
> On 05/25/17 22:11, Ard Biesheuvel wrote:
>> On 25 May 2017 at 13:06, Kinney, Michael D  
>> wrote:
>>> Laszlo and Andrew,
>>> 
>>> With the information that has been collected on this thread, I
>>> still think this patch in its original form is a good change
>>> to resolve the this one specific duplicate symbol issue for all
>>> tool chains.  'static' can not be mixed with
>>> GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
>>> the global variable is the easiest way to remove the duplicate.
>>> 
>> 
>> GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
>> was Felix who reported on this recently?
>> 
>> STATIC is really the only sensible way to deal with this for symbols
>> that are only referenced by a single compilation unit.
>> 
>>> I will continue to work on ways to detect duplicate symbols for
>>> all tool chains and will enter a Bugzilla issue to for that
>>> feature.
>>> 
>>> In addition, the idea of detecting if a library is exporting more
>>> than the library class defines is another good feature to consider
>>> and I will enter a Bugzilla issue for that one as well.
>>> 
>>> If we can find ways to both restrict the symbols exported by a
>>> library and strip all symbols that are unused, then we can have
>>> additional Bugzilla issues to perform that clean up on each
>>> library instance that is exporting more than the library class.
>>> 
>> 
>> A static library is nothing more than an archive containing a
>> collection of object files. Sadly, that implies that we cannot
>> distinguish between symbols that may only be referenced by other
>> objects in the same static library and symbols that are exported to
>> the library client.
> 
> Do we know for a fact that, with /OPT:REF, VS does not strip unused
> *static* variables and functions?
> 
> I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
> with STATIC in this case would lead to a size increase?
> 
> If that's the case, then I'm fine if we go ahead with this patch, I'd
> just like to request that Mike please file some of those BZs, and please
> reference them from the commit message (as the longer term solution),
> before committing the patch.
> 

Clang will warn if you have unused static variables when warnings are cranked 
up.

~/work/Compiler>cat static.c
static unsigned char gTest[] = { 42 };

static int test ()
{
  return 1;
}

int main ()
{
  return 0;
}
~/work/Compiler>clang -Os static.c -Wall
static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
static unsigned char gTest[] = { 42 };
 ^
static.c:3:12: warning: unused function 'test' [-Wunused-function]
static int test ()
   ^
2 warnings generated.


Thanks,

Andrew Fish

> Thanks
> Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Laszlo Ersek]

On 05/25/17 22:11, Ard Biesheuvel wrote:
> On 25 May 2017 at 13:06, Kinney, Michael D  wrote:
>> Laszlo and Andrew,
>>
>> With the information that has been collected on this thread, I
>> still think this patch in its original form is a good change
>> to resolve the this one specific duplicate symbol issue for all
>> tool chains.  'static' can not be mixed with
>> GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
>> the global variable is the easiest way to remove the duplicate.
>>
> 
> GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
> was Felix who reported on this recently?
> 
> STATIC is really the only sensible way to deal with this for symbols
> that are only referenced by a single compilation unit.
> 
>> I will continue to work on ways to detect duplicate symbols for
>> all tool chains and will enter a Bugzilla issue to for that
>> feature.
>>
>> In addition, the idea of detecting if a library is exporting more
>> than the library class defines is another good feature to consider
>> and I will enter a Bugzilla issue for that one as well.
>>
>> If we can find ways to both restrict the symbols exported by a
>> library and strip all symbols that are unused, then we can have
>> additional Bugzilla issues to perform that clean up on each
>> library instance that is exporting more than the library class.
>>
> 
> A static library is nothing more than an archive containing a
> collection of object files. Sadly, that implies that we cannot
> distinguish between symbols that may only be referenced by other
> objects in the same static library and symbols that are exported to
> the library client.

Do we know for a fact that, with /OPT:REF, VS does not strip unused
*static* variables and functions?

I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
with STATIC in this case would lead to a size increase?

If that's the case, then I'm fine if we go ahead with this patch, I'd
just like to request that Mike please file some of those BZs, and please
reference them from the commit message (as the longer term solution),
before committing the patch.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Ard Biesheuvel]

On 25 May 2017 at 13:06, Kinney, Michael D  wrote:
> Laszlo and Andrew,
>
> With the information that has been collected on this thread, I
> still think this patch in its original form is a good change
> to resolve the this one specific duplicate symbol issue for all
> tool chains.  'static' can not be mixed with
> GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
> the global variable is the easiest way to remove the duplicate.
>

GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
was Felix who reported on this recently?

STATIC is really the only sensible way to deal with this for symbols
that are only referenced by a single compilation unit.

> I will continue to work on ways to detect duplicate symbols for
> all tool chains and will enter a Bugzilla issue to for that
> feature.
>
> In addition, the idea of detecting if a library is exporting more
> than the library class defines is another good feature to consider
> and I will enter a Bugzilla issue for that one as well.
>
> If we can find ways to both restrict the symbols exported by a
> library and strip all symbols that are unused, then we can have
> additional Bugzilla issues to perform that clean up on each
> library instance that is exporting more than the library class.
>

A static library is nothing more than an archive containing a
collection of object files. Sadly, that implies that we cannot
distinguish between symbols that may only be referenced by other
objects in the same static library and symbols that are exported to
the library client.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Laszlo Ersek]

On 05/25/17 21:57, Kinney, Michael D wrote:
> Laszlo,
> 
> I have the same concern on final image sizes.  I have done some
> evaluation:
> 
> GCC5 OVMF X64 DEBUG without -whole-archive 
> ==
> FV Space Information
> SECFV [19%Full] 212992 total, 42000 used, 170992 free
> FVMAIN_COMPACT [33%Full] 3440640 total, 1162760 used, 2277880 free
> DXEFV [38%Full] 10485760 total, 4024024 used, 6461736 free
> PEIFV [19%Full] 917504 total, 180648 used, 736856 free
> Total used = 5409432
> 
> GCC5 OVMF X64 DEBUG with -whole-archive 
> ===
> FV Space Information
> SECFV [19%Full] 212992 total, 41936 used, 171056 free
> FVMAIN_COMPACT [33%Full] 3440640 total, 1158304 used, 2282336 free
> DXEFV [38%Full] 10485760 total, 4029656 used, 6456104 free
> PEIFV [19%Full] 917504 total, 181352 used, 736152 free
> Total used = 5411248
> 
> Total used difference = 1816 bytes larger with -whole-archive
> 
> I was also able to do a MSFT VS2015 build with /WHOLEARCHIVE set
> and it also catches the same duplicate symbol error now.
> 
> error C2220: warning treated as error - no 'executable' file generated
> warning C4744: 'mMemoryDiscoveredNotifyList' has different type in 
> 'd:\work\github\tianocore\edk2\mdemodulepkg\core\dxeiplpeim\dxeload.c' and 
> 'd:\work\github\tianocore\edk2\sourceleveldebugpkg\library\debugagent\secpeidebugagent\secpeidebugagentlib.c':
>  'struct (12 bytes)' and 'array (12 bytes)'
> DxeIpl.lib(DxeLoad.obj) : error LNK2005: _mMemoryDiscoveredNotifyList already 
> defined in SecPeiDebugAgentLib.lib(SecPeiDebugAgentLib.obj)
> d:\work\github\tianocore\edk2\Build\OvmfIa32\DEBUG_VS2015x86\IA32\MdeModulePkg\Core\DxeIplPeim\DxeIpl\DEBUG\DxeIpl.dll
>  : fatal error LNK1169: one or more multiply defined symbols found
> 
> VS2015 OVMF X64 DEBUG without /WHOLEARCHIVE 
> ===
> FV Space Information
> SECFV [22%Full] 212992 total, 48560 used, 164432 free
> FVMAIN_COMPACT [33%Full] 3440640 total, 1147464 used, 2293176 free
> DXEFV [39%Full] 10485760 total, 4163888 used, 6321872 free
> PEIFV [22%Full] 917504 total, 204840 used, 712664 free
> Total used = 5564752
> 
> 
> VS2015 OVMF X64 DEBUG with /WHOLEARCHIVE 
> ===
> FV Space Information
> SECFV [23%Full] 212992 total, 50384 used, 162608 free
> FVMAIN_COMPACT [33%Full] 3440640 total, 1147424 used, 2293216 free
> DXEFV [42%Full] 10485760 total, 4422992 used, 6062768 free
> PEIFV [27%Full] 917504 total, 255528 used, 661976 free
> Total used = 5875338
> 
> Total used difference = 310586 bytes larger with /WHOLEARCHIVE
> 
> For tool chains that do have size impacts, one option is to
> have a "test" build that enables the linker flags to detect
> duplicate symbols.  For example the following could be added
> to a DSC file.  May want to disable GenFds stage when doing
> this type of build.
> 
> [BuildOptions]
> !ifdef $(DETECT_DUPLICATE_SYMBOLS)
>   MSFT:*_VS2015_*_DLINK_FLAGS = /WHOLEARCHIVE
> !endif

Thank you (again) for the research! Looks like the gcc size impact is
friendly enough to keep --whole-archive enabled at all times (possibly
due to the --gc-sections flag mentioned by Ard, which we already have
enabled).

The VS2015 impact is really large however.

I was hoping we could add these flags to
"BaseTools/Conf/tools_def.template", regardless of platform DSC. (If the
flag is non-default, and/or it's platform-dependent, then it will almost
never be used, most likely.) But the VS2015 size increase really
precludes /WHOLEARCHIVE (for the MSFT family) from "tools_def.template".

Would it be acceptable to add --whole-archive to "tools_def.template"
only for GCC? After all, at the moment only XCODE*/XCLANG have "-all_load".

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Kinney, Michael D]

Laszlo and Andrew,

With the information that has been collected on this thread, I
still think this patch in its original form is a good change
to resolve the this one specific duplicate symbol issue for all
tool chains.  'static' can not be mixed with
GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
the global variable is the easiest way to remove the duplicate.

I will continue to work on ways to detect duplicate symbols for
all tool chains and will enter a Bugzilla issue to for that
feature.

In addition, the idea of detecting if a library is exporting more
than the library class defines is another good feature to consider
and I will enter a Bugzilla issue for that one as well.

If we can find ways to both restrict the symbols exported by a
library and strip all symbols that are unused, then we can have
additional Bugzilla issues to perform that clean up on each 
library instance that is exporting more than the library class.

Thanks,

Mike

> -Original Message-
> From: Fan, Jeff
> Sent: Tuesday, May 23, 2017 7:48 PM
> To: Kinney, Michael D ; edk2-devel@lists.01.org
> Cc: Wu, Hao A ; Kinney, Michael D
> ; Laszlo Ersek ; Andrew Fish
> 
> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> duplicate symbol
> 
> Reviewed-by: Jeff Fan 
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Michael
> Kinney
> Sent: Wednesday, May 24, 2017 7:21 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A; Kinney, Michael D; Laszlo Ersek; Andrew Fish; Fan, Jeff
> Subject: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> symbol
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=573
> 
> The SecPeiDebugAgentLib uses the global variable mMemoryDiscoveredNotifyList 
> for
> a PPI notification on the Memory Discovered PPI.  This same variable name is 
> used
> in the DxeIplPeim for the same PPI notification.
> 
> The XCODE5 tool chain detects this duplicate symbol when the OVMF platform is
> built with the flag -D SOURCE_DEBUG_ENABLE.
> 
> The fix is to rename this global variable in the SecPeiDebugAgentLib library.
> 
> Cc: Andrew Fish 
> Cc: Jeff Fan 
> Cc: Hao Wu 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael D Kinney 
> ---
>  .../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c | 4 
> ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git
> a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
> b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
> index b717e33..9f5223a 100644
> ---
> a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
> +++ b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebu
> +++ gAgentLib.c
> @@ -32,7 +32,7 @@ GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_PPI_DESCRIPTOR
> mVectorHandoffInf
>}
>  };
> 
> -GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR
> mMemoryDiscoveredNotifyList[1] = {
> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR
> +mDebugAgentMemoryDiscoveredNotifyList[1] = {
>{
>  (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK |
> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
>  ,
> @@ -554,7 +554,7 @@ InitializeDebugAgent (
>  // Register for a callback once memory has been initialized.
>  // If memery has been ready, the callback funtion will be invoked
> immediately
>  //
> -Status = PeiServicesNotifyPpi ([0]);
> +Status = PeiServicesNotifyPpi
> + ([0]);
>  if (EFI_ERROR (Status)) {
>DEBUG ((EFI_D_ERROR, "DebugAgent: Failed to register memory discovered
> callback function!\n"));
>CpuDeadLoop ();
> --
> 2.6.3.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Laszlo Ersek]

On 05/25/17 21:55, Ard Biesheuvel wrote:
> On 25 May 2017 at 11:06, Laszlo Ersek  wrote:
>> On 05/25/17 19:38, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> I think the equivalent flag for GCC builds is --whole-archive.
>>>
>>> I tried adding that flag to DLINK_FLAGS in GCC5, and I get the
>>> following error building OVMF from edk2/master with
>>> -D SOURCE_DEBUG_ENABLE set.
>>>
>>> DxeLoad.obj (symbol from plugin): In function 
>>> `InstallIplPermanentMemoryPpis':
>>> (.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
>>> SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
>>> collect2: error: ld returned 1 exit status
>>
>> Great find! That's the error message we should get.
>>
>> Unfortunately, after reading the "ld" manual on "--whole-archive", it
>> seems that the complete object files will actually be copied into the
>> resultant binary, even if several of their symbols will remain unused. I
>> think that's quite sub-optimal. (I haven't verified this though.) What
>> we'd like to get is (a) the full verification at link time, and (b)
>> inclusion of *only* those symbols that are actually necessary.
>>
>> In your testing, when you build OVMF with and without "--whole-archive",
>> do you see a difference in, say, the DXEFV footprint, when the build
>> completes?
>>
>> (If so, then I wonder if we should add "--whole-archive" only to the
>> NOOPT build... Not sure.)
>>
> 
> I haven't tried, but I would expect --gc-sections to deal with the
> unused objects.

Good point! (... After I read this option's description too, in the "ld"
manual.) This ELF and PE/COFF stuff is way too complex for my taste. :/

Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v5 07/14] OvmfPkg:IoMmuDxe: Add IoMmuDxe driver

[Laszlo Ersek]

On 05/25/17 20:56, Jordan Justen wrote:
> On 2017-05-25 10:58:45, Laszlo Ersek wrote:
>>
>> If you add a patch to the series where you change the DEPEX of
>> PciHostBridgeDxe to
>>
>>   gEdkiiIoMmuProtocolGuid OR gEdkiiIoMmuAbsentProtocolGuid
>>
>> then the maintainer (Ray or Jiewen I think) will likely reject that
>> patch, because afterwards, all platforms that include PciHostBridgeDxe
>> would suddenly have to produce gEdkiiIoMmuAbsentProtocolGuid, minimally.
> 
> I haven't worked with it, but it does appear that feature PCD's can be
> used to conditionally add new depex values.
> 
> $ git grep -e '|.*Pcd' -- '*.inf'

I've now reviewed the result list for this search, and none of the
results are contained in a [Depex] section actually. They are all under
[Protocols], [Guids], or [Pcd] -- there doesn't seem to be an example
for a FeaturePCD-dependent DEPEX.

> I'm not sure if that feature works with depex 'OR'.
> 
> Regarding IoMmuAbsent or IoMmuDetectionAttempted, they seem similar.

Right, I'm fine with either.

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Kinney, Michael D]

Laszlo,

I have the same concern on final image sizes.  I have done some
evaluation:

GCC5 OVMF X64 DEBUG without -whole-archive 
==
FV Space Information
SECFV [19%Full] 212992 total, 42000 used, 170992 free
FVMAIN_COMPACT [33%Full] 3440640 total, 1162760 used, 2277880 free
DXEFV [38%Full] 10485760 total, 4024024 used, 6461736 free
PEIFV [19%Full] 917504 total, 180648 used, 736856 free
Total used = 5409432

GCC5 OVMF X64 DEBUG with -whole-archive 
===
FV Space Information
SECFV [19%Full] 212992 total, 41936 used, 171056 free
FVMAIN_COMPACT [33%Full] 3440640 total, 1158304 used, 2282336 free
DXEFV [38%Full] 10485760 total, 4029656 used, 6456104 free
PEIFV [19%Full] 917504 total, 181352 used, 736152 free
Total used = 5411248

Total used difference = 1816 bytes larger with -whole-archive

I was also able to do a MSFT VS2015 build with /WHOLEARCHIVE set
and it also catches the same duplicate symbol error now.

error C2220: warning treated as error - no 'executable' file generated
warning C4744: 'mMemoryDiscoveredNotifyList' has different type in 
'd:\work\github\tianocore\edk2\mdemodulepkg\core\dxeiplpeim\dxeload.c' and 
'd:\work\github\tianocore\edk2\sourceleveldebugpkg\library\debugagent\secpeidebugagent\secpeidebugagentlib.c':
 'struct (12 bytes)' and 'array (12 bytes)'
DxeIpl.lib(DxeLoad.obj) : error LNK2005: _mMemoryDiscoveredNotifyList already 
defined in SecPeiDebugAgentLib.lib(SecPeiDebugAgentLib.obj)
d:\work\github\tianocore\edk2\Build\OvmfIa32\DEBUG_VS2015x86\IA32\MdeModulePkg\Core\DxeIplPeim\DxeIpl\DEBUG\DxeIpl.dll
 : fatal error LNK1169: one or more multiply defined symbols found

VS2015 OVMF X64 DEBUG without /WHOLEARCHIVE 
===
FV Space Information
SECFV [22%Full] 212992 total, 48560 used, 164432 free
FVMAIN_COMPACT [33%Full] 3440640 total, 1147464 used, 2293176 free
DXEFV [39%Full] 10485760 total, 4163888 used, 6321872 free
PEIFV [22%Full] 917504 total, 204840 used, 712664 free
Total used = 5564752


VS2015 OVMF X64 DEBUG with /WHOLEARCHIVE 
===
FV Space Information
SECFV [23%Full] 212992 total, 50384 used, 162608 free
FVMAIN_COMPACT [33%Full] 3440640 total, 1147424 used, 2293216 free
DXEFV [42%Full] 10485760 total, 4422992 used, 6062768 free
PEIFV [27%Full] 917504 total, 255528 used, 661976 free
Total used = 5875338

Total used difference = 310586 bytes larger with /WHOLEARCHIVE

For tool chains that do have size impacts, one option is to
have a "test" build that enables the linker flags to detect
duplicate symbols.  For example the following could be added
to a DSC file.  May want to disable GenFds stage when doing
this type of build.

[BuildOptions]
!ifdef $(DETECT_DUPLICATE_SYMBOLS)
  MSFT:*_VS2015_*_DLINK_FLAGS = /WHOLEARCHIVE
!endif

Best regards,

Mike

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, May 25, 2017 11:07 AM
> To: Kinney, Michael D ; Ard Biesheuvel
> ; Andrew Fish (af...@apple.com) 
> Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff
> 
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix 
> duplicate
> symbol
> 
> On 05/25/17 19:38, Kinney, Michael D wrote:
> > Laszlo,
> >
> > I think the equivalent flag for GCC builds is --whole-archive.
> >
> > I tried adding that flag to DLINK_FLAGS in GCC5, and I get the
> > following error building OVMF from edk2/master with
> > -D SOURCE_DEBUG_ENABLE set.
> >
> > DxeLoad.obj (symbol from plugin): In function 
> > `InstallIplPermanentMemoryPpis':
> > (.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
> > SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
> > collect2: error: ld returned 1 exit status
> 
> Great find! That's the error message we should get.
> 
> Unfortunately, after reading the "ld" manual on "--whole-archive", it
> seems that the complete object files will actually be copied into the
> resultant binary, even if several of their symbols will remain unused. I
> think that's quite sub-optimal. (I haven't verified this though.) What
> we'd like to get is (a) the full verification at link time, and (b)
> inclusion of *only* those symbols that are actually necessary.
> 
> In your testing, when you build OVMF with and without "--whole-archive",
> do you see a difference in, say, the DXEFV footprint, when the build
> completes?
> 
> (If so, then I wonder if we should add "--whole-archive" only to the
> NOOPT build... Not sure.)
> 
> > Visual Studio 2015 Update 2 has also added a new linker flag called
> > /WHOLEARCHIVE.  I am working on evaluating that flag to see if it
> > catches the same issue.
> 
> Thanks!
> Laszlo
> 
> >> -Original Message-
> >> From: Laszlo Ersek [mailto:ler...@redhat.com]
> >> Sent: Thursday, May 25, 2017 9:09 AM
> >> To: 

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Ard Biesheuvel]

On 25 May 2017 at 11:06, Laszlo Ersek  wrote:
> On 05/25/17 19:38, Kinney, Michael D wrote:
>> Laszlo,
>>
>> I think the equivalent flag for GCC builds is --whole-archive.
>>
>> I tried adding that flag to DLINK_FLAGS in GCC5, and I get the
>> following error building OVMF from edk2/master with
>> -D SOURCE_DEBUG_ENABLE set.
>>
>> DxeLoad.obj (symbol from plugin): In function 
>> `InstallIplPermanentMemoryPpis':
>> (.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
>> SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
>> collect2: error: ld returned 1 exit status
>
> Great find! That's the error message we should get.
>
> Unfortunately, after reading the "ld" manual on "--whole-archive", it
> seems that the complete object files will actually be copied into the
> resultant binary, even if several of their symbols will remain unused. I
> think that's quite sub-optimal. (I haven't verified this though.) What
> we'd like to get is (a) the full verification at link time, and (b)
> inclusion of *only* those symbols that are actually necessary.
>
> In your testing, when you build OVMF with and without "--whole-archive",
> do you see a difference in, say, the DXEFV footprint, when the build
> completes?
>
> (If so, then I wonder if we should add "--whole-archive" only to the
> NOOPT build... Not sure.)
>

I haven't tried, but I would expect --gc-sections to deal with the
unused objects.

>> Visual Studio 2015 Update 2 has also added a new linker flag called
>> /WHOLEARCHIVE.  I am working on evaluating that flag to see if it
>> catches the same issue.
>
> Thanks!
> Laszlo
>
>>> -Original Message-
>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>> Sent: Thursday, May 25, 2017 9:09 AM
>>> To: Kinney, Michael D ; Ard Biesheuvel
>>> ; Andrew Fish (af...@apple.com) 
>>> Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff
>>> 
>>> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix 
>>> duplicate
>>> symbol
>>>
>>> On 05/25/17 03:47, Kinney, Michael D wrote:
 Andrew,

 I think I have found an alternate fix for this XCODE5 specific
 build failure.  Since there appears to be a difference in the
 linker behavior between MSFT/GCC/XCODE tool chains, I reviewed
 the 'ld' command line options used in XCODE5 tool chain in
 tools_def.txt.

 There is a flag set call '-all_load'.  The description of this
 flag is 'Loads all members of static archive libraries.'.

 I tried removing this flag from the XCODE5 specific SLINK_FLAGS
 and DLINK_FLAGS statements in tools_def.txt, and the duplicate
 symbol build failure is no longer present.  I am able to build
 and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.

 This seems to make XCODE5 linker behavior match the MSFT and GCC
 linker behavior.

 Do you know why '-all_load' is used in XCODE5 and what impacts
 there may be from removing it?
>>>
>>> Please don't remove -all_load from there; instead we should figure out
>>> if the same can be brought to MSFT and GCC.
>>>
>>> The error message that XCODE5 emitted caught a real bug (undefined
>>> behavior according to ISO C, see my previous email), and so we should
>>> keep that detection enabled (we should even extend it to other
>>> toolchains, if that's possible).
>>>
>>> As for docs, I found this:
>>>
>>> http://www.manpages.info/macosx/ld.1.html
>>>
 -all_load
 Loads all members of static archive libraries. This option does
 not apply to dynamic shared libraries.
>>>
>>>
>>> Thanks
>>> Laszlo
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v5 07/14] OvmfPkg:IoMmuDxe: Add IoMmuDxe driver

[Jordan Justen]

On 2017-05-25 10:58:45, Laszlo Ersek wrote:
> 
> If you add a patch to the series where you change the DEPEX of
> PciHostBridgeDxe to
> 
>   gEdkiiIoMmuProtocolGuid OR gEdkiiIoMmuAbsentProtocolGuid
> 
> then the maintainer (Ray or Jiewen I think) will likely reject that
> patch, because afterwards, all platforms that include PciHostBridgeDxe
> would suddenly have to produce gEdkiiIoMmuAbsentProtocolGuid, minimally.

I haven't worked with it, but it does appear that feature PCD's can be
used to conditionally add new depex values.

$ git grep -e '|.*Pcd' -- '*.inf'

I'm not sure if that feature works with depex 'OR'.

Regarding IoMmuAbsent or IoMmuDetectionAttempted, they seem similar.

-Jordan
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v5 04/14] OvmfPkg/BaseMemcryptSevLib: Add SEV helper library

[Brijesh Singh]

On 05/25/2017 10:10 AM, Laszlo Ersek wrote:


The canonical way to write this DEBUG invocation is:

   DEBUG ((
 DEBUG_VERBOSE,
 "%a:%a Set C-bit Cr3 %Lx Base %Lx Length %Lx flush %d\n",
 gEfiCallerBaseName,
 __FUNCTION__,
 Cr3BaseAddress,
 PhysicalAddress,
 Length,
 Flush
 ));

(Do not miss the indentation of the closing paren(s)!)

Please refer to .

If it all fits on a single line, not exceeding 80 characters, then you
can keep it on a single line.

Otherwise, if you don't fit on a single line, then you have to break
every argument to a separate line. If your format string (or any other
argument) doesn't fit on a line in itself, then you have to break it up
too.

Earlier I'd been using a "meet in the middle" style, where I wouldn't
exceed 80 characters per line, and would indent the continuations by 2
additional spaces, but still wouldn't break each argument to a new line.
Example:

   DEBUG ((DEBUG_VERBOSE,
 "%a:%a Set C-bit Cr3 %Lx Base %Lx Length %Lx flush %d\n",
 gEfiCallerBaseName, __FUNCTION__, Cr3BaseAddress, PhysicalAddress, Length,
 Flush));

In my opinion, this would be the best compromise, since (a) it keeps
lines under 80 chars width, (b) conforms to the indentation requirement,
(c) doesn't waste vertical space like the official layout above.

However, this style had not been approved, and I abandoned it in favor
of the canonical style, when I filed
.


I will follow your recommendation. I will wait for Jordan's response
on your IoMmu patch suggestion and include all fixes in  v6.

Thank you so much for feedback.

-Brijesh
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Laszlo Ersek]

On 05/25/17 19:38, Kinney, Michael D wrote:
> Laszlo,
> 
> I think the equivalent flag for GCC builds is --whole-archive.
> 
> I tried adding that flag to DLINK_FLAGS in GCC5, and I get the 
> following error building OVMF from edk2/master with 
> -D SOURCE_DEBUG_ENABLE set.
> 
> DxeLoad.obj (symbol from plugin): In function `InstallIplPermanentMemoryPpis':
> (.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
> SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
> collect2: error: ld returned 1 exit status

Great find! That's the error message we should get.

Unfortunately, after reading the "ld" manual on "--whole-archive", it
seems that the complete object files will actually be copied into the
resultant binary, even if several of their symbols will remain unused. I
think that's quite sub-optimal. (I haven't verified this though.) What
we'd like to get is (a) the full verification at link time, and (b)
inclusion of *only* those symbols that are actually necessary.

In your testing, when you build OVMF with and without "--whole-archive",
do you see a difference in, say, the DXEFV footprint, when the build
completes?

(If so, then I wonder if we should add "--whole-archive" only to the
NOOPT build... Not sure.)

> Visual Studio 2015 Update 2 has also added a new linker flag called
> /WHOLEARCHIVE.  I am working on evaluating that flag to see if it 
> catches the same issue.

Thanks!
Laszlo

>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Thursday, May 25, 2017 9:09 AM
>> To: Kinney, Michael D ; Ard Biesheuvel
>> ; Andrew Fish (af...@apple.com) 
>> Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff
>> 
>> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix 
>> duplicate
>> symbol
>>
>> On 05/25/17 03:47, Kinney, Michael D wrote:
>>> Andrew,
>>>
>>> I think I have found an alternate fix for this XCODE5 specific
>>> build failure.  Since there appears to be a difference in the
>>> linker behavior between MSFT/GCC/XCODE tool chains, I reviewed
>>> the 'ld' command line options used in XCODE5 tool chain in
>>> tools_def.txt.
>>>
>>> There is a flag set call '-all_load'.  The description of this
>>> flag is 'Loads all members of static archive libraries.'.
>>>
>>> I tried removing this flag from the XCODE5 specific SLINK_FLAGS
>>> and DLINK_FLAGS statements in tools_def.txt, and the duplicate
>>> symbol build failure is no longer present.  I am able to build
>>> and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
>>>
>>> This seems to make XCODE5 linker behavior match the MSFT and GCC
>>> linker behavior.
>>>
>>> Do you know why '-all_load' is used in XCODE5 and what impacts
>>> there may be from removing it?
>>
>> Please don't remove -all_load from there; instead we should figure out
>> if the same can be brought to MSFT and GCC.
>>
>> The error message that XCODE5 emitted caught a real bug (undefined
>> behavior according to ISO C, see my previous email), and so we should
>> keep that detection enabled (we should even extend it to other
>> toolchains, if that's possible).
>>
>> As for docs, I found this:
>>
>> http://www.manpages.info/macosx/ld.1.html
>>
>>> -all_load
>>> Loads all members of static archive libraries. This option does
>>> not apply to dynamic shared libraries.
>>
>>
>> Thanks
>> Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v5 07/14] OvmfPkg:IoMmuDxe: Add IoMmuDxe driver

[Laszlo Ersek]

On 05/24/17 17:09, Laszlo Ersek wrote:
> (1) So, I don't think that splitting this driver off of AmdSevDxe is a
> significant improvement, given that we still need to add AmdSevDxe to
> the APRIORI DXE file, in order to clear the C bit on NonExistent and
> MMIO ranges.
> 
> If Jordan thinks it is an improvement nonetheless, I don't mind the
> split, of course.
> 
> On 05/22/17 17:23, Brijesh Singh wrote:
>> The IOMMU protocol driver provides capabilities to set a DMA access
>> attribute and methods to allocate, free, map and unmap the DMA memory
>> for the PCI Bus devices.
>>
>> Due to security reasons all DMA operations inside the SEV guest must
>> be performed on shared (i.e unencrypted) pages. The IOMMU protocol
>> driver for the SEV guest uses a bounce buffer to map guest DMA buffer
>> to shared pages inorder to provide the support for DMA operations inside
>> SEV guest.
>>
>> The patch adds a new synthetic/placeholder protocol
>> 'gIoMmuDetectedProtocolGuid" to allow other dependent modules to depend
>> on IoMmuDxe driver being run.
> 
> (2) If we add the protocol GUID to the OVMF DEC file, that should be a
> separate patch, in my opinion. The commit message should explain, in a
> stand-alone manner, what the protocol stands for.
> 
> (I see Jordan's suggestion for the proto name in
> ,
> namely "gOvmfIoMmuDetectionProtocolGuid", but I think that Brijesh's
> suggestion is closer to the protocol names we already have under
> [Protocols] in OvmfPkg.dec.)
> 
>> IoMmuDxe driver looks for SEV capabilities, if present then it installs
>> the real IOMMU protocol otherwise it installs placeholder protocol.
>> Currently, PciRoot Bridge and QemuFWCfgLib need to know the existance
>> of IOMMU protocol. So the modules needing the IOMMU support should add
>> both gEdkiiIoMmuProtocolGuid and gIoMmuDetectedProtocolGuid in there depex.
> 
> (3) This description (which again belongs to the separate patch that
> introduces the protocol) should be formulated without mentioning SEV or
> QemuFwCfgLib. Something like:
> 
>   Platforms that optionally provide an IOMMU protocol should do so by
>   including a DXE driver (usually called IoMmuDxe) that produces either
>   the IOMMU protocol -- if the underlying capabilities are available --,
>   or gIoMmuDetectedProtocolGuid, to signal that the IOMMU capability
>   detection completed with negative result (i.e., no IOMMU will be
>   available in the system).
> 
>   In turn, DXE drivers (and library instances) that are supposed to use
>   the IOMMU protocol if it is available should add the following to
>   their DEPEX:
> 
> gEdkiiIoMmuProtocolGuid OR gIoMmuDetectedProtocolGuid
> 
>   This ensures these client modules will only be dispatched after IOMMU
>   detection completes (with positive or negative result).
> 
>> Please note that since PciRoot Bridge driver does not run until the BDS
>> phase, and IoMmuDxe driver would have been dispatched by then hence we
>> do not need to add depex in PciRoot Bridge driver inf file.
> 
> (4) This statement is incorrect.
> 
> PciHostBridgeDxe is definitely dispatched before BDS is entered, it is a
> plain DXE driver. It uses platform knowledge (abstracted into
> PciHostBridgeLib --> PciHostBridgeGetRootBridges()) to produce root
> bridge IO protocol instances, in its entry point (-->
> InitializePciHostBridge()).
> 
> PciHostBridgeDxe indeed does not have a depex on the IOMMU protocol. It
> registers a protocol notify instead.
> 
> The reason why PciHostBridgeDxe gets away with this *usually* is that
> the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL member functions that it exposes are
> *usually* only called from within BDS, after:
> 
> - platform BDS connects the root bridge protocol instances to the PCI
> Bus driver (which is a UEFI driver),
> 
> - the PCI Bus driver produces PciIo protocol instances on top of the
> root bridge IO instances,
> 
> - then various PCI device drivers start massaging the devices via PciIo
> protocol instances, ultimately boiling down to PciHostBridgeDxe()'s
> Map() function and friends.
> 
> However, the following driver dispatch order is also possible, entirely
> within DXE:
> 
> (a) a platform DXE driver is dispatched and registers a protocol notify
> for root bridge IO,
> 
> (b) PciHostBridgeDxe is dispatched and produces a number of root bridge
> IO protocol instances,
> 
> (c) the platform DXE driver gets called back and it uses the root bridge
> IO member functions (such as Map etc),
> 
> (d) The IOMMU DXE driver is dispatched and installs the IOMMU protocol,
> 
> (e) the PciHostBridgeDxe driver is called back, and its Map() etc
> functions will rely on the IOMMU *only* from this point forward.
> 
> This suggests that:
> 
> -  "gIoMmuDetectedProtocolGuid" should actually be called
> "gEdkiiIoMmuAbsentProtocolGuid", and should be upstreamed to MdeModulePkg,
> 
> - all DXE drivers (no exceptions) that *conditionally* depend on
> 

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Kinney, Michael D]

Laszlo,

I think the equivalent flag for GCC builds is --whole-archive.

I tried adding that flag to DLINK_FLAGS in GCC5, and I get the 
following error building OVMF from edk2/master with 
-D SOURCE_DEBUG_ENABLE set.

DxeLoad.obj (symbol from plugin): In function `InstallIplPermanentMemoryPpis':
(.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
collect2: error: ld returned 1 exit status

Visual Studio 2015 Update 2 has also added a new linker flag called
/WHOLEARCHIVE.  I am working on evaluating that flag to see if it 
catches the same issue.

Mike

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, May 25, 2017 9:09 AM
> To: Kinney, Michael D ; Ard Biesheuvel
> ; Andrew Fish (af...@apple.com) 
> Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff
> 
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix 
> duplicate
> symbol
> 
> On 05/25/17 03:47, Kinney, Michael D wrote:
> > Andrew,
> >
> > I think I have found an alternate fix for this XCODE5 specific
> > build failure.  Since there appears to be a difference in the
> > linker behavior between MSFT/GCC/XCODE tool chains, I reviewed
> > the 'ld' command line options used in XCODE5 tool chain in
> > tools_def.txt.
> >
> > There is a flag set call '-all_load'.  The description of this
> > flag is 'Loads all members of static archive libraries.'.
> >
> > I tried removing this flag from the XCODE5 specific SLINK_FLAGS
> > and DLINK_FLAGS statements in tools_def.txt, and the duplicate
> > symbol build failure is no longer present.  I am able to build
> > and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
> >
> > This seems to make XCODE5 linker behavior match the MSFT and GCC
> > linker behavior.
> >
> > Do you know why '-all_load' is used in XCODE5 and what impacts
> > there may be from removing it?
> 
> Please don't remove -all_load from there; instead we should figure out
> if the same can be brought to MSFT and GCC.
> 
> The error message that XCODE5 emitted caught a real bug (undefined
> behavior according to ISO C, see my previous email), and so we should
> keep that detection enabled (we should even extend it to other
> toolchains, if that's possible).
> 
> As for docs, I found this:
> 
> http://www.manpages.info/macosx/ld.1.html
> 
> > -all_load
> > Loads all members of static archive libraries. This option does
> > not apply to dynamic shared libraries.
> 
> 
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [staging/apei]: New branch request for APEI work

[Laszlo Ersek]

On 05/25/17 14:29, Achin Gupta wrote:
> Hi All,
> 
> I would like to create a branch for implementing support for APEI in EDK2. The
> intent is to upstream modules that are capable of creating the HEST, BERT, 
> ERST
> and EINJ after gathering error and error source information from the platform
> and other sources. The work is still very nascent and the branch will be first
> populated with some workarounds. These will be used as the basis for further
> development.
> 
> Could you please do the needful. The attached readme has been populated with
> some basic information.

(

Side remark:

For running the ArmVirtQemu edk2 platform on QEMU at least, the approach
being taken is to generate all APEI related stuff in QEMU, and to keep
the virtual firmware fully un-enlightened about APEI. This is consistent
with how QEMU and ArmVirtQemu handle other ACPI tables, through the QEMU
linker/loader interface.

References to sub-threads:

http://mid.mail-archive.com/5b7352f4-4965-3ed5-3879-db871797be47@huawei.com

http://mid.mail-archive.com/1493530506-26833-1-git-send-email-gengdongjiu@huawei.com

This is not to say that I "oppose" the branch -- that would make zero
sense, as physical platforms need to create their APEI-related tables in
their firmwares, like they create all other ACPI tables. I'd just like
to highlight that with QEMU (hence with ArmVirtQemu and OVMF), all
hardware-related ACPI tables are generated on the QEMU side, and that
the APEI tables should be no different. In fact this closely matches
Achin's virt-oriented design, visible in the first above-referenced thread.

)

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Andrew Fish]

> On May 25, 2017, at 9:08 AM, Laszlo Ersek  wrote:
> 
> On 05/25/17 03:47, Kinney, Michael D wrote:
>> Andrew,
>> 
>> I think I have found an alternate fix for this XCODE5 specific
>> build failure.  Since there appears to be a difference in the 
>> linker behavior between MSFT/GCC/XCODE tool chains, I reviewed 
>> the 'ld' command line options used in XCODE5 tool chain in
>> tools_def.txt.
>> 
>> There is a flag set call '-all_load'.  The description of this
>> flag is 'Loads all members of static archive libraries.'.
>> 
>> I tried removing this flag from the XCODE5 specific SLINK_FLAGS
>> and DLINK_FLAGS statements in tools_def.txt, and the duplicate
>> symbol build failure is no longer present.  I am able to build
>> and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
>> 
>> This seems to make XCODE5 linker behavior match the MSFT and GCC
>> linker behavior.
>> 
>> Do you know why '-all_load' is used in XCODE5 and what impacts
>> there may be from removing it?
> 
> Please don't remove -all_load from there; instead we should figure out
> if the same can be brought to MSFT and GCC.
> 

Not to mention I was told by the Xcode linker developer that -all_load was 
required to ensure in all cases the Mach-O produced would be compatible with 
conversion to PE/COFF. 

Thanks,

Andrew Fish

> The error message that XCODE5 emitted caught a real bug (undefined
> behavior according to ISO C, see my previous email), and so we should
> keep that detection enabled (we should even extend it to other
> toolchains, if that's possible).
> 
> As for docs, I found this:
> 
> http://www.manpages.info/macosx/ld.1.html 
> 
> 
>> -all_load
>>Loads all members of static archive libraries. This option does
>>not apply to dynamic shared libraries.
> 
> 
> Thanks
> Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Laszlo Ersek]

On 05/25/17 03:47, Kinney, Michael D wrote:
> Andrew,
> 
> I think I have found an alternate fix for this XCODE5 specific
> build failure.  Since there appears to be a difference in the 
> linker behavior between MSFT/GCC/XCODE tool chains, I reviewed 
> the 'ld' command line options used in XCODE5 tool chain in
> tools_def.txt.
> 
> There is a flag set call '-all_load'.  The description of this
> flag is 'Loads all members of static archive libraries.'.
> 
> I tried removing this flag from the XCODE5 specific SLINK_FLAGS
> and DLINK_FLAGS statements in tools_def.txt, and the duplicate
> symbol build failure is no longer present.  I am able to build
> and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
> 
> This seems to make XCODE5 linker behavior match the MSFT and GCC
> linker behavior.
> 
> Do you know why '-all_load' is used in XCODE5 and what impacts
> there may be from removing it?

Please don't remove -all_load from there; instead we should figure out
if the same can be brought to MSFT and GCC.

The error message that XCODE5 emitted caught a real bug (undefined
behavior according to ISO C, see my previous email), and so we should
keep that detection enabled (we should even extend it to other
toolchains, if that's possible).

As for docs, I found this:

http://www.manpages.info/macosx/ld.1.html

> -all_load
> Loads all members of static archive libraries. This option does
> not apply to dynamic shared libraries.


Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Laszlo Ersek]

On 05/24/17 22:18, Kinney, Michael D wrote:
> Laszlo,
>
> I agree with the request to add 'static' to the variable declaration
> in the SecPeiDebugAgentLib.  The variable name change will be retained
> because the same symbol name can still be confusing when debugging.
>
> The part that is more concerning is why this duplicate symbol
> reference was not triggered by MSFT or GCC tool chain families, so I
> did some more investigation from the MSFT side this morning.
>
> My first thought was that the optimizer in the compiler/linker was
> optimizing away the duplicate symbol before the final link stage,
> so I tried -b NOOPT builds.  That also did not trigger a duplicate
> symbol error and the MAP file contains only the single reference to
> _mMemoryDiscoveredNotifyList from the DxeIpl.
>
>   0002:01b8   _mMemoryDiscoveredNotifyList 000164f8 
> DxeIpl:DxeLoad.obj
>
> I then evaluated what functions in the DebugAgentLib the DxeIpl
> is using.  It turns out that it is only using a single function
> SaveAndSetDebugTimerInterrupt() that is implemented in
>
>   SourceLevelDebugPkg\Library\DebugAgent\DebugAgentCommon\DebugAgent.c
>
> The mMemoryDiscoveredNotifyList duplicate symbol is implemented in
>
>   
> SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c
>
> Even with a MSFT -b NOOPT build, when I look at the MAP file, the
> linker only pulls in symbols from the obj in the DebugAgentLib that
> contains the one SaveAndSetDebugTimerInterrupt() function.  The
> symbols from the other objs in the DebugAgentLib are not included in
> the final DxeIpl PE/COFF image.
>
>   0001:81b0   _InitializeDebugTimer  8410 f   
> SecPeiDebugAgentLib:DebugTimer.obj
>   0001:8330   _IsDebugTimerTimeout   8590 f   
> SecPeiDebugAgentLib:DebugTimer.obj
>   0001:83d0   _SaveAndSetDebugTimerInterrupt 8630 f   
> SecPeiDebugAgentLib:DebugTimer.obj
>
> So it appears that even with -b NOOPT builds, the linker is
> filtering out unreferenced objs which explains why the duplicate
> symbol error is not triggered.
>
> The XCODE5 tool chain appears to be evaluating symbols
> across all objs in a lib and detects a duplicate symbol.

Thank you for the thorough investigation. I agree that keeping the name
change *and* adding STATIC is the best combination.

> The question is what is the required linker behavior for EDK II
> builds?  Require library filtering at obj level, or require no
> duplicates across all objs in all libs?

Consider "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c":

> CONST EFI_PEI_NOTIFY_DESCRIPTOR mMemoryDiscoveredNotifyList = {
>   (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | 
> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
>   ,
>   InstallIplPermanentMemoryPpis
> };

(a) This variable has external linkage. From ISO C99, "6.2.2 Linkages of
identifiers":

> 5 [...] If the declaration of an identifier for an object has file
>   scope and no storage-class specifier, its linkage is external.

(b) This variable has static storage duration. From ISO C99, "6.2.4
Storage durations of objects":

> 3 An object whose identifier is declared with external or internal
>   linkage, or with the storage-class specifier static has static
>   storage duration. Its lifetime is the entire execution of the
>   program and its stored value is initialized only once, prior to
>   program startup.

(c) The quoted declaration is an external definition of
"mMemoryDiscoveredNotifyList". From ISO C99, "6.9.2 External object
definitions":

> 1 If the declaration of an identifier for an object has file scope and
>   an initializer, the declaration is an external definition for the
>   identifier.

The exact same statements can be made about
"SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c":

> GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR 
> mMemoryDiscoveredNotifyList[1] = {
>   {
> (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | 
> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> ,
> DebugAgentCallbackMemoryDiscoveredPpi
>   }
> };

(I see "GLOBAL_REMOVE_IF_UNREFERENCED", but with GCC, that expands to
nothing.)

So, we have two external definitions for "mMemoryDiscoveredNotifyList".
(It's especially funny because even their types don't match.)

This is what ISO C99, "6.9 External definitions" says:

> Syntax
>
> [...]
>
> Constraints
>
> [...]
>
> Semantics
>
> [...]
>
> 5 [...] If an identifier declared with external linkage is used in an
>   expression (other than as part of the operand of a sizeof operator
>   whose result is an integer constant), somewhere in the entire
>   program there shall be exactly one external definition for the
>   identifier [...]

Given that we use "mMemoryDiscoveredNotifyList" in two expressions,
namely:

* "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c":

> Status = PeiServicesNotifyPpi ();

* 
"SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c"

> Status = PeiServicesNotifyPpi ([0]);

Re: [edk2] Unmount and mount a mass storage from shell

[Carsey, Jaben]

You can "disconnect" the driver
You can do "map -d" to delete a mapping

I am unsure what your goals are for mount/unmount

-Jaben

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> GN Keshava
> Sent: Wednesday, May 24, 2017 10:59 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Unmount and mount a mass storage from shell
> Importance: High
> 
> How can I unmount and mount a mass storage fs from UEFI shell?
> 
> In older EFI shells, I can do with "mount" command. But it just does
> "mapping" in newer shell. It doesn't actually remounting the device.
> 
> Please help.
> 
> thanks
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v5 04/14] OvmfPkg/BaseMemcryptSevLib: Add SEV helper library

[Laszlo Ersek]

On 05/25/17 00:12, Brijesh Singh wrote:
>
> Hi Laszlo,
>
> On 05/24/2017 08:06 AM, Laszlo Ersek wrote:
>>
>> (2) please check the lines where you added (as I asked, thanks)
>> gEfiCallerBaseName and __FUNCTION__. On most lines, the indentation
>> is incorrect, relative to "DEBUG ((".
>
> Just so I get it right this time, can you please confirm that below
> indentation is correct:
>
> DEBUG ((DEBUG_VERBOSE, "%a:%a Set C-bit Cr3 %Lx Base %Lx Length %Lx
> flush %d\n",
>   gEfiCallerBaseName, __FUNCTION__, Cr3BaseAddress, PhysicalAddress,
> Length, Flush));
>
> I was trying to look into other files and some have different style,
> and checkpatch didn't complain the formatting error hence I thought
> what I had was correct.

The canonical way to write this DEBUG invocation is:

  DEBUG ((
DEBUG_VERBOSE,
"%a:%a Set C-bit Cr3 %Lx Base %Lx Length %Lx flush %d\n",
gEfiCallerBaseName,
__FUNCTION__,
Cr3BaseAddress,
PhysicalAddress,
Length,
Flush
));

(Do not miss the indentation of the closing paren(s)!)

Please refer to .

If it all fits on a single line, not exceeding 80 characters, then you
can keep it on a single line.

Otherwise, if you don't fit on a single line, then you have to break
every argument to a separate line. If your format string (or any other
argument) doesn't fit on a line in itself, then you have to break it up
too.

Earlier I'd been using a "meet in the middle" style, where I wouldn't
exceed 80 characters per line, and would indent the continuations by 2
additional spaces, but still wouldn't break each argument to a new line.
Example:

  DEBUG ((DEBUG_VERBOSE,
"%a:%a Set C-bit Cr3 %Lx Base %Lx Length %Lx flush %d\n",
gEfiCallerBaseName, __FUNCTION__, Cr3BaseAddress, PhysicalAddress, Length,
Flush));

In my opinion, this would be the best compromise, since (a) it keeps
lines under 80 chars width, (b) conforms to the indentation requirement,
(c) doesn't waste vertical space like the official layout above.

However, this style had not been approved, and I abandoned it in favor
of the canonical style, when I filed
.

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2 1/2] UefiCpuPkg: Add CPUID definitions for AMD.

[Leo Duran]

Cc: Jordan Justen 
Cc: Jeff Fan 
Cc: Liming Gao 
Cc: Brijesh Singh 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leo Duran 
---
 UefiCpuPkg/Include/Register/Amd/Cpuid.h | 265 ++--
 1 file changed, 256 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/Include/Register/Amd/Cpuid.h 
b/UefiCpuPkg/Include/Register/Amd/Cpuid.h
index 74ffb95..4a26bf7 100644
--- a/UefiCpuPkg/Include/Register/Amd/Cpuid.h
+++ b/UefiCpuPkg/Include/Register/Amd/Cpuid.h
@@ -7,6 +7,7 @@
   not provided for that register.
 
   Copyright (c) 2017, Advanced Micro Devices. All rights reserved.
+
   This program and the accompanying materials are licensed and made available
   under the terms and conditions of the BSD License which accompanies this
   distribution. The full text of the license may be found at
@@ -24,8 +25,253 @@
 #define __AMD_CPUID_H__
 
 /**
+  CPUID Extended Processor Signature and Feature Bits
 
-  Memory Encryption Information
+  @param   EAX  CPUID_EXTENDED_CPU_SIG (0x8001)
+
+  @retval  EAX  CPUID_EXTENDED_CPU_SIG.
+  @retval  EBX  Reserved.
+  @retval  ECX  Extended Processor Signature and Feature Bits information
+described by the type CPUID_AMD_EXTENDED_CPU_SIG_ECX.
+  @retval  EDX  Extended Processor Signature and Feature Bits information
+described by the type CPUID_EXTENDED_CPU_SIG_EDX.
+
+  Example usage
+  @code
+  UINT32  Eax;
+  CPUID_AMD_EXTENDED_CPU_SIG_ECX  Ecx;
+  CPUID_EXTENDED_CPU_SIG_EDX  Edx;
+
+  AsmCpuid (CPUID_EXTENDED_CPU_SIG, , NULL, , );
+  @endcode
+**/
+
+/**
+  CPUID AMD Extended Processor Signature and Feature Bits ECX for CPUID leaf
+  #CPUID_EXTENDED_CPU_SIG.
+**/
+typedef union {
+  ///
+  /// Individual bit fields
+  ///
+  struct {
+///
+/// [Bit 0] LAHF/SAHF available in 64-bit mode.
+///
+UINT32  LAHF_SAHF:1;
+///
+/// [Bit 1] Core multi-processing legacy mode.
+///
+UINT32  CmpLegacy:1;
+///
+/// [Bit 2] Secure Virtual Mode feature.
+///
+UINT32  SVM:1;
+///
+/// [Bit 3] Extended APIC register space.
+///
+UINT32  ExtApicSpace:1;
+///
+/// [Bit 4] LOCK MOV CR0 means MOV CR8.
+///
+UINT32  AltMovCr8:1;
+///
+/// [Bit 5] LZCNT instruction support.
+///
+UINT32  LZCNT:1;
+///
+/// [Bit 6] SSE4A instruction support.
+///
+UINT32  SSE4A:1;
+///
+/// [Bit 7] Misaligned SSE Mode.
+///
+UINT32  MisAlignSse:1;
+///
+/// [Bit 8] ThreeDNow Prefetch instructions.
+///
+UINT32  PREFETCHW:1;
+///
+/// [Bit 9] OS Visible Work-around support.
+///
+UINT32  OSVW:1;
+///
+/// [Bit 10] Instruction Based Sampling.
+///
+UINT32  IBS:1;
+///
+/// [Bit 11] Extened Operation Support.
+///
+UINT32  XOP:1;
+///
+/// [Bit 12] SKINIT and STGI support.
+///
+UINT32  SKINIT:1;
+///
+/// [Bit 13] Watchdog Timer support.
+///
+UINT32  WDT:1;
+///
+/// [Bit 14] Reserved.
+///
+UINT32  Reserved1:1;
+///
+/// [Bit 15] Lightweight Profiling support.
+///
+UINT32  LWP:1;
+///
+/// [Bit 16] 4-Operand FMA instruction support.
+///
+UINT32  FMA4:1;
+///
+/// [Bit 17] Translation Cache Extension.
+///
+UINT32  TCE:1;
+///
+/// [Bit 21:18] Reserved.
+///
+UINT32  Reserved2:4;
+///
+/// [Bit 22] Topology Extensions support.
+///
+UINT32  TopologyExtensions:1;
+///
+/// [Bit 23] Core Performance Counter Extensions.
+///
+UINT32  PerfCtrExtCore:1;
+///
+/// [Bit 25:24] Reserved.
+///
+UINT32  Reserved3:2;
+///
+/// [Bit 26] Data Breakpoint Extension.
+///
+UINT32  DataBreakpointExtension:1;
+///
+/// [Bit 27] Performance Time-Stamp Counter.
+///
+UINT32  PerfTsc:1;
+///
+/// [Bit 28] L3 Performance Counter Extensions.
+///
+UINT32  PerfCtrExtL3:1;
+///
+/// [Bit 29] MWAITX and MONITORX capability.
+///
+UINT32  MwaitExtended:1;
+///
+/// [Bit 31:30] Reserved.
+///
+UINT32  Reserved4:2;
+  } Bits;
+  ///
+  /// All bit fields as a 32-bit value
+  ///
+  UINT32  Uint32;
+} CPUID_AMD_EXTENDED_CPU_SIG_ECX;
+
+
+/**
+  CPUID AMD Processor Topology
+
+  @param   EAX  CPUID_AMD_PROCESSOR_TOPOLOGY (0x801E)
+
+  @retval  EAX  Extended APIC ID.
+  @retval  EBX  Core Indentifiers.
+  @retval  ECX  Node Indentifiers.
+  @retval  EDX  Reserved.
+
+  Example usage
+  @code
+  UINT32 Eax;
+  UINT32 Ebx;
+  UINT32 Ecx;
+  UINT32 Edx;
+
+  AsmCpuid (CPUID_AMD_PROCESSOR_TOPOLOGY, , , , );
+  @endcode
+**/
+#define CPUID_AMD_PROCESSOR_TOPOLOGY 0x801E
+
+/**
+  CPUID AMD Processor Topology support information EAX for CPUID leaf
+  #CPUID_AMD_PROCESSOR_TOPOLOGY.
+**/

[edk2] [PATCH v2 0/2] UefiCpuPkg: Add CPUID support for AMD.

[Leo Duran]

This patch-set requires and builds upon this submission:
https://lists.01.org/pipermail/edk2-devel/2017-May/010867.html

Changes since v1:
- Revert to (MaxCoresPerPackage = 1) when CPUID is not explicit.

Leo Duran (2):
  UefiCpuPkg: Add CPUID definitions for AMD.
  UefiCpuPkg: Modify GetProcessorLocationByApicId() to support AMD.

 UefiCpuPkg/Include/Register/Amd/Cpuid.h| 265 -
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c |  85 ---
 .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c|  85 ---
 3 files changed, 354 insertions(+), 81 deletions(-)
 mode change 100644 => 100755 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c

-- 
2.7.4

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [staging/apei]: New branch request for APEI work

[Achin Gupta]

Hi All,

I would like to create a branch for implementing support for APEI in EDK2. The
intent is to upstream modules that are capable of creating the HEST, BERT, ERST
and EINJ after gathering error and error source information from the platform
and other sources. The work is still very nascent and the branch will be first
populated with some workarounds. These will be used as the basis for further
development.

Could you please do the needful. The attached readme has been populated with
some basic information.

cheers,
Achin
This branch will be used to develop support for ACPI Platform Error Interfaces
(APEI) on ARMv8-A standard platforms.

The branch owners: Achin Gupta , James Morse 


# Feature Introduction
The ACPI specification describes APEI which provide a mechanism for the firmware
to convey error information to OSPM. This branch will be used to implement
support for these interfaces in EDK2 and test them on ARM platforms.

# Aims
Upstream support for APEI to EDK2

# Related Modules
* MdeModulePkg
* ArmPkg
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Pressing ESC from "PXE windows Boot manager" causes ASSERT

[Karunakar P]

Hi Jiaxin,

Please find below details regards Server Configuration

I have configured DHCP and WDS server on same machine (Windows Server 2012 R2)
Server IP: 192.168.0.1

1. Configured Active Directory Domain Services, DHCP , WDS services
2. DHCPv4 Configuration
Address Pool :- 192.168.0.2 - 192.168.0.50
Scope Options:-
Option Name Value
-   
006 DNS server  192.168.0.1
060 PXE Client  PXEClient

3. WDS Configuration DHCP Properties
Check both of the following check boxes
1. Do not listen on DHCP ports
2. Configure DHCP options to indicate that this is also a PXE Server
 In WDS Server, added boot.wim and install.wim images of same server.
As you know, Once we configure WDS and added boot.wim and install.wim images to 
WDS server, bootmgfw.efi and wdsmgfw.efi files will be added to 
C:\RemoteInstall\Boot\x64 


I also tried with PXE server configured in Windows Server 2008 R2. ASSERT 
happens at the same place

Following are the debug messages
// DEBUG message printed before StartImage()
// StartImage() called
wdsmgfw.Entry(10001000)
Press ENTER for network boot service. ConvertPages: Incompatible memory types
bootmgfw.Entry(8510C000)
Windows Boot Manager (Server IP: 192.168.000.001)
Choose an operating system to start: (Use the arrow keys to highlight your 
choice, then press ENTER.)
WinServer2008_boot.wim
To specify an advanced option for this choice, press F8.
ENTER=Choose ESC=Exit
WinServer2008_boot.wim>
To specify an advanced option for this choice, press F8.
ASSERT d:\ProjectPath\MdeModulePkg\Core\Dxe\Mem\Pool.c(561): CR has Bad 
Signature

Yeah you are correct with above debug messages, It seems that issue is 
happening inside bootmgfw

Thanks,
karunakar

-Original Message-
From: Wu, Jiaxin [mailto:jiaxin...@intel.com] 
Sent: Thursday, May 25, 2017 4:07 PM
To: Karunakar P; af...@apple.com; edk2-devel@lists.01.org
Cc: Ye, Ting; Fu, Siyuan
Subject: RE: [edk2] Pressing ESC from "PXE windows Boot manager" causes ASSERT

Karunakar,

Can you share us the detailed DHCP and WDS configuration? E.g. DHCP options / 
WDS Properties (DHCP).  As you know, the different DHCP/WDS configuration may 
lead to the different PXE download process/behavior. We would like to reproduce 
the issue first.

>From your below debug messages, the issue seems to be triggered by 
>bootmgfw.Entry.

Thanks,
Jiaxin

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Karunakar P
> Sent: Thursday, May 25, 2017 2:38 PM
> To: af...@apple.com; edk2-devel@lists.01.org
> Cc: Ye, Ting 
> Subject: Re: [edk2] Pressing ESC from "PXE windows Boot manager" 
> causes ASSERT
> 
> Hello All,
> 
> I've added some traces to narrow down the issue.
> 
> Once the NBP file downloaded, control will be given to it.
> I've added some traces before StartImage() and after StartImage() call.
> 
> Following are the debug messages
> 
> // DEBUG message printed before StartImage() // StartImage() called
> wdsmgfw.Entry(100061C0)
> WDS Boot Manager version 0800
> Client IP: 192.168.0.6
> Server IP: 192.168.0.1
> Server Name: WIN-8PL637590SS
> Press ENTER for network boot service.
> Windows Deployment Services (Server IP: 192.168.0.1) Contacting Server 
> (192.168.0.1):
> ESC=Exit -ConvertPages: Incompatible memory types
> bootmgfw.Entry(849FE1C0)
> ASSERT d:\PathtoProject\MdeModulePkg\Core\Dxe\Mem\Pool.c(561): CR has 
> Bad Signature
> 
> It might be Boot Loader Issue, As control is NOT coming back.
> 
> Could you please help on this.
> 
> Thanks,
> karunakar
> 
> -Original Message-
> From: af...@apple.com [mailto:af...@apple.com]
> Sent: Wednesday, May 24, 2017 8:39 PM
> To: Karunakar P
> Cc: Ye, Ting; edk2-devel@lists.01.org
> Subject: Re: [edk2] Pressing ESC from "PXE windows Boot manager" 
> causes ASSERT
> 
> Karunakar,
> 
> Every Pool allocation has a header and a tail data structure that is 
> outside of the user visible data. Both these structures have 
> signatures. The ASSERT you are seeing is a bad signature in the header so 
> that looks like a buffer underflow.
> It could also be a use after free bug.
> 
> Head:
> POOL_HEAD
> Buffer:
> User Data
> Tail:
> POOL_TAIL
> 
> Given the checks only happen on Free it is possible it could be a 
> buffer overflow on a buffer that has not yet been freed that runs into this 
> buffer.
> 
> If you have a debugger dumping the memory before and after the buffer 
> can some times be useful. The pattern might give you some clues.
> 
> Thanks,
> 
> Andrew Fish
> 
> > On May 23, 2017, at 10:16 PM, Karunakar P 
> > 
> wrote:
> >
> > Hello All,
> >
> > The ASSERT happens in the following function
> >
> > /**
> >  Internal function to free a pool entry.
> >  Caller must have the memory lock held
> >
> >  @param  

Re: [edk2] Pressing ESC from "PXE windows Boot manager" causes ASSERT

[Karunakar P]

Hello All,

I've added some traces to narrow down the issue.

Once the NBP file downloaded, control will be given to it.
I've added some traces before StartImage() and after StartImage() call.

Following are the debug messages

// DEBUG message printed before StartImage()
// StartImage() called
wdsmgfw.Entry(100061C0)
WDS Boot Manager version 0800
Client IP: 192.168.0.6
Server IP: 192.168.0.1
Server Name: WIN-8PL637590SS
Press ENTER for network boot service.
Windows Deployment Services (Server IP: 192.168.0.1)
Contacting Server (192.168.0.1):
ESC=Exit -ConvertPages: Incompatible memory types
bootmgfw.Entry(849FE1C0)
ASSERT d:\PathtoProject\MdeModulePkg\Core\Dxe\Mem\Pool.c(561): CR has Bad 
Signature

It might be Boot Loader Issue, As control is NOT coming back.

Could you please help on this.

Thanks,
karunakar

-Original Message-
From: af...@apple.com [mailto:af...@apple.com] 
Sent: Wednesday, May 24, 2017 8:39 PM
To: Karunakar P
Cc: Ye, Ting; edk2-devel@lists.01.org
Subject: Re: [edk2] Pressing ESC from "PXE windows Boot manager" causes ASSERT

Karunakar,

Every Pool allocation has a header and a tail data structure that is outside of 
the user visible data. Both these structures have signatures. The ASSERT you 
are seeing is a bad signature in the header so that looks like a buffer 
underflow. It could also be a use after free bug.

Head:
POOL_HEAD
Buffer:
User Data
Tail:
POOL_TAIL

Given the checks only happen on Free it is possible it could be a buffer 
overflow on a buffer that has not yet been freed that runs into this buffer. 

If you have a debugger dumping the memory before and after the buffer can some 
times be useful. The pattern might give you some clues. 

Thanks,

Andrew Fish

> On May 23, 2017, at 10:16 PM, Karunakar P  wrote:
> 
> Hello All,
> 
> The ASSERT happens in the following function
> 
> /**
>  Internal function to free a pool entry.
>  Caller must have the memory lock held
> 
>  @param  Buffer The allocated pool entry to free
>  @param  PoolType   Pointer to pool type
> 
>  @retval EFI_INVALID_PARAMETER  Buffer not valid
>  @retval EFI_SUCCESSBuffer successfully freed.
> 
> **/
> EFI_STATUS
> CoreFreePoolI (
>  IN VOID   *Buffer,
>  OUT EFI_MEMORY_TYPE   *PoolType OPTIONAL
>  )
> {
> .
> .
> ASSERT(Buffer != NULL);
>  //
>  // Get the head & tail of the pool entry  //
>  Head = CR (Buffer, POOL_HEAD, Data, POOL_HEAD_SIGNATURE);// ASSERT 
> happens here
>  ASSERT(Head != NULL);
> .
> .
> }
> 
> We are using NetworkPkg: SHA- ef810bc807188224a752ffbcf5e7f4b651291cee
> 
> I think  here I'm unable attach the files.
> You can find the attached screenshots in the following Bug571
> https://bugzilla.tianocore.org/show_bug.cgi?id=571
> 
> Thanks,
> Karunakar
> 
> 
> -Original Message-
> From: Ye, Ting [mailto:ting...@intel.com]
> Sent: Wednesday, May 24, 2017 10:29 AM
> To: Karunakar P; edk2-devel@lists.01.org
> Subject: RE: Pressing ESC from "PXE windows Boot manager" causes 
> ASSERT
> 
> Hi Karunakar,
> 
> Sorry I did not find your attached files. Would you please send them again? 
> Besides that, do you mind telling us which code base are you using for PXE 
> boot?  Are you using some revision of EDKII main trunk or UDK release?
> 
> Thanks,
> Ting
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Karunakar P
> Sent: Wednesday, May 24, 2017 12:20 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Pressing ESC from "PXE windows Boot manager" causes 
> ASSERT
> 
> Hi All,
> 
> We have facing an issue with PXE boot.
> [Issue]
> When ESC is pressed from Windows Boot manager during PXE boot (IPv4 or 
> IPv6) system Hangs with following ASSERT
> 
> ASSERT [DxeCore] \MdeModulePkg\Core\Dxe\Mem\Pool.c : CR has Bad 
> Signature
> 
> [Reproduction Steps]
> 1. Perform UEFI PXEv4 or UEFI PXEv6 boot 2. It will start PXE boot over 
> IPv4/6 and Downloads NBP file successfully.
>   Attached the Screenshot for the same(ScreenShot1.jpg)
> 
>   It will Displays the info like "Press ENTER for network boot service"
>   Attached Screensho(ScreenShot2.jpg)
> 
> 3. Press ENTER and then press ESC immediately to see the Windows Boot Manager 
> Menu
>   It will list the available Operating Systems
>   Attached the screenshot(ScreenShot3.png)
> 
> 4. Press ESC to come back to Setup or next Boot option
> 
> [Result]
> System hangs with ASSERT
> 
> [Expected Result]
> On pressing ESC from Windows Boot Manager, it should come back to 
> setup/Next boot option in boot order
> 
> Note:
> We have PXE server configured in Windows Server 2012 R2.
> 
> Please look into it.
> 
> 
> Thanks,
> karunakar
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> 

[edk2] Unmount and mount a mass storage from shell

[GN Keshava]

How can I unmount and mount a mass storage fs from UEFI shell?

In older EFI shells, I can do with "mount" command. But it just does
"mapping" in newer shell. It doesn't actually remounting the device.

Please help.

thanks
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel