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

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

On Tue, Mar 14, 2017 at 09:03:43AM +0000, Ard Biesheuvel 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.

Well, it's shuffled all over the place, so for now I'm just looking
for a continuous improvement :)

In this case, moving one or two lines up is equivalent.

/
    Leif

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