Re: [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for Wake up Buffer.

["Dong, Eric" <eric.dong@intel.com>]

Hi Star,

This logic seems much complicated than mine. Also after CSM retired from EDKII, we will change this code back to only require allocate buffer below 1M. I will add such notes in the code comments.  So I prefer to use my change.

Thanks,
Eric

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, March 5, 2019 3:33 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek (lersek@redhat.com)
> <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for
> Wake up Buffer.
> 
> Just an idea to avoid hard code value 0x88000.
> 
> Before EndOfDxe: Allocate buffer in AllocateResetVector(), and free buffer
> in FreeResetVector().
> At EndOfDxe (it is after LegacyBiosDxe driver entry point) callback: Allocate
> buffer and record it to CpuMpData->WakeupBuffer, and always occupy the
> buffer, that means no free.
> After EndOfDxe: Use CpuMpData->WakeupBuffer.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Eric Dong
> Sent: Tuesday, March 5, 2019 10:07 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch] UefiCpuPkg/MpInitLib: Direct allocate buffer for
> Wake up Buffer.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1538
> 
> Current CpuDxe driver "borrows" Wakeup Buffer (through Allocate & free to
> get the buffer pointer, backup the buffer data before using it and restore it
> after using).  Now this logic met a problem described in the above BZ
> because the test tool and the CpuDxe both use the same memory at the
> same time.
> 
> In order to fix the above issue, CpuDxe changed to allocate the buffer below
> 1M instead of borrow it. After investigation, we found below
> 0x88000 is the possible space which can be used. For now, range
> 0x60000 ~ 0x88000 used for Legacy OPROMs by LegacyBios driver. And it tries
> to allocate these range page(4K size) by page. It just reports warning
> message if specific page been used by others already.
> 
> Also CpuDxe driver will produce CPU arch protocol and LegacyBios driver has
> dependency for this protocol. So CpuDxe driver will start before LegacyBios
> driver and CpuDxe driver can allocate that space successful.
> 
> With this change, CpuDxe driver will use 0x87000 ~ 0x88000 as wakeup buffer.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 30 +++++++++++++++++++----
> -------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index b2307cbb61..5bc9a47efb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -76,7 +76,7 @@ SaveCpuMpData (
>  }
> 
>  /**
> -  Get available system memory below 1MB by specified size.
> +  Get available system memory below 0x88000 by specified size.
> 
>    @param[in] WakeupBufferSize   Wakeup buffer size required
> 
> @@ -91,7 +91,19 @@ GetWakeupBuffer (
>    EFI_STATUS              Status;
>    EFI_PHYSICAL_ADDRESS    StartAddress;
> 
> -  StartAddress = BASE_1MB;
> +  //
> +  // Current "Borrow" space mechanism caused potential race condition
> + if both  // AP and the original owner use the share space.
> +  //
> +  // LegacyBios driver tries to allocate 4K pages between 0x60000 ~
> + 0x88000  // space. It will just report an waring message if the page
> + has been allocate  // by other drivers.
> +  // LagacyBios driver depends on CPU Arch protocol, so it will start
> + after  // CpuDxe driver which produce Cpu Arch Protocol and use this
> library.
> +  // So below allocate logic will be trigged before LegacyBios driver
> + and it  // will always return success.
> +  //
> +  StartAddress = BASE_512KB + BASE_32KB;
>    Status = gBS->AllocatePages (
>                    AllocateMaxAddress,
>                    EfiBootServicesData,
> @@ -99,17 +111,13 @@ GetWakeupBuffer (
>                    &StartAddress
>                    );
>    ASSERT_EFI_ERROR (Status);
> -  if (!EFI_ERROR (Status)) {
> -    Status = gBS->FreePages(
> -               StartAddress,
> -               EFI_SIZE_TO_PAGES (WakeupBufferSize)
> -               );
> -    ASSERT_EFI_ERROR (Status);
> -    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
> = %x\n",
> -                        (UINTN) StartAddress, WakeupBufferSize));
> -  } else {
> +  if (EFI_ERROR (Status)) {
>      StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>    }
> +
> +  DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize
> = %x\n",
> +                      (UINTN) StartAddress, WakeupBufferSize));
> +
>    return (UINTN) StartAddress;
>  }
> 
> --
> 2.15.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel