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

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

On 14 March 2017 at 09:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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.
>

Yeah, I pondered this for a while an gave up. I can just fix it
entirely if you prefer.

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

OK. I copied this from PrePiMemoryAllocationLib, but I agree that your
suggestion is better.

>> +  }
>> +
>> +  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>
>