On 1/6/23 05:12, Ni, Ray wrote:
>>> @@ -555,19 +556,25 @@ InitMpGlobalData (
>>> )
>>> );
>>>
>>> - mReservedTopOfApStack = (UINTN)AllocateReservedPages (EFI_SIZE_TO_PAGES
>>> (ApSafeBufferSize));
>>> - ASSERT (mReservedTopOfApStack != 0);
>>> - ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
>>> - ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0);
>>> -
>>> - mReservedApLoopFunc = (VOID *)(mReservedTopOfApStack +
>>> CpuMpData->CpuCount * AP_SAFE_STACK_SIZE);
>>> - if (StandardSignatureIsAuthenticAMD ()) {
>>> + if (StandardSignatureIsAuthenticAMD () && (sizeof (UINTN) == sizeof
>>> (UINT64))) {
>>
>> This looks the wrong way around.
>
>
> Ard,
>
> Only AMD X64 (including SEV and without SEV) runs the code that
> switches to 32bit paging disabled mode.
> Intel X64 runs the code that stays at 64bit paging mode. So no need
> for <4G memory.
> All IA32 CPUs (including intel and AMD) stays at 32bit paging disabled
> mode. The AllocateReservedPages() call should not return a memory
> above 4GB in 32bit env.
This argument about the allocations sounds valid, thanks.
The code still remains incredibly hard to read. It needs serious
cleanup.
(1) Wherever we have "Amd" in an identifier, let's rename it to "Amd64",
to better reflect the revised check.
(2) Wherever we have an _AMD in a typedef, rename it to _AMD64. For
example, in the ASM_RELOCATE_AP_LOOP_AMD typedef. The leading
*comment* on that typedef should be updated, too.
(3) The way the mReservedApLoopFunc variable is used (populated, and
then consumed) in C code is super awkward. We have casts left and
right, and duplicated code, too.
(4) Before commit 73ccde8f6d04, we had two separate allocations: namely,
for the AP loop func, and then the stacks of the CPUs together. The
size of the loop func was rounded up to a whole multiple of
EFI_PAGE_SIZE, and then the pages accommodating the loop func were
set to executable. (Unfortunately the name for the rounded-up size
was terribly non-descriptive: "ApSafeBufferSize".) This was done in
commit bc2288f59ba2 ("UefiCpuPkg/MpInitLib: put mReservedApLoopFunc
in executable memory", 2018-03-08). And, because we had two separate
allocations, which could of course be discontiguous between each
other, we needed two variables for storing their addresses,
mReservedApLoopFunc and mReservedTopOfApStack.
Commit 73ccde8f6d04 ("UefiCpuPkg: Has APs in 64 bit long-mode before
booting to OS.", 2022-12-20) *removed* the executable marking.
(4a) Is that not a problem? And if it's not a problem, why was it not
done (or at least explained) separately?
(4b) After commit 73ccde8f6d04, we have a single allocation only. The
two allocations have been fused. The "mReservedTopOfApStack"
variable is now effectively a duplicate of mReservedApLoopFunc, so
it's only good for confusion. It should be eliminated.
(5) The "ApSafeBufferSize" variable name is now totally bogus. It's OK
to have a variable for expressing the allocation size, but
"ApSafeBufferSize" is wrong. It should be renamed.
(6) Here's yet another bug in commit 73ccde8f6d04 (which is not fixed by
the currently proposed patch): the size of the fused allocation,
expressed in "ApSafeBufferSize", does not take into account which
variant of the AP loop func we're going to use; it only accounts for
the non-AMD64 version.
This bug is likely masked by the rounding-up to EFI_PAGE_SIZE, but
it's still a bug.
(7) We should assert that AP_SAFE_STACK_SIZE is correctly aligned with
STATIC_ASSERT(), not ASSERT().
Here's a rough patch (on top of this one) to demonstrate some of the
improvements, all squashed together (just to show the ideas):
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 1994ee44c259..e77bdc4f201d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -25,11 +25,16 @@ EFI_EVENT mCheckAllApsEvent = NULL;
> EFI_EVENT mMpInitExitBootServicesEvent = NULL;
> EFI_EVENT mLegacyBootEvent = NULL;
> volatile BOOLEAN mStopCheckAllApsStatus = TRUE;
> -VOID *mReservedApLoopFunc = NULL;
> UINTN mReservedTopOfApStack;
> volatile UINT32 mNumberToFinish = 0;
> UINTN mApPageTable;
>
> +STATIC union {
> + VOID *Data;
> + ASM_RELOCATE_AP_LOOP_AMD Amd64Entry;
> + ASM_RELOCATE_AP_LOOP GenericEntry;
> +} mReservedApLoop;
> +
> //
> // Begin wakeup buffer allocation below 0x88000
> //
> @@ -381,8 +386,6 @@ RelocateApLoop (
> {
> CPU_MP_DATA *CpuMpData;
> BOOLEAN MwaitSupport;
> - ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc;
> - ASM_RELOCATE_AP_LOOP_AMD AsmRelocateApLoopFuncAmd;
> UINTN ProcessorNumber;
> UINTN StackStart;
>
> @@ -391,8 +394,7 @@ RelocateApLoop (
> MwaitSupport = IsMwaitSupport ();
> if (StandardSignatureIsAuthenticAMD () && (sizeof (UINTN) == sizeof
> (UINT64))) {
> StackStart = CpuMpData->UseSevEsAPMethod ?
> CpuMpData->SevEsAPResetStackStart : mReservedTopOfApStack;
> - AsmRelocateApLoopFuncAmd =
> (ASM_RELOCATE_AP_LOOP_AMD)(UINTN)mReservedApLoopFunc;
> - AsmRelocateApLoopFuncAmd (
> + mReservedApLoop.Amd64Entry (
> MwaitSupport,
> CpuMpData->ApTargetCState,
> CpuMpData->PmCodeSegment,
> @@ -404,8 +406,7 @@ RelocateApLoop (
> );
> } else {
> StackStart = mReservedTopOfApStack;
> - AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP)(UINTN)mReservedApLoopFunc;
> - AsmRelocateApLoopFunc (
> + mReservedApLoop.GenericEntry (
> MwaitSupport,
> CpuMpData->ApTargetCState,
> StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
> @@ -475,12 +476,15 @@ InitMpGlobalData (
> )
> {
> EFI_STATUS Status;
> - UINTN ApSafeBufferSize;
> + MP_ASSEMBLY_ADDRESS_MAP *AddressMap;
> + UINTN AllocSize;
> UINTN Index;
> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
> UINTN StackBase;
> CPU_INFO_IN_HOB *CpuInfoInHob;
> EFI_PHYSICAL_ADDRESS Address;
> + UINT8 *ApLoopFuncData;
> + UINTN ApLoopFuncSize;
>
> SaveCpuMpData (CpuMpData);
>
> @@ -549,48 +553,58 @@ InitMpGlobalData (
> // | Stack * N |
> // +------------+ (low address)
> //
> - ApSafeBufferSize = EFI_PAGES_TO_SIZE (
> - EFI_SIZE_TO_PAGES (
> - CpuMpData->CpuCount * AP_SAFE_STACK_SIZE
> - + CpuMpData->AddressMap.RelocateApLoopFuncSize
> - )
> - );
> +
> + AddressMap = &CpuMpData->AddressMap;
>
> if (StandardSignatureIsAuthenticAMD () && (sizeof (UINTN) == sizeof
> (UINT64))) {
> - Address = BASE_4GB - 1;
> - Status = gBS->AllocatePages (
> - AllocateMaxAddress,
> - EfiReservedMemoryType,
> - EFI_SIZE_TO_PAGES (ApSafeBufferSize),
> - &Address
> - );
> - ASSERT_EFI_ERROR (Status);
> - mReservedApLoopFunc = (VOID *)((UINTN)Address + CpuMpData->CpuCount *
> AP_SAFE_STACK_SIZE);
> - CopyMem (
> - mReservedApLoopFunc,
> - CpuMpData->AddressMap.RelocateApLoopFuncAddressAmd,
> - CpuMpData->AddressMap.RelocateApLoopFuncSizeAmd
> - );
> + //
> + // 64-bit AMD Processor
> + //
> + Address = BASE_4GB - 1;
> + ApLoopFuncData = AddressMap->RelocateApLoopFuncAddressAmd;
> + ApLoopFuncSize = AddressMap->RelocateApLoopFuncSizeAmd;
> } else {
> - Address = (UINTN)AllocateReservedPages (EFI_SIZE_TO_PAGES
> (ApSafeBufferSize));
> - ASSERT (Address != 0);
> - mReservedApLoopFunc = (VOID *)((UINTN)Address + CpuMpData->CpuCount *
> AP_SAFE_STACK_SIZE);
> - CopyMem (
> - mReservedApLoopFunc,
> - CpuMpData->AddressMap.RelocateApLoopFuncAddress,
> - CpuMpData->AddressMap.RelocateApLoopFuncSize
> - );
> + //
> + // Intel Processor (32-bit or 64-bit), or 32-bit AMD Processor
> + //
> + Address = MAX_ADDRESS;
> + ApLoopFuncData = AddressMap->RelocateApLoopFuncAddress;
> + ApLoopFuncSize = AddressMap->RelocateApLoopFuncSize;
> + }
>
> + STATIC_ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0);
> + AllocSize = EFI_PAGES_TO_SIZE (
> + EFI_SIZE_TO_PAGES (
> + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE + ApLoopFuncSize
> + )
> + );
> +
> + Status = gBS->AllocatePages (
> + AllocateMaxAddress,
> + EfiReservedMemoryType,
> + EFI_SIZE_TO_PAGES (AllocSize),
> + &Address
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> + mReservedTopOfApStack = ((UINTN)Address +
> + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE);
> + ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
> +
> + mReservedApLoop.Data = (VOID *)mReservedTopOfApStack;
> + CopyMem (mReservedApLoop.Data, ApLoopFuncData, ApLoopFuncSize);
> +
> + if (!StandardSignatureIsAuthenticAMD () &&
> + (sizeof (UINTN) == sizeof (UINT64))) {
> + //
> + // 64-bit Intel Processor
> + //
> mApPageTable = CreatePageTable (
> (UINTN)Address,
> - ApSafeBufferSize
> + AllocSize
> );
> }
>
> - mReservedTopOfApStack = (UINTN)Address + CpuMpData->CpuCount *
> AP_SAFE_STACK_SIZE;
> - ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
> - ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0);
> -
> Status = gBS->CreateEvent (
> EVT_TIMER | EVT_NOTIFY_SIGNAL,
> TPL_NOTIFY,
Honestly, at this point I'm *even more convinced* that the original
series should be reverted, and redone from the ground up. There is way
too much cruft for sensible incremental fixing. If you consider just the
renames I'm requesting, that's going to introduce huge churn already. So
let's please first undo the damage done by 73ccde8f6d04, and then build
up the desired functionality in *very small*, careful steps, using the
correct variable and type names, right off the bat. And please let us
consider *cleaning up* the source code a primary goal while at it,
removing code duplication and so on.
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98067): https://edk2.groups.io/g/devel/message/98067
Mute This Topic: https://groups.io/mt/96067843/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
