I can't find patch 1 in the series in my email so putting a few comments
here. I really hope this patch series can wait for PRs so code comments
can more easily be correlated with code change.
Looking at your PR with commit: Allocate Varaible cache buffer in PEI by
td36 · Pull Request #5607 · tianocore/edk2 (github.com)
<https://github.com/tianocore/edk2/pull/5607/commits/36c2cac5fa4196be7fc85d842e8056e376010479>
A few comments for now.
Variable is spelled incorrectly in commit message.
I would really prefer to not mix PCDs and Hobs. It is really confusing
what has to be turned on and set to what to get the different
behaviors. For a hob producer it is OK to take that info from PCDs but
lets not mix in the consumer code PCDs and hob data. The HOB definition
should not reference a PCD and a HOB definition should not be focused on
producer/consumer but on the data.
The existence of a hob is also a good indicator that a feature is used
so you may not need to have "enable" PCDs anymore.
Please don't include other header files in public header files
(especially for super common headers like PiPei.h. i know over the last
few years this practice has become more common but it just creates pain
when debugging build errors. The trade off is not worth it.
Hobs really shouldn't use UINTN sizes. Since hobs helps transfer config
state across phases where the size of UINTN may be different this causes
problems.
Hobs used for cross phase sharing shouldn't have pointers for the reason.
Better and more complete comments on the different field members would
be helpful. I assume the "Buffer" fields are physical addresses and the
"Pages" are number of 4K pages. I would suggest EFI_PHYSICAL_ADDRESS
for addresses and UINT64 for lengths.
More details on what the three cache's are would be helpful or at least
reference the feature of the cache they are supporting.
This hob definition file isn't perfect and I believe some of the comment
in the file header belong in different places, it is at least a good
template for a hob definition.
edk2/MdeModulePkg/Include/Guid/VariableFlashInfo.h at
284dbac43da752ee34825c8b3f6f9e8281cb5a19 · tianocore/edk2 (github.com)
<https://github.com/tianocore/edk2/blob/284dbac43da752ee34825c8b3f6f9e8281cb5a19/MdeModulePkg/Include/Guid/VariableFlashInfo.h>
Thanks
Sean
On 5/17/2024 2:49 AM, duntan wrote:
This patch set defines a new VARIABLE_RUNTIME_CACHE_INFO HOB. The HOB is used
to store the address and size of the buffer that will be used for variable
runtime service when the PcdEnableVariableRuntimeCache is TRUE.
In following patches, when PcdEnableVariableRuntimeCache is TRUE, VariablePei
will install a callback of gEfiPeiMemoryDiscoveredPpiGuid to allocate the
needed buffer for different type variable runtime cache and build the HOB.
Then VariableSmmRuntimeDxe driver will consume
gEdkiiVariableRuntimeCacheInfoHobGuid to initialize the variable runtime cache
related content. The code to allocate and unblock the runtime cache buffer in
VariableSmmRuntimeDxe is also removed in this patc set.
PR for review:https://github.com/tianocore/edk2/pull/5607
Cc: Ray Ni<ray...@intel.com>
Cc: Liming Gao<gaolim...@byosoft.com.cn>
Cc: Jiaxin Wu<jiaxin...@intel.com>
Cc: Ard Biesheuvel<ardb+tianoc...@kernel.org>
Cc: Leif Lindholm<quic_llind...@quicinc.com>
Cc: Sami Mujawar<sami.muja...@arm.com>
Cc: Gerd Hoffmann<kra...@redhat.com>
Cc: Andrew Fish<af...@apple.com>
Cc: Jiewen Yao<jiewen....@intel.com>
Dun Tan (9):
MdeModulePkg:Add new gEdkiiVariableRuntimeCacheInfoHobGuid
ArmVirtPkg: Add MmUnblockMemoryLib in DSC
EmulatorPkg: Add MmUnblockMemoryLib in DSC
OvmfPkg: Add MmUnblockMemoryLib in DSC
MdeModulePkg:Create gEdkiiVariableRuntimeCacheInfoHobGuid
MdeModulePkg:Remove unnecessary global variable
MdeModulePkg:Consume gEdkiiVariableRuntimeCacheInfoHobGuid
MdeModulePkg: Refine InitVariableCache()
MdeModulePkg:Add global variable mVariableRtCacheInfo
ArmVirtPkg/ArmVirtCloudHv.dsc | 2 ++
EmulatorPkg/EmulatorPkg.dsc | 1 +
MdeModulePkg/Include/Guid/VariableRuntimeCacheInfo.h | 65
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 3 +++
MdeModulePkg/Universal/Variable/Pei/Variable.c | 298
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
MdeModulePkg/Universal/Variable/Pei/Variable.h | 3 +++
MdeModulePkg/Universal/Variable/Pei/VariablePei.inf | 8
+++++++-
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 293
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 5
+++--
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
10 files changed, 506 insertions(+), 174 deletions(-)
create mode 100644 MdeModulePkg/Include/Guid/VariableRuntimeCacheInfo.h
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119089): https://edk2.groups.io/g/devel/message/119089
Mute This Topic: https://groups.io/mt/106150796/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
