Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Mar 14, 2017 at 08:01:04AM +0000, Ard Biesheuvel wrote:
> The recently introduced memory protection features inadvertently broke
> the boot on all PrePi platforms, because the changes to explicitly use
> EfiBootServicesCode for loading the DxeCore PE/COFF image need to be
> applied in a different way for PrePi. So add a simple helper function
> that sets the type of an allocation to EfiBootServicesCode, and invoke
> it to allocate the space for DxeCore.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Tested-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
> ---
> v2: add missing MemoryAllocationLib.h include
>     add Michael's T/b
> 
>  EmbeddedPkg/Library/PrePiLib/PrePi.h    |  1 +
>  EmbeddedPkg/Library/PrePiLib/PrePiLib.c | 34 +++++++++++++++++++-
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Library/PrePiLib/PrePi.h b/EmbeddedPkg/Library/PrePiLib/PrePi.h
> index 607561cd2496..84eca23ec8a6 100644
> --- a/EmbeddedPkg/Library/PrePiLib/PrePi.h
> +++ b/EmbeddedPkg/Library/PrePiLib/PrePi.h
> @@ -28,6 +28,7 @@
>  #include <Library/CacheMaintenanceLib.h>
>  #include <Library/TimerLib.h>
>  #include <Library/PerformanceLib.h>
> +#include <Library/MemoryAllocationLib.h>

Mandatory "don't need to fix sorting, but at least don't make it
worse" comment. Can be folded in.

>  
>  #include <Guid/MemoryAllocationHob.h>
>  
> diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> index 9a1ef344df6e..bba8e7384edc 100644
> --- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> +++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> @@ -28,6 +28,38 @@ SecWinNtPeiLoadFile (
>    IN  EFI_PHYSICAL_ADDRESS    *EntryPoint
>    );
>  
> +STATIC
> +VOID*
> +EFIAPI
> +AllocateCodePages (
> +  IN  UINTN     Pages
> +  )
> +{
> +  VOID                    *Alloc;
> +  EFI_PEI_HOB_POINTERS    Hob;
> +
> +  Alloc = AllocatePages (Pages);
> +  if (Alloc == NULL) {
> +    return NULL;
> +  }
> +
> +  // find the HOB we just created, and change the type to EfiBootServicesCode
> +  Hob.Raw = GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION);
> +  while (Hob.Raw != NULL) {
> +    if (Hob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress == (UINTN)Alloc) {
> +      Hob.MemoryAllocation->AllocDescriptor.MemoryType = EfiBootServicesCode;
> +      return Alloc;
> +    }
> +    Hob.Raw = GET_NEXT_HOB (Hob);
> +    Hob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, Hob.Raw);

I've seen this written elsewhere as
  Hob.Raw = GetNextHob (EFI_HOB_TYPE_XXX, GET_NEXT_HOB (Hob));

Which looks like a nicer pattern to me. (And means I only need to read
"hob" 5 times instead of 7.)

Fold in if you agree.

> +  }
> +
> +  ASSERT (FALSE);
> +
> +  FreePages (Alloc, Pages);
> +  return NULL;
> +}
> +
>  
>  EFI_STATUS
>  EFIAPI
> @@ -54,7 +86,7 @@ LoadPeCoffImage (
>    //
>    // Allocate Memory for the image
>    //
> -  Buffer = AllocatePages (EFI_SIZE_TO_PAGES((UINT32)ImageContext.ImageSize));
> +  Buffer = AllocateCodePages (EFI_SIZE_TO_PAGES((UINT32)ImageContext.ImageSize));
>    ASSERT (Buffer != 0);
>  
>  
> -- 
> 2.7.4

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>