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

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

Eric,
I agree with the code change. It fixes the memtest86 problem through
a very simple code change.
But I think we do need a very meaningful comments to describe a such simple
code change.

Comments below:
1. Please make sure each line of commit message doesn't exceed 70.

> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Tuesday, March 5, 2019 10:07 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: [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.

2. Can you describe the issue more clearly in the commit message?
What problem is met? What's the tool?

> 
> 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. 

3. What investigation was made to choose 0x88000?

> 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.

4. What if someone allocates the exact page? Will CpuDxe driver fail to start?

> 
> 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.
> +  //

5. future code reader cannot understand what "current borrow mechanism" because
you change the implementation.

> +  // 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.

6. The above wording describes the behavior of LegacyBios driver. What's the
relation ship between the LegacyBios driver behavior and the new implementation?

> +  // 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;

7. If 0x88000 is chosen, why use BASE_512KB + BASE_32KB instead of 0x88000?
Will you remove the hard-code 0x88000 and use BASE_1MB after CSM is EOL?

>    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