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

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

On 14 March 2017 at 09:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 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>
>>

Pushed with the suggested changes

Thanks,