Re: [edk2] [PATCH 0/2] Capsule: Fix typo 'Press' to 'Process'
Series reviewed-by: jiewen@intel.com > -Original Message- > From: Zeng, Star > Sent: Tuesday, July 10, 2018 6:23 PM > To: edk2-devel@lists.01.org > Cc: Zeng, Star ; Kinney, Michael D > ; Yao, Jiewen ; Shao, > Ming > Subject: [PATCH 0/2] Capsule: Fix typo 'Press' to 'Process' > > Cc: Michael D Kinney > Cc: Jiewen Yao > Cc: Ming Shao > > Star Zeng (2): > MdeModulePkg DxeCapsuleLibFmp: Fix typo 'Press' to 'Process' > SignedCapsulePkg RecoveryModuleLoadPei: Fix typo 'Press' to 'Process' > > MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c > | 2 +- > .../Universal/RecoveryModuleLoadPei/RecoveryModuleLoadPei.c | > 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > -- > 2.7.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 06/37] CorebootModulePkg: Removing ipf from edk2.
Reviewed-by: Benjamin You > -Original Message- > From: Chen, Chen A > Sent: Wednesday, June 13, 2018 11:43 AM > To: edk2-devel@lists.01.org > Cc: Chen, Chen A ; Ma, Maurice > ; Agyeman, Prince ; You, > Benjamin ; Kinney, Michael D > > Subject: [PATCH 06/37] CorebootModulePkg: Removing ipf from edk2. > > Removing rules for Ipf sources file: > * Remove the source file which path with "ipf" and also listed in > [Sources.IPF] section of INF file. > * Remove the source file which listed in [Components.IPF] section > of DSC file and not listed in any other [Components] section. > * Remove the embedded Ipf code for MDE_CPU_IPF. > > Removing rules for Inf file: > * Remove IPF from VALID_ARCHITECTURES comments. > * Remove DXE_SAL_DRIVER from LIBRARY_CLASS in [Defines] section. > * Remove the INF which only listed in [Components.IPF] section in DSC. > * Remove statements from [BuildOptions] that provide IPF specific flags. > * Remove any IPF sepcific sections. > > Removing rules for Dec file: > * Remove [Includes.IPF] section from Dec. > > Removing rules for Dsc file: > * Remove IPF from SUPPORTED_ARCHITECTURES in [Defines] section of DSC. > * Remove any IPF specific sections. > * Remove statements from [BuildOptions] that provide IPF specific flags. > > Cc: Maurice Ma > Cc: Prince Agyeman > Cc: Benjamin You > Cc: Michael D Kinney > Signed-off-by: chenc2 > Contributed-under: TianoCore Contribution Agreement 1.1 > --- > CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf | 2 +- > CorebootModulePkg/SataControllerDxe/SataControllerDxe.inf | 2 +- > CorebootModulePkg/SecCore/SecCore.inf | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf > b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf > index 15b0dac774..63f6ab35db 100644 > --- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf > +++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf > @@ -25,7 +25,7 @@ > # > # The following information is for reference only and not required by the > build > tools. > # > -# VALID_ARCHITECTURES = IA32 X64 IPF EBC > +# VALID_ARCHITECTURES = IA32 X64 EBC > # > > [Sources] > diff --git a/CorebootModulePkg/SataControllerDxe/SataControllerDxe.inf > b/CorebootModulePkg/SataControllerDxe/SataControllerDxe.inf > index b0ed2f4db5..b40f0d2668 100644 > --- a/CorebootModulePkg/SataControllerDxe/SataControllerDxe.inf > +++ b/CorebootModulePkg/SataControllerDxe/SataControllerDxe.inf > @@ -24,7 +24,7 @@ > # > # The following information is for reference only and not required by the > build > tools. > # > -# VALID_ARCHITECTURES = IA32 X64 IPF EBC > +# VALID_ARCHITECTURES = IA32 X64 EBC > # > > [Sources] > diff --git a/CorebootModulePkg/SecCore/SecCore.inf > b/CorebootModulePkg/SecCore/SecCore.inf > index 3eb8aa029b..431f235665 100644 > --- a/CorebootModulePkg/SecCore/SecCore.inf > +++ b/CorebootModulePkg/SecCore/SecCore.inf > @@ -24,7 +24,7 @@ > # > # The following information is for reference only and not required by the > build > tools. > # > -# VALID_ARCHITECTURES = IA32 X64 IPF EBC > +# VALID_ARCHITECTURES = IA32 X64 EBC > # > > [Sources] > -- > 2.16.2.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] Platform/ARM: Correct LevelID in PLPI packages of DSDT
On 10 July 2018 at 13:55, Evan Lloyd wrote:
> Reviewed-by: Evan Lloyd
>
>> -Original Message-
>> From: AlexeiFedorov
>> Sent: 04 July 2018 14:05
>> To: edk2-devel@lists.01.org
>> Cc: Arvind Chauhan ; Thomas Abraham
>> ; ard.biesheu...@linaro.org;
>> leif.lindh...@linaro.org; Matteo Carlini ;
>> Stephanie Hughes-Fitt ; nd
>> ; Thomas Abraham ; Evan
>> Lloyd ; Sami Mujawar
>> Subject: [PATCH] Platform/ARM: Correct LevelID in PLPI packages of DSDT
>>
>> From: Alexei Fedorov
>>
>> Juno's DSDT contains 2 PLPI packages in Clusters #0 and #1 and _OSC method
>> reports support for platform coordinated mode only.
>> According to the description of LevelID field in ACPI 6.2 Errata A
>> Specification
>> #8.4.4.3, "In a platform that only supports platform coordinated mode, this
>> field must be 0."
>>
>> This patch fixes the above issue by changing value of LevelID fields from 1
>> to
>> 0.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Alexei Fedorov
Thanks all
Pushed as 5ba6b64686bf
>> ---
>> All the changes can be reviewed at:
>> https://github.com/AlexeiFedorov/edk2-
>> platforms/tree/282_correct_levelid_v1
>>
>> Notes:
>> v1:
>> - Change LevelID Value of PLPI package from 1 to 0.
>>
>> Platform/ARM/JunoPkg/AcpiTables/Dsdt.asl | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/Platform/ARM/JunoPkg/AcpiTables/Dsdt.asl
>> b/Platform/ARM/JunoPkg/AcpiTables/Dsdt.asl
>> index
>> 07e32bae21f891461fde0183028e4c0f817e45a7..702b057757457fee40ddfc10
>> e91d38c5dd7ca0b8 100644
>> --- a/Platform/ARM/JunoPkg/AcpiTables/Dsdt.asl
>> +++ b/Platform/ARM/JunoPkg/AcpiTables/Dsdt.asl
>> @@ -1,7 +1,7 @@
>> /** @file
>>Differentiated System Description Table Fields (DSDT)
>>
>> - Copyright (c) 2014-2015, ARM Ltd. All rights reserved.
>> + Copyright (c) 2014-2018, ARM Ltd. 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 @@ -65,7 +65,7 @@ DefinitionBlock("DsdtTable.aml", "DSDT", 1,
>> "ARMLTD", "ARM-JUNO", EFI_ACPI_ARM_O
>>})
>>Name(PLPI, Package() {
>> 0, // Version
>> -1, // Level Index
>> +0, // Level Index
>> 2, // Count
>> Package() { // WFI for CPU
>>1, // Min residency (uS)
>> @@ -157,7 +157,7 @@ DefinitionBlock("DsdtTable.aml", "DSDT", 1,
>> "ARMLTD", "ARM-JUNO", EFI_ACPI_ARM_O
>>})
>>Name(PLPI, Package() {
>> 0, // Version
>> -1, // Level Index
>> +0, // Level Index
>> 2, // Count
>> Package() { // WFI for CPU
>>1, // Min residency (uS)
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 07/37] CorebootPayloadPkg: Removing ipf from edk2.
Reviewed-by: Benjamin You > -Original Message- > From: Chen, Chen A > Sent: Wednesday, June 13, 2018 11:44 AM > To: edk2-devel@lists.01.org > Cc: Chen, Chen A ; Ma, Maurice > ; Agyeman, Prince ; You, > Benjamin ; Kinney, Michael D > > Subject: [PATCH 07/37] CorebootPayloadPkg: Removing ipf from edk2. > > Removing rules for Ipf sources file: > * Remove the source file which path with "ipf" and also listed in > [Sources.IPF] section of INF file. > * Remove the source file which listed in [Components.IPF] section > of DSC file and not listed in any other [Components] section. > * Remove the embedded Ipf code for MDE_CPU_IPF. > > Removing rules for Inf file: > * Remove IPF from VALID_ARCHITECTURES comments. > * Remove DXE_SAL_DRIVER from LIBRARY_CLASS in [Defines] section. > * Remove the INF which only listed in [Components.IPF] section in DSC. > * Remove statements from [BuildOptions] that provide IPF specific flags. > * Remove any IPF sepcific sections. > > Removing rules for Dec file: > * Remove [Includes.IPF] section from Dec. > > Removing rules for Dsc file: > * Remove IPF from SUPPORTED_ARCHITECTURES in [Defines] section of DSC. > * Remove any IPF specific sections. > * Remove statements from [BuildOptions] that provide IPF specific flags. > > Cc: Maurice Ma > Cc: Prince Agyeman > Cc: Benjamin You > Cc: Michael D Kinney > Signed-off-by: chenc2 > Contributed-under: TianoCore Contribution Agreement 1.1 > --- > CorebootPayloadPkg/FbGop/FbGop.inf | 2 +- > CorebootPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf | 4 ++-- > CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 2 +- > CorebootPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf | 2 +- > 4 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/CorebootPayloadPkg/FbGop/FbGop.inf > b/CorebootPayloadPkg/FbGop/FbGop.inf > index 0d97387679..b95add240b 100644 > --- a/CorebootPayloadPkg/FbGop/FbGop.inf > +++ b/CorebootPayloadPkg/FbGop/FbGop.inf > @@ -29,7 +29,7 @@ > # > # The following information is for reference only and not required by the > build > tools. > # > -# VALID_ARCHITECTURES = IA32 X64 IPF EBC > +# VALID_ARCHITECTURES = IA32 X64 EBC > # > # DRIVER_BINDING= gBiosVideoDriverBinding > # COMPONENT_NAME= gBiosVideoComponentName > diff --git a/CorebootPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf > b/CorebootPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf > index 2bff61094a..4c5684cb09 100644 > --- a/CorebootPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf > +++ b/CorebootPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf > @@ -25,7 +25,7 @@ > # > # The following information is for reference only and not required by the > build > tools. > # > -# VALID_ARCHITECTURES = IA32 X64 IPF EBC > +# VALID_ARCHITECTURES = IA32 X64 EBC > # > > [Sources] > @@ -43,4 +43,4 @@ >DebugLib > > [Guids] > - gUefiAcpiBoardInfoGuid > \ No newline at end of file > + gUefiAcpiBoardInfoGuid > diff --git a/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > b/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > index 15ffb45f84..7fc1326a70 100644 > --- a/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > +++ b/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > @@ -27,7 +27,7 @@ > # The following information is for reference only and not required by the > build > # tools. > # > -# VALID_ARCHITECTURES = IA32 X64 IPF EBC > +# VALID_ARCHITECTURES = IA32 X64 EBC > # > > [Sources] > diff --git a/CorebootPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf > b/CorebootPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf > index 1af96c6e23..9e025ecee2 100644 > --- a/CorebootPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf > +++ b/CorebootPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf > @@ -23,7 +23,7 @@ > # > # The following information is for reference only and not required by the > build > tools. > # > -# VALID_ARCHITECTURES = IA32 X64 IPF > +# VALID_ARCHITECTURES = IA32 X64 > # > > [Sources] > -- > 2.16.2.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/3] CapsuleApp: Some enhancements
Series reviewed-by: jiewen@intel.com > -Original Message- > From: Zeng, Star > Sent: Wednesday, July 11, 2018 11:51 AM > To: edk2-devel@lists.01.org > Cc: Zeng, Star ; Kinney, Michael D > ; Yao, Jiewen ; Zhu, > Yonghong > Subject: [PATCH 0/3] CapsuleApp: Some enhancements > > Cc: Michael D Kinney > Cc: Jiewen Yao > Cc: Yonghong Zhu > > Star Zeng (3): > MdeModulePkg CapsuleApp: Check Arg count for -D option > MdeModulePkg CapsuleApp: Refine -D option help information > MdeModulePkg CapsuleApp: Fix typo EFI_CAPSULE_RPORT_GUID > > MdeModulePkg/Application/CapsuleApp/CapsuleApp.c | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > -- > 2.7.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] question about armclang support
(+ Leif) On 10 July 2018 at 15:14, Laszlo Ersek wrote: > Hello Sau Kae, > > On 07/10/18 11:14, Tan, Sau Kae wrote: >> Hi All, >> >> May I know is armclang already supported in EDK2? >> Or only armcc is supported currently? >> Thanks. > > Looking at "BaseTools/Conf/tools_def.template", the following toolchains > appear supported for both ARM and AARCH64: > - CLANG35 > - CLANG38 > > They are documented (in the same file) as: > > # CLANG35 -Linux,Windows- Requires: > # Clang v3.5 or later, and GNU binutils targeting > aarch64-linux-gnu or arm-linux-gnueabi > #Optional: > # Required to build platforms or ACPI tables: > # Intel(r) ACPI Compiler from > # https://acpica.org/downloads > # CLANG38 -Linux- Requires: > # Clang v3.8, LLVMgold plugin and GNU binutils > 2.26 targeting x86_64-linux-gnu, aarch64-linux-gnu or arm-linux-gnueabi > # Clang v3.9 or later, LLVMgold plugin and GNU > binutils 2.28 targeting x86_64-linux-gnu, aarch64-linux-gnu or > arm-linux-gnueabi > #Optional: > # Required to build platforms or ACPI tables: > # Intel(r) ACPI Compiler from > # https://acpica.org/downloads > I suppose ARMCLANG is the commercial ARM compiler based on LLVM/CLANG? In that case, could you please share your experiences with these toolchain profiles? CLANG3x support was added primarily to ensure the code base is supported by ARMCLANG but I don't know whether anyone has actually tried building it like that. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch 2/3] UefiCpuPkg/MpInitLib: Use BSP uCode for APs if possible.
Search uCode costs much time, if AP has same processor type
with BSP, AP can use BSP saved uCode info to get better performance.
This change enables this solution.
Cc: Laszlo Ersek
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
---
UefiCpuPkg/Library/MpInitLib/Microcode.c | 34 +---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 4 ++--
UefiCpuPkg/Library/MpInitLib/MpLib.h | 11 +--
3 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index e47f9f4f8f..351975e2a2 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -35,11 +35,13 @@ GetCurrentMicrocodeSignature (
/**
Detect whether specified processor can find matching microcode patch and
load it.
- @param[in] CpuMpData The pointer to CPU MP Data structure.
+ @param[in] CpuMpDataThe pointer to CPU MP Data structure.
+ @param[in] IsBspCallIn Indicate whether the caller is BSP or not.
**/
VOID
MicrocodeDetect (
- IN CPU_MP_DATA *CpuMpData
+ IN CPU_MP_DATA *CpuMpData,
+ IN BOOLEAN IsBspCallIn
)
{
UINT32 ExtendedTableLength;
@@ -58,6 +60,7 @@ MicrocodeDetect (
BOOLEAN CorrectMicrocode;
VOID*MicrocodeData;
MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
+ UINT32 ProcessorFlags;
if (CpuMpData->MicrocodePatchRegionSize == 0) {
//
@@ -67,7 +70,7 @@ MicrocodeDetect (
}
CurrentRevision = GetCurrentMicrocodeSignature ();
- if (CurrentRevision != 0) {
+ if (CurrentRevision != 0 && !IsBspCallIn) {
//
// Skip loading microcode if it has been loaded successfully
//
@@ -87,6 +90,19 @@ MicrocodeDetect (
PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
+ //
+ // Check whether AP has same processor with BSP.
+ // If yes, direct use microcode info saved by BSP.
+ //
+ if (!IsBspCallIn) {
+if ((CpuMpData->ProcessorSignature == Eax.Uint32) &&
+(CpuMpData->ProcessorFlags & (1 << PlatformId)) != 0) {
+MicrocodeData = (VOID *)(UINTN) CpuMpData->MicrocodeDataAddress;
+LatestRevision = CpuMpData->MicrocodeRevision;
+goto Done;
+}
+ }
+
LatestRevision = 0;
MicrocodeData = NULL;
MicrocodeEnd = (UINTN) (CpuMpData->MicrocodePatchAddress +
CpuMpData->MicrocodePatchRegionSize);
@@ -117,6 +133,7 @@ MicrocodeDetect (
}
if (CheckSum32 == 0) {
CorrectMicrocode = TRUE;
+ ProcessorFlags = MicrocodeEntryPoint->ProcessorFlags;
}
} else if ((MicrocodeEntryPoint->DataSize != 0) &&
(MicrocodeEntryPoint->UpdateRevision > LatestRevision)) {
@@ -151,6 +168,7 @@ MicrocodeDetect (
// Find one
//
CorrectMicrocode = TRUE;
+ProcessorFlags = ExtendedTable->ProcessorFlag;
break;
}
}
@@ -188,6 +206,7 @@ MicrocodeDetect (
MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
MicrocodeEntryPoint) + TotalSize);
} while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
+Done:
if (LatestRevision > CurrentRevision) {
//
// BIOS only authenticate updates that contain a numerically larger
revision
@@ -211,4 +230,13 @@ MicrocodeDetect (
ReleaseSpinLock(>MpLock);
}
}
+
+ if (IsBspCallIn && (LatestRevision != 0)) {
+CpuMpData->ProcessorSignature = Eax.Uint32;
+CpuMpData->ProcessorFlags = ProcessorFlags;
+CpuMpData->MicrocodeDataAddress = (UINTN) MicrocodeData;
+CpuMpData->MicrocodeRevision = LatestRevision;
+DEBUG ((DEBUG_INFO, "BSP Microcode:: signature [0x%08x], ProcessorFlags
[0x%08x], \
+ MicroData [0x%08x], Revision [0x%08x]\n", Eax.Uint32, ProcessorFlags,
(UINTN) MicrocodeData, LatestRevision));
+ }
}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 8b458a4a3a..9179f9ae6d 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -410,7 +410,7 @@ ApInitializeSync (
//
// Load microcode on AP
//
- MicrocodeDetect (CpuMpData);
+ MicrocodeDetect (CpuMpData, FALSE);
//
// Sync BSP's MTRR table to AP
//
@@ -1601,7 +1601,7 @@ MpInitLibInitialize (
//
// Load Microcode on BSP
//
- MicrocodeDetect (CpuMpData);
+ MicrocodeDetect (CpuMpData, TRUE);
//
// Store BSP's MTRR setting
//
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 73e689d969..d897497b77 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -246,6 +246,11 @@
[edk2] [Patch 0/3] Optimize load uCode performance
Use below three rules to optimize load uCode performance: 1. Let BSP relocate uCode from flash to memory for better performance. 2. BSP caches the CPU ID and address of uCode so AP doesnt need to look for the uCode again if the CPU ID is same as BSPs. 3. Only apply uCode in one thread of a core when hyper threading is enabled. Test: Use an sample platform which has 1 socket, 4 core, 8 threads, the CpuMpPei driver cost time reduce from 108.4ms to 27.2ms Eric Dong (3): UefiCpuPkg/MpInitLib: Relocate uCode to memory to save time. UefiCpuPkg/MpInitLib: Use BSP uCode for APs if possible. UefiCpuPkg/MpInitLib: Load uCode once for one core. UefiCpuPkg/Library/MpInitLib/Microcode.c | 43 +--- UefiCpuPkg/Library/MpInitLib/MpLib.c | 17 ++--- UefiCpuPkg/Library/MpInitLib/MpLib.h | 11 ++-- 3 files changed, 63 insertions(+), 8 deletions(-) -- 2.15.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch 1/3] UefiCpuPkg/MpInitLib: Relocate uCode to memory to save time.
Read uCode from memory has better performance than from flash.
But it needs extra effort to let BSP copy uCode from flash to
memory. Also BSP already enable cache in SEC phase, so it use
less time to relocate uCode from flash to memory. After
verification, if system has more than one processor, it will
reduce some time if load uCode from memory.
This change enable this optimization.
Cc: Laszlo Ersek
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index eec178b419..8b458a4a3a 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1560,8 +1560,19 @@ MpInitLibInitialize (
CpuMpData->SwitchBspFlag= FALSE;
CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
CpuMpData->CpuInfoInHob = (UINT64) (UINTN) (CpuMpData->CpuData +
MaxLogicalProcessorNumber);
- CpuMpData->MicrocodePatchAddress= PcdGet64 (PcdCpuMicrocodePatchAddress);
CpuMpData->MicrocodePatchRegionSize = PcdGet64
(PcdCpuMicrocodePatchRegionSize);
+ //
+ // if platform has more than one CPU, relocate microcode to memory to reduce
loading microcode time.
+ //
+ if (MaxLogicalProcessorNumber > 1) {
+CpuMpData->MicrocodePatchAddress = (UINT64) (UINTN) AllocatePages
(EFI_SIZE_TO_PAGES ((UINTN)CpuMpData->MicrocodePatchRegionSize));
+if (CpuMpData->MicrocodePatchAddress != 0) {
+ CopyMem ((VOID *) (UINTN)CpuMpData->MicrocodePatchAddress, (VOID
*)(UINTN)(PcdGet64 (PcdCpuMicrocodePatchAddress)),
(UINTN)CpuMpData->MicrocodePatchRegionSize);
+}
+ } else {
+CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress);
+ }
+
InitializeSpinLock(>MpLock);
//
// Save BSP's Control registers to APs
--
2.15.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.
Hi
I believe using stack pointer is not a robust way if the stack guard feature is
not enabled. Stack pointer may overflow.
Can we use GDT? Each AP has its own GDT.
Thank you
Yao Jiewen
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dong,
> Eric
> Sent: Monday, July 9, 2018 2:13 PM
> To: Dong, Eric ; Laszlo Ersek ; Fan
> Jeff ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu
> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor
> number performance.
>
> Hi Laszlo,
>
> I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 to request
> to add AsmReadEsp() / AsmReadRsp().
>
> Thanks,
> Eric
>
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Dong, Eric
> > Sent: Monday, July 9, 2018 11:04 AM
> > To: Laszlo Ersek ; Fan Jeff ;
> > edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu
> > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> > processor number performance.
> >
> > Hi Laszlo,
> >
> > > -Original Message-
> > > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > > Sent: Thursday, July 5, 2018 9:04 PM
> > > To: Fan Jeff ; Dong, Eric
> > > ; edk2-devel@lists.01.org
> > > Cc: Ni, Ruiyu
> > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> > > processor number performance.
> > >
> > > Hi Jeff,
> > >
> > > On 07/04/18 11:39, Fan Jeff wrote:
> > > > Eric,
> > > >
> > > > Current implementation does not call GetApicid() many times, Please
> > > correct you commit message. Your fix is to improve the performance
> > > against the current implementation.
> > >
> > > I think the original commit message does make sense. Without the
> > > patch,
> > > GetProcessorNumber() may call GetApicId() up to TotalProcessorNumber
> > > times. With the patch, even if we skip the stack range search,
> > > GetProcessorNumber() will call GetApicId() just once.
> > >
> > > [...]
> > >
> > > Some more questions below, for the patch:
> > >
> > > > 发件人: Eric Dong
> > > > 发送时间: Wednesday, July 4, 2018 4:37:36 PM
> > > > 收件人: edk2-devel@lists.01.org
> > > > 抄送: Ruiyu Ni; Jeff Fan; Laszlo Ersek
> > > > 主题: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number
> > > performance.
> > > >
> > > > Current function has low performance because it calls GetApicId many
> > > > times.
> > > >
> > > > New logic first try to base on the stack range used by AP to find
> > > > the processor number. If this solution failed, then call GetApicId
> > > > once and base on this value to search the processor.
> > > >
> > > > Cc: Ruiyu Ni
> > > > Cc: Jeff Fan
> > > > Cc: Laszlo Ersek
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Eric Dong
> > > > ---
> > > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 25
> ++---
> > > > 1 file changed, 22 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > > index eb2765910c..abd65bee1a 100644
> > > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > > @@ -418,7 +418,8 @@ ApInitializeSync ( }
> > > >
> > > > /**
> > > > - Find the current Processor number by APIC ID.
> > > > + First try to find the current Processor number by stack address,
> > > > + if it failed, then base on APIC ID.
> > > >
> > > >@param[in] CpuMpData Pointer to PEI CPU MP Data
> > > >@param[out] ProcessorNumber Return the pocessor number found
> > > > @@ -435,16 +436,34 @@ GetProcessorNumber (
> > > >UINTN TotalProcessorNumber;
> > > >UINTN Index;
> > > >CPU_INFO_IN_HOB *CpuInfoInHob;
> > > > + UINT32 CurrentApicId;
> > > >
> > > > + TotalProcessorNumber = CpuMpData->CpuCount;
> > > >CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData-
> > > >CpuInfoInHob;
> > > >
> > > > - TotalProcessorNumber = CpuMpData->CpuCount;
> > > > + //
> > > > + // First try to base on current stack address to find the AP index.
> > > > + // value located in the stack range.
> > > > + //
> > > >for (Index = 0; Index < TotalProcessorNumber; Index ++) {
> > > > -if (CpuInfoInHob[Index].ApicId == GetApicId ()) {
> > > > +if ((CpuInfoInHob[Index].ApTopOfStack > (UINTN)
> > > ()) &&
> > > > +(CpuInfoInHob[Index].ApTopOfStack -
> > > > + CpuMpData->CpuApStackSize < (UINTN) ())) {
> > > >*ProcessorNumber = Index;
> > > >return EFI_SUCCESS;
> > > > }
> > > >}
> > >
> > > (1) If I understand correctly, ApTopOfStack is the exclusive end
> > > (highest
> > > address) of the AP stack, so any local variable is supposed to start
> > > strictly below it (the stack grows down). This seems to justify the
> > > ">" relational operator, in the first subcondition; OK.
> > >
> > > However, what guarantees that the TotalProcessorNumber local
[edk2] [Patch 3/3] UefiCpuPkg/MpInitLib: Load uCode once for one core.
SDM requires one core only needs to load uCode once.
Also load uCode once can save some time.
This change enables this solution.
Cc: Laszlo Ersek
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
---
UefiCpuPkg/Library/MpInitLib/Microcode.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 351975e2a2..4586e037d2 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -61,6 +61,7 @@ MicrocodeDetect (
VOID*MicrocodeData;
MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
UINT32 ProcessorFlags;
+ UINT32 ThreadId;
if (CpuMpData->MicrocodePatchRegionSize == 0) {
//
@@ -77,6 +78,14 @@ MicrocodeDetect (
return;
}
+ GetProcessorLocationByApicId (GetInitialApicId(), NULL, NULL, );
+ if (ThreadId != 0) {
+//
+// Skip loading microcode if it is not the first thread in one core.
+//
+return;
+ }
+
ExtendedTableLength = 0;
//
// Here data of CPUID leafs have not been collected into context buffer, so
--
2.15.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
On 11 July 2018 at 14:05, Laszlo Ersek wrote:
> Hi Roman,
>
> On 07/11/18 00:51, rba...@gmail.com wrote:
>> From: Roman Bacik
>>
>> When secure boot is enabled, if one loads keys from a FAT formatted
>> eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu,
>> an assert in StrLen() occurs.
>> This is because the filename starts on odd address, which is not a uint16
>> aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
>>
>> Cc: Chao Zhang
>> Cc: Jiewen Yao
>> Cc: Laszlo Ersek
>> Cc: Vladimir Olovyannikov
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Roman Bacik
>> ---
>>
>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>> | 13 +++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> Thank you for sending a well-formed patch.
>
> I notice that you sent this email from , which is not
> the same as the Signed-off-by line. I realize you posted from
> for technical reasons, and it should be no problem.
>
> However, I *think* in such cases we usually request the following:
>
> - Using your broadcom.com email address, please respond to this patch
> (not my present email, but your original git posting), keeping full
> context, and just repeat your Signed-off-by line (referencing the
> broadcom address).
>
> I'm CC'ing Jordan and Ard for confirmation -- I believe this is what
> we've done in the past, in cases when submitters had to post their work
> from private addresses due to company email issues.
>
Yes, please.
> Technical comments below:
>
>> diff --git
>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>>
>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>> index 1b6f88804275..19b13a5569a6 100644
>> ---
>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>> +++
>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>> @@ -123,6 +123,8 @@ OpenFileByDevicePath(
>>EFI_FILE_PROTOCOL *Handle1;
>>EFI_FILE_PROTOCOL *Handle2;
>>EFI_HANDLE DeviceHandle;
>> + CHAR16 *PathName;
>> + UINTN PathLength;
>>
>>if ((FilePath == NULL || FileHandle == NULL)) {
>> return EFI_INVALID_PARAMETER;
>> @@ -173,6 +175,11 @@ OpenFileByDevicePath(
>> //
>> Handle2 = Handle1;
>> Handle1 = NULL;
>> +PathLength = DevicePathNodeLength(*FilePath) -
>> sizeof(EFI_DEVICE_PATH_PROTOCOL);
>> +PathName = AllocateCopyPool(PathLength,
>> ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName);
>
> (1) On both lines above, space characters are missing after:
> DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style.)
> I think we can fix this up for you when we push the patch. (I'm willing
> to help with that, but we need SecurityPkg maintainer review first.)
>
>
>> +if (PathName == NULL) {
>> + return EFI_OUT_OF_RESOURCES;
>> +}
>
> (2) I have now reviewed the original state of the function more
> carefully, and, while the above "return" branch introduces a leak
> *path*, it does not introduce a leak that doesn't already exist!
>
> In fact, the original function has multiple issues:
>
> - If the OpenVolume() call fails, "FileHandle" is set to NULL. That's
> useless; the intent is obviously to set (*FileHandle) to NULL.
>
> - At the top of the "while" loop body, "Handle1" stands for an open
> EFI_FILE_PROTOCOL. If the device path type check at the top of the loop
> body returns EFI_INVALID_PARAMETER, then it (a) performs the same
> useless assignment to "FileHandle" as described above, and (b) fails to
> close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES
> branch introduces no new leak, just a new path to the existent leak.
>
> - The OpenFileByDevicePath() function is duplicated in the following
> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". With the
> implication that the alignment issue you found affects all three drivers!
>
>
> Roman, I realize this could be more than what you signed up for; so
> please pick one:
>
> (2a) you could submit a patch series:
>
> * Write a patch that sets (*FilePath) to NULL right after the
> (FileHandle==NULL) check, in preparation for failure, and removes all
> the bogus FileHandle=NULL assignments.
>
> * Write another patch that plugs the leak when the device path type
> check fails -- introduce a "CloseHandle1" label at the end of the
> function, and jump to it when the devpath type check fails, so that we
> close "Handle1". This patch should also invert the meanings of Handle2
> and Handle1 -- the reassignment to Handle1 should only occur *after* we
> successfully open Handle2. "Handle1" should *always* remain suitable for
> closing through the "CloseHandle1" error
Re: [edk2] [PATCH v4 6/9] ArmPkg/PlatformBDS: Implement PlatformBootManagerUnableToBoot
On 4 July 2018 at 03:50, Ruiyu Ni wrote:
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni
> Cc: Leif Lindholm
> Cc: Ard Biesheuvel
Acked-by: Ard Biesheuvel
> ---
> ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 18 +-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 079f1552d5..f9c71d430c 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -3,7 +3,7 @@
>
>Copyright (C) 2015-2016, Red Hat, Inc.
>Copyright (c) 2014, ARM Ltd. All rights reserved.
> - Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.
> + Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.
>Copyright (c) 2016, Linaro Ltd. All rights reserved.
>
>This program and the accompanying materials are licensed and made available
> @@ -766,3 +766,19 @@ PlatformBootManagerWaitCallback (
> Print (L".");
>}
> }
> +
> +/**
> + The function is called when no boot option could be launched,
> + including platform recovery options and options pointing to applications
> + built into firmware volumes.
> +
> + If this function returns, BDS attempts to enter an infinite loop.
> +**/
> +VOID
> +EFIAPI
> +PlatformBootManagerUnableToBoot (
> + VOID
> + )
> +{
> + return;
> +}
> --
> 2.16.1.windows.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms v2 1/6] Hisilicon/D0x: Fix invoke SetMemorySpaceAttributes error bug
On 4 July 2018 at 09:51, Ming Huang wrote:
> The edk2 commit bacfd6e let CpuDxe running latter.
> CpuDxe is needed by gDS->SetMemorySpaceAttributes, and
> gDS->SetMemorySpaceAttributes is invoked by some drivers.
>
> This issue can solve by adding Depex on gEfiCpuArchProtocolGuid
> to RealTimeClockLib.
>
If this is the case, why do we still need the APRIORI DXE section?
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ming Huang
> Signed-off-by: Heyi Guo
> ---
> Platform/Hisilicon/D03/D03.fdf
> | 4
>
> Platform/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> | 2 ++
> 2 files changed, 6 insertions(+)
>
> diff --git a/Platform/Hisilicon/D03/D03.fdf b/Platform/Hisilicon/D03/D03.fdf
> index 1383aa1091..73e2b7e958 100644
> --- a/Platform/Hisilicon/D03/D03.fdf
> +++ b/Platform/Hisilicon/D03/D03.fdf
> @@ -146,6 +146,10 @@ READ_STATUS= TRUE
> READ_LOCK_CAP = TRUE
> READ_LOCK_STATUS = TRUE
>
> + APRIORI DXE {
> +INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> + }
> +
>INF MdeModulePkg/Core/Dxe/DxeMain.inf
>INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
>
> diff --git
> a/Platform/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
>
> b/Platform/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> index 319c35c724..ae7116dc31 100644
> ---
> a/Platform/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> +++
> b/Platform/Hisilicon/D03/Library/DS3231RealTimeClockLib/DS3231RealTimeClockLib.inf
> @@ -46,3 +46,5 @@
>
> [Pcd]
>
> +[Depex]
> + gEfiCpuArchProtocolGuid
> --
> 2.17.0
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.
On 07/11/18 13:31, Dong, Eric wrote: > Hi Jiewen, > > I checked the code, found in the AP function (ApWakeupFunction), it updated > the GDT value with the saved GDT value from BSP. So I think we can't use GDT > in this case. Right? > > // > // Sync BSP's Control registers to APs > // > RestoreVolatileRegisters (>CpuData[0].VolatileRegisters, > FALSE); Side remark: please remember that this particular chunk of code is subject to change; due to the reviewed (but not yet committed) patch from Ray: [PATCH] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP 20180702060135.264676-1-ruiyu.ni@intel.com">http://mid.mail-archive.com/20180702060135.264676-1-ruiyu.ni@intel.com Said patch has *not* been committed yet, and *only* because Ray himself has push rights, but he is currently away. So nobody has picked up the patch yet. I suggest that, before we do anything else for MpInitLib, we commit Ray's patch first. Eric, do you agree? If so, can you push the patch, or do you want me to do it? I'm glad to do it if you prefer. Thanks, Laszlo >> -Original Message- >> From: Yao, Jiewen >> Sent: Wednesday, July 11, 2018 3:45 PM >> To: Dong, Eric ; Dong, Eric ; >> Laszlo Ersek ; Fan Jeff ; >> edk2-devel@lists.01.org >> Cc: Ni, Ruiyu >> Subject: RE: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get >> processor number performance. >> >> Hi >> I believe using stack pointer is not a robust way if the stack guard feature >> is >> not enabled. Stack pointer may overflow. >> >> Can we use GDT? Each AP has its own GDT. >> >> Thank you >> Yao Jiewen >> >> >>> -Original Message- >>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >>> Dong, Eric >>> Sent: Monday, July 9, 2018 2:13 PM >>> To: Dong, Eric ; Laszlo Ersek >>> ; Fan Jeff ; >>> edk2-devel@lists.01.org >>> Cc: Ni, Ruiyu >>> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get >>> processor number performance. >>> >>> Hi Laszlo, >>> >>> I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 to >>> request to add AsmReadEsp() / AsmReadRsp(). >>> >>> Thanks, >>> Eric >>> -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dong, Eric Sent: Monday, July 9, 2018 11:04 AM To: Laszlo Ersek ; Fan Jeff ; edk2-devel@lists.01.org Cc: Ni, Ruiyu Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance. Hi Laszlo, > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, July 5, 2018 9:04 PM > To: Fan Jeff ; Dong, Eric > ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > processor number performance. > > Hi Jeff, > > On 07/04/18 11:39, Fan Jeff wrote: >> Eric, >> >> Current implementation does not call GetApicid() many times, >> Please > correct you commit message. Your fix is to improve the performance > against the current implementation. > > I think the original commit message does make sense. Without the > patch, > GetProcessorNumber() may call GetApicId() up to > TotalProcessorNumber times. With the patch, even if we skip the > stack range search, > GetProcessorNumber() will call GetApicId() just once. > > [...] > > Some more questions below, for the patch: > >> 发件人: Eric Dong >> 发送时间: Wednesday, July 4, 2018 4:37:36 PM >> 收件人: edk2-devel@lists.01.org >> 抄送: Ruiyu Ni; Jeff Fan; Laszlo Ersek >> 主题: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number > performance. >> >> Current function has low performance because it calls GetApicId >> many times. >> >> New logic first try to base on the stack range used by AP to >> find the processor number. If this solution failed, then call >> GetApicId once and base on this value to search the processor. >> >> Cc: Ruiyu Ni >> Cc: Jeff Fan >> Cc: Laszlo Ersek >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Eric Dong >> --- >> UefiCpuPkg/Library/MpInitLib/MpLib.c | 25 >>> ++--- >> 1 file changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c >> b/UefiCpuPkg/Library/MpInitLib/MpLib.c >> index eb2765910c..abd65bee1a 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >> @@ -418,7 +418,8 @@ ApInitializeSync ( } >> >> /** >> - Find the current Processor number by APIC ID. >> + First try to find the current Processor number by stack >> + address, if it failed, then base on APIC ID. >> >>@param[in] CpuMpData Pointer to PEI CPU MP Data >>@param[out]
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, July 11, 2018 5:16 AM
> To: rba...@gmail.com; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Vladimir Olovyannikov
> ; Justen, Jordan L
> ; Gao, Liming ; Carsey,
> Jaben ; Yao, Jiewen ;
> Kinney, Michael D ; Zhang, Chao B
>
> Subject: Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from
> eMMC/SD/USB
> Importance: High
>
> On 07/11/18 14:05, Laszlo Ersek wrote:
>
> > - The OpenFileByDevicePath() function is duplicated in the following
> > modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
> > "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c".
> With the
> > implication that the alignment issue you found affects all three drivers!
>
> Note that I've mutually diffed the three function definitions between
> each other (with "diff -b"), and there are only whitespace and comment
> wrapping differences.
>
> So, again, this function looks like a prime suspect for UefiLib.
>
> I do see the strange reference at the end ("undefined SHELL_FILE_HANDLE
> format"); I think that's simply stale, and should be removed.
Looks like the stale reference may be from origination as derived from
ShellOpenFileByDevicePath. I agree this should be moved to a lib. Any way we
could merge the shell's only slightly different version also into the mix?
-Jaben
>
> 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] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
On 07/11/18 19:10, Carsey, Jaben wrote:
>
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, July 11, 2018 5:16 AM
>> To: rba...@gmail.com; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu ; Vladimir Olovyannikov
>> ; Justen, Jordan L
>> ; Gao, Liming ; Carsey,
>> Jaben ; Yao, Jiewen ;
>> Kinney, Michael D ; Zhang, Chao B
>>
>> Subject: Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from
>> eMMC/SD/USB
>> Importance: High
>>
>> On 07/11/18 14:05, Laszlo Ersek wrote:
>>
>>> - The OpenFileByDevicePath() function is duplicated in the following
>>> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
>>> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c".
>> With the
>>> implication that the alignment issue you found affects all three drivers!
>>
>> Note that I've mutually diffed the three function definitions between
>> each other (with "diff -b"), and there are only whitespace and comment
>> wrapping differences.
>>
>> So, again, this function looks like a prime suspect for UefiLib.
>>
>> I do see the strange reference at the end ("undefined SHELL_FILE_HANDLE
>> format"); I think that's simply stale, and should be removed.
>
> Looks like the stale reference may be from origination as derived from
> ShellOpenFileByDevicePath. I agree this should be moved to a lib. Any way
> we could merge the shell's only slightly different version also into the mix?
Wow, that's incredible -- ShellOpenFileByDevicePath() contains the
*exact same* alignment fix that Roman is contributing with this patch;
from commit 0b6cb335fa82 ("Fixed some alignment faults in IPF platform",
2013-01-25).
ShellOpenFileByDevicePath() -- including the alignment fix -- also has
the same kind of resource leak (regarding "Handle1").
So, code duplication has bitten us again. We have four instances of
basically the same logic in the tree, all four are affected by the same
resource leak, and the alignment fix that was discovered in or before
2013 was applied to only one of the four.
So, yes. The top of the ShellOpenFileByDevicePath() function (the "UEFI
Shell 2.0 method") should be preserved, but then the last part ("use old
shell method") should be replaced with a call to the new UefiLib API.
And, the final "weak spot" comment remains valid, but only for
ShellOpenFileByDevicePath(), not for the new EfiOpenFileByDevicePath() API.
Thank you Jaben for finding this!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
Hi Roman,
On 07/11/18 00:51, rba...@gmail.com wrote:
> From: Roman Bacik
>
> When secure boot is enabled, if one loads keys from a FAT formatted
> eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu,
> an assert in StrLen() occurs.
> This is because the filename starts on odd address, which is not a uint16
> aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
>
> Cc: Chao Zhang
> Cc: Jiewen Yao
> Cc: Laszlo Ersek
> Cc: Vladimir Olovyannikov
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Roman Bacik
> ---
>
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
Thank you for sending a well-formed patch.
I notice that you sent this email from , which is not
the same as the Signed-off-by line. I realize you posted from
for technical reasons, and it should be no problem.
However, I *think* in such cases we usually request the following:
- Using your broadcom.com email address, please respond to this patch
(not my present email, but your original git posting), keeping full
context, and just repeat your Signed-off-by line (referencing the
broadcom address).
I'm CC'ing Jordan and Ard for confirmation -- I believe this is what
we've done in the past, in cases when submitters had to post their work
from private addresses due to company email issues.
Technical comments below:
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> index 1b6f88804275..19b13a5569a6 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> @@ -123,6 +123,8 @@ OpenFileByDevicePath(
>EFI_FILE_PROTOCOL *Handle1;
>EFI_FILE_PROTOCOL *Handle2;
>EFI_HANDLE DeviceHandle;
> + CHAR16 *PathName;
> + UINTN PathLength;
>
>if ((FilePath == NULL || FileHandle == NULL)) {
> return EFI_INVALID_PARAMETER;
> @@ -173,6 +175,11 @@ OpenFileByDevicePath(
> //
> Handle2 = Handle1;
> Handle1 = NULL;
> +PathLength = DevicePathNodeLength(*FilePath) -
> sizeof(EFI_DEVICE_PATH_PROTOCOL);
> +PathName = AllocateCopyPool(PathLength,
> ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName);
(1) On both lines above, space characters are missing after:
DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style.)
I think we can fix this up for you when we push the patch. (I'm willing
to help with that, but we need SecurityPkg maintainer review first.)
> +if (PathName == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> +}
(2) I have now reviewed the original state of the function more
carefully, and, while the above "return" branch introduces a leak
*path*, it does not introduce a leak that doesn't already exist!
In fact, the original function has multiple issues:
- If the OpenVolume() call fails, "FileHandle" is set to NULL. That's
useless; the intent is obviously to set (*FileHandle) to NULL.
- At the top of the "while" loop body, "Handle1" stands for an open
EFI_FILE_PROTOCOL. If the device path type check at the top of the loop
body returns EFI_INVALID_PARAMETER, then it (a) performs the same
useless assignment to "FileHandle" as described above, and (b) fails to
close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES
branch introduces no new leak, just a new path to the existent leak.
- The OpenFileByDevicePath() function is duplicated in the following
modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
"MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". With the
implication that the alignment issue you found affects all three drivers!
Roman, I realize this could be more than what you signed up for; so
please pick one:
(2a) you could submit a patch series:
* Write a patch that sets (*FilePath) to NULL right after the
(FileHandle==NULL) check, in preparation for failure, and removes all
the bogus FileHandle=NULL assignments.
* Write another patch that plugs the leak when the device path type
check fails -- introduce a "CloseHandle1" label at the end of the
function, and jump to it when the devpath type check fails, so that we
close "Handle1". This patch should also invert the meanings of Handle2
and Handle1 -- the reassignment to Handle1 should only occur *after* we
successfully open Handle2. "Handle1" should *always* remain suitable for
closing through the "CloseHandle1" error path.
* Include your current patch, for fixing the alignment issue.
* Write another patch that moves the OpenFileByDevicePath() function to
UefiLib in MdePkg -- under the name EfiOpenFileByDevicePath() -- from
[edk2] reg: PXE Boot Enhancement
Hello All, During PXE boot if the operation fails to connect the PXE server, system waits for the user's manual Key input to abort the operation and proceed further. It would be nice to have an option to set timeout before starting PXE boot so that if there's any issue with the PXE boot process, the system can automatically abort the boot and proceed to next boot after the specified time. Could you please feedback on this. -Siva ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
On 07/11/18 14:05, Laszlo Ersek wrote:
> - The OpenFileByDevicePath() function is duplicated in the following
> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". With the
> implication that the alignment issue you found affects all three drivers!
Note that I've mutually diffed the three function definitions between
each other (with "diff -b"), and there are only whitespace and comment
wrapping differences.
So, again, this function looks like a prime suspect for UefiLib.
I do see the strange reference at the end ("undefined SHELL_FILE_HANDLE
format"); I think that's simply stale, and should be removed.
Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] OVMF dynamic NVRAM size discovering
Hello! I'd like to ask whether it's a good idea or not to make the omfv discover the variable image size in the runtime. We have a setting in our product to use separate pflash images for OVMF code and variables parts in QEMU/KVM based virtual machines. Currently, we use a 2M code image but want to switch to using a 4M ones to extend the variable values storage, because of Windows certification requirements. We think of adding an ability to OVFM to discover the size of the variable part dynamically. It could work like the following: 1. teach qemu to provide the size of pflash0 and pflash1 via fw_cfg 2. modify OVMF in order to discover the variable size part dynamically via fw_cfg and adjust the OVMF's inner parameters accordingly We have some concerns about our understanding of OVMF and would like to ask to evaluate the approach suggested. Is it feasible? Do we miss something? -- Best, Denis ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.
Hi Jiewen, I checked the code, found in the AP function (ApWakeupFunction), it updated the GDT value with the saved GDT value from BSP. So I think we can't use GDT in this case. Right? // // Sync BSP's Control registers to APs // RestoreVolatileRegisters (>CpuData[0].VolatileRegisters, FALSE); Thanks, Eric > -Original Message- > From: Yao, Jiewen > Sent: Wednesday, July 11, 2018 3:45 PM > To: Dong, Eric ; Dong, Eric ; > Laszlo Ersek ; Fan Jeff ; > edk2-devel@lists.01.org > Cc: Ni, Ruiyu > Subject: RE: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > processor number performance. > > Hi > I believe using stack pointer is not a robust way if the stack guard feature > is > not enabled. Stack pointer may overflow. > > Can we use GDT? Each AP has its own GDT. > > Thank you > Yao Jiewen > > > > -Original Message- > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > > Dong, Eric > > Sent: Monday, July 9, 2018 2:13 PM > > To: Dong, Eric ; Laszlo Ersek > > ; Fan Jeff ; > > edk2-devel@lists.01.org > > Cc: Ni, Ruiyu > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > > processor number performance. > > > > Hi Laszlo, > > > > I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 to > > request to add AsmReadEsp() / AsmReadRsp(). > > > > Thanks, > > Eric > > > > > -Original Message- > > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf > > > Of Dong, Eric > > > Sent: Monday, July 9, 2018 11:04 AM > > > To: Laszlo Ersek ; Fan Jeff > > > ; edk2-devel@lists.01.org > > > Cc: Ni, Ruiyu > > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > > > processor number performance. > > > > > > Hi Laszlo, > > > > > > > -Original Message- > > > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > > > Sent: Thursday, July 5, 2018 9:04 PM > > > > To: Fan Jeff ; Dong, Eric > > > > ; edk2-devel@lists.01.org > > > > Cc: Ni, Ruiyu > > > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > > > > processor number performance. > > > > > > > > Hi Jeff, > > > > > > > > On 07/04/18 11:39, Fan Jeff wrote: > > > > > Eric, > > > > > > > > > > Current implementation does not call GetApicid() many times, > > > > > Please > > > > correct you commit message. Your fix is to improve the performance > > > > against the current implementation. > > > > > > > > I think the original commit message does make sense. Without the > > > > patch, > > > > GetProcessorNumber() may call GetApicId() up to > > > > TotalProcessorNumber times. With the patch, even if we skip the > > > > stack range search, > > > > GetProcessorNumber() will call GetApicId() just once. > > > > > > > > [...] > > > > > > > > Some more questions below, for the patch: > > > > > > > > > 发件人: Eric Dong > > > > > 发送时间: Wednesday, July 4, 2018 4:37:36 PM > > > > > 收件人: edk2-devel@lists.01.org > > > > > 抄送: Ruiyu Ni; Jeff Fan; Laszlo Ersek > > > > > 主题: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number > > > > performance. > > > > > > > > > > Current function has low performance because it calls GetApicId > > > > > many times. > > > > > > > > > > New logic first try to base on the stack range used by AP to > > > > > find the processor number. If this solution failed, then call > > > > > GetApicId once and base on this value to search the processor. > > > > > > > > > > Cc: Ruiyu Ni > > > > > Cc: Jeff Fan > > > > > Cc: Laszlo Ersek > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > Signed-off-by: Eric Dong > > > > > --- > > > > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 25 > > ++--- > > > > > 1 file changed, 22 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > > > index eb2765910c..abd65bee1a 100644 > > > > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > > > @@ -418,7 +418,8 @@ ApInitializeSync ( } > > > > > > > > > > /** > > > > > - Find the current Processor number by APIC ID. > > > > > + First try to find the current Processor number by stack > > > > > + address, if it failed, then base on APIC ID. > > > > > > > > > >@param[in] CpuMpData Pointer to PEI CPU MP Data > > > > >@param[out] ProcessorNumber Return the pocessor number found > > > > > @@ -435,16 +436,34 @@ GetProcessorNumber ( > > > > >UINTN TotalProcessorNumber; > > > > >UINTN Index; > > > > >CPU_INFO_IN_HOB *CpuInfoInHob; > > > > > + UINT32 CurrentApicId; > > > > > > > > > > + TotalProcessorNumber = CpuMpData->CpuCount; > > > > >CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData- > > > > >CpuInfoInHob; > > > > > > > > > > - TotalProcessorNumber = CpuMpData->CpuCount; > > > > > + // > > > > > + // First try to base on current
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
Signed-off-by: Roman Bacik
On Tue, Jul 10, 2018 at 3:51 PM, wrote:
> From: Roman Bacik
>
> When secure boot is enabled, if one loads keys from a FAT formatted
> eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu,
> an assert in StrLen() occurs.
> This is because the filename starts on odd address, which is not a uint16
> aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
>
> Cc: Chao Zhang
> Cc: Jiewen Yao
> Cc: Laszlo Ersek
> Cc: Vladimir Olovyannikov
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Roman Bacik
> ---
>
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> index 1b6f88804275..19b13a5569a6 100644
> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> @@ -123,6 +123,8 @@ OpenFileByDevicePath(
>EFI_FILE_PROTOCOL *Handle1;
>EFI_FILE_PROTOCOL *Handle2;
>EFI_HANDLE DeviceHandle;
> + CHAR16 *PathName;
> + UINTN PathLength;
>
>if ((FilePath == NULL || FileHandle == NULL)) {
> return EFI_INVALID_PARAMETER;
> @@ -173,6 +175,11 @@ OpenFileByDevicePath(
> //
> Handle2 = Handle1;
> Handle1 = NULL;
> +PathLength = DevicePathNodeLength(*FilePath) -
> sizeof(EFI_DEVICE_PATH_PROTOCOL);
> +PathName = AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH*)*
> FilePath)->PathName);
> +if (PathName == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> +}
>
> //
> // Try to test opening an existing file
> @@ -180,7 +187,7 @@ OpenFileByDevicePath(
> Status = Handle2->Open (
>Handle2,
>,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> + PathName,
>OpenMode &~EFI_FILE_MODE_CREATE,
>0
> );
> @@ -192,7 +199,7 @@ OpenFileByDevicePath(
>Status = Handle2->Open (
> Handle2,
> ,
> -((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> +PathName,
> OpenMode,
> Attributes
> );
> @@ -202,6 +209,8 @@ OpenFileByDevicePath(
> //
> Handle2->Close (Handle2);
>
> +FreePool (PathName);
> +
> if (EFI_ERROR(Status)) {
>return (Status);
> }
> --
> 2.17.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] OVMF dynamic NVRAM size discovering
On 07/11/18 15:01, Denis Plotnikov wrote:
> Hello!
>
> I'd like to ask whether it's a good idea or not to make the omfv
> discover the variable image size in the runtime.
Not a good idea.
> We have a setting in our product to use separate pflash images for OVMF
> code and variables parts in QEMU/KVM based virtual machines.
That's a good choice.
> Currently, we use a 2M code image but want to switch to using a 4M ones
> to extend the variable values storage, because of Windows certification
> requirements.
That's correct.
You will have to abandon the existent variable store files (for existent
VMs), and recreate them from the new variable store template file that
comes from the 4MB build, when you switch the firmware binary from the
2MB build to the 4MB build.
In other words, you can switch an (offline) VM from one firmware binary
to another, but at the same time you have to recreate the variable store
for the VM from the pristine variable store template that comes from the
new (4MB unified) firmware build. For example, OVMF_CODE.fd from the 2MB
build, and a variable store file instantiated from OVMF_VARS.fd from the
4MB build, are not compatible. Vice versa for 4MB/2MB.
If you can't recreate varstore files like this, then you can only
provide the new (4MB unified) firmware build *in addition* to the other
firmware build, and only newly created domains will be able to use it.
> We think of adding an ability to OVFM to discover the size of the
> variable part dynamically.
>
> It could work like the following:
> 1. teach qemu to provide the size of pflash0 and pflash1 via fw_cfg
> 2. modify OVMF in order to discover the variable size part dynamically
> via fw_cfg and adjust the OVMF's inner parameters accordingly
>
> We have some concerns about our understanding of OVMF and would like to
> ask to evaluate the approach suggested.
> Is it feasible? Do we miss something?
In general, using fw_cfg files for setting dynamic PCDs, is a widely
used technique.
In this particular case, it's not feasible, to my understanding.
- There are several build constants related to the internal structure of
the pflash chip in question -- basically that chip is subdivided into
multiple areas. Knowing the outermost size is insufficient.
- The variable store template file is prepared at build, and it depends
on -- and can't avoid open-coding -- some of those lower-level constants.
- As much as I dislike it, OVMF still contains support for the "-bios"
QEMU option (when there is no pflash at all). That complicates things in
obscure and unpleasant ways.
Clearly, I can't "guarantee" that the idea is impossible to implement.
However, it does make me *really* uncomfortable.
I strongly suggest going statically with the 4MB build, and tracking the
firmware choice explicitly, both in your host package system, and at the
domain level, in your virtualization management product.
In case you use libvirt as a building block for that, libvirt already
supports this.
See also QEMU commit 3a0adfc9bfcf ("docs/interop: add "firmware.json"",
2018-06-01).
Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 0/3] Optimize load uCode performance
Hi Eric, On 07/11/18 13:07, Eric Dong wrote: > Use below three rules to optimize load uCode performance: > 1. Let BSP relocate uCode from flash to memory for better performance. > 2. BSP caches the CPU ID and address of uCode so AP doesn’t need to look >for the uCode again if the CPU ID is same as BSP’s. > 3. Only apply uCode in one thread of a core when hyper threading is enabled. > > Test: > Use an sample platform which has 1 socket, 4 core, 8 threads, the CpuMpPei > driver cost time reduce from 108.4ms to 27.2ms > > Eric Dong (3): > UefiCpuPkg/MpInitLib: Relocate uCode to memory to save time. > UefiCpuPkg/MpInitLib: Use BSP uCode for APs if possible. > UefiCpuPkg/MpInitLib: Load uCode once for one core. > > UefiCpuPkg/Library/MpInitLib/Microcode.c | 43 > +--- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 17 ++--- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 11 ++-- > 3 files changed, 63 insertions(+), 8 deletions(-) I'm putting off the regression-testing of this series until we come to a conclusion on pushing Ray's patch first (see my message at <26d8afe6-f89b-ddfc-0e8b-6acdc311f121@redhat.com">http://mid.mail-archive.com/26d8afe6-f89b-ddfc-0e8b-6acdc311f121@redhat.com>). Thanks! Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
Hi Laszlo,
Thank you very much for your review and help. I would prefer the option 2b.
Thanks,
Roman
On Wed, Jul 11, 2018 at 5:05 AM, Laszlo Ersek wrote:
> Hi Roman,
>
> On 07/11/18 00:51, rba...@gmail.com wrote:
> > From: Roman Bacik
> >
> > When secure boot is enabled, if one loads keys from a FAT formatted
> > eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu,
> > an assert in StrLen() occurs.
> > This is because the filename starts on odd address, which is not a uint16
> > aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
> >
> > Cc: Chao Zhang
> > Cc: Jiewen Yao
> > Cc: Laszlo Ersek
> > Cc: Vladimir Olovyannikov
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Roman Bacik
> > ---
> >
> > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> | 13 +++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
>
> Thank you for sending a well-formed patch.
>
> I notice that you sent this email from , which is not
> the same as the Signed-off-by line. I realize you posted from
> for technical reasons, and it should be no problem.
>
> However, I *think* in such cases we usually request the following:
>
> - Using your broadcom.com email address, please respond to this patch
> (not my present email, but your original git posting), keeping full
> context, and just repeat your Signed-off-by line (referencing the
> broadcom address).
>
> I'm CC'ing Jordan and Ard for confirmation -- I believe this is what
> we've done in the past, in cases when submitters had to post their work
> from private addresses due to company email issues.
>
> Technical comments below:
>
> > diff --git
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> > index 1b6f88804275..19b13a5569a6 100644
> > --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
> SecureBootConfigFileExplorer.c
> > @@ -123,6 +123,8 @@ OpenFileByDevicePath(
> >EFI_FILE_PROTOCOL *Handle1;
> >EFI_FILE_PROTOCOL *Handle2;
> >EFI_HANDLE DeviceHandle;
> > + CHAR16 *PathName;
> > + UINTN PathLength;
> >
> >if ((FilePath == NULL || FileHandle == NULL)) {
> > return EFI_INVALID_PARAMETER;
> > @@ -173,6 +175,11 @@ OpenFileByDevicePath(
> > //
> > Handle2 = Handle1;
> > Handle1 = NULL;
> > +PathLength = DevicePathNodeLength(*FilePath) -
> sizeof(EFI_DEVICE_PATH_PROTOCOL);
> > +PathName = AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH*)*
> FilePath)->PathName);
>
> (1) On both lines above, space characters are missing after:
> DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style.)
> I think we can fix this up for you when we push the patch. (I'm willing
> to help with that, but we need SecurityPkg maintainer review first.)
>
>
> > +if (PathName == NULL) {
> > + return EFI_OUT_OF_RESOURCES;
> > +}
>
> (2) I have now reviewed the original state of the function more
> carefully, and, while the above "return" branch introduces a leak
> *path*, it does not introduce a leak that doesn't already exist!
>
> In fact, the original function has multiple issues:
>
> - If the OpenVolume() call fails, "FileHandle" is set to NULL. That's
> useless; the intent is obviously to set (*FileHandle) to NULL.
>
> - At the top of the "while" loop body, "Handle1" stands for an open
> EFI_FILE_PROTOCOL. If the device path type check at the top of the loop
> body returns EFI_INVALID_PARAMETER, then it (a) performs the same
> useless assignment to "FileHandle" as described above, and (b) fails to
> close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES
> branch introduces no new leak, just a new path to the existent leak.
>
> - The OpenFileByDevicePath() function is duplicated in the following
> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". With the
> implication that the alignment issue you found affects all three drivers!
>
>
> Roman, I realize this could be more than what you signed up for; so
> please pick one:
>
> (2a) you could submit a patch series:
>
> * Write a patch that sets (*FilePath) to NULL right after the
> (FileHandle==NULL) check, in preparation for failure, and removes all
> the bogus FileHandle=NULL assignments.
>
> * Write another patch that plugs the leak when the device path type
> check fails -- introduce a "CloseHandle1" label at the end of the
> function, and jump to it when the devpath type check fails, so that we
> close "Handle1". This patch should also invert the meanings of Handle2
> and Handle1 -- the reassignment to Handle1 should only occur
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
On 07/11/18 17:44, Roman Bacik wrote:
> Hi Laszlo,
>
> Thank you very much for your review and help. I would prefer the option 2b.
Great, thanks! Let's wait for the SecurityPkg maintainers then, to give
their R-b's for your patch. Chao Zhang, Jiewen, can you please comment?
>From my side, dependent on the pending commit message and patch
whitespace corrections (which I'm willing to implement myself, at push):
Reviewed-by: Laszlo Ersek
Thanks!
Laszlo
> Thanks,
>
> Roman
>
> On Wed, Jul 11, 2018 at 5:05 AM, Laszlo Ersek wrote:
>
>> Hi Roman,
>>
>> On 07/11/18 00:51, rba...@gmail.com wrote:
>>> From: Roman Bacik
>>>
>>> When secure boot is enabled, if one loads keys from a FAT formatted
>>> eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu,
>>> an assert in StrLen() occurs.
>>> This is because the filename starts on odd address, which is not a uint16
>>> aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
>>>
>>> Cc: Chao Zhang
>>> Cc: Jiewen Yao
>>> Cc: Laszlo Ersek
>>> Cc: Vladimir Olovyannikov
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Roman Bacik
>>> ---
>>>
>>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>> | 13 +++--
>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> Thank you for sending a well-formed patch.
>>
>> I notice that you sent this email from , which is not
>> the same as the Signed-off-by line. I realize you posted from
>> for technical reasons, and it should be no problem.
>>
>> However, I *think* in such cases we usually request the following:
>>
>> - Using your broadcom.com email address, please respond to this patch
>> (not my present email, but your original git posting), keeping full
>> context, and just repeat your Signed-off-by line (referencing the
>> broadcom address).
>>
>> I'm CC'ing Jordan and Ard for confirmation -- I believe this is what
>> we've done in the past, in cases when submitters had to post their work
>> from private addresses due to company email issues.
>>
>> Technical comments below:
>>
>>> diff --git
>>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
>> SecureBootConfigFileExplorer.c
>>> index 1b6f88804275..19b13a5569a6 100644
>>> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
>> SecureBootConfigFileExplorer.c
>>> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
>> SecureBootConfigFileExplorer.c
>>> @@ -123,6 +123,8 @@ OpenFileByDevicePath(
>>>EFI_FILE_PROTOCOL *Handle1;
>>>EFI_FILE_PROTOCOL *Handle2;
>>>EFI_HANDLE DeviceHandle;
>>> + CHAR16 *PathName;
>>> + UINTN PathLength;
>>>
>>>if ((FilePath == NULL || FileHandle == NULL)) {
>>> return EFI_INVALID_PARAMETER;
>>> @@ -173,6 +175,11 @@ OpenFileByDevicePath(
>>> //
>>> Handle2 = Handle1;
>>> Handle1 = NULL;
>>> +PathLength = DevicePathNodeLength(*FilePath) -
>> sizeof(EFI_DEVICE_PATH_PROTOCOL);
>>> +PathName = AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH*)*
>> FilePath)->PathName);
>>
>> (1) On both lines above, space characters are missing after:
>> DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style.)
>> I think we can fix this up for you when we push the patch. (I'm willing
>> to help with that, but we need SecurityPkg maintainer review first.)
>>
>>
>>> +if (PathName == NULL) {
>>> + return EFI_OUT_OF_RESOURCES;
>>> +}
>>
>> (2) I have now reviewed the original state of the function more
>> carefully, and, while the above "return" branch introduces a leak
>> *path*, it does not introduce a leak that doesn't already exist!
>>
>> In fact, the original function has multiple issues:
>>
>> - If the OpenVolume() call fails, "FileHandle" is set to NULL. That's
>> useless; the intent is obviously to set (*FileHandle) to NULL.
>>
>> - At the top of the "while" loop body, "Handle1" stands for an open
>> EFI_FILE_PROTOCOL. If the device path type check at the top of the loop
>> body returns EFI_INVALID_PARAMETER, then it (a) performs the same
>> useless assignment to "FileHandle" as described above, and (b) fails to
>> close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES
>> branch introduces no new leak, just a new path to the existent leak.
>>
>> - The OpenFileByDevicePath() function is duplicated in the following
>> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
>> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". With the
>> implication that the alignment issue you found affects all three drivers!
>>
>>
>> Roman, I realize this could be more than what you signed up for; so
>> please pick one:
>>
>> (2a) you could submit a patch series:
>>
>> * Write a patch that sets (*FilePath) to NULL right after the
>>
[edk2] Reg: Intel Rangley Support in EDK
Hi, I have booted Intel Rangley / MohonPeak platform in coreboot with Intel FSP. I am unable to install UEFI OS in coreboot (sometime). EDK bios will install both UEFI and Legacy OS. Does Open EDK supports Intel Rangley platform?. Is code base available for Intel Rangely platform? Thanks, Dhanasekar ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.
Hi Laszlo, > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, July 11, 2018 11:12 PM > To: Dong, Eric ; Yao, Jiewen ; > Fan Jeff ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > processor number performance. > > On 07/11/18 13:31, Dong, Eric wrote: > > Hi Jiewen, > > > > I checked the code, found in the AP function (ApWakeupFunction), it > updated the GDT value with the saved GDT value from BSP. So I think we can't > use GDT in this case. Right? > > > > // > > // Sync BSP's Control registers to APs > > // > > RestoreVolatileRegisters > > (>CpuData[0].VolatileRegisters, FALSE); > > Side remark: please remember that this particular chunk of code is subject to > change; due to the reviewed (but not yet committed) patch from Ray: > > [PATCH] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP > > 20180702060135.264676-1-ruiyu.ni@intel.com">http://mid.mail-archive.com/20180702060135.264676-1-ruiyu.ni@intel.com > > Said patch has *not* been committed yet, and *only* because Ray himself > has push rights, but he is currently away. So nobody has picked up the patch > yet. > > I suggest that, before we do anything else for MpInitLib, we commit Ray's > patch first. > > Eric, do you agree? > > If so, can you push the patch, or do you want me to do it? I'm glad to do it > if > you prefer. > Agree, just push Ray's patch: SHA-1: c563077a380437c114aba4c95be65eb963ebc1f3 Let's continue. > Thanks, > Laszlo > > > >> -Original Message- > >> From: Yao, Jiewen > >> Sent: Wednesday, July 11, 2018 3:45 PM > >> To: Dong, Eric ; Dong, Eric > >> ; Laszlo Ersek ; Fan Jeff > >> ; edk2-devel@lists.01.org > >> Cc: Ni, Ruiyu > >> Subject: RE: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > >> processor number performance. > >> > >> Hi > >> I believe using stack pointer is not a robust way if the stack guard > >> feature is not enabled. Stack pointer may overflow. > >> > >> Can we use GDT? Each AP has its own GDT. > >> > >> Thank you > >> Yao Jiewen > >> > >> > >>> -Original Message- > >>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf > >>> Of Dong, Eric > >>> Sent: Monday, July 9, 2018 2:13 PM > >>> To: Dong, Eric ; Laszlo Ersek > >>> ; Fan Jeff ; > >>> edk2-devel@lists.01.org > >>> Cc: Ni, Ruiyu > >>> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > >>> processor number performance. > >>> > >>> Hi Laszlo, > >>> > >>> I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 > >>> to request to add AsmReadEsp() / AsmReadRsp(). > >>> > >>> Thanks, > >>> Eric > >>> > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf > Of Dong, Eric > Sent: Monday, July 9, 2018 11:04 AM > To: Laszlo Ersek ; Fan Jeff > ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > processor number performance. > > Hi Laszlo, > > > -Original Message- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Thursday, July 5, 2018 9:04 PM > > To: Fan Jeff ; Dong, Eric > > ; edk2-devel@lists.01.org > > Cc: Ni, Ruiyu > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > > processor number performance. > > > > Hi Jeff, > > > > On 07/04/18 11:39, Fan Jeff wrote: > >> Eric, > >> > >> Current implementation does not call GetApicid() many times, > >> Please > > correct you commit message. Your fix is to improve the performance > > against the current implementation. > > > > I think the original commit message does make sense. Without the > > patch, > > GetProcessorNumber() may call GetApicId() up to > > TotalProcessorNumber times. With the patch, even if we skip the > > stack range search, > > GetProcessorNumber() will call GetApicId() just once. > > > > [...] > > > > Some more questions below, for the patch: > > > >> 发件人: Eric Dong > >> 发送时间: Wednesday, July 4, 2018 4:37:36 PM > >> 收件人: edk2-devel@lists.01.org > >> 抄送: Ruiyu Ni; Jeff Fan; Laszlo Ersek > >> 主题: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number > > performance. > >> > >> Current function has low performance because it calls GetApicId > >> many times. > >> > >> New logic first try to base on the stack range used by AP to find > >> the processor number. If this solution failed, then call > >> GetApicId once and base on this value to search the processor. > >> > >> Cc: Ruiyu Ni > >> Cc: Jeff Fan > >> Cc: Laszlo Ersek > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Eric Dong > >> --- > >> UefiCpuPkg/Library/MpInitLib/MpLib.c | 25 > >>>
[edk2] [Patch][edk2-platforms/devel-IntelAtomProcessorE3900] Change BIOS version
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Guo Mang --- Platform/BroxtonPlatformPkg/BiosId.env | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Platform/BroxtonPlatformPkg/BiosId.env b/Platform/BroxtonPlatformPkg/BiosId.env index ff1aa87..b9cb0fd 100644 --- a/Platform/BroxtonPlatformPkg/BiosId.env +++ b/Platform/BroxtonPlatformPkg/BiosId.env @@ -31,5 +31,5 @@ BOARD_ID = APLKRVP BOARD_REV = 3 BUILD_TYPE= D VERSION_MAJOR = 0070 -VERSION_MINOR = 02 +VERSION_MINOR = 03 BOARD_EXT = X64 -- 2.10.1.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch] ShellPkg/TftpDynamicCommand: Fix the potential assertion and memory leak issue.
From: Jiaxin Wu
This patch is to fix the issue reported from
https://bugzilla.tianocore.org/show_bug.cgi?id=925.
DataSize variable was not assigned the value if ShellOpenFileByName returns
error.
In the such a case, it should not be used to FreePages. Instead, DataSize can be
used to record the file size once DownloadFile successfully.
Cc: Ye Ting
Cc: Fu Siyuan
Cc: Jaben Carsey
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin
---
ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
index e2491cd54c..44be6d4e76 100644
--- a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
+++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
@@ -517,10 +517,12 @@ RunTftp (
mTftpHiiHandle, RemoteFilePath, NicName, Status
);
goto NextHandle;
}
+DataSize = FileSize;
+
if (!EFI_ERROR (ShellFileExists (LocalFilePath))) {
ShellDeleteFileByName (LocalFilePath);
}
Status = ShellOpenFileByName (
@@ -537,11 +539,10 @@ RunTftp (
mTftpHiiHandle, L"tftp", LocalFilePath
);
goto NextHandle;
}
-DataSize = FileSize;
Status = ShellWriteFile (FileHandle, , Data);
if (!EFI_ERROR (Status)) {
ShellStatus = SHELL_SUCCESS;
} else {
ShellPrintHiiEx (
--
2.17.1.windows.2
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 07/14] BaseTools: Use absolute import in BPDG
On Thu, Jul 12, 2018 at 12:52:32AM +, Zhu, Yonghong wrote: > Hi Gary, > > I got below error: > > Traceback (most recent call last): > File "C:\TCWork\Edk2\BaseTools\Source\Python\BPDG\BPDG.py", line 34, in > > from . import StringTable as st > ValueError: Attempted relative import in non-package > Hmmm, so BPDG also uses the module in its own directory. I'll modify BPDG in BinWrappers as GenFds. Thanks, Gary Lin > Best Regards, > Zhu Yonghong > > > -Original Message- > From: Gary Lin [mailto:g...@suse.com] > Sent: Tuesday, July 10, 2018 11:31 AM > To: edk2-devel@lists.01.org > Cc: Zhu, Yonghong ; Gao, Liming > Subject: [PATCH 07/14] BaseTools: Use absolute import in BPDG > > Based on "futurize -f libfuturize.fixes.fix_absolute_import > > Contributed-under: TianoCore Contribution Agreement 1.1 > Cc: Yonghong Zhu > Cc: Liming Gao > Signed-off-by: Gary Lin > --- > BaseTools/Source/Python/BPDG/BPDG.py | 5 +++-- > BaseTools/Source/Python/BPDG/GenVpd.py | 3 ++- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/BaseTools/Source/Python/BPDG/BPDG.py > b/BaseTools/Source/Python/BPDG/BPDG.py > index 07cee8976208..2ec1516c0a08 100644 > --- a/BaseTools/Source/Python/BPDG/BPDG.py > +++ b/BaseTools/Source/Python/BPDG/BPDG.py > @@ -21,6 +21,7 @@ > # Import Modules > # > from __future__ import print_function > +from __future__ import absolute_import > import Common.LongFilePathOs as os > import sys > import encodings.ascii > @@ -30,8 +31,8 @@ from Common import EdkLogger from Common.BuildToolError > import * from Common.BuildVersion import gBUILD_VERSION > > -import StringTable as st > -import GenVpd > +from . import StringTable as st > +from . import GenVpd > > PROJECT_NAME = st.LBL_BPDG_LONG_UNI > VERSION= (st.LBL_BPDG_VERSION + " Build " + gBUILD_VERSION) > diff --git a/BaseTools/Source/Python/BPDG/GenVpd.py > b/BaseTools/Source/Python/BPDG/GenVpd.py > index 2eefcc24905f..cd272a2d9a79 100644 > --- a/BaseTools/Source/Python/BPDG/GenVpd.py > +++ b/BaseTools/Source/Python/BPDG/GenVpd.py > @@ -13,9 +13,10 @@ > # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > # > > +from __future__ import absolute_import > import Common.LongFilePathOs as os > from io import BytesIO > -import StringTable as st > +from . import StringTable as st > import array > import re > from Common.LongFilePathSupport import OpenLongFilePath as open > -- > 2.18.0 > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] BaseTool: Fixed the incorrect cache key.
Reviewed-by: Liming Gao > -Original Message- > From: Feng, Bob C > Sent: Wednesday, July 11, 2018 12:19 AM > To: edk2-devel@lists.01.org > Cc: Feng, Bob C ; Gao, Liming > Subject: [PATCH] BaseTool: Fixed the incorrect cache key. > > From: "bob.c.f...@intel.com" > > This patch is to fix the incorrect cache key of > skip ModuleAutoGen cache. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Bob Feng > Cc: Liming Gao > --- > BaseTools/Source/Python/AutoGen/AutoGen.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py > b/BaseTools/Source/Python/AutoGen/AutoGen.py > index b27290989e..54c6b7330f 100644 > --- a/BaseTools/Source/Python/AutoGen/AutoGen.py > +++ b/BaseTools/Source/Python/AutoGen/AutoGen.py > @@ -4320,7 +4320,7 @@ class ModuleAutoGen(AutoGen): > # If any source file is newer than the module than we cannot skip > # > def CanSkip(self): > -if self.MetaFile in GlobalData.gSikpAutoGenCache: > +if self.MakeFileDir in GlobalData.gSikpAutoGenCache: > return True > if not os.path.exists(self.GetTimeStampPath()): > return False > @@ -4340,7 +4340,7 @@ class ModuleAutoGen(AutoGen): > ModuleAutoGen.TimeDict[source] = os.stat(source)[8] > if ModuleAutoGen.TimeDict[source] > DstTimeStamp: > return False > -GlobalData.gSikpAutoGenCache.add(self.MetaFile) > +GlobalData.gSikpAutoGenCache.add(self.MakeFileDir) > return True > > def GetTimeStampPath(self): > -- > 2.18.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 07/14] BaseTools: Use absolute import in BPDG
Hi Gary, I got below error: Traceback (most recent call last): File "C:\TCWork\Edk2\BaseTools\Source\Python\BPDG\BPDG.py", line 34, in from . import StringTable as st ValueError: Attempted relative import in non-package Best Regards, Zhu Yonghong -Original Message- From: Gary Lin [mailto:g...@suse.com] Sent: Tuesday, July 10, 2018 11:31 AM To: edk2-devel@lists.01.org Cc: Zhu, Yonghong ; Gao, Liming Subject: [PATCH 07/14] BaseTools: Use absolute import in BPDG Based on "futurize -f libfuturize.fixes.fix_absolute_import Contributed-under: TianoCore Contribution Agreement 1.1 Cc: Yonghong Zhu Cc: Liming Gao Signed-off-by: Gary Lin --- BaseTools/Source/Python/BPDG/BPDG.py | 5 +++-- BaseTools/Source/Python/BPDG/GenVpd.py | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/BaseTools/Source/Python/BPDG/BPDG.py b/BaseTools/Source/Python/BPDG/BPDG.py index 07cee8976208..2ec1516c0a08 100644 --- a/BaseTools/Source/Python/BPDG/BPDG.py +++ b/BaseTools/Source/Python/BPDG/BPDG.py @@ -21,6 +21,7 @@ # Import Modules # from __future__ import print_function +from __future__ import absolute_import import Common.LongFilePathOs as os import sys import encodings.ascii @@ -30,8 +31,8 @@ from Common import EdkLogger from Common.BuildToolError import * from Common.BuildVersion import gBUILD_VERSION -import StringTable as st -import GenVpd +from . import StringTable as st +from . import GenVpd PROJECT_NAME = st.LBL_BPDG_LONG_UNI VERSION= (st.LBL_BPDG_VERSION + " Build " + gBUILD_VERSION) diff --git a/BaseTools/Source/Python/BPDG/GenVpd.py b/BaseTools/Source/Python/BPDG/GenVpd.py index 2eefcc24905f..cd272a2d9a79 100644 --- a/BaseTools/Source/Python/BPDG/GenVpd.py +++ b/BaseTools/Source/Python/BPDG/GenVpd.py @@ -13,9 +13,10 @@ # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. # +from __future__ import absolute_import import Common.LongFilePathOs as os from io import BytesIO -import StringTable as st +from . import StringTable as st import array import re from Common.LongFilePathSupport import OpenLongFilePath as open -- 2.18.0 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] Organization of github.com/IntelFsp/FSP
Hi All, Recently a discussion came up on the coreboot mailing list regarding the organization of the github.com/IntelFsp/FSP repository. Currently the repository is organized with each platform on its own branch. This organization makes it difficult to set up a git submodule that pulls the FSP, since "n" submodules will be needed where n == the number of platforms. An alternate organization would be to put all packages in to the master branch: - ApolloLakeFspBinPkg - BayTrailFspBinPkg - BraswellFspBinPkg - BroadwellFspBinPkg - KabylakeFspBinPkg - SkylakeFspBinPkg - etc. This would make it easier to configure a git submodule, and it would also more closely match the organization of github.com/tianocore/edk2. The idea seems reasonable to me. Does anyone here have thoughts or input? With Best Regards, Nate ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] SecurityPkg: Fix assert when setting key from eMMC/SD/USB
On 07/11/18 18:06, Laszlo Ersek wrote:
> On 07/11/18 17:44, Roman Bacik wrote:
>> Hi Laszlo,
>>
>> Thank you very much for your review and help. I would prefer the option 2b.
>
> Great, thanks! Let's wait for the SecurityPkg maintainers then, to give
> their R-b's for your patch. Chao Zhang, Jiewen, can you please comment?
>
> From my side, dependent on the pending commit message and patch
> whitespace corrections (which I'm willing to implement myself, at push):
>
> Reviewed-by: Laszlo Ersek
Here's the TianoCore BZ that I plan to reference in the updated commit
message, as "further known problems":
https://bugzilla.tianocore.org/show_bug.cgi?id=1008
Thanks
Laszlo
>> Thanks,
>>
>> Roman
>>
>> On Wed, Jul 11, 2018 at 5:05 AM, Laszlo Ersek wrote:
>>
>>> Hi Roman,
>>>
>>> On 07/11/18 00:51, rba...@gmail.com wrote:
From: Roman Bacik
When secure boot is enabled, if one loads keys from a FAT formatted
eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu,
an assert in StrLen() occurs.
This is because the filename starts on odd address, which is not a uint16
aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
Cc: Chao Zhang
Cc: Jiewen Yao
Cc: Laszlo Ersek
Cc: Vladimir Olovyannikov
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik
---
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>>> | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> Thank you for sending a well-formed patch.
>>>
>>> I notice that you sent this email from , which is not
>>> the same as the Signed-off-by line. I realize you posted from
>>> for technical reasons, and it should be no problem.
>>>
>>> However, I *think* in such cases we usually request the following:
>>>
>>> - Using your broadcom.com email address, please respond to this patch
>>> (not my present email, but your original git posting), keeping full
>>> context, and just repeat your Signed-off-by line (referencing the
>>> broadcom address).
>>>
>>> I'm CC'ing Jordan and Ard for confirmation -- I believe this is what
>>> we've done in the past, in cases when submitters had to post their work
>>> from private addresses due to company email issues.
>>>
>>> Technical comments below:
>>>
diff --git
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
>>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
>>> SecureBootConfigFileExplorer.c
index 1b6f88804275..19b13a5569a6 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
>>> SecureBootConfigFileExplorer.c
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/
>>> SecureBootConfigFileExplorer.c
@@ -123,6 +123,8 @@ OpenFileByDevicePath(
EFI_FILE_PROTOCOL *Handle1;
EFI_FILE_PROTOCOL *Handle2;
EFI_HANDLE DeviceHandle;
+ CHAR16 *PathName;
+ UINTN PathLength;
if ((FilePath == NULL || FileHandle == NULL)) {
return EFI_INVALID_PARAMETER;
@@ -173,6 +175,11 @@ OpenFileByDevicePath(
//
Handle2 = Handle1;
Handle1 = NULL;
+PathLength = DevicePathNodeLength(*FilePath) -
>>> sizeof(EFI_DEVICE_PATH_PROTOCOL);
+PathName = AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH*)*
>>> FilePath)->PathName);
>>>
>>> (1) On both lines above, space characters are missing after:
>>> DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style.)
>>> I think we can fix this up for you when we push the patch. (I'm willing
>>> to help with that, but we need SecurityPkg maintainer review first.)
>>>
>>>
+if (PathName == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+}
>>>
>>> (2) I have now reviewed the original state of the function more
>>> carefully, and, while the above "return" branch introduces a leak
>>> *path*, it does not introduce a leak that doesn't already exist!
>>>
>>> In fact, the original function has multiple issues:
>>>
>>> - If the OpenVolume() call fails, "FileHandle" is set to NULL. That's
>>> useless; the intent is obviously to set (*FileHandle) to NULL.
>>>
>>> - At the top of the "while" loop body, "Handle1" stands for an open
>>> EFI_FILE_PROTOCOL. If the device path type check at the top of the loop
>>> body returns EFI_INVALID_PARAMETER, then it (a) performs the same
>>> useless assignment to "FileHandle" as described above, and (b) fails to
>>> close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES
>>> branch introduces no new leak, just a new path to the existent leak.
>>>
>>> - The OpenFileByDevicePath() function is duplicated in the following
>>> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
>>>
