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]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to