On 3/3/2018 11:10 PM, Laszlo Ersek wrote:
Hi Ray,
On 03/02/18 12:57, Ni, Ruiyu wrote:
On 3/2/2018 7:45 PM, Laszlo Ersek wrote:
On 03/02/18 06:58, Jian J Wang wrote:
if PcdDxeNxMemoryProtectionPolicy is enabled for EfiReservedMemoryType
of memory, #PF will be triggered for each APs after ExitBootServices
in SCRT test. The root cause is that AP wakeup code executed at that
time is stored in memory of type EfiReservedMemoryType (referenced by
global mReservedApLoopFunc), which is marked as non-executable.
This patch fixes this issue by setting memory of mReservedApLoopFunc to
be executable immediately after allocation.
Cc: Ruiyu Ni <ruiyu...@intel.com>
Cc: Eric Dong <eric.d...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index fd2317924f..5fcb08677c 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -399,6 +399,21 @@ InitMpGlobalData (
&Address
);
ASSERT_EFI_ERROR (Status);
+
+ //
+ // Make sure that the buffer memory is executable.
+ //
+ Status = gDS->GetMemorySpaceDescriptor (Address, &MemDesc);
+ if (!EFI_ERROR (Status)) {
+ gDS->SetMemorySpaceAttributes (
+ Address,
+ EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (
+ CpuMpData->AddressMap.RelocateApLoopFuncSize
+ )),
+ MemDesc.Attributes & (~EFI_MEMORY_XP)
+ );
+ }
+
mReservedApLoopFunc = (VOID *) (UINTN) Address;
ASSERT (mReservedApLoopFunc != NULL);
mReservedTopOfApStack = (UINTN) Address + EFI_PAGES_TO_SIZE
(EFI_SIZE_TO_PAGES (ApSafeBufferSize));
Honestly, I see little point in the "Dxe Nx Memory Protection Policy"
when we then override it *every time* it gets in our way.
"RelocateApLoopFuncSize" is likely significantly smaller than a full
page, so we're making a good chunk of the "safe stack(s)" executable too.
Anyway, can you perhaps check BIT0 (standing for EfiReservedMemoryType)
in PcdDxeNxMemoryProtectionPolicy, to see if the above hack is necessary?
Thanks
Laszlo
Checking PCD is not very good I think.
I'll look at v2 next week, just a short comment now: I don't understand
why you are opposed to the PCD check. Reserved memory is generally
expected to be executable, and the issue surfaces *precisely* when
reserved memory is marked as noexec in the PCD in question. That's
exactly the reason why the above logic is needed.
Sorry I didn't see this comments.
Standing from CPU owner's perspective, I don't like to check PCD here.
Or, to be straight, I want to have least knowledge of any extra
non-spec defined interfaces.
Approach it from this side: if I was reading the code (without the PCD
check), I would ask myself, "why are we checking for noexec here? we
just allocated this chunk of reserved memory from normal system memory.
It should be executable already".
So, I think the PCD check is somewhat important functionally, and quite
important for documentation purposes. And it's a lot better than adding
a comment.
If checking is really needed, how about check MemDesc.Attributes
EFI_MEMORY_XP bit?
I think that would check for the symptom, not for the root cause. To a
person reading the code, it doesn't provide any more information than
the current code. "Okay, it's not executable, so we mark it executable
manually. But why isn't it executable in the first place? We just
allocated it from system memory."This code is for AP to sleep, even at runtime,
before OS takes control
of AP. So making it as BS code may cause issues.
Thanks,
Laszlo
--
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
