Re: [PATCH 3/6] MdePkg/UefiLib: introduce EfiAllocatePeiAccessiblePages routine

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 22 May 2018 at 19:40, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Ard,
>
> The corner case that does not work with this
> approach is when X64 DXE is combined with an
> X64 PEI.  OVMF uses this and other platforms
> could choose to use X64 PEI phase.
>

Actually, this approach simply encodes the current status quo. X64 DXE
builds unconditionally limit the allocations to < 4 GB if PEI needs to
access them, regardless of whether PEI is 32-bit or 64-bit.

Also, OVMF's X64 PEI still only maps the lower 4 GB of DRAM, so it
actually relies on the current behavior, and allocating above 4 GB
under the assumption that a 64-bit PEI will be able to access it will
actually break things.

> The other mismatch here is adding some PI Spec
> Concepts (e.g. PEI phase) to a UEFI library.
> Maybe DxeServicesLib would be a better place
> to put this type of API.
>

OK, fair enough.

> One clue we have about the memory usage in the
> PEI phase is from the EFI_HOB_HANDOFF_INFO_TABLE
> HOB.
>
>   ///
>   /// The highest address location of memory that is allocated for use by the HOB producer
>   /// phase. This address must be 4-KB aligned to meet page restrictions of UEFI.
>   ///
>   EFI_PHYSICAL_ADDRESS    EfiMemoryTop;
>
>   ///
>   /// The highest address location of free memory that is currently available
>   /// for use by the HOB producer phase.
>   ///
>   EFI_PHYSICAL_ADDRESS    EfiFreeMemoryTop;
>
> So maybe we could have an X64 specific implementation
> of this new API that checks one of these HOB fields.
> If they are below 4GB, then allocate memory below
> 4GB.  If one is above 4GB, then no restrictions.
> All other archs allocate with no restrictions.
>

That works for me.

> Now this approach will still allocate below 4GB for
> X64 PEI if the this HOB contains addressed below 4GB,
> but that would match the memory usage for that
> X64 PEI implementation.
>

OK, to summarize:
- move the implementation of EfiAllocatePeiAccessiblePages() to
DxeServicesLib (and perhaps rename it to something more appropriate
for its new home)
- only restrict the X64 version to below 4 GB if EfiMemoryTop and
EfiFreeMemoryTop are both below 4 GB.

I will give others some time to respond to this.

Thanks,
Ard.


>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Tuesday, May 22, 2018 7:09 AM
>> To: edk2-devel@lists.01.org
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo
>> Ersek <lersek@redhat.com>; Leif Lindholm
>> <leif.lindholm@linaro.org>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Bi, Dandan <dandan.bi@intel.com>
>> Subject: [PATCH 3/6] MdePkg/UefiLib: introduce
>> EfiAllocatePeiAccessiblePages routine
>>
>> Add a routine to UefiLib that abstracts the allocation
>> of memory that
>> should be accessible by PEI after a warm reboot. We will
>> use it to
>> replace open coded implementations that limit the
>> address to < 4 GB,
>> which may not be possible on non-Intel systems that have
>> no 32-bit
>> addressable memory at all.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> ---
>>  MdePkg/Include/Library/UefiLib.h | 23 ++++++++++
>>  MdePkg/Library/UefiLib/UefiLib.c | 48
>> ++++++++++++++++++++
>>  2 files changed, 71 insertions(+)
>>
>> diff --git a/MdePkg/Include/Library/UefiLib.h
>> b/MdePkg/Include/Library/UefiLib.h
>> index 256498e3fd8d..8fa077af41e0 100644
>> --- a/MdePkg/Include/Library/UefiLib.h
>> +++ b/MdePkg/Include/Library/UefiLib.h
>> @@ -1520,4 +1520,27 @@ EfiLocateProtocolBuffer (
>>    OUT UINTN     *NoProtocols,
>>    OUT VOID      ***Buffer
>>    );
>> +
>> +/**
>> +  Allocates one or more 4KB pages of a given type from
>> a memory region that is
>> +  accessible to PEI.
>> +
>> +  Allocates the number of 4KB pages of type
>> 'MemoryType' and returns a
>> +  pointer to the allocated buffer.  The buffer returned
>> is aligned on a 4KB
>> +  boundary.  If Pages is 0, then NULL is returned.  If
>> there is not enough
>> +  memory remaining to satisfy the request, then NULL is
>> returned.
>> +
>> +  @param[in]  MemoryType            The memory type to
>> allocate
>> +  @param[in]  Pages                 The number of 4 KB
>> pages to allocate.
>> +
>> +  @return A pointer to the allocated buffer or NULL if
>> allocation fails.
>> +
>> +**/
>> +VOID *
>> +EFIAPI
>> +EfiAllocatePeiAccessiblePages (
>> +  IN EFI_MEMORY_TYPE  MemoryType,
>> +  IN UINTN            Pages
>> +  );
>> +
>>  #endif
>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
>> b/MdePkg/Library/UefiLib/UefiLib.c
>> index ba449a1c34ce..3a9d75149dd7 100644
>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>> @@ -1715,3 +1715,51 @@ EfiLocateProtocolBuffer (
>>
>>    return EFI_SUCCESS;
>>  }
>> +
>> +/**
>> +  Allocates one or more 4KB pages of a given type from
>> a memory region that is
>> +  accessible to PEI.
>> +
>> +  Allocates the number of 4KB pages of type
>> 'MemoryType' and returns a
>> +  pointer to the allocated buffer.  The buffer returned
>> is aligned on a 4KB
>> +  boundary.  If Pages is 0, then NULL is returned.  If
>> there is not enough
>> +  memory remaining to satisfy the request, then NULL is
>> returned.
>> +
>> +  @param[in]  MemoryType            The memory type to
>> allocate
>> +  @param[in]  Pages                 The number of 4 KB
>> pages to allocate.
>> +
>> +  @return A pointer to the allocated buffer or NULL if
>> allocation fails.
>> +
>> +**/
>> +VOID *
>> +EFIAPI
>> +EfiAllocatePeiAccessiblePages (
>> +  IN EFI_MEMORY_TYPE  MemoryType,
>> +  IN UINTN            Pages
>> +  )
>> +{
>> +  EFI_STATUS            Status;
>> +  EFI_PHYSICAL_ADDRESS  Memory;
>> +  EFI_ALLOCATE_TYPE     AllocType;
>> +
>> +  if (Pages == 0) {
>> +    return NULL;
>> +  }
>> +
>> +#ifdef MDE_CPU_X64
>> +  //
>> +  // On X64 systems, a X64 build of DXE may be combined
>> with a 32-bit build of
>> +  // PEI, and so we need to allocate below 4 GB to
>> ensure that the allocation
>> +  // is accessible by PEI.
>> +  //
>> +  AllocType = AllocateMaxAddress;
>> +  Memory = MAX_UINT32;
>> +#else
>> +  AllocType = AllocateAnyPages;
>> +#endif
>> +  Status = gBS->AllocatePages (AllocType, MemoryType,
>> Pages, &Memory);
>> +  if (EFI_ERROR (Status)) {
>> +    return NULL;
>> +  }
>> +  return (VOID *)(UINTN)Memory;
>> +}
>> --
>> 2.17.0
>