Hi Yanbo,
Can you help me understand the memory layout which causes this issue?
If a single EfiRuntimeServicesCode descriptor needs to be split because
an image is within the memory range. I think that descriptor is split
like so in the case you're encountering:
------------------- --- ---
| DATA | | |
------------------- | |
| CODE | | Image |
------------------- | Memory | EfiRuntimeServicesCode
| DATA | | |
------------------- --- |
| Extra Pages | |
------------------- ---
In this case, because the memory type of the buffer is
EfiRuntimeServicesCode, shouldn't the final pages be EFI_MEMORY_RO?
Thanks!
-Taylor
On 4/11/2024 10:14 PM, Huang, Yanbo wrote:
Hi Beebe,
Recently we found this commit " MdeModulePkg: Fix MAT SplitRecord() Logic "
will cause SUT reset after enable some knobs.
I filed one Bugzilla for it: https://bugzilla.tianocore.org/show_bug.cgi?id=4751
After debug, we found in SplitRecord API, many entries attribute are set to 0,
not align with the UEFI spec:
"Memory Attributes Table (MAT):
EFI_MEMORY_ATTRIBUTES_TABLE. The entire UEFI runtime must be described by this
table.
All entries must include attributes EFI_MEMORY_RO, EFI_MEMORY_XP, or both. Memory
MUST be either readable and executable OR writeable and non-executable."
This should be the root cause of this issue.
When we update "NewRecord->Attribute = TempRecord.Attribute;" to
"NewRecord->Attribute = TempRecord.Attribute | EFI_MEMORY_XP;", SUT can boot to windows.
@taylor.d.be...@gmail.com Could you please help to send one formal fix patch
for this issue?
Thanks!
Best Regards,
Yanbo Huang
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Taylor Beebe
Sent: Tuesday, November 28, 2023 2:18 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.w...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Bi,
Dandan <dandan...@intel.com>
Subject: [edk2-devel] [PATCH v5 10/16] MdeModulePkg: Fix MAT SplitRecord() Logic
SplitRecord() does not handle the case where a memory descriptor describes an
image region plus extra pages before or after the image region. This patch
fixes this case by carving off the unrelated regions into their own descriptors.
Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Dandan Bi <dandan...@intel.com>
Signed-off-by: Taylor Beebe <taylor.d.be...@gmail.com>
Reviewed-by: Liming Gao <gaolim...@byosoft.com.cn>
---
MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 56
++++++++++----------
1 file changed, 27 insertions(+), 29 deletions(-)
diff --git
a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
index 7c0ecd07c1bb..9d4082280bf5 100644
--- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
+++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecor
+++ dLib.c
@@ -323,7 +323,6 @@ SplitRecord (
UINT64 PhysicalEnd;
UINTN NewRecordCount;
UINTN TotalNewRecordCount;
- BOOLEAN IsLastRecordData;
if (MaxSplitRecordCount == 0) {
CopyMem (NewRecord, OldRecord, DescriptorSize); @@ -344,35 +343,16 @@
SplitRecord (
NewImageRecord = GetImageRecordByAddress (PhysicalStart, PhysicalEnd -
PhysicalStart, ImageRecordList);
if (NewImageRecord == NULL) {
//
- // No more image covered by this range, stop
+ // No more images cover this range, check if we've reached the end of
the old descriptor. If not,
+ // add the remaining range to the new descriptor list.
//
- if ((PhysicalEnd > PhysicalStart) && (ImageRecord != NULL)) {
- //
- // If this is still address in this record, need record.
- //
- NewRecord = PREVIOUS_MEMORY_DESCRIPTOR (NewRecord,
DescriptorSize);
- IsLastRecordData = FALSE;
- if ((NewRecord->Attribute & EFI_MEMORY_XP) != 0) {
- IsLastRecordData = TRUE;
- }
-
- if (IsLastRecordData) {
- //
- // Last record is DATA, just merge it.
- //
- NewRecord->NumberOfPages = EfiSizeToPages (PhysicalEnd -
NewRecord->PhysicalStart);
- } else {
- //
- // Last record is CODE, create a new DATA entry.
- //
- NewRecord = NEXT_MEMORY_DESCRIPTOR (NewRecord,
DescriptorSize);
- NewRecord->Type = TempRecord.Type;
- NewRecord->PhysicalStart = TempRecord.PhysicalStart;
- NewRecord->VirtualStart = 0;
- NewRecord->NumberOfPages = TempRecord.NumberOfPages;
- NewRecord->Attribute = TempRecord.Attribute | EFI_MEMORY_XP;
- TotalNewRecordCount++;
- }
+ if (PhysicalEnd > PhysicalStart) {
+ NewRecord->Type = TempRecord.Type;
+ NewRecord->PhysicalStart = PhysicalStart;
+ NewRecord->VirtualStart = 0;
+ NewRecord->NumberOfPages = EfiSizeToPages (PhysicalEnd -
PhysicalStart);
+ NewRecord->Attribute = TempRecord.Attribute;
+ TotalNewRecordCount++;
}
break;
@@ -380,6 +360,24 @@ SplitRecord (
ImageRecord = NewImageRecord;
+ //
+ // Update PhysicalStart to exclude the portion before the image buffer
+ //
+ if (TempRecord.PhysicalStart < ImageRecord->ImageBase) {
+ NewRecord->Type = TempRecord.Type;
+ NewRecord->PhysicalStart = TempRecord.PhysicalStart;
+ NewRecord->VirtualStart = 0;
+ NewRecord->NumberOfPages = EfiSizeToPages (ImageRecord->ImageBase -
TempRecord.PhysicalStart);
+ NewRecord->Attribute = TempRecord.Attribute;
+ TotalNewRecordCount++;
+
+ PhysicalStart = ImageRecord->ImageBase;
+ TempRecord.PhysicalStart = PhysicalStart;
+ TempRecord.NumberOfPages = EfiSizeToPages (PhysicalEnd -
+ PhysicalStart);
+
+ NewRecord = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)NewRecord +
DescriptorSize);
+ }
+
//
// Set new record
//
--
2.42.0.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117710): https://edk2.groups.io/g/devel/message/117710
Mute This Topic: https://groups.io/mt/105477564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
