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

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

[Ard Biesheuvel]

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>
 
 #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);
+  }
+
+  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

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

[Leif Lindholm]

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>

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

[Ard Biesheuvel]

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

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

[Leif Lindholm]

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

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

[Ryan Harkin]

Hi Ard,

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

I'm not quite sure which issue this patch is supposed to fix.

EDK2 is broken on Juno for me right now. I see this output at boot:

EhcDriverBindingStart: failed to enable 64-bit DMA on 64-bit capable
controller @ FD41B898 (Unsupported)

ASSERT_EFI_ERROR (Status = Unsupported)
ASSERT [ArmJunoDxe]
/linaro/platforms/uefi/edk2/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c(361):
!EFI_ERROR (Status)

Bisecting tells me this patch is to blame:

$ git bisect bad
e7b24ec9785d206f1d3faf8f646e63a1b540d6a5 is the first bad commit
commit e7b24ec9785d206f1d3faf8f646e63a1b540d6a5
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Tue Feb 28 12:13:12 2017 +0000

    ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable

    The primary use case for UncachedMemoryAllocationLib is non-coherent DMA,
    which implies that such regions are not used to fetch instructions from.

    So let's map them as non-executable, to avoid creating a security hole
    when the rest of the platform may be enforcing strict memory permissions
    on ordinary allocations.

    Contributed-under: TianoCore Contribution Agreement 1.0
    Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
    Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

:040000 040000 6310e6a81c3b1e9eda315a9f4cad2bc918f74b25
12b11759ebd83f91a127c1f016b8e41cbfd097d4 M    ArmPkg

Applying the patch from $subject and "[PATCH] Platforms/ARM: enable
memory protection features" does not resolve the issue.

AFAICT, only reverting e7b24ec9785d206f1d3faf8f646e63a1b540d6a5 makes
things work again. I suspect there are further Juno specific changes
that could be applied instead of reverting the problematic patch.

So, Juno and TC2 boot fine if my edk2 tree looks like this:

50e5735  2017-03-14  Revert "ArmPkg/UncachedMemoryAllocationLib: map
uncached allocations non-executable"       [Ryan Harkin]
7e9c1b0  2017-03-14  EmbeddedPkg/PrePiLib: allocate code pages for
DxeCore      [Ard Biesheuvel]
c03f5b2  2017-03-10  BaseTools/UPT: Fix an issue in subst command
 [Hess Chen]

And OpenPlatformPkg looks like this:

d6a743d  2017-03-07  Platforms/ARM: enable memory protection features
 [Ard Biesheuvel]
408d600  2016-01-29  HACK: Platforms/ARM: TC2: set
gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride TRUE  [Ryan
Harkin]
17dbc2a  2017-02-24  Treewide: remove references to
VirtualUncachedPages libraries and PCDs     [Ard Biesheuvel]

If I go on to drop $subject patch, Juno and TC2 no longer boot, so I
presume that it's supposed to fix the issue where OpenPlatformPkg has
enabled the memory protection features. And that the upstream Juno
problems are different.

Cheers,
Ryan.


> 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>
>
>  #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);
> +  }
> +
> +  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
>

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

[Ard Biesheuvel]

On 14 March 2017 at 17:13, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> Hi Ard,
>
> On 14 March 2017 at 08:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.
>>
>
> I'm not quite sure which issue this patch is supposed to fix.
>
> EDK2 is broken on Juno for me right now. I see this output at boot:
>
> EhcDriverBindingStart: failed to enable 64-bit DMA on 64-bit capable
> controller @ FD41B898 (Unsupported)
>

This bit is unrelated (unless you are booting from USB)

> ASSERT_EFI_ERROR (Status = Unsupported)
> ASSERT [ArmJunoDxe]
> /linaro/platforms/uefi/edk2/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c(361):
> !EFI_ERROR (Status)
>

... but this is obviously the culprit.

I'm afraid I made a thinko there: setting the XP bit in the GCD memory
space attributes is only supported if the memory has the XP capability
to begin with, and given that it does not propagate to the page tables
anyway, there is no point in setting this capability, and so we never
bother.

> Bisecting tells me this patch is to blame:
>
> $ git bisect bad
> e7b24ec9785d206f1d3faf8f646e63a1b540d6a5 is the first bad commit
> commit e7b24ec9785d206f1d3faf8f646e63a1b540d6a5
> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Date:   Tue Feb 28 12:13:12 2017 +0000
>
>     ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable
>
>     The primary use case for UncachedMemoryAllocationLib is non-coherent DMA,
>     which implies that such regions are not used to fetch instructions from.
>
>     So let's map them as non-executable, to avoid creating a security hole
>     when the rest of the platform may be enforcing strict memory permissions
>     on ordinary allocations.
>
>     Contributed-under: TianoCore Contribution Agreement 1.0
>     Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>     Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> :040000 040000 6310e6a81c3b1e9eda315a9f4cad2bc918f74b25
> 12b11759ebd83f91a127c1f016b8e41cbfd097d4 M    ArmPkg
>
> Applying the patch from $subject and "[PATCH] Platforms/ARM: enable
> memory protection features" does not resolve the issue.
>
> AFAICT, only reverting e7b24ec9785d206f1d3faf8f646e63a1b540d6a5 makes
> things work again. I suspect there are further Juno specific changes
> that could be applied instead of reverting the problematic patch.
>
> So, Juno and TC2 boot fine if my edk2 tree looks like this:
>
> 50e5735  2017-03-14  Revert "ArmPkg/UncachedMemoryAllocationLib: map
> uncached allocations non-executable"       [Ryan Harkin]
> 7e9c1b0  2017-03-14  EmbeddedPkg/PrePiLib: allocate code pages for
> DxeCore      [Ard Biesheuvel]
> c03f5b2  2017-03-10  BaseTools/UPT: Fix an issue in subst command
>  [Hess Chen]
>
> And OpenPlatformPkg looks like this:
>
> d6a743d  2017-03-07  Platforms/ARM: enable memory protection features
>  [Ard Biesheuvel]
> 408d600  2016-01-29  HACK: Platforms/ARM: TC2: set
> gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride TRUE  [Ryan
> Harkin]
> 17dbc2a  2017-02-24  Treewide: remove references to
> VirtualUncachedPages libraries and PCDs     [Ard Biesheuvel]
>
> If I go on to drop $subject patch, Juno and TC2 no longer boot, so I
> presume that it's supposed to fix the issue where OpenPlatformPkg has
> enabled the memory protection features. And that the upstream Juno
> problems are different.
>
> Cheers,
> Ryan.
>
>
>> 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>
>>
>>  #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);
>> +  }
>> +
>> +  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
>>

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

[Ard Biesheuvel]

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,