Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows

["Dandan Bi" <dandan.bi@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3336 bytes --]

Hi Taylor,

>>Extra EfiRuntimeServicesCode regions which aren't part of loaded runtime images.
This may be related to the original size of EfiRuntimeServicesCode in memory map, and the size can be configured via PcdPlatformEfiRtCodeMemorySize.

If the size is large enough to hold all the runtime images and still has some remaining regions, then these regions are with EfiRuntimeServicesCode type and aren't part of loaded runtime images, right?
Pre-change https://github.com/tianocore/edk2/commit/e2f2bbe208b4c7ebcedacfc8333df1e52cbf07eb, such EfiRuntimeServicesCode regions are set with EFI_MEMORY_XP attribute.
Post-change https://github.com/tianocore/edk2/commit/e2f2bbe208b4c7ebcedacfc8333df1e52cbf07eb,  these regions don’t have any access attribute.
And now this patch [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map (groups.io)<https://edk2.groups.io/g/devel/message/117889> set these regions with EFI_MEMORY_RO.
I am not sure which attribute should be set for these regions.  And old codes with EFI_MEMORY_XP attribute for a long time and seems not cause any issue.


Thanks,
Dandan
From: Taylor Beebe <taylor.d.beebe@gmail.com>
Sent: Thursday, April 18, 2024 7:54 AM
To: Huang, Yanbo <yanbo.huang@intel.com>; devel@edk2.groups.io; Bi, Dandan <dandan.bi@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Zhou, Jianfeng <jianfeng.zhou@intel.com>
Subject: Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows


Hi Yanbo,
I didn't do it in the way you suggest for the same reason that the SplitTable() logic doesn't set attributes
on descriptors of type EfiRuntimeServicesData or other memory types. The purpose of the SplitTable() function
is to use the input image records to split descriptors so each image section has it's own descriptor. I think
it's reasonable to set the attributes on descriptors associated with images because those new descriptors are the
intended side effect of the function, but I don't think setting attributes on other descriptors is a good design pattern.
This pattern matches what's done in PiSmmCore/MemoryAttributesTable.c. Also, even if we did
it the way you suggest, we would still need call EnforceMemoryMapAttribute() later to set XP on the
EfiRuntimeServicesData descriptors.

Can you or Dandan explain the origin of the extra  EfiRuntimeServicesCode regions which aren't
part of loaded runtime images? It would be a good datapoint for our discussion on the proposed fix.

-Taylor
On 4/17/2024 7:04 AM, Huang, Yanbo wrote:
Hi Taylor,

Thanks for your update.
After test, issue can be fixed by your patch.
But why we not set the EFI_MEMORY_XP or EFI_MEMORY_RO attribute in SplitRecord API?
If we set the attribute in the beginning of the NewRecord created, it seems we don’t need to EnforceMemoryMapAttribute later?

Best Regards,
Yanbo Huang


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117989): https://edk2.groups.io/g/devel/message/117989
Mute This Topic: https://groups.io/mt/105477564/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 7399 bytes --]