Re: [Patch v2] UefiCpuPkg/MpInitLib: Fix MemTest86 failure.

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Dong, Eric
> Sent: Wednesday, April 3, 2019 10:46 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: [Patch v2] UefiCpuPkg/MpInitLib: Fix MemTest86 failure.
> 
> V2 changes:
>   Update the commit message and comments in the code.
> 
> When waking vector buffer allocated by CpuDxe is tested by MemTest86
> in MP mode, an error is reported because the same range of memory is
> modified by both CpuDxe driver and MemTest86.
> 
> The waking vector buffer is not expected to be tested by MemTest86 if
> it is allocated out because MemTest86 only tests free memory. But
> current CpuDxe driver "borrows" buffer instead of allocate buffer for
> waking vector buffer (through allocate & free to get the buffer
> pointer, backup the buffer data before using it and restore it after
> using). With this implementation, if the buffer borrowed is not used
> by any other drivers, MemTest86 tool will treat it as free memory
> and test it.
> 
> In order to fix the above issue, CpuDxe changes to allocate the
> buffer below 1M instead of borrowing it. But directly allocating
> memory below 1MB causes LegacyBios driver fails to start. LegacyBios
> driver allocates memory range from
> "0xA0000 - PcdEbdaReservedMemorySize" to 0xA0000 as Ebda Reserved
> Memory. The minimum value for "0xA0000 - PcdEbdaReservedMemorySize"
> is 0x88000. If LegacyBios driver allocate this range failed, it
> asserts.
> 
> LegacyBios also reserves range from 0x60000 to
> "0x60000 + PcdOpromReservedMemorySize", it will be used as Oprom
> Reserve Memory. The maximum value for "0x60000 +
> PcdOpromReservedMemorySize" is 0x88000. LegacyBios driver tries to
> allocate these range page(4K size) by page. It just reports warning
> message if some pages are already allocated by others.
> Base on above investigation, one page in range 0x60000 ~ 0x88000 can
> be used as the waking vector buffer.
> 
> LegacyBios driver only reports warning when page allocation in range
> [0x60000, 0x88000) fails. This library is consumed by CpuDxe driver
> to produce CPU Arch protocol. LagacyBios driver depends on CPU Arch
> protocol which guarantees below allocation runs earlier than
> LegacyBios driver.
> 
> 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 | 26 ++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index b2307cbb61..cef5b49dde 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,15 @@ GetWakeupBuffer (
>    EFI_STATUS              Status;
>    EFI_PHYSICAL_ADDRESS    StartAddress;
> 
> -  StartAddress = BASE_1MB;
> +  //
> +  // Try to allocate buffer below 1M for waking vector.
> +  // LegacyBios driver only reports warning when page allocation in range
> +  // [0x60000, 0x88000) fails.
> +  // This library is consumed by CpuDxe driver to produce CPU Arch protocol.
> +  // LagacyBios driver depends on CPU Arch protocol which guarantees below
> +  // allocation runs earlier than LegacyBios driver.
> +  //
> +  StartAddress = 0x88000;
>    Status = gBS->AllocatePages (
>                    AllocateMaxAddress,
>                    EfiBootServicesData,
> @@ -99,17 +107,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.21.0.windows.1