[edk2-devel] [PATCH] BaseTools: Optimize GenerateByteArrayValue and CollectPlatformGuids APIs
During the Incremental build GenerateByteArrayValue used to generate the
ByteArrayValue even when there is no change in the PCD/VPDs. which is
time consuming API based on the number of PCD/VPDs and SKU IDs.
The optimization is that GenerateByteArrayValue is used to store the
PcdRecordList in a JSON file for each of the arch. and during the
Incremental build this API will check if there is any change in the PCD
/VPDs then rest of the flow remains the same. if there is no change then
it will return the provious build data.
Flow:
during the 1st build PcdRecordList.json is not exists, PcdRecordList
will be dumped to json file. and it will copy the output.txt as well.
Note: as the output.txt are different for different Arch, so it will be
stored in the Arch folder.
During the Incremental build check if there is any change in PCD/VPD.
if there is a change in VPD/PCD then recreate the PcdRecordList.json.
and rest of the flow remains same.
if there is no change in VPD/PCD read the output.txt and return the data
Cc: Yuwei Chen
Cc: Rebecca Cran
Cc: Liming Gao
Cc: Bob Feng
Cc: Amy Chan
Cc: Sai Chaganty
Cc: Digant H Solanki
Signed-off-by: Ashraf Ali S
---
.../Source/Python/AutoGen/WorkspaceAutoGen.py | 16 ++---
.../Source/Python/Workspace/DscBuildData.py | 72 +++
2 files changed, 64 insertions(+), 24 deletions(-)
diff --git a/BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
b/BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
index 160e3a3cd3..eec9280c8e 100644
--- a/BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
@@ -160,22 +160,18 @@ class WorkspaceAutoGen(AutoGen):
def CollectPlatformGuids(self):
oriInfList = []
-oriPkgSet = set()
-PlatformPkg = set()
+pkgSet = set()
for Arch in self.ArchList:
Platform = self.BuildDatabase[self.MetaFile, Arch,
self.BuildTarget, self.ToolChain]
oriInfList = Platform.Modules
for ModuleFile in oriInfList:
ModuleData = self.BuildDatabase[ModuleFile, Platform._Arch,
Platform._Target, Platform._Toolchain]
-oriPkgSet.update(ModuleData.Packages)
-for Pkg in oriPkgSet:
-Guids = Pkg.Guids
-GlobalData.gGuidDict.update(Guids)
+pkgSet.update(ModuleData.Packages)
if Platform.Packages:
-PlatformPkg.update(Platform.Packages)
-for Pkg in PlatformPkg:
-Guids = Pkg.Guids
-GlobalData.gGuidDict.update(Guids)
+pkgSet.update(Platform.Packages)
+for Pkg in pkgSet:
+Guids = Pkg.Guids
+GlobalData.gGuidDict.update(Guids)
@cached_property
def FdfProfile(self):
diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py
b/BaseTools/Source/Python/Workspace/DscBuildData.py
index 4768099343..740b8e22be 100644
--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
@@ -37,6 +37,8 @@ from functools import reduce
from Common.Misc import SaveFileOnChange
from Workspace.BuildClassObject import PlatformBuildClassObject, StructurePcd,
PcdClassObject, ModuleBuildClassObject
from collections import OrderedDict, defaultdict
+import json
+import shutil
def _IsFieldValueAnArray (Value):
Value = Value.strip()
@@ -56,6 +58,7 @@ def _IsFieldValueAnArray (Value):
PcdValueInitName = 'PcdValueInit'
PcdValueCommonName = 'PcdValueCommon'
+PcdRecordListName = 'PcdRecordList.json'
PcdMainCHeader = '''
/**
@@ -1599,7 +1602,7 @@ class DscBuildData(PlatformBuildClassObject):
S_pcd_set = DscBuildData.OverrideByComm(S_pcd_set)
# Create a tool to caculate structure pcd value
-Str_Pcd_Values = self.GenerateByteArrayValue(S_pcd_set)
+Str_Pcd_Values = self.GenerateByteArrayValue(S_pcd_set, RecordList)
if Str_Pcd_Values:
for (skuname, StoreName, PcdGuid, PcdName, PcdValue) in
Str_Pcd_Values:
@@ -2750,12 +2753,61 @@ class DscBuildData(PlatformBuildClassObject):
ccflags.add(item)
i +=1
return ccflags
-def GenerateByteArrayValue (self, StructuredPcds):
+
+def GetStructurePcdSet (self, OutputValueFile):
+if not os.path.isfile(OutputValueFile):
+EdkLogger.error("GetStructurePcdSet", FILE_NOT_FOUND, "Output.txt
doesn't exist", ExtraData=OutputValueFile)
+return []
+File = open (OutputValueFile, 'r')
+FileBuffer = File.readlines()
+File.close()
+
+#start update structure pcd final value
+StructurePcdSet = []
+for Pcd in FileBuffer:
+PcdValue = Pcd.split ('|')
+PcdInfo = PcdValue[0].split ('.')
+StructurePcdSet.append((PcdInfo[0], PcdInfo[1], PcdInfo[2],
PcdInfo[3], PcdValue[2].strip()))
+return StructurePcdSet
Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & TCBZ4118
Hi Jiewen, All EDK2 PR CI builds of OvmfPkg are broken due to this issue. Maybe we didn't have enough time to wait feedback and should fix the CI issue first. Regards, Yi -Original Message- From: devel@edk2.groups.io On Behalf Of Yao, Jiewen Sent: Tuesday, January 16, 2024 10:38 PM To: Gerd Hoffmann ; devel@edk2.groups.io Cc: dougfl...@microsoft.com; Douglas Flick [MSFT] Subject: Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & TCBZ4118 Sure. Let's start from OVMF. We have leaf enough time for feedback, but I see no comment from other people. > -Original Message- > From: Gerd Hoffmann > Sent: Tuesday, January 16, 2024 10:35 PM > To: devel@edk2.groups.io; Yao, Jiewen > Cc: dougfl...@microsoft.com; Douglas Flick [MSFT] > > Subject: Re: Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & > TCBZ4118 > > On Tue, Jan 16, 2024 at 01:30:43PM +, Yao, Jiewen wrote: > > Gerd > > I have merged this patch set today. > > > > I am fine to remove TPM1.2 in OVMF because of the known security limitation. > > I was thinking about the complete edk2 code base not only OVMF. > > But I can surely start with OVMF. Maybe it is the only platform > affected because on physical hardware you usually know whenever TPM > 1.2 or TPM 2.0 is present so there is no need to include both. > > take care, > Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113933): https://edk2.groups.io/g/devel/message/113933 Mute This Topic: https://groups.io/mt/103675434/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver
Laszlo, Michael, When timer interrupt happens, the calling flow is: [Timer Interrupt #1] CPU IDT handler calls into LocalApicTimerDxe::TimerInterruptHandler(), which [Timer Interrupt #1]1. RaiseTPL (HIGH) from APPLICATION causing CPU interrupt be disabled. [Timer Interrupt #1]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts) [Timer Interrupt #1]3. Call DxeCore::CoreTimerTick() [Timer Interrupt #1]4. RestoreTPL (APPLICATION) from HIGH. (All callbacks registered at NOTIFY and CALLBACK will run.) [Timer Interrupt #1]4.1. When there are Callbacks registered at NOTIFY, current TPL is set to NOTIFY and interrupt is enabled. CoreDispatchEventNotifies() is called to run the NOTIFY callbacks. [Timer Interrupt #2] Immediately after interrupt is enabled, CPU runs to LocalApicTimerDxe::TimerInterruptHandler(). But stack is not fully popped to the initial state. [Timer Interrupt #2]1. RaiseTPL (HIGH) from NOTIFY causing CPU interrupt be disabled. [Timer Interrupt #2]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts) [Timer Interrupt #2]3. Call DxeCore::CoreTimerTick() [Timer Interrupt #2]4. RestoreTPL (NOTIFY) from HIGH. No callback runs as no callback can be registered at TPL > NOTIFY. In the end of RestoreTPL(), CPU interrupt is enabled. [Timer Interrupt #3] Immediately after interrupt is enabled, CPU runs to LocalApicTimerDxe::TimerInterruptHandler(). But stack is not fully popped to the initial state. [Timer Interrupt #3]1. RaiseTPL (HIGH) from NOTIFY causing CPU interrupt be disabled. [Timer Interrupt #3]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts) [Timer Interrupt #3]3. Call DxeCore::CoreTimerTick() [Timer Interrupt #3]4. RestoreTPL (NOTIFY) from HIGH. No callback runs as no callback can be registered at TPL > NOTIFY. In the end of RestoreTPL(), CPU interrupt is enabled. [Timer Interrupt #4] Immediately after interrupt is enabled, CPU runs to LocalApicTimerDxe::TimerInterruptHandler(). But stack is not fully popped to the initial state. [Timer Interrupt #4]... The above flow shows endless re-entrance of timer interrupt handler. But, my question is: above flow only can happen in real platform when the below 4 steps occupies more time than the timer period (usually 10ms). [Timer Interrupt #2]1. RaiseTPL (HIGH) from NOTIFY causing CPU interrupt be disabled. [Timer Interrupt #2]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts) [Timer Interrupt #2]3. Call DxeCore::CoreTimerTick() [Timer Interrupt #2]4. RestoreTPL (NOTIFY) from HIGH. No callback runs as no callback can be registered at TPL > NOTIFY. In the end of RestoreTPL(), CPU interrupt is enabled. But, in my opinion, it's impossible. Thanks, Ray > -Original Message- > From: Laszlo Ersek > Sent: Tuesday, January 16, 2024 11:37 PM > To: devel@edk2.groups.io; mc...@ipxe.org; kra...@redhat.com > Cc: Pedro Falcato ; Ni, Ray ; > Kinney, Michael D ; Desimone, Nathaniel L > ; Kumar, Rahul R > > Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: > Duplicate OvmfPkg/LocalApicTimerDxe driver > > On 1/16/24 16:16, Michael Brown wrote: > > On 16/01/2024 14:34, Laszlo Ersek wrote: > >> On 1/16/24 10:48, Michael Brown wrote: > >> IOW, my impression is that NestedInterruptTplLib can certainly handle > >> all scenarios thrown at it, but where it really matters is in the face > >> of an interrupt storm (not just "normal nesting"), and a storm is > >> unlikely (or even impossible?) on physical hardware. > >> > >> ... Oh, scratch that. "Interrupt storm" simply means that interrupts are > >> being delivered at a rate higher than the handler routine can service > >> them. IOW, the "storm" is not that interrupts are delivered *very > >> rapidly* in an absoulte sense. If interrupts are delivered at normal > >> frequency, but the handler is too slow to service *even that rate*, then > >> that also qualifies as "storm", because the nesting depth will *keep > >> growing*. It's not really the growth rate that matters; what matter is > >> the *trend*, i.e., the fact that there *is* growth (the stack gets > >> deeper and deeper). The stack might not overflow immediately, and if the > >> handler speeds up (for whatever reason), the stack might recover, but > >> there is nothing to prevent an overflow. > >> > >> So, in the end, I think you've convinced me. > > > > :) > > > >>> I'm happy to send a patch to migrate NestedInterruptTplLib to > >>> MdeModulePkg, so that it can be consumed outside of OvmfPkg. Shall I > do > >>> this? > >> > >> Sounds like a valid idea to me. > >> > >> Could be greatly supported by a test case (to be run on
[edk2-devel] [PATCH 2/2] MdeModulePkg: Optimize CoreConnectSingleController
CoreConnectSingleController() searches for the Driver Family Override
Protocol drivers by looping and checking each Driver Binding Handles.
This loop can be skipped by checking if any Driver Family Override
Protocol installed in the platform first, to improve the performance.
Cc: Liming Gao
Cc: Ray Ni
Signed-off-by: Zhi Jin
---
MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index 0b824c62b7..64d7474f15 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -497,7 +497,12 @@ CoreConnectSingleController (
//
// Add the Driver Family Override Protocol drivers for ControllerHandle
//
- while (TRUE) {
+ Status = CoreLocateProtocol (
+ ,
+ NULL,
+ (VOID **)
+ );
+ while (!EFI_ERROR (Status) && (DriverFamilyOverride != NULL)) {
HighestIndex = DriverBindingHandleCount;
HighestVersion = 0;
for (Index = 0; Index < DriverBindingHandleCount; Index++) {
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113931): https://edk2.groups.io/g/devel/message/113931
Mute This Topic: https://groups.io/mt/103781274/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH 1/2] MdeModulePkg: Remove the handle validation check in CoreGetProtocolInterface
CoreGetProtocolInterface() is called by CoreOpenProtocol(),
CoreCloseProtocol() and CoreOpenProtocolInformation().
Before CoreOpenProtocol() calls CoreGetProtocolInterface(), the input
parameter UserHandle has been already checked for validation. So does
CoreCloseProtocol().
Removing the handle validation check in CoreGetProtocolInterface()
could improve the performance, as CoreOpenProtocol() is called very
frequently.
Meanwhile, need to make it the caller's responsibility to check the
parameters, and add the check in CoreOpenProtocolInformation().
Cc: Liming Gao
Cc: Ray Ni
Signed-off-by: Zhi Jin
---
MdeModulePkg/Core/Dxe/Hand/Handle.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 51e5b5d3b3..a0d2d03267 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -916,6 +916,8 @@ CoreUninstallMultipleProtocolInterfaces (
/**
Locate a certain GUID protocol interface in a Handle's protocols.
+ Note: This function doesn't do parameters checking, it's caller's
responsibility
+ to pass in valid parameters.
@param UserHandle The handle to obtain the protocol interface on
@param Protocol The GUID of the protocol
@@ -929,17 +931,11 @@ CoreGetProtocolInterface (
IN EFI_GUID*Protocol
)
{
- EFI_STATUS Status;
PROTOCOL_ENTRY *ProtEntry;
PROTOCOL_INTERFACE *Prot;
IHANDLE *Handle;
LIST_ENTRY *Link;
- Status = CoreValidateHandle (UserHandle);
- if (EFI_ERROR (Status)) {
-return NULL;
- }
-
Handle = (IHANDLE *)UserHandle;
//
@@ -1392,6 +1388,15 @@ CoreOpenProtocolInformation (
//
CoreAcquireProtocolLock ();
+ //
+ // Check for invalid UserHandle
+ //
+ Status = CoreValidateHandle (UserHandle);
+ if (EFI_ERROR (Status)) {
+Status = EFI_NOT_FOUND;
+goto Done;
+ }
+
//
// Look at each protocol interface for a match
//
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113930): https://edk2.groups.io/g/devel/message/113930
Mute This Topic: https://groups.io/mt/103781273/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2 2/2] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent
The purpose of writing CR3 in ConvertMemoryPageToNotPresent is just to flush TLB, because CR3 won't be changed in function ConvertMemoryPageToNotPresent. After ConvertMemoryPageToNotPresent, there is always a flush TLB function. Also, because ConvertMemoryPageToNotPresent in called in a loop, to improve performance, there is no need to flush TLB inside ConvertMemoryPageToNotPresent. Just flushing TLB after the loop is enough. Cc: Ray Ni Cc: Laszlo Ersek Cc: Rahul Kumar Cc: Gerd Hoffmann Signed-off-by: Zhiguang Liu --- UefiCpuPkg/CpuMpPei/CpuPaging.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c index 15c7015fb8..b923d9b502 100644 --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c @@ -76,7 +76,8 @@ AllocatePageTableMemory ( /** This function modifies the page attributes for the memory region specified - by BaseAddress and Length to not present. + by BaseAddress and Length to not present. This function only change page + table, but not flush TLB. Caller have the responsbility to flush TLB. Caller should make sure BaseAddress and Length is at page boundary. @@ -167,7 +168,6 @@ ConvertMemoryPageToNotPresent ( } ASSERT_EFI_ERROR (Status); - AsmWriteCr3 (PageTable); return Status; } -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113929): https://edk2.groups.io/g/devel/message/113929 Mute This Topic: https://groups.io/mt/103781133/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2 1/2] UefiCpuPkg/CpuPageTableLib: Enhance function header for PageTableMap()
PageTableMap() only modifies the PageTable root pointer when creating from zero.
Explicitly explain it in function header.
Cc: Ray Ni
Cc: Laszlo Ersek
Cc: Rahul Kumar
Cc: Gerd Hoffmann
Signed-off-by: Zhiguang Liu
---
UefiCpuPkg/Include/Library/CpuPageTableLib.h | 1 +
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index 78aa83b8de..755c8ab084 100644
--- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
+++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
@@ -61,6 +61,7 @@ typedef enum {
Create or update page table to map [LinearAddress, LinearAddress + Length)
with specified attribute.
@param[in, out] PageTable The pointer to the page table to update, or
pointer to NULL if a new page table is to be created.
+ If not pointer to NULL, the value it points
to won't be changed in this function.
@param[in] PagingMode The paging mode.
@param[in] Buffer The free buffer to be used for page table
creation/updating.
@param[in, out] BufferSize The buffer size.
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 36b2c4e6a3..25bd9fc8d8 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -620,6 +620,7 @@ PageTableLibMapInLevel (
Create or update page table to map [LinearAddress, LinearAddress + Length)
with specified attribute.
@param[in, out] PageTable The pointer to the page table to update, or
pointer to NULL if a new page table is to be created.
+ If not pointer to NULL, the value it points
to won't be changed in this function.
@param[in] PagingMode The paging mode.
@param[in] Buffer The free buffer to be used for page table
creation/updating.
@param[in, out] BufferSize The buffer size.
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113928): https://edk2.groups.io/g/devel/message/113928
Mute This Topic: https://groups.io/mt/103781121/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Recall: [edk2-devel] [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError Feature
Zhang, Zifeng would like to recall the message, "[edk2-devel] [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError Feature". -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113927): https://edk2.groups.io/g/devel/message/113927 Mute This Topic: https://groups.io/mt/103780703/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError Feature
Hi Gua, Arthur. Do you have opens for this solution suggested from Liming? Jira: https://jira.devtools.intel.com/browse/CRJ-23954 Best Regards, Zifeng -Original Message- From: gaoliming Sent: Tuesday, January 16, 2024 11:04 PM To: devel@edk2.groups.io; Zhang, Zifeng ; Yang, Yuting2 Cc: Chen, Christine Subject: 回复: [edk2-devel] [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError Feature This option is for the module check. I suggest to set this option in INF instead of DSC. If so, DSC is not required to configure the additional options. And, this module will have the same build result when it is specified in the different platform DSC files. I understand your requirement to enable this option for the modules in some specific package. I think you can easily find those modules those have VFR/HFR, then update their INFs with new check option. Thanks Liming > -邮件原件- > 发件人: devel@edk2.groups.io 代表 Zhang, Zifeng > 发送时间: 2024年1月12日 13:20 > 收件人: Yang, Yuting2 ; Gao, Liming > > 抄送: Chen, Christine ; devel@edk2.groups.io > 主题: Re: [edk2-devel] [Patch V2] BaseTools: VfrCompiler Adds > DefaultValueError Feature > > Hi Liming, > > Could you help to share the update for this patch solution? > > Best Regards, > Zifeng > > -Original Message- > From: Yang, Yuting2 > Sent: Monday, December 25, 2023 3:10 PM > To: Gao, Liming > Cc: Zhang, Zifeng ; Chen, Christine > ; devel@edk2.groups.io > Subject: RE: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > Feature > > Hi Liming, > > Thank you for adding the account for me. > I have created a Bugzilla > https://bugzilla.tianocore.org/show_bug.cgi?id=4629 > for this feature ~ > > Best Regards,, > Yuting > > -Original Message- > From: gaoliming > Sent: Monday, December 25, 2023 9:14 AM > To: Yang, Yuting2 > Subject: 回复: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > Feature > > Your account yuting2.y...@intel.com has been added. PW: tiano@123 > > > -邮件原件- > > 发件人: Yang, Yuting2 > > 发送时间: 2023年12月22日 13:41 > > 收件人: Gao, Liming > > 抄送: Rebecca Cran ; Feng, Bob C > > ; Chen, Arthur G ; > > Chen, Christine ; Zhang, Zifeng > > ; devel@edk2.groups.io > > 主题: RE: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > > Feature > > > > Hi Liming, > > > > Thank you for reviewing ~ > > Could you please help me create a Bugzilla account? Currently, I do > > not > have > > access to the Bugzilla. > > > > Best Regards, > > Yuting > > > > -Original Message- > > From: Zhang, Zifeng > > Sent: Thursday, December 21, 2023 2:44 PM > > To: Gao, Liming ; Yang, Yuting2 > > > > Cc: Rebecca Cran ; Feng, Bob C > > ; Chen, Arthur G ; > > devel@edk2.groups.io; Chen, Christine > > Subject: RE: [Patch V2] BaseTools: VfrCompiler Adds > > DefaultValueError Feature > > > > Hi Liming, > > > > Thanks for reviewing. > > For background of this change, we will remove default flags in > > VFR/HFR in new platform. So we need help from VFR complier to make a > > default flag check to avoid manually adding. > > @Yang, Yuting2, could you help to create a BZ for this feature and > > share > in > > mail thread? > > Then let me make a clarification for your questions. > > > > #1: The purpose of --catch_default > > We send --catch_default flag in build option to indicate which > > platform > should > > check default flag in VFR/HFR. > > Actually maybe some platforms used same EDK2 downstream branch, so > > we only send --catch_default flag for the platforms which need this check. > > > > #2: The purpose of --except_list > > VFR compiler will receive VFR/HFR configurations from all folders > including > > Intel and EDK2. But in our expectation VFR compiler only do this > > check in Intel. > > So We use --except_list to deliver package list in EDK2 to avoid > > this > check. > > > > Best Regards, > > Zifeng > > > > -Original Message- > > From: Yang, Yuting2 > > Sent: Tuesday, December 12, 2023 5:12 PM > > To: Zhang, Zifeng ; Chen, Arthur G > > ; devel@edk2.groups.io > > Cc: Rebecca Cran ; Gao, Liming > > ; Feng, Bob C > > Subject: RE: [Patch V2] BaseTools: VfrCompiler Adds > > DefaultValueError Feature > > > > +Cc Zhang, Zifeng, Chen, Arthur G > > > > -Original Message- > > From: Chen, Christine > > Sent: Tuesday, December 12, 2023 5:04 PM > > To: Yang, Yuting2 ; devel@edk2.groups.io > > Cc: Rebecca Cran ; Gao, Liming > > ; Feng, Bob C > > Subject: RE: [Patch V2] BaseTools: VfrCompiler Adds > > DefaultValueError Feature > > > > +Cc Yang, Yuting2 > > > > > -Original Message- > > > From: Yang, Yuting2 > > > Sent: Tuesday, December 12, 2023 5:01 PM > > > To: devel@edk2.groups.io > > > Cc: Rebecca Cran ; Gao, Liming > > > ; Feng, Bob C ; > > > Chen, Christine > > > Subject: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > > > Feature > > > > > > Add --catch_default option > > > Raise a DefaultValueError when encountering VFR default > > >
Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/BaseXApicX2ApicLib: Implements AMD extended cpu topology
Can you remove "EFIAPI" from the two Amd* local functions?
The two are *local* functions called from the accordingly public LIB APIs.
Thanks,
Ray
> -Original Message-
> From: Abdul Lateef Attar
> Sent: Tuesday, January 16, 2024 3:01 PM
> To: devel@edk2.groups.io
> Cc: Abdul Lateef Attar ; Ni, Ray
> ; Kumar, Rahul R ; Gerd
> Hoffmann
> Subject: [PATCH v1 2/2] UefiCpuPkg/BaseXApicX2ApicLib: Implements AMD
> extended cpu topology
>
> From: Abdul Lateef Attar
>
> This patch adds support for AMD's new extended topology.
> If processor supports CPUID 8026 leaf then obtain
> the topology information using new method.
>
> Algorithm:
> if CPUID is AMD:
> then
> check for AMD's extended cpu tology leaf.
> if yes
>then extract cpu tology based on
>AMD programmer manual's instruction.
> else
>then fallback to existing topology function.
> endif
> endif
>
> Cc: Ray Ni
> Cc: Rahul Kumar
> Cc: Gerd Hoffmann
> Signed-off-by: Abdul Lateef Attar
> ---
> .../Library/BaseXApicLib/BaseXApicLib.c | 122
> +-
> .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 122
> +-
> 2 files changed, 242 insertions(+), 2 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> index efb9d71ca1..5e941d0dc8 100644
> --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> @@ -4,7 +4,7 @@
>This local APIC library instance supports xAPIC mode only.
>
>Copyright (c) 2010 - 2023, Intel Corporation. All rights reserved.
> - Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.
> + Copyright (c) 2017 - 2024, AMD Inc. All rights reserved.
>
>SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -1157,6 +1157,121 @@ GetProcessorLocationByApicId (
>}
> }
>
> +/**
> + Get Package ID/Die ID/Module ID/Core ID/Thread ID of a AMD processor
> family.
> +
> + The algorithm assumes the target system has symmetry across physical
> + package boundaries with respect to the number of threads per core,
> number of
> + cores per module, number of modules per die, number
> + of dies per package.
> +
> + @param[in] InitialApicId Initial APIC ID of the target logical processor.
> + @param[out] Package Returns the processor package ID.
> + @param[out] Die Returns the processor die ID.
> + @param[out] Tile Returns zero.
> + @param[out] ModuleReturns the processor module ID.
> + @param[out] Core Returns the processor core ID.
> + @param[out] ThreadReturns the processor thread ID.
> +**/
> +VOID
> +EFIAPI
> +AmdGetProcessorLocation2ByApicId (
> + IN UINT32 InitialApicId,
> + OUT UINT32 *Package OPTIONAL,
> + OUT UINT32 *Die OPTIONAL,
> + OUT UINT32 *Tile OPTIONAL,
> + OUT UINT32 *Module OPTIONAL,
> + OUT UINT32 *Core OPTIONAL,
> + OUT UINT32 *Thread OPTIONAL
> + )
> +{
> + CPUID_EXTENDED_TOPOLOGY_EAX ExtendedTopologyEax;
> + CPUID_EXTENDED_TOPOLOGY_EBX ExtendedTopologyEbx;
> + CPUID_EXTENDED_TOPOLOGY_ECX ExtendedTopologyEcx;
> + UINT32 MaxExtendedCpuIdIndex;
> + UINT32 SubIndex;
> + UINT32 PreviousLevel;
> + UINT32 Data;
> +
> + if (Die != NULL) {
> +*Die = 0;
> + }
> +
> + if (Tile != NULL) {
> +*Tile = 0;
> + }
> +
> + if (Module != NULL) {
> +*Module = 0;
> + }
> +
> + /// Check if extended toplogy supported
> + AsmCpuid (CPUID_EXTENDED_FUNCTION, ,
> NULL, NULL, NULL);
> + if (MaxExtendedCpuIdIndex < AMD_CPUID_EXTENDED_TOPOLOGY) {
> +GetProcessorLocationByApicId (InitialApicId, Package, Core, Thread);
> +return;
> + }
> +
> + PreviousLevel = 0;
> + SubIndex = 0;
> + do {
> +AsmCpuidEx (
> + AMD_CPUID_EXTENDED_TOPOLOGY,
> + SubIndex,
> + ,
> + ,
> + ,
> + NULL
> + );
> +
> +if (ExtendedTopologyEbx.Bits.LogicalProcessors ==
> CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_INVALID) {
> + break;
> +}
> +
> +Data = InitialApicId >> PreviousLevel;
> +Data &= (1 << (ExtendedTopologyEax.Bits.ApicIdShift - PreviousLevel)) -
> 1;
> +
> +switch (ExtendedTopologyEcx.Bits.LevelType) {
> + case CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_SMT:
> +if (Thread != NULL) {
> + *Thread = Data;
> +}
> +
> +break;
> + case CPUID_EXTENDED_TOPOLOGY_LEVEL_TYPE_CORE:
> +if (Core != NULL) {
> + *Core = Data;
> +}
> +
> +break;
> + case CPUID_V2_EXTENDED_TOPOLOGY_LEVEL_TYPE_MODULE:
> +if (Module != NULL) {
> + *Module = Data;
> +}
> +
> +break;
> + case CPUID_V2_EXTENDED_TOPOLOGY_LEVEL_TYPE_TILE:
> +if (Die != NULL) {
> + *Die = Data;
> +}
> +
> +break;
> + default:
> +break;
> +}
> +
> +
Re: [edk2-devel] [PATCH 0/2] OvmfPkg: drop support for TPM 1.2
Gerd I am OK with the patch. Quick question: Have you validated that the TPM2 is still working? > -Original Message- > From: devel@edk2.groups.io On Behalf Of Gerd > Hoffmann > Sent: Tuesday, January 16, 2024 11:42 PM > To: devel@edk2.groups.io > Cc: Oliver Steffen ; Gerd Hoffmann > Subject: [edk2-devel] [PATCH 0/2] OvmfPkg: drop support for TPM 1.2 > > > > Gerd Hoffmann (2): > OvmfPkg: remove TPM1_ENABLE build option > OvmfPkg/Tcg2Config: remove unused TPM 1.2 support > > .../Include/Dsc/OvmfTpmComponentsDxe.dsc.inc | 6 -- > .../Include/Dsc/OvmfTpmComponentsPei.dsc.inc | 5 -- > OvmfPkg/Include/Dsc/OvmfTpmDefines.dsc.inc| 3 - > OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc | 9 -- > .../Include/Dsc/OvmfTpmSecurityStub.dsc.inc | 6 -- > OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf | 56 - > OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c | 83 --- > OvmfPkg/Include/Fdf/OvmfTpmDxe.fdf.inc| 3 - > OvmfPkg/Include/Fdf/OvmfTpmPei.fdf.inc| 5 -- > OvmfPkg/PlatformCI/ReadMe.md | 2 +- > 10 files changed, 1 insertion(+), 177 deletions(-) > delete mode 100644 OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf > delete mode 100644 OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c > > -- > 2.43.0 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113924): https://edk2.groups.io/g/devel/message/113924 Mute This Topic: https://groups.io/mt/103764204/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH edk2-platforms 1/1] Platform/SbsaQemu: update doc a bit
We emulate XHCI controller already. No need to add it. Signed-off-by: Marcin Juszkiewicz --- Platform/Qemu/SbsaQemu/Readme.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Platform/Qemu/SbsaQemu/Readme.md b/Platform/Qemu/SbsaQemu/Readme.md index 3355adebd4c6..883535dfd20e 100644 --- a/Platform/Qemu/SbsaQemu/Readme.md +++ b/Platform/Qemu/SbsaQemu/Readme.md @@ -113,9 +113,9 @@ Create a directory $WORKSPACE that would hold source code of the components. ``` $INSTALL_PATH/qemu-system-aarch64 -m 1024 -M sbsa-ref -pflash SBSA_FLASH0.fd -pflash SBSA_FLASH1.fd -serial stdio ``` - You can add XHCI controller with keyboard and mouse by: + You can keyboard and mouse by: ``` - -device qemu-xhci -device usb-mouse -device usb-kbd + -device usb-mouse -device usb-kbd ``` You can add the hard drive to platform AHCI controller by `hda` parameter: ``` -- 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113923): https://edk2.groups.io/g/devel/message/113923 Mute This Topic: https://groups.io/mt/103768126/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver
Unit tests for the math calculations would help with reviews too. Mike > -Original Message- > From: Laszlo Ersek > Sent: Tuesday, January 16, 2024 2:03 AM > To: Kinney, Michael D ; Pedro Falcato > ; devel@edk2.groups.io; Ni, Ray > > Cc: Desimone, Nathaniel L ; Kumar, Rahul > R ; Gerd Hoffmann > Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: > Duplicate OvmfPkg/LocalApicTimerDxe driver > > On 1/15/24 19:10, Kinney, Michael D wrote: > > Hi Ray, > > > > I think nesting may be possible in physical platforms, but very hard > > to induce. > > > > One option is to consolidate to a single LocalApicTimerDxe > > implementation in the UefiCpuPkg, but allow the platform DSC to either > > specify a Null NestedInterruptTplLib for physical platforms or the > > full one from the OvmfPkg for VM use cases. > > > > The other changes could be included for OvmfPkg use cases. If the VM > > does not support the CPUID leafs, then the PCD value should be used. > > All of this sounds great! > > (And I do think that *some* nesting should not be a problem, on either > physical or virtual platforms, as (IIRC) the interrupt handler stack can > accommodate a certain level of reentrancy. It's the "storm" that is a > problem, but that should never occur on physical machines, I reckon.) > > Assuming a v2 is coming up, my advance request would be for Ray to > re-review the math in patch #4, before posting v2, focusing on integer > overflows, and SafeIntLib (if needed). I've not looked at the patch in > detail yet, but I reviewed something similar not so long ago [1]. The > latter frequency calculation code had been quite commonly used in edk2, > and I needed hours to review just that one patch. Plus, the review > turned up a number of issues to fix. So, preferably such a patch should > not only prevent any integer overflows, but also clarify, in comments, > why overflows are impossible, and/or how they are prevented. > > [1] https://edk2.groups.io/g/devel/message/111613 > http://mid.mail-archive.com/2e42db7c-a927-f47b-7d80- > 632895b11...@redhat.com > > Thanks! > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113922): https://edk2.groups.io/g/devel/message/113922 Mute This Topic: https://groups.io/mt/103734961/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3 1/6] OvmfPkg/VirtNorFlashDxe: add casts to UINTN and UINT32
This is needed to avoid bit operations being applied to signed integers.
Suggested-by: László Érsek
Signed-off-by: Gerd Hoffmann
Reviewed-by: Laszlo Ersek
---
OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h | 2 +-
OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
index b7f5d208b236..455eafacc2cf 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h
@@ -61,7 +61,7 @@
#define P30_MAX_BUFFER_SIZE_IN_BYTES ((UINTN)128)
#define P30_MAX_BUFFER_SIZE_IN_WORDS (P30_MAX_BUFFER_SIZE_IN_BYTES/((UINTN)4))
#define MAX_BUFFERED_PROG_ITERATIONS 1000
-#define BOUNDARY_OF_32_WORDS 0x7F
+#define BOUNDARY_OF_32_WORDS ((UINTN)0x7F)
// CFI Addresses
#define P30_CFI_ADDR_QUERY_UNIQUE_QRY 0x10
diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 1afd60ce66eb..7f4743b00399 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -581,7 +581,7 @@ NorFlashWriteSingleBlock (
// contents, while checking whether the old version had any bits cleared
// that we want to set. In that case, we will need to erase the block
first.
for (CurOffset = 0; CurOffset < *NumBytes; CurOffset++) {
- if (~OrigData[CurOffset] & Buffer[CurOffset]) {
+ if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
goto DoErase;
}
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113917): https://edk2.groups.io/g/devel/message/113917
Mute This Topic: https://groups.io/mt/103766775/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3 3/6] OvmfPkg/VirtNorFlashDxe: add a loop for NorFlashWriteBuffer calls.
Replace the two NorFlashWriteBuffer() calls with a loop containing a
single NorFlashWriteBuffer() call.
With the changes in place the code is able to handle updates larger
than two P30_MAX_BUFFER_SIZE_IN_BYTES blocks, even though the patch
does not actually change the size limit.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Laszlo Ersek
---
OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 21 -
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 88a4d2c23fc0..3d1343b381bc 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -521,6 +521,7 @@ NorFlashWriteSingleBlock (
UINTN BlockAddress;
UINT8 *OrigData;
UINTN Start, End;
+ UINT32 Index, Count;
DEBUG ((DEBUG_BLKIO, "NorFlashWriteSingleBlock(Parameters: Lba=%ld,
Offset=0x%x, *NumBytes=0x%x, Buffer @ 0x%08x)\n", Lba, Offset, *NumBytes,
Buffer));
@@ -621,23 +622,17 @@ NorFlashWriteSingleBlock (
goto Exit;
}
-Status = NorFlashWriteBuffer (
- Instance,
- BlockAddress + Start,
- P30_MAX_BUFFER_SIZE_IN_BYTES,
- Instance->ShadowBuffer
- );
-if (EFI_ERROR (Status)) {
- goto Exit;
-}
-
-if ((End - Start) > P30_MAX_BUFFER_SIZE_IN_BYTES) {
+Count = (End - Start) / P30_MAX_BUFFER_SIZE_IN_BYTES;
+for (Index = 0; Index < Count; Index++) {
Status = NorFlashWriteBuffer (
Instance,
- BlockAddress + Start + P30_MAX_BUFFER_SIZE_IN_BYTES,
+ BlockAddress + Start + Index * P30_MAX_BUFFER_SIZE_IN_BYTES,
P30_MAX_BUFFER_SIZE_IN_BYTES,
- Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BYTES
+ Instance->ShadowBuffer + Index * P30_MAX_BUFFER_SIZE_IN_BYTES
);
+ if (EFI_ERROR (Status)) {
+goto Exit;
+ }
}
Exit:
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113919): https://edk2.groups.io/g/devel/message/113919
Mute This Topic: https://groups.io/mt/103766777/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3 6/6] OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function
Move the DoErase code block into a separate function, call the function
instead of jumping around with goto.
Signed-off-by: Gerd Hoffmann
---
OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 76 ++
1 file changed, 52 insertions(+), 24 deletions(-)
diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 3d1d20daa1e5..e6aaed27ceba 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -502,6 +502,38 @@ NorFlashRead (
return EFI_SUCCESS;
}
+STATIC
+EFI_STATUS
+NorFlashWriteSingleBlockWithErase (
+ INNOR_FLASH_INSTANCE *Instance,
+ INEFI_LBA Lba,
+ INUINTN Offset,
+ IN OUTUINTN *NumBytes,
+ INUINT8 *Buffer
+ )
+{
+ EFI_STATUS Status;
+
+ // Read NOR Flash data into shadow buffer
+ Status = NorFlashReadBlocks (Instance, Lba, Instance->BlockSize,
Instance->ShadowBuffer);
+ if (EFI_ERROR (Status)) {
+// Return one of the pre-approved error statuses
+return EFI_DEVICE_ERROR;
+ }
+
+ // Put the data at the appropriate location inside the buffer area
+ CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer,
*NumBytes);
+
+ // Write the modified buffer back to the NorFlash
+ Status = NorFlashWriteBlocks (Instance, Lba, Instance->BlockSize,
Instance->ShadowBuffer);
+ if (EFI_ERROR (Status)) {
+// Return one of the pre-approved error statuses
+return EFI_DEVICE_ERROR;
+ }
+
+ return EFI_SUCCESS;
+}
+
/*
Write a full or portion of a block. It must not span block boundaries; that
is,
Offset + *NumBytes <= Instance->BlockSize.
@@ -607,7 +639,14 @@ NorFlashWriteSingleBlock (
// that we want to set. In that case, we will need to erase the block
first.
for (CurOffset = 0; CurOffset < *NumBytes; CurOffset++) {
if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
-goto DoErase;
+Status = NorFlashWriteSingleBlockWithErase (
+ Instance,
+ Lba,
+ Offset,
+ NumBytes,
+ Buffer
+ );
+return Status;
}
OrigData[CurOffset] = Buffer[CurOffset];
@@ -636,33 +675,22 @@ NorFlashWriteSingleBlock (
goto Exit;
}
}
-
-Exit:
-// Put device back into Read Array mode
-SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
-
+ } else {
+Status = NorFlashWriteSingleBlockWithErase (
+ Instance,
+ Lba,
+ Offset,
+ NumBytes,
+ Buffer
+ );
return Status;
}
-DoErase:
- // Read NOR Flash data into shadow buffer
- Status = NorFlashReadBlocks (Instance, Lba, BlockSize,
Instance->ShadowBuffer);
- if (EFI_ERROR (Status)) {
-// Return one of the pre-approved error statuses
-return EFI_DEVICE_ERROR;
- }
+Exit:
+ // Put device back into Read Array mode
+ SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
- // Put the data at the appropriate location inside the buffer area
- CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer,
*NumBytes);
-
- // Write the modified buffer back to the NorFlash
- Status = NorFlashWriteBlocks (Instance, Lba, BlockSize,
Instance->ShadowBuffer);
- if (EFI_ERROR (Status)) {
-// Return one of the pre-approved error statuses
-return EFI_DEVICE_ERROR;
- }
-
- return EFI_SUCCESS;
+ return Status;
}
EFI_STATUS
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113921): https://edk2.groups.io/g/devel/message/113921
Mute This Topic: https://groups.io/mt/103766779/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3 4/6] OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
Raise the limit for writes without block erase from two to four
P30_MAX_BUFFER_SIZE_IN_BYTES blocks. With this in place almost all efi
variable updates are handled without block erase. With the old limit
some variable updates (with device paths) took the block erase code
path.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Laszlo Ersek
---
OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 3d1343b381bc..3d1d20daa1e5 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -550,13 +550,15 @@ NorFlashWriteSingleBlock (
return EFI_BAD_BUFFER_SIZE;
}
- // Pick P30_MAX_BUFFER_SIZE_IN_BYTES (== 128 bytes) as a good start for word
- // operations as opposed to erasing the block and writing the data regardless
- // if an erase is really needed. It looks like most individual NV variable
- // writes are smaller than 128 bytes.
- // To avoid pathological cases were a 2 byte write is disregarded because it
- // occurs right at a 128 byte buffered write alignment boundary, permit up to
- // twice the max buffer size, and perform two writes if needed.
+ // Pick 4 * P30_MAX_BUFFER_SIZE_IN_BYTES (== 512 bytes) as a good
+ // start for word operations as opposed to erasing the block and
+ // writing the data regardless if an erase is really needed.
+ //
+ // Many NV variable updates are small enough for a a single
+ // P30_MAX_BUFFER_SIZE_IN_BYTES block write. In case the update is
+ // larger than a single block, or the update crosses a
+ // P30_MAX_BUFFER_SIZE_IN_BYTES boundary (as shown in the diagram
+ // below), or both, we might have to write two or more blocks.
//
//0 128 256
//[|]
@@ -578,7 +580,7 @@ NorFlashWriteSingleBlock (
Start = Offset & ~BOUNDARY_OF_32_WORDS;
End = ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYTES);
- if ((End - Start) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
+ if ((End - Start) <= (4 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
// Check to see if we need to erase before programming the data into NOR.
// If the destination bits are only changing from 1s to 0s we can just
write.
// After a block is erased all bits in the block is set to 1.
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113918): https://edk2.groups.io/g/devel/message/113918
Mute This Topic: https://groups.io/mt/103766776/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3 5/6] OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too
It is possible to find variable entries with State being 0xff, i.e. not
updated since flash block erase. This indicates the variable driver
could not complete the header write while appending a new entry, and
therefore State was not set to VAR_HEADER_VALID_ONLY.
This can only happen at the end of the variable list, so treat this as
additional "end of variable list" condition.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Laszlo Ersek
---
OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
index 8fcd999ac6df..c8b5e0be1379 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
@@ -302,6 +302,11 @@ ValidateFvHeader (
break;
}
+if (VarHeader->State == 0xff) {
+ DEBUG ((DEBUG_INFO, "%a: end of var list (unwritten state)\n",
__func__));
+ break;
+}
+
VarName = NULL;
switch (VarHeader->State) {
// usage: State = VAR_HEADER_VALID_ONLY
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113920): https://edk2.groups.io/g/devel/message/113920
Mute This Topic: https://groups.io/mt/103766778/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3 2/6] OvmfPkg/VirtNorFlashDxe: clarify block write logic & fix shadowbuffer reads
Introduce 'Start' and 'End' variables to make it easier to follow the
logic and code flow. Also add a ascii art diagram (based on a
suggestion by Laszlo).
This also fixes the 'Size' calculation for the NorFlashRead() call.
Without this patch the code will read only one instead of two
P30_MAX_BUFFER_SIZE_IN_BYTES blocks in case '*NumBytes' is smaller than
P30_MAX_BUFFER_SIZE_IN_BYTES but 'Offset + *NumBytes' is not, i.e. the
update range crosses a P30_MAX_BUFFER_SIZE_IN_BYTES boundary.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Laszlo Ersek
---
OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 36 --
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 7f4743b00399..88a4d2c23fc0 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -520,6 +520,7 @@ NorFlashWriteSingleBlock (
UINTN BlockSize;
UINTN BlockAddress;
UINT8 *OrigData;
+ UINTN Start, End;
DEBUG ((DEBUG_BLKIO, "NorFlashWriteSingleBlock(Parameters: Lba=%ld,
Offset=0x%x, *NumBytes=0x%x, Buffer @ 0x%08x)\n", Lba, Offset, *NumBytes,
Buffer));
@@ -555,7 +556,28 @@ NorFlashWriteSingleBlock (
// To avoid pathological cases were a 2 byte write is disregarded because it
// occurs right at a 128 byte buffered write alignment boundary, permit up to
// twice the max buffer size, and perform two writes if needed.
- if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <= (2 *
P30_MAX_BUFFER_SIZE_IN_BYTES)) {
+ //
+ //0 128 256
+ //[|]
+ //^ ^ ^ ^
+ //| | | |
+ //| | |End, the next "word" boundary beyond
+ //| | |the (logical) update
+ //| | |
+ //| | (Offset & BOUNDARY_OF_32_WORDS) + NumBytes;
+ //| | i.e., the relative offset inside (or just past)
+ //| | the *double-word* such that it is the
+ //| | *exclusive* end of the (logical) update.
+ //| |
+ //| Offset & BOUNDARY_OF_32_WORDS; i.e., Offset within the
"word";
+ //| this is where the (logical) update is supposed to start
+ //|
+ //Start = Offset & ~BOUNDARY_OF_32_WORDS; i.e., Offset truncated to
"word" boundary
+
+ Start = Offset & ~BOUNDARY_OF_32_WORDS;
+ End = ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYTES);
+
+ if ((End - Start) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
// Check to see if we need to erase before programming the data into NOR.
// If the destination bits are only changing from 1s to 0s we can just
write.
// After a block is erased all bits in the block is set to 1.
@@ -565,8 +587,8 @@ NorFlashWriteSingleBlock (
Status = NorFlashRead (
Instance,
Lba,
- Offset & ~BOUNDARY_OF_32_WORDS,
- (*NumBytes | BOUNDARY_OF_32_WORDS) + 1,
+ Start,
+ End - Start,
Instance->ShadowBuffer
);
if (EFI_ERROR (Status)) {
@@ -601,7 +623,7 @@ NorFlashWriteSingleBlock (
Status = NorFlashWriteBuffer (
Instance,
- BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
+ BlockAddress + Start,
P30_MAX_BUFFER_SIZE_IN_BYTES,
Instance->ShadowBuffer
);
@@ -609,12 +631,10 @@ NorFlashWriteSingleBlock (
goto Exit;
}
-if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) >
P30_MAX_BUFFER_SIZE_IN_BYTES) {
- BlockAddress += P30_MAX_BUFFER_SIZE_IN_BYTES;
-
+if ((End - Start) > P30_MAX_BUFFER_SIZE_IN_BYTES) {
Status = NorFlashWriteBuffer (
Instance,
- BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
+ BlockAddress + Start + P30_MAX_BUFFER_SIZE_IN_BYTES,
P30_MAX_BUFFER_SIZE_IN_BYTES,
Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BYTES
);
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113916): https://edk2.groups.io/g/devel/message/113916
Mute This Topic: https://groups.io/mt/103766774/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3 0/6] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements
This is a little series containing the flash corruption fix sent yesterday with an slightly improved commit message and some small improvements on top of this. v3: - fix diagram - fix DoErase control flow - pick up reviewed-by tags v2: - drop broken bugfix, fix the bug when introducing Start+End variables instead. - add patch with UINTN and UINT32 casts. - add patch splitting the DoErase code path into a new function. - add the diagram sent by Laszlo. Gerd Hoffmann (6): OvmfPkg/VirtNorFlashDxe: add casts to UINTN and UINT32 OvmfPkg/VirtNorFlashDxe: clarify block write logic & fix shadowbuffer reads OvmfPkg/VirtNorFlashDxe: add a loop for NorFlashWriteBuffer calls. OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function OvmfPkg/VirtNorFlashDxe/VirtNorFlash.h| 2 +- OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c| 145 ++ OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 5 + 3 files changed, 101 insertions(+), 51 deletions(-) -- 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113915): https://edk2.groups.io/g/devel/message/113915 Mute This Topic: https://groups.io/mt/103766773/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/2] OvmfPkg: drop support for TPM 1.2
(cc Jiewen. Laszlo) On Tue, 16 Jan 2024 at 16:42, Gerd Hoffmann wrote: > > > > Gerd Hoffmann (2): > OvmfPkg: remove TPM1_ENABLE build option > OvmfPkg/Tcg2Config: remove unused TPM 1.2 support > Good riddance Acked-by: Ard Biesheuvel -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113914): https://edk2.groups.io/g/devel/message/113914 Mute This Topic: https://groups.io/mt/103764204/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH 1/2] OvmfPkg: remove TPM1_ENABLE build option
Signed-off-by: Gerd Hoffmann
---
OvmfPkg/Include/Dsc/OvmfTpmComponentsDxe.dsc.inc | 6 --
OvmfPkg/Include/Dsc/OvmfTpmComponentsPei.dsc.inc | 5 -
OvmfPkg/Include/Dsc/OvmfTpmDefines.dsc.inc | 3 ---
OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc | 9 -
OvmfPkg/Include/Dsc/OvmfTpmSecurityStub.dsc.inc | 6 --
OvmfPkg/Include/Fdf/OvmfTpmDxe.fdf.inc | 3 ---
OvmfPkg/Include/Fdf/OvmfTpmPei.fdf.inc | 5 -
OvmfPkg/PlatformCI/ReadMe.md | 2 +-
8 files changed, 1 insertion(+), 38 deletions(-)
diff --git a/OvmfPkg/Include/Dsc/OvmfTpmComponentsDxe.dsc.inc
b/OvmfPkg/Include/Dsc/OvmfTpmComponentsDxe.dsc.inc
index 75ae09571e8c..eef20b77149a 100644
--- a/OvmfPkg/Include/Dsc/OvmfTpmComponentsDxe.dsc.inc
+++ b/OvmfPkg/Include/Dsc/OvmfTpmComponentsDxe.dsc.inc
@@ -15,12 +15,6 @@
NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
}
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
-!if $(TPM1_ENABLE) == TRUE
- SecurityPkg/Tcg/TcgDxe/TcgDxe.inf {
-
-
Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
- }
-!endif
SecurityPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf {
TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
diff --git a/OvmfPkg/Include/Dsc/OvmfTpmComponentsPei.dsc.inc
b/OvmfPkg/Include/Dsc/OvmfTpmComponentsPei.dsc.inc
index fa486eed82d2..b91f29e5a64b 100644
--- a/OvmfPkg/Include/Dsc/OvmfTpmComponentsPei.dsc.inc
+++ b/OvmfPkg/Include/Dsc/OvmfTpmComponentsPei.dsc.inc
@@ -4,12 +4,7 @@
!if $(TPM2_ENABLE) == TRUE
OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
-!if $(TPM1_ENABLE) == TRUE
- OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf
- SecurityPkg/Tcg/TcgPei/TcgPei.inf
-!else
OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
-!endif
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
diff --git a/OvmfPkg/Include/Dsc/OvmfTpmDefines.dsc.inc
b/OvmfPkg/Include/Dsc/OvmfTpmDefines.dsc.inc
index a65564d8d9d2..ad3740a4737a 100644
--- a/OvmfPkg/Include/Dsc/OvmfTpmDefines.dsc.inc
+++ b/OvmfPkg/Include/Dsc/OvmfTpmDefines.dsc.inc
@@ -3,6 +3,3 @@
##
DEFINE TPM2_ENABLE = FALSE
-
- # has no effect unless TPM2_ENABLE == TRUE
- DEFINE TPM1_ENABLE = TRUE
diff --git a/OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
b/OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
index b97244695b52..e02a5d02d1a5 100644
--- a/OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
+++ b/OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
@@ -4,9 +4,6 @@
[LibraryClasses]
!if $(TPM2_ENABLE) == TRUE
-!if $(TPM1_ENABLE) == TRUE
- Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
-!endif
Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
@@ -27,16 +24,10 @@ [LibraryClasses]
[LibraryClasses.common.PEIM]
!if $(TPM2_ENABLE) == TRUE
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
-!if $(TPM1_ENABLE) == TRUE
- Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
-!endif
Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
!endif
[LibraryClasses.common.DXE_DRIVER]
!if $(TPM2_ENABLE) == TRUE
-!if $(TPM1_ENABLE) == TRUE
- Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
-!endif
Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
!endif
diff --git a/OvmfPkg/Include/Dsc/OvmfTpmSecurityStub.dsc.inc
b/OvmfPkg/Include/Dsc/OvmfTpmSecurityStub.dsc.inc
index 89455feca4d9..c40d6b0a0e78 100644
--- a/OvmfPkg/Include/Dsc/OvmfTpmSecurityStub.dsc.inc
+++ b/OvmfPkg/Include/Dsc/OvmfTpmSecurityStub.dsc.inc
@@ -2,12 +2,6 @@
#SPDX-License-Identifier: BSD-2-Clause-Patent
##
-!if $(TPM2_ENABLE) == TRUE
-!if $(TPM1_ENABLE) == TRUE
- NULL|SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.inf
-!endif
-!endif
-
!if $(TPM2_ENABLE) == TRUE || $(CC_MEASUREMENT_ENABLE) == TRUE
#
# DxeTpm2MeasureBootLib provides security service of TPM2 measure boot
and
diff --git a/OvmfPkg/Include/Fdf/OvmfTpmDxe.fdf.inc
b/OvmfPkg/Include/Fdf/OvmfTpmDxe.fdf.inc
index 7fc2bf8590a4..bd0be8fedbd5 100644
--- a/OvmfPkg/Include/Fdf/OvmfTpmDxe.fdf.inc
+++ b/OvmfPkg/Include/Fdf/OvmfTpmDxe.fdf.inc
@@ -3,9 +3,6 @@
##
!if $(TPM2_ENABLE) == TRUE
-!if $(TPM1_ENABLE) == TRUE
-INF SecurityPkg/Tcg/TcgDxe/TcgDxe.inf
-!endif
INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
INF SecurityPkg/Tcg/Tcg2PlatformDxe/Tcg2PlatformDxe.inf
INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
diff --git a/OvmfPkg/Include/Fdf/OvmfTpmPei.fdf.inc
b/OvmfPkg/Include/Fdf/OvmfTpmPei.fdf.inc
index 9f8b9bdd5bed..add012afab67 100644
---
[edk2-devel] [PATCH 2/2] OvmfPkg/Tcg2Config: remove unused TPM 1.2 support
Signed-off-by: Gerd Hoffmann
---
OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf | 56 ---
OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c | 83 ---
2 files changed, 139 deletions(-)
delete mode 100644 OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf
delete mode 100644 OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c
diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf
b/OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf
deleted file mode 100644
index e8e0b88e6058..
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf
+++ /dev/null
@@ -1,56 +0,0 @@
-## @file
-# Set TPM device type - supports TPM 1.2 and 2.0
-#
-# In SecurityPkg, this module initializes the TPM device type based on a UEFI
-# variable and/or hardware detection. In OvmfPkg, the module only performs TPM
-# hardware detection.
-#
-# Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.
-# Copyright (C) 2018, Red Hat, Inc.
-#
-# SPDX-License-Identifier: BSD-2-Clause-Patent
-##
-
-[Defines]
- INF_VERSION= 0x00010005
- BASE_NAME = Tcg2ConfigPei
- FILE_GUID = 8AD3148F-945F-46B4-8ACD-71469EA73945
- MODULE_TYPE= PEIM
- VERSION_STRING = 1.0
- ENTRY_POINT= Tcg2ConfigPeimEntryPoint
-
-[Sources]
- Tcg2ConfigPeim.c
- Tpm12Support.h
- Tpm12Support.c
-
-[Packages]
- MdePkg/MdePkg.dec
- MdeModulePkg/MdeModulePkg.dec
- OvmfPkg/OvmfPkg.dec
- SecurityPkg/SecurityPkg.dec
-
-[LibraryClasses]
- PeimEntryPoint
- DebugLib
- PeiServicesLib
- Tpm2DeviceLib
- BaseLib
- Tpm12DeviceLib
-
-[Guids]
- gEfiTpmDeviceSelectedGuid ## PRODUCES ## GUID # Used as a PPI GUID
- gEfiTpmDeviceInstanceTpm20DtpmGuid ## SOMETIMES_CONSUMES
- gEfiTpmDeviceInstanceTpm12Guid ## SOMETIMES_CONSUMES
-
-[Ppis]
- gPeiTpmInitializationDonePpiGuid## SOMETIMES_PRODUCES
-
-[Pcd]
- gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES
-
-[Depex.IA32, Depex.X64]
- gOvmfTpmMmioAccessiblePpiGuid
-
-[Depex.ARM, Depex.AARCH64]
- gOvmfTpmDiscoveredPpiGuid
diff --git a/OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c
b/OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c
deleted file mode 100644
index c88da5758b44..
--- a/OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c
+++ /dev/null
@@ -1,83 +0,0 @@
-/** @file
- Implement the InternalTpm12Detect() function on top of the Tpm12DeviceLib
- class.
-
- Copyright (C) 2020, Red Hat, Inc.
-
- SPDX-License-Identifier: BSD-2-Clause-Patent
-**/
-
-#include
-#include
-
-#include "Tpm12Support.h"
-
-#pragma pack (1)
-typedef struct {
- TPM_RSP_COMMAND_HDRHdr;
- TPM_CURRENT_TICKS CurrentTicks;
-} TPM_RSP_GET_TICKS;
-#pragma pack ()
-
-/**
- Probe for the TPM for 1.2 version, by sending TPM1.2 GetTicks
-
- Sending a TPM1.2 command to a TPM2 should return a TPM1.2
- header (tag = 0xc4) and error code (TPM_BADTAG = 0x1e)
-
- @retval EFI_SUCCESS TPM version 1.2 probing successful.
-
- @return Error codes propagated from Tpm12SubmitCommand().
-**/
-STATIC
-EFI_STATUS
-TestTpm12 (
- )
-{
- EFI_STATUS Status;
- TPM_RQU_COMMAND_HDR Command;
- TPM_RSP_GET_TICKSResponse;
- UINT32 Length;
-
- Command.tag = SwapBytes16 (TPM_TAG_RQU_COMMAND);
- Command.paramSize = SwapBytes32 (sizeof (Command));
- Command.ordinal = SwapBytes32 (TPM_ORD_GetTicks);
-
- Length = sizeof (Response);
- Status = Tpm12SubmitCommand (
- sizeof (Command),
- (UINT8 *),
- ,
- (UINT8 *)
- );
- if (EFI_ERROR (Status)) {
-return Status;
- }
-
- return EFI_SUCCESS;
-}
-
-/**
- Detect the presence of a TPM with interface version 1.2.
-
- @retval EFI_SUCCESS TPM-1.2 available. The Tpm12RequestUseTpm() and
- Tpm12SubmitCommand(TPM_ORD_GetTicks) operations
- (from the Tpm12DeviceLib class) have succeeded.
-
- @return Error codes propagated from Tpm12RequestUseTpm() and
- Tpm12SubmitCommand().
-**/
-EFI_STATUS
-InternalTpm12Detect (
- VOID
- )
-{
- EFI_STATUS Status;
-
- Status = Tpm12RequestUseTpm ();
- if (EFI_ERROR (Status)) {
-return Status;
- }
-
- return TestTpm12 ();
-}
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113912): https://edk2.groups.io/g/devel/message/113912
Mute This Topic: https://groups.io/mt/103764205/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH 0/2] OvmfPkg: drop support for TPM 1.2
Gerd Hoffmann (2): OvmfPkg: remove TPM1_ENABLE build option OvmfPkg/Tcg2Config: remove unused TPM 1.2 support .../Include/Dsc/OvmfTpmComponentsDxe.dsc.inc | 6 -- .../Include/Dsc/OvmfTpmComponentsPei.dsc.inc | 5 -- OvmfPkg/Include/Dsc/OvmfTpmDefines.dsc.inc| 3 - OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc | 9 -- .../Include/Dsc/OvmfTpmSecurityStub.dsc.inc | 6 -- OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf | 56 - OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c | 83 --- OvmfPkg/Include/Fdf/OvmfTpmDxe.fdf.inc| 3 - OvmfPkg/Include/Fdf/OvmfTpmPei.fdf.inc| 5 -- OvmfPkg/PlatformCI/ReadMe.md | 2 +- 10 files changed, 1 insertion(+), 177 deletions(-) delete mode 100644 OvmfPkg/Tcg/Tcg2Config/Tcg12ConfigPei.inf delete mode 100644 OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c -- 2.43.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113911): https://edk2.groups.io/g/devel/message/113911 Mute This Topic: https://groups.io/mt/103764204/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver
On 1/16/24 16:16, Michael Brown wrote: > On 16/01/2024 14:34, Laszlo Ersek wrote: >> On 1/16/24 10:48, Michael Brown wrote: >> IOW, my impression is that NestedInterruptTplLib can certainly handle >> all scenarios thrown at it, but where it really matters is in the face >> of an interrupt storm (not just "normal nesting"), and a storm is >> unlikely (or even impossible?) on physical hardware. >> >> ... Oh, scratch that. "Interrupt storm" simply means that interrupts are >> being delivered at a rate higher than the handler routine can service >> them. IOW, the "storm" is not that interrupts are delivered *very >> rapidly* in an absoulte sense. If interrupts are delivered at normal >> frequency, but the handler is too slow to service *even that rate*, then >> that also qualifies as "storm", because the nesting depth will *keep >> growing*. It's not really the growth rate that matters; what matter is >> the *trend*, i.e., the fact that there *is* growth (the stack gets >> deeper and deeper). The stack might not overflow immediately, and if the >> handler speeds up (for whatever reason), the stack might recover, but >> there is nothing to prevent an overflow. >> >> So, in the end, I think you've convinced me. > > :) > >>> I'm happy to send a patch to migrate NestedInterruptTplLib to >>> MdeModulePkg, so that it can be consumed outside of OvmfPkg. Shall I do >>> this? >> >> Sounds like a valid idea to me. >> >> Could be greatly supported by a test case (to be run on the bare metal) >> installing a slow handler that *eventually* exhausted the stack, when >> not using NestedInterruptTplLib. >> >> (FWIW, IIRC, the UEFI spec warns about this -- it says something like, >> "return from TPL_HIGH as soon as you can, otherwise the system will >> become unstable".) >> >> Sorry for the wall of text, I find this very difficult to reason about. > > I also find it very difficult to reason about, which is why > NestedInterruptRestoreTpl() has 126 lines of comments providing a > semi-formal proof of correctness for a mere 15 statements of C code! > > In particular, I find it difficult to reason about when it would be safe > for a platform to *not* use NestedInterruptTplLib. It's clearly > empirically difficult to trigger stack underflow via an interrupt > "storm" on physical hardware, but I'm not convinced it's impossible. > > I find it mentally easier to rely on the hard guarantee that > NestedInterruptTplLib provides: that nested interrupts will continue to > be delivered but that the number of interrupt-induced stack frames is > bounded by the (small, finite) number of distinct TPL levels in existence. > > > > While developing NestedInterruptTplLib, I did hack together a test case > for a slow handler that would deliberately induce an interrupt storm, > since I needed this to test that my code was working. When triggered, > this test would cause the machine to effectively hang due to servicing > an endless storm of timer interrupts. Before NestedInterruptTplLib, the > stack would soon underflow and would typically cause a reboot (or other > crash). With NestedInterruptTplLib the machine would continue to > service interrupts indefinitely. > > How might such a test case be included in upstream EDK2? I'm > peripherally aware of EDK2 test infrastructure such as UEFI SCT, but > I've never interacted with it yet. I'm vaguely aware of a unit test framework inside edk2, but the best I can give you is just this link: https://github.com/tianocore/edk2/tree/master/UnitTestFrameworkPkg#unit-test-framework-package There are some files under the directory "MdeModulePkg/Test" too; git-log on that subdir, and perhaps the MdeModulePkg maintainers, might provide more pointers. The end of the readme linked above says to ask Bret, Mike and Sean, as well. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113910): https://edk2.groups.io/g/devel/message/113910 Mute This Topic: https://groups.io/mt/103734961/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/6] OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function
On 1/16/24 14:44, Laszlo Ersek wrote:
> On 1/15/24 16:59, Gerd Hoffmann wrote:
>> Move the DoErase code block into a separate function, call the function
>> instead of jumping around with goto.
>>
>> Signed-off-by: Gerd Hoffmann
>> ---
>> OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 76 +-
>> 1 file changed, 51 insertions(+), 25 deletions(-)
>>
>> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>> index d80e9f0a2f3a..203bd64f2bdf 100644
>> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
>> @@ -502,6 +502,37 @@ NorFlashRead (
>>return EFI_SUCCESS;
>> }
>>
>> +STATIC EFI_STATUS
>
> (1) EFI_STATUS is not needed (and if it were needed, then we'd put it on
> a separate line)
>
>> +NorFlashWriteSingleBlockWithErase (
>> + INNOR_FLASH_INSTANCE *Instance,
>> + INEFI_LBA Lba,
>> + INUINTN Offset,
>> + IN OUTUINTN *NumBytes,
>> + INUINT8 *Buffer
>> + )
>> +{
Sigh. In your patch, I mistook / misread "EFI_STATUS" for "EFIAPI". :/
So, please ignore this; we obviously need the EFI_STATUS return type.
... Perhaps consider breaking "EFI_STATUS" to its own line.
(Strange how the brain works; my "mental alarm" about having typed
something foolish went off approx. 90 minutes after hitting Send --
while I was running outside. I know that some people get the best
programming ideas while showering...)
Sorry!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113909): https://edk2.groups.io/g/devel/message/113909
Mute This Topic: https://groups.io/mt/103741665/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver
On 16/01/2024 14:34, Laszlo Ersek wrote: On 1/16/24 10:48, Michael Brown wrote: IOW, my impression is that NestedInterruptTplLib can certainly handle all scenarios thrown at it, but where it really matters is in the face of an interrupt storm (not just "normal nesting"), and a storm is unlikely (or even impossible?) on physical hardware. ... Oh, scratch that. "Interrupt storm" simply means that interrupts are being delivered at a rate higher than the handler routine can service them. IOW, the "storm" is not that interrupts are delivered *very rapidly* in an absoulte sense. If interrupts are delivered at normal frequency, but the handler is too slow to service *even that rate*, then that also qualifies as "storm", because the nesting depth will *keep growing*. It's not really the growth rate that matters; what matter is the *trend*, i.e., the fact that there *is* growth (the stack gets deeper and deeper). The stack might not overflow immediately, and if the handler speeds up (for whatever reason), the stack might recover, but there is nothing to prevent an overflow. So, in the end, I think you've convinced me. :) I'm happy to send a patch to migrate NestedInterruptTplLib to MdeModulePkg, so that it can be consumed outside of OvmfPkg. Shall I do this? Sounds like a valid idea to me. Could be greatly supported by a test case (to be run on the bare metal) installing a slow handler that *eventually* exhausted the stack, when not using NestedInterruptTplLib. (FWIW, IIRC, the UEFI spec warns about this -- it says something like, "return from TPL_HIGH as soon as you can, otherwise the system will become unstable".) Sorry for the wall of text, I find this very difficult to reason about. I also find it very difficult to reason about, which is why NestedInterruptRestoreTpl() has 126 lines of comments providing a semi-formal proof of correctness for a mere 15 statements of C code! In particular, I find it difficult to reason about when it would be safe for a platform to *not* use NestedInterruptTplLib. It's clearly empirically difficult to trigger stack underflow via an interrupt "storm" on physical hardware, but I'm not convinced it's impossible. I find it mentally easier to rely on the hard guarantee that NestedInterruptTplLib provides: that nested interrupts will continue to be delivered but that the number of interrupt-induced stack frames is bounded by the (small, finite) number of distinct TPL levels in existence. While developing NestedInterruptTplLib, I did hack together a test case for a slow handler that would deliberately induce an interrupt storm, since I needed this to test that my code was working. When triggered, this test would cause the machine to effectively hang due to servicing an endless storm of timer interrupts. Before NestedInterruptTplLib, the stack would soon underflow and would typically cause a reboot (or other crash). With NestedInterruptTplLib the machine would continue to service interrupts indefinitely. How might such a test case be included in upstream EDK2? I'm peripherally aware of EDK2 test infrastructure such as UEFI SCT, but I've never interacted with it yet. Thanks, Michael -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113908): https://edk2.groups.io/g/devel/message/113908 Mute This Topic: https://groups.io/mt/103734961/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
回复: [edk2-devel] [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError Feature
This option is for the module check. I suggest to set this option in INF instead of DSC. If so, DSC is not required to configure the additional options. And, this module will have the same build result when it is specified in the different platform DSC files. I understand your requirement to enable this option for the modules in some specific package. I think you can easily find those modules those have VFR/HFR, then update their INFs with new check option. Thanks Liming > -邮件原件- > 发件人: devel@edk2.groups.io 代表 Zhang, Zifeng > 发送时间: 2024年1月12日 13:20 > 收件人: Yang, Yuting2 ; Gao, Liming > > 抄送: Chen, Christine ; devel@edk2.groups.io > 主题: Re: [edk2-devel] [Patch V2] BaseTools: VfrCompiler Adds > DefaultValueError Feature > > Hi Liming, > > Could you help to share the update for this patch solution? > > Best Regards, > Zifeng > > -Original Message- > From: Yang, Yuting2 > Sent: Monday, December 25, 2023 3:10 PM > To: Gao, Liming > Cc: Zhang, Zifeng ; Chen, Christine > ; devel@edk2.groups.io > Subject: RE: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > Feature > > Hi Liming, > > Thank you for adding the account for me. > I have created a Bugzilla https://bugzilla.tianocore.org/show_bug.cgi?id=4629 > for this feature ~ > > Best Regards,, > Yuting > > -Original Message- > From: gaoliming > Sent: Monday, December 25, 2023 9:14 AM > To: Yang, Yuting2 > Subject: 回复: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > Feature > > Your account yuting2.y...@intel.com has been added. PW: tiano@123 > > > -邮件原件- > > 发件人: Yang, Yuting2 > > 发送时间: 2023年12月22日 13:41 > > 收件人: Gao, Liming > > 抄送: Rebecca Cran ; Feng, Bob C > > ; Chen, Arthur G ; > > Chen, Christine ; Zhang, Zifeng > > ; devel@edk2.groups.io > > 主题: RE: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > > Feature > > > > Hi Liming, > > > > Thank you for reviewing ~ > > Could you please help me create a Bugzilla account? Currently, I do > > not > have > > access to the Bugzilla. > > > > Best Regards, > > Yuting > > > > -Original Message- > > From: Zhang, Zifeng > > Sent: Thursday, December 21, 2023 2:44 PM > > To: Gao, Liming ; Yang, Yuting2 > > > > Cc: Rebecca Cran ; Feng, Bob C > > ; Chen, Arthur G ; > > devel@edk2.groups.io; Chen, Christine > > Subject: RE: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > > Feature > > > > Hi Liming, > > > > Thanks for reviewing. > > For background of this change, we will remove default flags in VFR/HFR > > in new platform. So we need help from VFR complier to make a default > > flag check to avoid manually adding. > > @Yang, Yuting2, could you help to create a BZ for this feature and > > share > in > > mail thread? > > Then let me make a clarification for your questions. > > > > #1: The purpose of --catch_default > > We send --catch_default flag in build option to indicate which > > platform > should > > check default flag in VFR/HFR. > > Actually maybe some platforms used same EDK2 downstream branch, so we > > only send --catch_default flag for the platforms which need this check. > > > > #2: The purpose of --except_list > > VFR compiler will receive VFR/HFR configurations from all folders > including > > Intel and EDK2. But in our expectation VFR compiler only do this check > > in Intel. > > So We use --except_list to deliver package list in EDK2 to avoid this > check. > > > > Best Regards, > > Zifeng > > > > -Original Message- > > From: Yang, Yuting2 > > Sent: Tuesday, December 12, 2023 5:12 PM > > To: Zhang, Zifeng ; Chen, Arthur G > > ; devel@edk2.groups.io > > Cc: Rebecca Cran ; Gao, Liming > > ; Feng, Bob C > > Subject: RE: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > > Feature > > > > +Cc Zhang, Zifeng, Chen, Arthur G > > > > -Original Message- > > From: Chen, Christine > > Sent: Tuesday, December 12, 2023 5:04 PM > > To: Yang, Yuting2 ; devel@edk2.groups.io > > Cc: Rebecca Cran ; Gao, Liming > > ; Feng, Bob C > > Subject: RE: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > > Feature > > > > +Cc Yang, Yuting2 > > > > > -Original Message- > > > From: Yang, Yuting2 > > > Sent: Tuesday, December 12, 2023 5:01 PM > > > To: devel@edk2.groups.io > > > Cc: Rebecca Cran ; Gao, Liming > > > ; Feng, Bob C ; > > > Chen, Christine > > > Subject: [Patch V2] BaseTools: VfrCompiler Adds DefaultValueError > > > Feature > > > > > > Add --catch_default option > > > Raise a DefaultValueError when encountering VFR default definitions > > > to help remove default variables. > > > Add --except_list option > > > Exclude packages that don't require enabling the catch_default function. > > > > > > Cc: Rebecca Cran > > > Cc: Liming Gao > > > Cc: Bob Feng > > > Cc: Christine Chen > > > Cc: Yuting Yang > > > > > > Signed-off-by: Yuting Yang > > > --- > > > BaseTools/Source/C/VfrCompile/VfrCompiler.cpp | 40 ++- > > > BaseTools/Source/C/VfrCompile/VfrCompiler.h | 3 + > >
Re: [edk2-devel] [PATCH v7 25/37] ArmVirtPkg: Move PlatformBootManagerLib to OvmfPkg
On 1/16/24 12:54, Chao Li wrote:
> On 2024/1/15 16:46, Laszlo Ersek wrote:
>> On 1/12/24 09:25, Chao Li wrote:
>>> @@ -29,7 +29,6 @@
>>>QemuKernel.c
>>>
>>> [Packages]
>>> - ArmVirtPkg/ArmVirtPkg.dec
>>>MdeModulePkg/MdeModulePkg.dec
>>>MdePkg/MdePkg.dec
>>>OvmfPkg/OvmfPkg.dec
>> Hmmm. This makes me wonder.
>>
>> If we can remove the ArmVirtPkg package dependency from the lib instance
>> in *this patch*, then we should be able to remove it *earlier* too
>> (i.e., independently), while the lib instance still exists under ArmVirtPkg.
>>
>> I don't see why the "ArmVirtPkg.dec" dep becomes superfluous *right here*.
>>
>> If I look at this INF file, as of commit 4a443f73fd67, I see at least
>> two "ArmVirtPkg.dec" dependencies:
>>
>> [FixedPcd]
>> gArmVirtTokenSpaceGuid.PcdUninstallMemAttrProtocol
>>
>> [Pcd]
>> gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer
>>
>> In patch 24 ("ArmVirtPkg: Move two PCD variables into OvmfPkg"), you
>> move these PCDs to OvmfPkg.
>>
>> Ah, I understand now. In brief: this particular hunk belongs to patch 24
>> (where you correctly modify "PlatformBootManagerLib.inf" anyway). The
>> point is that, with the movement of both PCDs from the ArmVirt token
>> space to the OVMF token space, "PlatformBootManagerLib.inf"'s dependency
>> on "ArmVirtPkg.dec" disappears. Therefore the above hunk belongs to
>> patch 24.
>>
>> ... When you implement that, please build-test both patches 24 and 25.
>>
>> More precisely, your patch set should build at every stage, considering
>> both ArmVirt and OVMF platforms.
>>
>> The command "git rebase --exec" is useful for building a series at every
>> stage.
>
> Do you means this change should belong in patch 24 is better?
Yes, please.
>
> BTW, I did build and tested after applying patches 24 and 25(building
> and testing with ArmVirtQemu.dec) and it works fine.
Thank you.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113906): https://edk2.groups.io/g/devel/message/113906
Mute This Topic: https://groups.io/mt/103679477/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
回复: [edk2-devel] [PATCH v3 4/4] MdeModulePkg/Hob: Integer Overflow in CreateHob()
Gua:
I think new code logic is same to old one. Can you point what difference
here?
Thanks
Liming
> -邮件原件-
> 发件人: devel@edk2.groups.io 代表 Guo, Gua
> 发送时间: 2024年1月12日 10:25
> 收件人: devel@edk2.groups.io
> 抄送: gua@intel.com; Marc Beatove ; Liming
> Gao ; John Mathew ;
> Gerd Hoffmann
> 主题: [edk2-devel] [PATCH v3 4/4] MdeModulePkg/Hob: Integer Overflow in
> CreateHob()
>
> From: Gua Guo
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166
>
> Fix integer overflow in various CreateHob instances.
> Fixes: CVE-2022-36765
>
> The CreateHob() function aligns the requested size to 8
> performing the following operation:
> ```
> HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
> ```
>
> No checks are performed to ensure this value doesn't
> overflow, and could lead to CreateHob() returning a smaller
> HOB than requested, which could lead to OOB HOB accesses.
>
> Reported-by: Marc Beatove
> Cc: Liming Gao
> Cc: John Mathew
> Authored-by: Gerd Hoffmann
> Signed-off-by: Gua Guo
> ---
> MdeModulePkg/Core/Pei/Hob/Hob.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/Pei/Hob/Hob.c
> b/MdeModulePkg/Core/Pei/Hob/Hob.c
> index c4882a23cd..985da50995 100644
> --- a/MdeModulePkg/Core/Pei/Hob/Hob.c
> +++ b/MdeModulePkg/Core/Pei/Hob/Hob.c
> @@ -85,7 +85,7 @@ PeiCreateHob (
>//
>
>// Check Length to avoid data overflow.
>
>//
>
> - if (0x1 - Length <= 0x7) {
>
> + if (MAX_UINT16 - Length < 0x7) {
>
> return EFI_INVALID_PARAMETER;
>
>}
>
>
>
> --
> 2.39.2.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#113643):
> https://edk2.groups.io/g/devel/message/113643
> Mute This Topic: https://groups.io/mt/103675965/4905953
> Group Owner: devel+ow...@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [gaolim...@byosoft.com.cn]
> -=-=-=-=-=-=
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113905): https://edk2.groups.io/g/devel/message/113905
Mute This Topic: https://groups.io/mt/103762835/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & TCBZ4118
Sure. Let's start from OVMF. We have leaf enough time for feedback, but I see no comment from other people. > -Original Message- > From: Gerd Hoffmann > Sent: Tuesday, January 16, 2024 10:35 PM > To: devel@edk2.groups.io; Yao, Jiewen > Cc: dougfl...@microsoft.com; Douglas Flick [MSFT] > Subject: Re: Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & > TCBZ4118 > > On Tue, Jan 16, 2024 at 01:30:43PM +, Yao, Jiewen wrote: > > Gerd > > I have merged this patch set today. > > > > I am fine to remove TPM1.2 in OVMF because of the known security limitation. > > I was thinking about the complete edk2 code base not only OVMF. > > But I can surely start with OVMF. Maybe it is the only platform > affected because on physical hardware you usually know whenever > TPM 1.2 or TPM 2.0 is present so there is no need to include both. > > take care, > Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113904): https://edk2.groups.io/g/devel/message/113904 Mute This Topic: https://groups.io/mt/103675434/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & TCBZ4118
On Tue, Jan 16, 2024 at 01:30:43PM +, Yao, Jiewen wrote: > Gerd > I have merged this patch set today. > > I am fine to remove TPM1.2 in OVMF because of the known security limitation. I was thinking about the complete edk2 code base not only OVMF. But I can surely start with OVMF. Maybe it is the only platform affected because on physical hardware you usually know whenever TPM 1.2 or TPM 2.0 is present so there is no need to include both. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113903): https://edk2.groups.io/g/devel/message/113903 Mute This Topic: https://groups.io/mt/103675434/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver
On 1/16/24 10:48, Michael Brown wrote: > On 16/01/2024 08:47, Gerd Hoffmann wrote: >> I think the reason is that the next timer interrupt arriving while the >> handler is running still is *much* more likely in virtual machines >> because the vCPU does not get 100% of the of the physical CPU time >> slice. > > The interrupt handler can run for an arbitrary length of time, because > the call to RestoreTPL() can end up calling an application callback > which in turn waits on further timer interrupts. > > This is a legitimate use case within UEFI, so all timer interrupt > handlers (not just in virtual machines) need to allow for the > possibility that nested interrupts will occur. The more naive, original interrupt handler implementation (i.e., the one without NestedInterruptTplLib) still "allowed" for nesting, simply by virtue of letting itself be interrupted, if I remember correctly. That in itself didn't cause a problem; the problem arose when this reentrant interruption got limitlessly deep, due to interrupts having been queued on the host side, and then being injected as a "storm" in the guest. The naive handler impl. effectively translated the host-side "interrupt queue" to a "guest side stack". "queue length" became "stack depth", leading to stack overflow. Thus, even the original (more naive) handler could work fine (for nesting too) as long as there was no storm (put differently, as long as queue length, aka stack depth, were small); is that correct? (I admit that I can't really recall the details at this point.) The sophistication of NestedInterruptTplLib is that it supports nesting while *resisting* a storm (= preventing infinite nesting by careful masking of interrupt delivery, IIRC). It does not eagerly slurp all pending (queued) interrupts into the handler stack. IOW, my impression is that NestedInterruptTplLib can certainly handle all scenarios thrown at it, but where it really matters is in the face of an interrupt storm (not just "normal nesting"), and a storm is unlikely (or even impossible?) on physical hardware. ... Oh, scratch that. "Interrupt storm" simply means that interrupts are being delivered at a rate higher than the handler routine can service them. IOW, the "storm" is not that interrupts are delivered *very rapidly* in an absoulte sense. If interrupts are delivered at normal frequency, but the handler is too slow to service *even that rate*, then that also qualifies as "storm", because the nesting depth will *keep growing*. It's not really the growth rate that matters; what matter is the *trend*, i.e., the fact that there *is* growth (the stack gets deeper and deeper). The stack might not overflow immediately, and if the handler speeds up (for whatever reason), the stack might recover, but there is nothing to prevent an overflow. So, in the end, I think you've convinced me. > >> So on OVMF we will continue to need NestedInterruptTplLib. I like the >> idea to have a Null version of the library, so OVMF does not need its >> own version of the driver just because of NestedInterruptTplLib. > > I agree that there should not need to be two versions of LocalApicTimerDxe. > > I would suggest moving NestedInterruptTplLib from OvmfPkg to MdeModulePkg. > > The code is complex, but for the caller the complexity is completely > hidden behind the calls to NestedInterruptRaiseTPL() and > NestedInterruptRestoreTPL(), which straightforwardly replace what would > otherwise be (unsafe) calls to RaiseTPL() and RestoreTPL(), as shown in > > https://github.com/tianocore/edk2/commit/a086f4a#diff-1418ec21e24e4ce2f3d621113585e3bea11fbcfa3b77d54046e61be7928e0c92 > > For a new CPU architecture, the only requirement is to provide a short > fragment (~5 lines) of code that can clear the interrupts-enabled flag > in the EFI_SYSTEM_CONTEXT, as shown in > OvmfPkg/Library/NestedInterruptTplLib/Iret.c. > > I'm happy to send a patch to migrate NestedInterruptTplLib to > MdeModulePkg, so that it can be consumed outside of OvmfPkg. Shall I do > this? Sounds like a valid idea to me. Could be greatly supported by a test case (to be run on the bare metal) installing a slow handler that *eventually* exhausted the stack, when not using NestedInterruptTplLib. (FWIW, IIRC, the UEFI spec warns about this -- it says something like, "return from TPL_HIGH as soon as you can, otherwise the system will become unstable".) Sorry for the wall of text, I find this very difficult to reason about. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113902): https://edk2.groups.io/g/devel/message/113902 Mute This Topic: https://groups.io/mt/103734961/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
回复: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of EFI_FIRMWARE_MANAGEMENT_PROTOCOL
I am OK for this change. Reviewed-by: Liming Gao > -邮件原件- > 发件人: devel@edk2.groups.io 代表 Xu, GuoX > 发送时间: 2024年1月9日 17:24 > 收件人: devel@edk2.groups.io > 抄送: Kinney, Michael D ; Gao, Liming > ; Liu, Zhiguang > 主题: Re: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of > EFI_FIRMWARE_MANAGEMENT_PROTOCOL > > Hi all, any comments about this patch? > > -Original Message- > From: devel@edk2.groups.io On Behalf Of GuoX Xu > Sent: Monday, December 25, 2023 9:21 AM > To: devel@edk2.groups.io > Cc: Kinney, Michael D ; Gao, Liming > ; Liu, Zhiguang ; Li, Yi1 > > Subject: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of > EFI_FIRMWARE_MANAGEMENT_PROTOCOL > > 1.For EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImage(): > Add the following sentence at the end of the Image parameter description. > "May be NULL with a zero ImageSize in order to determine the size of the > buffer needed". > > Modify the description of "EFI_INVALID_PARAMETER" return code as "The > ImageSize is not too small and Image is NULL." > > 2.For EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo(): > Add the following sentence at the end of the ImageInfo parameter > description."May be NULL with a zero ImageInfoSize in order to determine > the size of the buffer needed". > > Modify the description of "EFI_INVALID_PARAMETER" return code as "The > ImageInfoSize is not too small and Image is NULL." and add new descriptions > for "EFI_INVALID_PARAMETER" return code. > > REF: UEFI Spec 2.7B Chapter 23.1. > > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Zhiguang Liu > Cc: Yi Li > Signed-off-by: GuoX Xu > --- > MdePkg/Include/Protocol/FirmwareManagement.h | 13 +++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/MdePkg/Include/Protocol/FirmwareManagement.h > b/MdePkg/Include/Protocol/FirmwareManagement.h > index f37067df3455..93c8b7658e1a 100644 > --- a/MdePkg/Include/Protocol/FirmwareManagement.h > +++ b/MdePkg/Include/Protocol/FirmwareManagement.h > @@ -293,6 +293,8 @@ EFI_STATUS > to contain the image(s) > information if the buffer was too small. >@param[in, out] ImageInfo A pointer to the buffer in which > firmware places the current image(s) > information. The information > is an array of EFI_FIRMWARE_IMAGE_DESCRIPTORs. > + May be NULL with a zero > ImageInfoSize in order to determine the size of the > + buffer needed. >@param[out] DescriptorVersion A pointer to the location in which > firmware returns the version number > associated with the > EFI_FIRMWARE_IMAGE_DESCRIPTOR. >@param[out] DescriptorCountA pointer to the location in > which firmware returns the number of > @@ -313,7 +315,12 @@ EFI_STATUS >@retval EFI_SUCCESSThe device was successfully > updated with the new image. >@retval EFI_BUFFER_TOO_SMALL The ImageInfo buffer was too > small. The current buffer size > needed to hold the image(s) > information is returned in ImageInfoSize. > - @retval EFI_INVALID_PARAMETER ImageInfoSize is NULL. > + @retval EFI_INVALID_PARAMETER ImageInfoSize is not too small > and ImageInfo is NULL. > + @retval EFI_INVALID_PARAMETER ImageInfoSize is non-zero and > DescriptorVersion is NULL. > + @retval EFI_INVALID_PARAMETER ImageInfoSize is non-zero and > DescriptorCount is NULL. > + @retval EFI_INVALID_PARAMETER ImageInfoSize is non-zero and > DescriptorSize is NULL. > + @retval EFI_INVALID_PARAMETER ImageInfoSize is non-zero and > PackageVersion is NULL. > + @retval EFI_INVALID_PARAMETER ImageInfoSize is non-zero and > PackageVersionName is NULL. >@retval EFI_DEVICE_ERROR Valid information could not be > returned. Possible corrupted image. > > **/ > @@ -340,6 +347,8 @@ EFI_STATUS >@param[in] ImageIndex A unique number identifying the > firmware image(s) within the device. > The number is between 1 and > DescriptorCount. >@param[out] Image Points to the buffer where the > current image is copied to. > + May be NULL with a zero > ImageSize in order to determine the size of the > + buffer needed. >@param[in, out] ImageSize On entry, points to the size of the > buffer pointed to by Image, in bytes. > On return, points to the length of > the image, in bytes. > > @@ -347,7 +356,7 @@ EFI_STATUS >@retval EFI_BUFFER_TOO_SMALL The buffer specified by ImageSize is > too small to hold the > image. The current buffer size > needed to hold the image is returned > in ImageSize. > - @retval EFI_INVALID_PARAMETER The Image was NULL. >
回复: [edk2-devel] [PATCH v2 1/1] MdePkg/IndustryStandard: Add _PSD/_CPC/Coord types definitions
I am OK to merge it. Reviewed-by: Liming Gao 发件人: devel@edk2.groups.io 代表 Sami Mujawar 发送时间: 2024年1月12日 22:37 收件人: PierreGondois ; devel@edk2.groups.io 主题: Re: [edk2-devel] [PATCH v2 1/1] MdePkg/IndustryStandard: Add _PSD/_CPC/Coord types definitions Hi Liming, If there are no further comments on this patch, can you let me know if I can merge this, please? Regards, Sami Mujawar -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113900): https://edk2.groups.io/g/devel/message/113900 Mute This Topic: https://groups.io/mt/103762110/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/6] OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function
On 1/15/24 16:59, Gerd Hoffmann wrote:
> Move the DoErase code block into a separate function, call the function
> instead of jumping around with goto.
>
> Signed-off-by: Gerd Hoffmann
> ---
> OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 76 +-
> 1 file changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index d80e9f0a2f3a..203bd64f2bdf 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -502,6 +502,37 @@ NorFlashRead (
>return EFI_SUCCESS;
> }
>
> +STATIC EFI_STATUS
(1) EFI_STATUS is not needed (and if it were needed, then we'd put it on
a separate line)
> +NorFlashWriteSingleBlockWithErase (
> + INNOR_FLASH_INSTANCE *Instance,
> + INEFI_LBA Lba,
> + INUINTN Offset,
> + IN OUTUINTN *NumBytes,
> + INUINT8 *Buffer
> + )
> +{
> + EFI_STATUS Status;
> +
> + // Read NOR Flash data into shadow buffer
> + Status = NorFlashReadBlocks (Instance, Lba, Instance->BlockSize,
> Instance->ShadowBuffer);
> + if (EFI_ERROR (Status)) {
> +// Return one of the pre-approved error statuses
> +return EFI_DEVICE_ERROR;
> + }
> +
> + // Put the data at the appropriate location inside the buffer area
> + CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer,
> *NumBytes);
> +
> + // Write the modified buffer back to the NorFlash
> + Status = NorFlashWriteBlocks (Instance, Lba, Instance->BlockSize,
> Instance->ShadowBuffer);
> + if (EFI_ERROR (Status)) {
> +// Return one of the pre-approved error statuses
> +return EFI_DEVICE_ERROR;
> + }
> +
> + return EFI_SUCCESS;
> +}
Good; "git-show --color-moved=zebra" is really helpful here.
What changes across the code movement is the BufferSizeInBytes argument
for the NorFlashReadBlocks / NorFlashWriteBlocks calls. Previously, we'd
pass in the local variable BlockSize, with type UINTN. After, we pass in
Instance->BlockSize, a UINT32. However, this is all fine, given that the
BufferSizeInBytes param of both callees is UINTN, and even the local
variable is assigned from Instance->BlockSize, pre-patch. OK.
> +
> /*
>Write a full or portion of a block. It must not span block boundaries;
> that is,
>Offset + *NumBytes <= Instance->BlockSize.
> @@ -606,7 +637,14 @@ NorFlashWriteSingleBlock (
> // that we want to set. In that case, we will need to erase the block
> first.
> for (CurOffset = 0; CurOffset < *NumBytes; CurOffset++) {
>if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) {
> -goto DoErase;
> +Status = NorFlashWriteSingleBlockWithErase (
> + Instance,
> + Lba,
> + Offset,
> + NumBytes,
> + Buffer
> + );
> +goto Exit;
>}
>
>OrigData[CurOffset] = Buffer[CurOffset];
> @@ -635,33 +673,21 @@ NorFlashWriteSingleBlock (
> goto Exit;
>}
> }
> + } else {
> +Status = NorFlashWriteSingleBlockWithErase (
> + Instance,
> + Lba,
> + Offset,
> + NumBytes,
> + Buffer
> + );
> + }
>
> Exit:
> -// Put device back into Read Array mode
> -SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
> + // Put device back into Read Array mode
> + SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
>
> -return Status;
> - }
> -
> -DoErase:
> - // Read NOR Flash data into shadow buffer
> - Status = NorFlashReadBlocks (Instance, Lba, BlockSize,
> Instance->ShadowBuffer);
> - if (EFI_ERROR (Status)) {
> -// Return one of the pre-approved error statuses
> -return EFI_DEVICE_ERROR;
> - }
> -
> - // Put the data at the appropriate location inside the buffer area
> - CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer,
> *NumBytes);
> -
> - // Write the modified buffer back to the NorFlash
> - Status = NorFlashWriteBlocks (Instance, Lba, BlockSize,
> Instance->ShadowBuffer);
> - if (EFI_ERROR (Status)) {
> -// Return one of the pre-approved error statuses
> -return EFI_DEVICE_ERROR;
> - }
> -
> - return EFI_SUCCESS;
> + return Status;
> }
>
> EFI_STATUS
(2) The extraction of the erase code path is fine, but how it is being
put to use is not identical to the pre-patch state.
The key observation is that, pre-patch, the Exit label is *semantically
local* to the "word-based writes" branch.
Of course this statement makes no sense at the C language level, because
labels are local to functions, not to blocks within functions. Either
way, the original logic is:
- if the logical update is too big (i.e., we don't run the word-based
optimization), just run DoErase. "Exit" is not reached, and we don't put
device back into
Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & TCBZ4118
Gerd I have merged this patch set today. I am fine to remove TPM1.2 in OVMF because of the known security limitation. Thank you Yao, Jiewen > -Original Message- > From: Gerd Hoffmann > Sent: Tuesday, January 16, 2024 8:01 PM > To: devel@edk2.groups.io; dougfl...@microsoft.com > Cc: Douglas Flick [MSFT] ; Yao, Jiewen > > Subject: Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & TCBZ4118 > > On Thu, Jan 11, 2024 at 10:16:00AM -0800, Doug Flick via groups.io wrote: > > This patch series include the combined / merged security patches > > (as seperate commits) for TCBZ4117 (CVE-2022-36763) and TCBZ4118 > > (CVE-2022-36764) for DxeTpm2MeasureBootLib and DxeTpmMeasureBootLib. > > These patches have already been reviewed by SecurityPkg Maintainer > > (Jiewen) on GHSA. > > This patch series breaks ovmf build (duplicate symbols) in case both > TPM2 and TPM1 support are enabled (-D TPM2_ENABLE=TRUE > -DTPM1_ENABLE=TRUE). Compiling with TPM2 only (-D TPM2_ENABLE=TRUE > -DTPM1_ENABLE=FALSE) works fine. > > I see two options to deal with the problem: > > (1) Rename the Sanitize* functions in the TPM2 version of the library > to carry a '2' somewhere in the function name, simliar to all other > TPM2 functions, to avoid the name clash. > (2) Remove TPM1 support from the edk2 code base. The relevance of > TPM 1.2 support should be close to zero given that the TPM 2.0 > specification was released almost a decade ago ... > > take care, > Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113898): https://edk2.groups.io/g/devel/message/113898 Mute This Topic: https://groups.io/mt/103675434/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/BaseXApicX2ApicLib: Implements AMD extended cpu topology
Thanks Gerd for providing the inputs.
I'll enhance the logic accordingly and submit the V2 patch.
Thanks
AbduL
On 16-01-2024 15:11, Gerd Hoffmann wrote:
Caution: This message originated from an External Source. Use proper caution
when opening attachments, clicking links, or responding.
On Tue, Jan 16, 2024 at 12:31:21PM +0530, Abdul Lateef Attar wrote:
From: Abdul Lateef Attar
This patch adds support for AMD's new extended topology.
If processor supports CPUID 8026 leaf then obtain
the topology information using new method.
+ /// Check if extended toplogy supported
+ AsmCpuid (CPUID_EXTENDED_FUNCTION, , NULL, NULL, NULL);
+ if (MaxExtendedCpuIdIndex < AMD_CPUID_EXTENDED_TOPOLOGY) {
+GetProcessorLocationByApicId (InitialApicId, Package, Core, Thread);
+return;
+ }
Sure this is correct?
On Intel processors checking MaxExtendedCpuIdIndex alone is not enough,
see commit 170d4ce8e90a ("UefiCpuPkg/BaseXApicX2ApicLib: fix
CPUID_V2_EXTENDED_TOPOLOGY detection")
Especially in virtual machines it can happen that the vCPU supports
extended cpuid leaf N but does not support N-1, so checking
MaxExtendedCpuIdIndex alone looks rather fragile to me.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113897): https://edk2.groups.io/g/devel/message/113897
Mute This Topic: https://groups.io/mt/103757657/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 5/6] OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too
On 1/15/24 16:59, Gerd Hoffmann wrote:
> It is possible to find variable entries with State being 0xff, i.e. not
> updated since flash block erase. This indicates the variable driver
> could not complete the header write while appending a new entry, and
> therefore State was not set to VAR_HEADER_VALID_ONLY.
>
> This can only happen at the end of the variable list, so treat this as
> additional "end of variable list" condition.
>
> Signed-off-by: Gerd Hoffmann
> ---
> OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> index 8fcd999ac6df..c8b5e0be1379 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> @@ -302,6 +302,11 @@ ValidateFvHeader (
>break;
> }
>
> +if (VarHeader->State == 0xff) {
> + DEBUG ((DEBUG_INFO, "%a: end of var list (unwritten state)\n",
> __func__));
> + break;
> +}
> +
> VarName = NULL;
> switch (VarHeader->State) {
>// usage: State = VAR_HEADER_VALID_ONLY
Reviewed-by: Laszlo Ersek
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113896): https://edk2.groups.io/g/devel/message/113896
Mute This Topic: https://groups.io/mt/103741666/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 4/6] OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
On 1/15/24 16:59, Gerd Hoffmann wrote:
> Raise the limit for writes without block erase from two to four
> P30_MAX_BUFFER_SIZE_IN_BYTES blocks. With this in place almost all efi
> variable updates are handled without block erase. With the old limit
> some variable updates (with device paths) took the block erase code
> path.
>
> Signed-off-by: Gerd Hoffmann
> ---
> OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 18 ++
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 67610d6920f7..d80e9f0a2f3a 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -550,13 +550,15 @@ NorFlashWriteSingleBlock (
> return EFI_BAD_BUFFER_SIZE;
>}
>
> - // Pick P30_MAX_BUFFER_SIZE_IN_BYTES (== 128 bytes) as a good start for
> word
> - // operations as opposed to erasing the block and writing the data
> regardless
> - // if an erase is really needed. It looks like most individual NV variable
> - // writes are smaller than 128 bytes.
> - // To avoid pathological cases were a 2 byte write is disregarded because
> it
> - // occurs right at a 128 byte buffered write alignment boundary, permit up
> to
> - // twice the max buffer size, and perform two writes if needed.
> + // Pick 4 * P30_MAX_BUFFER_SIZE_IN_BYTES (== 512 bytes) as a good
> + // start for word operations as opposed to erasing the block and
> + // writing the data regardless if an erase is really needed.
> + //
> + // Many NV variable updates are small enough for a a single
> + // P30_MAX_BUFFER_SIZE_IN_BYTES block write. In case the update is
> + // larger than a single block, or the update crosses a
> + // P30_MAX_BUFFER_SIZE_IN_BYTES boundary (as shown in the diagram
> + // below), or both, we might have to write two or more blocks.
>//
>//0 128 256
>//[|]
> @@ -577,7 +579,7 @@ NorFlashWriteSingleBlock (
>Start = Offset & ~BOUNDARY_OF_32_WORDS;
>End = ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYTES);
>
> - if ((End - Start) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
> + if ((End - Start) <= (4 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
> // Check to see if we need to erase before programming the data into NOR.
> // If the destination bits are only changing from 1s to 0s we can just
> write.
> // After a block is erased all bits in the block is set to 1.
Reviewed-by: Laszlo Ersek
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113895): https://edk2.groups.io/g/devel/message/113895
Mute This Topic: https://groups.io/mt/103741664/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 3/6] OvmfPkg/VirtNorFlashDxe: add a loop for NorFlashWriteBuffer calls.
On 1/15/24 16:59, Gerd Hoffmann wrote:
> Replace the two NorFlashWriteBuffer() calls with a loop containing a
> single NorFlashWriteBuffer() call.
>
> With the changes in place the code is able to handle updates larger
> than two P30_MAX_BUFFER_SIZE_IN_BYTES blocks, even though the patch
> does not actually change the size limit.
>
> Signed-off-by: Gerd Hoffmann
> ---
> OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 21 -
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 54251633d0ee..67610d6920f7 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -521,6 +521,7 @@ NorFlashWriteSingleBlock (
>UINTN BlockAddress;
>UINT8 *OrigData;
>UINTN Start, End;
> + UINT32 Index, Count;
>
>DEBUG ((DEBUG_BLKIO, "NorFlashWriteSingleBlock(Parameters: Lba=%ld,
> Offset=0x%x, *NumBytes=0x%x, Buffer @ 0x%08x)\n", Lba, Offset, *NumBytes,
> Buffer));
>
> @@ -620,23 +621,17 @@ NorFlashWriteSingleBlock (
>goto Exit;
> }
>
> -Status = NorFlashWriteBuffer (
> - Instance,
> - BlockAddress + Start,
> - P30_MAX_BUFFER_SIZE_IN_BYTES,
> - Instance->ShadowBuffer
> - );
> -if (EFI_ERROR (Status)) {
> - goto Exit;
> -}
> -
> -if ((End - Start) > P30_MAX_BUFFER_SIZE_IN_BYTES) {
> +Count = (End - Start) / P30_MAX_BUFFER_SIZE_IN_BYTES;
> +for (Index = 0; Index < Count; Index++) {
>Status = NorFlashWriteBuffer (
> Instance,
> - BlockAddress + Start + P30_MAX_BUFFER_SIZE_IN_BYTES,
> + BlockAddress + Start + Index * P30_MAX_BUFFER_SIZE_IN_BYTES,
> P30_MAX_BUFFER_SIZE_IN_BYTES,
> - Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BYTES
> + Instance->ShadowBuffer + Index *
> P30_MAX_BUFFER_SIZE_IN_BYTES
> );
> + if (EFI_ERROR (Status)) {
> +goto Exit;
> + }
> }
>
> Exit:
Reviewed-by: Laszlo Ersek
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113894): https://edk2.groups.io/g/devel/message/113894
Mute This Topic: https://groups.io/mt/103741663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 2/6] OvmfPkg/VirtNorFlashDxe: clarify block write logic & fix shadowbuffer reads
On 1/15/24 16:59, Gerd Hoffmann wrote:
> Introduce 'Start' and 'End' variables to make it easier to follow the
> logic and code flow. Also add a ascii art diagram (based on a
> suggestion by Laszlo).
>
> This also fixes the 'Size' calculation for the NorFlashRead() call.
> Without this patch the code will read only one instead of two
> P30_MAX_BUFFER_SIZE_IN_BYTES blocks in case '*NumBytes' is smaller than
> P30_MAX_BUFFER_SIZE_IN_BYTES but 'Offset + *NumBytes' is not, i.e. the
> update range crosses a P30_MAX_BUFFER_SIZE_IN_BYTES boundary.
>
> Signed-off-by: Gerd Hoffmann
> ---
> OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 35 --
> 1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 7f4743b00399..54251633d0ee 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -520,6 +520,7 @@ NorFlashWriteSingleBlock (
>UINTN BlockSize;
>UINTN BlockAddress;
>UINT8 *OrigData;
> + UINTN Start, End;
>
>DEBUG ((DEBUG_BLKIO, "NorFlashWriteSingleBlock(Parameters: Lba=%ld,
> Offset=0x%x, *NumBytes=0x%x, Buffer @ 0x%08x)\n", Lba, Offset, *NumBytes,
> Buffer));
>
> @@ -555,7 +556,27 @@ NorFlashWriteSingleBlock (
>// To avoid pathological cases were a 2 byte write is disregarded because
> it
>// occurs right at a 128 byte buffered write alignment boundary, permit up
> to
>// twice the max buffer size, and perform two writes if needed.
> - if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <= (2 *
> P30_MAX_BUFFER_SIZE_IN_BYTES)) {
> + //
> + //0 128 256
> + //[|]
> + //^ ^ ^ ^
> + //| | | |
> + //| | |End, the next "word" boundary beyond
> + //| | |the (logical) update
> + //| | |
> + //| | (Offset & 0x7F) + NumBytes; i.e., the Offset inside
> + //| | (or just past) the *double-word* such that Offset is
> + //| | the *exclusive* end of the (logical) update.
Obviously, when I proposed this diagram, I messed up this text.
Clearly, there's no better time for making a mistake in a comment than
when complaining about comments... :)
Two warts:
- 0x7F has not been replaced with BOUNDARY_OF_32_WORDS
- the uppercase "Offset" identifier (= proper noun), from my original
proposal, is misleading here. The common noun "offset" is what's need.
So I suggest refreshing it as:
//| | (Offset & BOUNDARY_OF_32_WORDS) + NumBytes;
//| | i.e., the relative offset inside (or just past)
//| | the *double-word* such that it is the
//| | *exclusive* end of the (logical) update.
With that comment update:
Reviewed-by: Laszlo Ersek
Thanks!
Laszlo
> + //| |
> + //| Offset & BOUNDARY_OF_32_WORDS; i.e., Offset within the
> "word";
> + //| this is where the (logical) update is supposed to start
> + //|
> + //Start = Offset & ~BOUNDARY_OF_32_WORDS; i.e., Offset truncated to
> "word" boundary
> +
> + Start = Offset & ~BOUNDARY_OF_32_WORDS;
> + End = ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYTES);
> +
> + if ((End - Start) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
> // Check to see if we need to erase before programming the data into NOR.
> // If the destination bits are only changing from 1s to 0s we can just
> write.
> // After a block is erased all bits in the block is set to 1.
> @@ -565,8 +586,8 @@ NorFlashWriteSingleBlock (
> Status = NorFlashRead (
> Instance,
> Lba,
> - Offset & ~BOUNDARY_OF_32_WORDS,
> - (*NumBytes | BOUNDARY_OF_32_WORDS) + 1,
> + Start,
> + End - Start,
> Instance->ShadowBuffer
> );
> if (EFI_ERROR (Status)) {
> @@ -601,7 +622,7 @@ NorFlashWriteSingleBlock (
>
> Status = NorFlashWriteBuffer (
> Instance,
> - BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
> + BlockAddress + Start,
> P30_MAX_BUFFER_SIZE_IN_BYTES,
> Instance->ShadowBuffer
> );
> @@ -609,12 +630,10 @@ NorFlashWriteSingleBlock (
>goto Exit;
> }
>
> -if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) >
> P30_MAX_BUFFER_SIZE_IN_BYTES) {
> - BlockAddress += P30_MAX_BUFFER_SIZE_IN_BYTES;
> -
> +if ((End - Start) > P30_MAX_BUFFER_SIZE_IN_BYTES) {
>Status = NorFlashWriteBuffer (
> Instance,
> - BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
> + BlockAddress + Start + P30_MAX_BUFFER_SIZE_IN_BYTES,
>
Re: [edk2-devel] [PATCH v7 23/37] ArmVirtPkg: Move the FdtSerialPortAddressLib to OvmfPkg
Hi Laszlo,
Thanks,
Chao
On 2024/1/15 16:32, Laszlo Ersek wrote:
On 1/12/24 09:25, Chao Li wrote:
Move the FdtSerialPortAddressLib to Ovmfpkg so that other ARCH can
easily use it.
Build-tested only (with "ArmVirtQemu.dsc").
BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=4584
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Cc: Sami Mujawar
Cc: Gerd Hoffmann
Cc: Jiewen Yao
Cc: Laszlo Ersek
Signed-off-by: Chao Li
---
ArmVirtPkg/ArmVirt.dsc.inc| 2 +-
.../Include/Library/FdtSerialPortAddressLib.h | 0
.../Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c | 0
.../FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf | 2 +-
OvmfPkg/OvmfPkg.dec | 4
5 files changed, 6 insertions(+), 2 deletions(-)
rename {ArmVirtPkg => OvmfPkg}/Include/Library/FdtSerialPortAddressLib.h
(100%)
rename {ArmVirtPkg =>
OvmfPkg}/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c (100%)
rename {ArmVirtPkg =>
OvmfPkg}/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf (90%)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 9b23ef97ec..2bc6a29eb1 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -122,7 +122,7 @@
# ARM PL011 UART Driver
PL011UartLib|ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.inf
SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf
-
FdtSerialPortAddressLib|ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
+
FdtSerialPortAddressLib|OvmfPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
PeCoffExtraActionLib|ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
#PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf
diff --git a/ArmVirtPkg/Include/Library/FdtSerialPortAddressLib.h
b/OvmfPkg/Include/Library/FdtSerialPortAddressLib.h
similarity index 100%
rename from ArmVirtPkg/Include/Library/FdtSerialPortAddressLib.h
rename to OvmfPkg/Include/Library/FdtSerialPortAddressLib.h
diff --git
a/ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c
b/OvmfPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c
similarity index 100%
rename from ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c
rename to OvmfPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c
diff --git
a/ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
b/OvmfPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
similarity index 90%
rename from
ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
rename to OvmfPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
index ae6d0d374b..e27742e9fa 100644
--- a/ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
+++ b/OvmfPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
@@ -18,9 +18,9 @@
FdtSerialPortAddressLib.c
[Packages]
- ArmVirtPkg/ArmVirtPkg.dec
EmbeddedPkg/EmbeddedPkg.dec
MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
[LibraryClasses]
BaseLib
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 7bc2bf1674..13e69e6648 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -29,6 +29,10 @@
## @libraryclass Verify blobs read from the VMM
BlobVerifierLib|Include/Library/BlobVerifierLib.h
+ ## @libraryclass FdtSerialPortAddressLib
+ #
+ FdtSerialPortAddressLib|Include/Library/FdtSerialPortAddressLib.h
+
## @libraryclass Loads and boots a Linux kernel image
#
LoadLinuxLib|Include/Library/LoadLinuxLib.h
IIUC, the library class is not removed from "ArmVirtPkg.dec"; please do
that as well, in the same patch.
Yes, the changes to ArmVirtPkg.dec is in the patch 25, it should belong
to this patch, I will fix it in V8.
Otherwise, the patch looks OK to me.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113892): https://edk2.groups.io/g/devel/message/113892
Mute This Topic: https://groups.io/mt/103679474/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/2] ShellPkg: Update smbiosview for LoongArch
Looks good to me.
Reviewed-by: Chao Li
Hi Zhichao,
Please help review this patch.
Thanks,
Chao
On 2024/1/16 15:11, Dongyan Qian wrote:
According to SMBIOS spec3.6, LoongArch information support has been added,
so this patch is submitted for display as information in smbiosview.
Cc: Zhichao Gao
Cc: Chao Li
Signed-off-by: Dongyan Qian
---
.../SmbiosView/PrintInfo.c| 68 +++
.../SmbiosView/QueryTable.c | 8 +++
2 files changed, 76 insertions(+)
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
index b6968cb36a..ba6e7b15fc 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
@@ -2610,6 +2610,74 @@ DisplayProcessorFamily2 (
Print (L"RISC-V RV128\n");
break;
+case 0x258:
+ Print (L"LoongArch\n");
+ break;
+
+case 0x259:
+ Print (L"Loongson1\n");
+ break;
+
+case 0x25a:
+ Print (L"Loongson2\n");
+ break;
+
+case 0x25b:
+ Print (L"Loongson3\n");
+ break;
+
+case 0x25c:
+ Print (L"Loongson2K\n");
+ break;
+
+case 0x25d:
+ Print (L"Loongson3A\n");
+ break;
+
+case 0x25e:
+ Print (L"Loongson3B\n");
+ break;
+
+case 0x25f:
+ Print (L"Loongson3C\n");
+ break;
+
+case 0x260:
+ Print (L"Loongson3D\n");
+ break;
+
+case 0x261:
+ Print (L"Loongson3E\n");
+ break;
+
+case 0x262:
+ Print (L"DualCoreLoongson2K\n");
+ break;
+
+case 0x26C:
+ Print (L"QuadCoreLoongson3A\n");
+ break;
+
+case 0x26D:
+ Print (L"MultiCoreLoongson3A\n");
+ break;
+
+case 0x26E:
+ Print (L"QuadCoreLoongson3B\n");
+ break;
+
+case 0x26F:
+ Print (L"MultiCoreLoongson3B\n");
+ break;
+
+case 0x270:
+ Print (L"MultiCoreLoongson3C\n");
+ break;
+
+case 0x271:
+ Print (L"MultiCoreLoongson3D\n");
+ break;
+
default:
ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
(STR_SMBIOSVIEW_PRINTINFO_UNDEFINED_PROC_FAMILY), gShellDebug1HiiHandle);
}
diff --git
a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c
b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c
index 82bb7a41f0..f57093c91c 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/QueryTable.c
@@ -3652,6 +3652,14 @@ TABLE_ITEM ProcessorArchitectureTypesTable[] = {
{
8,
L" 128-bit RISC-V (RV128) "
+ },
+ {
+9,
+L" 32-bit LoongArch (LoongArch32) "
+ },
+ {
+10,
+L" 64-bit LoongArch (LoongArch64) "
}
};
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113891): https://edk2.groups.io/g/devel/message/113891
Mute This Topic: https://groups.io/mt/103757733/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] MdeModulePkg: Dxe: add LOONGARCH64 to mMachineTypeInfo
Looks good to me.
Reviewed-by: Chao Li
Hi Liming,
Please help review this patch.
Thanks,
Chao
On 2024/1/16 15:11, Dongyan Qian wrote:
This fixes messages like:
"Image type X64 can't be loaded on UEFI system"
Cc: Liming Gao
Cc: Chao Li
Signed-off-by: Dongyan Qian
---
MdeModulePkg/Core/Dxe/Image/Image.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
b/MdeModulePkg/Core/Dxe/Image/Image.c
index 6bc3a549ae..66d9840b7b 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -77,12 +77,13 @@ typedef struct {
} MACHINE_TYPE_INFO;
GLOBAL_REMOVE_IF_UNREFERENCED MACHINE_TYPE_INFO mMachineTypeInfo[] = {
- { EFI_IMAGE_MACHINE_IA32, L"IA32"},
- { EFI_IMAGE_MACHINE_IA64, L"IA64"},
- { EFI_IMAGE_MACHINE_X64,L"X64" },
- { EFI_IMAGE_MACHINE_ARMTHUMB_MIXED, L"ARM" },
- { EFI_IMAGE_MACHINE_AARCH64,L"AARCH64" },
- { EFI_IMAGE_MACHINE_RISCV64,L"RISCV64" },
+ { EFI_IMAGE_MACHINE_IA32, L"IA32"},
+ { EFI_IMAGE_MACHINE_IA64, L"IA64"},
+ { EFI_IMAGE_MACHINE_X64,L"X64" },
+ { EFI_IMAGE_MACHINE_ARMTHUMB_MIXED, L"ARM" },
+ { EFI_IMAGE_MACHINE_AARCH64,L"AARCH64" },
+ { EFI_IMAGE_MACHINE_RISCV64,L"RISCV64" },
+ { EFI_IMAGE_MACHINE_LOONGARCH64,L"LOONGARCH64" },
};
UINT16 mDxeCoreImageMachineType = 0;
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113890): https://edk2.groups.io/g/devel/message/113890
Mute This Topic: https://groups.io/mt/103757732/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & TCBZ4118
On Thu, Jan 11, 2024 at 10:16:00AM -0800, Doug Flick via groups.io wrote: > This patch series include the combined / merged security patches > (as seperate commits) for TCBZ4117 (CVE-2022-36763) and TCBZ4118 > (CVE-2022-36764) for DxeTpm2MeasureBootLib and DxeTpmMeasureBootLib. > These patches have already been reviewed by SecurityPkg Maintainer > (Jiewen) on GHSA. This patch series breaks ovmf build (duplicate symbols) in case both TPM2 and TPM1 support are enabled (-D TPM2_ENABLE=TRUE -DTPM1_ENABLE=TRUE). Compiling with TPM2 only (-D TPM2_ENABLE=TRUE -DTPM1_ENABLE=FALSE) works fine. I see two options to deal with the problem: (1) Rename the Sanitize* functions in the TPM2 version of the library to carry a '2' somewhere in the function name, simliar to all other TPM2 functions, to avoid the name clash. (2) Remove TPM1 support from the edk2 code base. The relevance of TPM 1.2 support should be close to zero given that the TPM 2.0 specification was released almost a decade ago ... take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113889): https://edk2.groups.io/g/devel/message/113889 Mute This Topic: https://groups.io/mt/103675434/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 25/37] ArmVirtPkg: Move PlatformBootManagerLib to OvmfPkg
Hi Laszlo,
Thanks,
Chao
On 2024/1/15 16:46, Laszlo Ersek wrote:
On 1/12/24 09:25, Chao Li wrote:
Moved the PlatformBootManagerLib to OvmfPkg and renamed to
PlatformBootManagerLibLight for easy use by other ARCH.
Build-tested only (with "ArmVirtQemu.dsc").
BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=4584
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Cc: Sami Mujawar
Cc: Gerd Hoffmann
Cc: Jiewen Yao
Cc: Lazlo Ersek
Signed-off-by: Chao Li
---
ArmVirtPkg/ArmVirtPkg.ci.yaml | 1 -
ArmVirtPkg/ArmVirtPkg.dec | 1 -
ArmVirtPkg/ArmVirtQemu.dsc | 2 +-
ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 +-
.../Library/PlatformBootManagerLibLight}/PlatformBm.c | 0
.../Library/PlatformBootManagerLibLight}/PlatformBm.h | 0
.../PlatformBootManagerLibLight}/PlatformBootManagerLib.inf| 3 +--
.../Library/PlatformBootManagerLibLight}/QemuKernel.c | 0
8 files changed, 3 insertions(+), 6 deletions(-)
rename {ArmVirtPkg/Library/PlatformBootManagerLib =>
OvmfPkg/Library/PlatformBootManagerLibLight}/PlatformBm.c (100%)
rename {ArmVirtPkg/Library/PlatformBootManagerLib =>
OvmfPkg/Library/PlatformBootManagerLibLight}/PlatformBm.h (100%)
rename {ArmVirtPkg/Library/PlatformBootManagerLib =>
OvmfPkg/Library/PlatformBootManagerLibLight}/PlatformBootManagerLib.inf (92%)
rename {ArmVirtPkg/Library/PlatformBootManagerLib =>
OvmfPkg/Library/PlatformBootManagerLibLight}/QemuKernel.c (100%)
diff --git a/ArmVirtPkg/ArmVirtPkg.ci.yaml b/ArmVirtPkg/ArmVirtPkg.ci.yaml
index 506b0e72f0..b186d4eb42 100644
--- a/ArmVirtPkg/ArmVirtPkg.ci.yaml
+++ b/ArmVirtPkg/ArmVirtPkg.ci.yaml
@@ -24,7 +24,6 @@
],
## Both file path and directory path are accepted.
"IgnoreFiles": [
-"Library/PlatformBootManagerLib/PlatformBm.c"
]
},
## options defined .pytool/Plugin/CompilerPlugin
OK
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 315db4e8ea..6aa5ea05f4 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -27,7 +27,6 @@
[LibraryClasses]
ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
- FdtSerialPortAddressLib|Include/Library/FdtSerialPortAddressLib.h
[Guids.common]
gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E,
0x36, 0x5B, 0x80, 0x63, 0x66 } }
This hunk belongs to patch "ArmVirtPkg: Move the FdtSerialPortAddressLib
to OvmfPkg".
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 147180f645..e48c75b5e9 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -70,7 +70,7 @@
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
-
PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+
PlatformBootManagerLib|OvmfPkg/Library/PlatformBootManagerLibLight/PlatformBootManagerLib.inf
PlatformBmPrintScLib|OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
OK
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index c22a422353..668a65ba64 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -69,7 +69,7 @@
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
-
PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+
PlatformBootManagerLib|OvmfPkg/Library/PlatformBootManagerLibLight/PlatformBootManagerLib.inf
PlatformBmPrintScLib|OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
OK
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
b/OvmfPkg/Library/PlatformBootManagerLibLight/PlatformBm.c
similarity index 100%
rename from ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
rename to OvmfPkg/Library/PlatformBootManagerLibLight/PlatformBm.c
OK
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.h
b/OvmfPkg/Library/PlatformBootManagerLibLight/PlatformBm.h
similarity index 100%
rename from ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.h
rename to OvmfPkg/Library/PlatformBootManagerLibLight/PlatformBm.h
OK
diff --git
a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
b/OvmfPkg/Library/PlatformBootManagerLibLight/PlatformBootManagerLib.inf
similarity index 92%
rename from
Re: [edk2-devel] [PATCH v7 00/37] Enable LoongArch virtual machine in edk2
Hi Laszlo, Thanks, Chao On 2024/1/15 16:33, Laszlo Ersek wrote: On 1/12/24 09:21, Chao Li wrote: **Changes from V6 to V7:** [...] For the review opinions: 1. Moved the changes to OvmfPkg.dec from old patch 24 to new patch 23. Questioner: Laszlo. IIUC, you may have inteded to do this, but didn't actually do it. I did do that in this patches set, please check the patch 24 in V6, the changes to OvmPkg.dec have been moved moved to V7 patch 23. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113887): https://edk2.groups.io/g/devel/message/113887 Mute This Topic: https://groups.io/mt/103679423/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] When TPM is enabled, Ubuntu doesn't boot
On 1/15/24 13:34, Hamit Can Karaca wrote: > Hi Yao, Jiewen, > I actually tried to get help from the ubuntu people but they really > don't understand what is going in the UEFI side. I am trying to fix > this problem for 3 weeks now and I am about the give up. I hope > somebody can help me :( The log you attached up-thread ends like this: > FSOpen: Open '\EFI\ubuntu\BOOTX64.CSV' Success > Tcg2GetCapability ... > Size - 0x24 > 1.1 - 0x24, 1.0 - 0x1C > Tcg2GetCapability - Success > Reset System > PROGRESS CODE: V0311100A I0 > DXE ResetSystem2: ResetType Cold, Call Depth = 1. this is consistent with the "shim" project's "fallback" utility resetting your system, when the TPM is enabled. In particular, the "Reset System" message is printed by "fallback.c". For getting a more verbose log, you can try setting the FALLBACK_VERBOSE UEFI variable: - boot Ubuntu with the TPM disabled (for now) - in a root shell, issue the following command: mokutil --set-fallback-verbosity true - reboot (then reenable the TPM) This would give us some extra insight into fallback's thinking / process. However, even without the verbose fallback log, we can speculate. In particular, in the fallback code and commit log, I've found the following commit: > commit 431b8a2e75a71a0b1f47d47d3f045b1e3efbce53 > Author: Peter Jones > Date: Mon Jul 31 13:10:41 2017 -0400 > > Make fallback aware of tpm measurements, and reboot if tpm is used. > > Since booting the entry with fallback in the stack of things that got > measured will result in all the wrong PCR values, in the cases where TPM > is present and enabled, use ->Reset() instead of loading the Boot > variable and executing its target. > > Signed-off-by: Peter Jones The idea is that: (1) you have an installed Ubuntu system (2) for some reason, you don't have proper UEFI Boot options, matching your installed Ubuntu OS. (3) consequently, your platform firmware loads the default UEFI boot loader, not a specific, designated OS boot loader (4) it is actually "shim" that fills both roles (default / fallback boot loader, and specific first stage OS boot loader too), however, shim behaves differently, dependent on which role it is being invoked in. In the former role, it invokes (IIUC) the "fallback" utility (also built from the shim project). (5) what the fallback utility does is that it tries to *recreate* your missing UEFI Boot variables, and then *at once* proceed to booting your Ubuntu OS. For the recreation of the variables, fallback looks at the file "\EFI\ubuntu\BOOTX64.CSV", which is basically an alternative (disk-based fallback) storage for boot option information. That's why you see that file mentioned in the log; the file system driver reports opening that file. (6) Now, where I write "at once proceeds to booting your Ubuntu OS" is where the TPM plays a part, and where the above-quoted commit is relevant. The TPM measures the entire boot path, and if the boot path changes, the values (= hash results) in the PCRs (platform config registers) will not match those that the TPM expects. This means that the TPM will not "unseal" secrets for you, such as a full disk (LUKS) encryption key. Now, including the fallback utility in the boot process is certainly a boot path change, so when fallback runs, and it detects that a TPM is enabled (because it finds TPM-related UEFI protocols in the protocol database), it *knows* that the TPM will "catch" the change in the boot path. Therefore, after recreating the missing UEFI Boot variables (boot options), fallback decides to *reboot* -- i.e., relaunch the boot path from zero. Because next time around, the just-recreated Boot options should take effect (i.e., fallback should not be part of the next boot path), and then TPM should be happy too. (7) Your problem actually seems to be that the Boot option recreation step fails! The *first* reset itself is fine and justified (with the TPM enabled); the problem is that fallback is reached during *next boot* again. That should not happen; the Boot options should be in place by then. Your platform firmware (= motherboard) may be busted by design. Some platform vendors do not permit the modification of Boot options. This is actually "platform policy", so it is not a UEFI spec violation per se. But the end result is that "fallback" cannot modify Boot options, so it always try to re-create them, and always resets due to the TPMs presence. If you really want to enable the TPM, you might have to use the "FB_NO_REBOOT" UEFI variable of shim. This is described in the following shim commit: > commit a5db51a52e8d4cae938fc807b991383309dffca7 > Author: Gary Lin > Date: Wed May 23 18:13:05 2018 +0800 > > fallback: show a countdown menu before reset > > Some machines with the faulty firmware may keep booting the default boot > path instead of the boot option we create. To avoid the infinite reset > loop, this commit
Re: [edk2-devel] [PATCH 0/4] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements
On Tue, 16 Jan 2024 at 10:37, Laszlo Ersek wrote: > > On 1/15/24 18:56, Ard Biesheuvel wrote: > > On Mon, 15 Jan 2024 at 11:21, Laszlo Ersek wrote: > >> > >> On 1/12/24 12:37, Gerd Hoffmann wrote: > >>> This is a little series containing the flash corruption fix sent > >>> yesterday with an slightly improved commit message and some small > >>> improvements on top of this. > >>> > >>> Gerd Hoffmann (4): > >>> OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads > >>> OvmfPkg/VirtNorFlashDxe: clarify block write logic > >>> OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase > >>> OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too > >>> > >>> OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c| 33 +++ > >>> OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 5 > >>> 2 files changed, 21 insertions(+), 17 deletions(-) > >>> > >> > >> Looking at the original code makes me throw a fit (no offense -- I don't > >> know who wrote it, and I don't want to check). > >> > > > > Hi Laszlo, > > > > I am not the author of the original code, but I suppose I should take > > at least some of the blame here, having added some of the logic to > > reduce the number of MMIO accesses (which are disproportionately > > expensive under virtualization), and this is where the bug got > > introduced afaict. > > ... sorry about being needlessly harsh. If it's any excuse: in all such > cases I make a fully committed, honest effort to dig down to the "roots" > of the code, and the more I struggle to form a mental image, the more > annoyed/stressed I get. Comments and diagrams would definitely help with > my efforts, but just because I get annoyed during first analysis, that > is not sufficient reason to let that *leak* to the list. It's a > personality defect on my end. I'll keep working on it. > Don't worry about it, really. I don't mind unfiltered criticism from long-time collaborators as long as it is constructive - email is such a lossy medium in terms of subtext that I'd rather suffer a minor ego bruise than having to unwrap layers of politeness to get at the real meaning. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113885): https://edk2.groups.io/g/devel/message/113885 Mute This Topic: https://groups.io/mt/103680930/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver
On 1/15/24 19:10, Kinney, Michael D wrote: > Hi Ray, > > I think nesting may be possible in physical platforms, but very hard > to induce. > > One option is to consolidate to a single LocalApicTimerDxe > implementation in the UefiCpuPkg, but allow the platform DSC to either > specify a Null NestedInterruptTplLib for physical platforms or the > full one from the OvmfPkg for VM use cases. > > The other changes could be included for OvmfPkg use cases. If the VM > does not support the CPUID leafs, then the PCD value should be used. All of this sounds great! (And I do think that *some* nesting should not be a problem, on either physical or virtual platforms, as (IIRC) the interrupt handler stack can accommodate a certain level of reentrancy. It's the "storm" that is a problem, but that should never occur on physical machines, I reckon.) Assuming a v2 is coming up, my advance request would be for Ray to re-review the math in patch #4, before posting v2, focusing on integer overflows, and SafeIntLib (if needed). I've not looked at the patch in detail yet, but I reviewed something similar not so long ago [1]. The latter frequency calculation code had been quite commonly used in edk2, and I needed hours to review just that one patch. Plus, the review turned up a number of issues to fix. So, preferably such a patch should not only prevent any integer overflows, but also clarify, in comments, why overflows are impossible, and/or how they are prevented. [1] https://edk2.groups.io/g/devel/message/111613 2e42db7c-a927-f47b-7d80-632895b11e1b@redhat.com">http://mid.mail-archive.com/2e42db7c-a927-f47b-7d80-632895b11e1b@redhat.com Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113884): https://edk2.groups.io/g/devel/message/113884 Mute This Topic: https://groups.io/mt/103734961/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver
On 16/01/2024 08:47, Gerd Hoffmann wrote: I think the reason is that the next timer interrupt arriving while the handler is running still is *much* more likely in virtual machines because the vCPU does not get 100% of the of the physical CPU time slice. The interrupt handler can run for an arbitrary length of time, because the call to RestoreTPL() can end up calling an application callback which in turn waits on further timer interrupts. This is a legitimate use case within UEFI, so all timer interrupt handlers (not just in virtual machines) need to allow for the possibility that nested interrupts will occur. So on OVMF we will continue to need NestedInterruptTplLib. I like the idea to have a Null version of the library, so OVMF does not need its own version of the driver just because of NestedInterruptTplLib. I agree that there should not need to be two versions of LocalApicTimerDxe. I would suggest moving NestedInterruptTplLib from OvmfPkg to MdeModulePkg. The code is complex, but for the caller the complexity is completely hidden behind the calls to NestedInterruptRaiseTPL() and NestedInterruptRestoreTPL(), which straightforwardly replace what would otherwise be (unsafe) calls to RaiseTPL() and RestoreTPL(), as shown in https://github.com/tianocore/edk2/commit/a086f4a#diff-1418ec21e24e4ce2f3d621113585e3bea11fbcfa3b77d54046e61be7928e0c92 For a new CPU architecture, the only requirement is to provide a short fragment (~5 lines) of code that can clear the interrupts-enabled flag in the EFI_SYSTEM_CONTEXT, as shown in OvmfPkg/Library/NestedInterruptTplLib/Iret.c. I'm happy to send a patch to migrate NestedInterruptTplLib to MdeModulePkg, so that it can be consumed outside of OvmfPkg. Shall I do this? Thanks, Michael -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113883): https://edk2.groups.io/g/devel/message/113883 Mute This Topic: https://groups.io/mt/103734961/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/BaseXApicX2ApicLib: Implements AMD extended cpu topology
On Tue, Jan 16, 2024 at 12:31:21PM +0530, Abdul Lateef Attar wrote:
> From: Abdul Lateef Attar
>
> This patch adds support for AMD's new extended topology.
> If processor supports CPUID 8026 leaf then obtain
> the topology information using new method.
> + /// Check if extended toplogy supported
> + AsmCpuid (CPUID_EXTENDED_FUNCTION, , NULL, NULL,
> NULL);
> + if (MaxExtendedCpuIdIndex < AMD_CPUID_EXTENDED_TOPOLOGY) {
> +GetProcessorLocationByApicId (InitialApicId, Package, Core, Thread);
> +return;
> + }
Sure this is correct?
On Intel processors checking MaxExtendedCpuIdIndex alone is not enough,
see commit 170d4ce8e90a ("UefiCpuPkg/BaseXApicX2ApicLib: fix
CPUID_V2_EXTENDED_TOPOLOGY detection")
Especially in virtual machines it can happen that the vCPU supports
extended cpuid leaf N but does not support N-1, so checking
MaxExtendedCpuIdIndex alone looks rather fragile to me.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113882): https://edk2.groups.io/g/devel/message/113882
Mute This Topic: https://groups.io/mt/103757657/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/4] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements
On 1/15/24 18:56, Ard Biesheuvel wrote: > On Mon, 15 Jan 2024 at 11:21, Laszlo Ersek wrote: >> >> On 1/12/24 12:37, Gerd Hoffmann wrote: >>> This is a little series containing the flash corruption fix sent >>> yesterday with an slightly improved commit message and some small >>> improvements on top of this. >>> >>> Gerd Hoffmann (4): >>> OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads >>> OvmfPkg/VirtNorFlashDxe: clarify block write logic >>> OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase >>> OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too >>> >>> OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c| 33 +++ >>> OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 5 >>> 2 files changed, 21 insertions(+), 17 deletions(-) >>> >> >> Looking at the original code makes me throw a fit (no offense -- I don't >> know who wrote it, and I don't want to check). >> > > Hi Laszlo, > > I am not the author of the original code, but I suppose I should take > at least some of the blame here, having added some of the logic to > reduce the number of MMIO accesses (which are disproportionately > expensive under virtualization), and this is where the bug got > introduced afaict. ... sorry about being needlessly harsh. If it's any excuse: in all such cases I make a fully committed, honest effort to dig down to the "roots" of the code, and the more I struggle to form a mental image, the more annoyed/stressed I get. Comments and diagrams would definitely help with my efforts, but just because I get annoyed during first analysis, that is not sufficient reason to let that *leak* to the list. It's a personality defect on my end. I'll keep working on it. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113881): https://edk2.groups.io/g/devel/message/113881 Mute This Topic: https://groups.io/mt/103680930/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver
Hi, > Right, I have the same question -- although, admittedly, I've not been > CC'd on the cover letter (0/6), and the reason could be captured there. It wasn't. > On a second thought, I'm (retroactively?) surprised that > LocalApicTimerDxe was (apparently?) first introduced under OvmfPkg -- > i.e., for VMs? That's not impossible, but feels a bit strange. I think the reason is that the next timer interrupt arriving while the handler is running still is *much* more likely in virtual machines because the vCPU does not get 100% of the of the physical CPU time slice. So on OVMF we will continue to need NestedInterruptTplLib. I like the idea to have a Null version of the library, so OVMF does not need its own version of the driver just because of NestedInterruptTplLib. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113880): https://edk2.groups.io/g/devel/message/113880 Mute This Topic: https://groups.io/mt/103734961/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-redfish-client][PATCH 3/3] RedfishClientPkg/ConverterLib: Function to remove Redfish unchangeable properties
[AMD Official Use Only - General] Got it and thanks. I will fix the typo, put your RB then merge this patch. Abner > -Original Message- > From: Nickle Wang > Sent: Tuesday, January 16, 2024 2:20 PM > To: Chang, Abner ; devel@edk2.groups.io > Cc: Igor Kulchytskyy > Subject: RE: [edk2-redfish-client][PATCH 3/3] RedfishClientPkg/ConverterLib: > Function to remove Redfish unchangeable properties > > [AMD Official Use Only - General] > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > > I didn't choose ReadOnly is because at BIOS perspective as a provider, we > > still > > update the read only property defined in schema such as BootOptions. Thus > I > > thought to use ReadOnly is not quite accurate. How do you think? > > I see. Thanks for your explanation. Please fix the typos and keep the function > name untouched. > > Regards, > Nickle > > > -Original Message- > > From: Chang, Abner > > Sent: Tuesday, January 16, 2024 12:08 PM > > To: Nickle Wang ; devel@edk2.groups.io > > Cc: Igor Kulchytskyy > > Subject: RE: [edk2-redfish-client][PATCH 3/3] > RedfishClientPkg/ConverterLib: > > Function to remove Redfish unchangeable properties > > > > External email: Use caution opening links or attachments > > > > > > [AMD Official Use Only - General] > > > > > -Original Message- > > > From: Nickle Wang > > > Sent: Tuesday, January 16, 2024 11:18 AM > > > To: Chang, Abner ; devel@edk2.groups.io > > > Cc: Igor Kulchytskyy > > > Subject: RE: [edk2-redfish-client][PATCH 3/3] > RedfishClientPkg/ConverterLib: > > > Function to remove Redfish unchangeable properties > > > > > > Caution: This message originated from an External Source. Use proper > > > caution when opening attachments, clicking links, or responding. > > > > > > > > > I found typos. Please see below. > > > > > > Regards, > > > Nickle > > > > > > > -Original Message- > > > > From: abner.ch...@amd.com > > > > Sent: Friday, January 12, 2024 11:26 AM > > > > To: devel@edk2.groups.io > > > > Cc: Nickle Wang ; Igor Kulchytskyy > > > > > > > > Subject: [edk2-redfish-client][PATCH 3/3] > RedfishClientPkg/ConverterLib: > > > > Function to remove Redfish unchangeable properties > > > > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > From: Abner Chang > > > > > > > > Update RedfishCsCommon.c to add a function to remove Redfish > > > unchangeable > > > > properties. > > > > > > > > Signed-off-by: Abner Chang > > > > Cc: Nickle Wang > > > > Cc: Igor Kulchytskyy > > > > --- > > > > .../ConverterLib/include/RedfishCsCommon.h| 20 +++ > > > > .../ConverterLib/src/RedfishCsCommon.c| 55 > +++ > > > > 2 files changed, 75 insertions(+) > > > > > > > > diff --git a/RedfishClientPkg/ConverterLib/include/RedfishCsCommon.h > > > > b/RedfishClientPkg/ConverterLib/include/RedfishCsCommon.h > > > > index e454ab0b73..f5278015aa 100644 > > > > --- a/RedfishClientPkg/ConverterLib/include/RedfishCsCommon.h > > > > +++ b/RedfishClientPkg/ConverterLib/include/RedfishCsCommon.h > > > > @@ -104,6 +104,26 @@ DestoryCsMemory ( > > > >RedfishCS_void *rootCs > > > >); > > > > > > > > +/** > > > > + This function removes the unchangeable Redfish properties from > > > > +JSON raw text > > > > + The content in JsonString is left unmodified, the caller has to > > > > +give enoungh > > > > + memory pointed by NewJsonBuffer in the size of BufferSize. > > > > + > > > > + JsonString Input JSON raw string > > > > + NewJsonBuffer Pointer to memory for the updated JSON raw string in > > > > + size of BuufferSize. > > > > + BuufferSizeThe buffer size of NewJsonBuffer > > > > + > > > > + Return RedfishCS_status. > > > > + > > > > +**/ > > > > +RedfishCS_status > > > > +RemoveUnchangeableProperties ( > > > > + RedfishCS_char *JsonString, > > > > + RedfishCS_char *NewJsonBuffer, > > > > + RedfishCS_uint32 BuufferSize > > > > + ); > > > > > > BufferSize. You have double 'u' above. And how about to use ReadOnly > > > instead of "Unchangeable"? > > Thanks for catching the typo. > > > > I didn't choose ReadOnly is because at BIOS perspective as a provider, we > > still > > update the read only property defined in schema such as BootOptions. Thus > I > > thought to use ReadOnly is not quite accurate. How do you think? > > > > Abner > > > > > > > > > > > > + > > > > typedef struct _RedfishCS_char_Array RedfishCS_char_Array; > > > > typedef struct _RedfishCS_int64_Array RedfishCS_int64_Array; > > > > typedef struct _RedfishCS_bool_Array RedfishCS_bool_Array; > > > > diff --git a/RedfishClientPkg/ConverterLib/src/RedfishCsCommon.c > > > > b/RedfishClientPkg/ConverterLib/src/RedfishCsCommon.c > > > > index fd31e5296b..c6996d7d5d 100644 > > > > --- a/RedfishClientPkg/ConverterLib/src/RedfishCsCommon.c > > > > +++
