* [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore
@ 2017-03-14 8:01 Ard Biesheuvel
2017-03-14 9:00 ` Leif Lindholm
2017-03-14 17:13 ` Ryan Harkin
0 siblings, 2 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2017-03-14 8:01 UTC (permalink / raw)
To: edk2-devel, leif.lindholm
Cc: ryan.harkin, sigmaepsilon92, lersek, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore
2017-03-14 8:01 [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore Ard Biesheuvel
@ 2017-03-14 9:00 ` Leif Lindholm
2017-03-14 9:03 ` Ard Biesheuvel
2017-03-14 17:13 ` Ryan Harkin
1 sibling, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2017-03-14 9:00 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, ryan.harkin, sigmaepsilon92, lersek
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore
2017-03-14 9:00 ` Leif Lindholm
@ 2017-03-14 9:03 ` Ard Biesheuvel
2017-03-14 10:45 ` Leif Lindholm
2017-03-14 20:29 ` Ard Biesheuvel
0 siblings, 2 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2017-03-14 9:03 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Ryan Harkin, Michael Zimmermann,
Laszlo Ersek
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>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore
2017-03-14 9:03 ` Ard Biesheuvel
@ 2017-03-14 10:45 ` Leif Lindholm
2017-03-14 20:29 ` Ard Biesheuvel
1 sibling, 0 replies; 7+ messages in thread
From: Leif Lindholm @ 2017-03-14 10:45 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Ryan Harkin, Michael Zimmermann,
Laszlo Ersek
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>
> >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore
2017-03-14 9:03 ` Ard Biesheuvel
2017-03-14 10:45 ` Leif Lindholm
@ 2017-03-14 20:29 ` Ard Biesheuvel
1 sibling, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2017-03-14 20:29 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Ryan Harkin, Michael Zimmermann,
Laszlo Ersek
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,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore
2017-03-14 8:01 [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore Ard Biesheuvel
2017-03-14 9:00 ` Leif Lindholm
@ 2017-03-14 17:13 ` Ryan Harkin
2017-03-14 19:23 ` Ard Biesheuvel
1 sibling, 1 reply; 7+ messages in thread
From: Ryan Harkin @ 2017-03-14 17:13 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm, Michael Zimmermann,
Laszlo Ersek
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore
2017-03-14 17:13 ` Ryan Harkin
@ 2017-03-14 19:23 ` Ard Biesheuvel
0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2017-03-14 19:23 UTC (permalink / raw)
To: Ryan Harkin
Cc: edk2-devel@lists.01.org, Leif Lindholm, Michael Zimmermann,
Laszlo Ersek
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
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-14 20:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-14 8:01 [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore Ard Biesheuvel
2017-03-14 9:00 ` Leif Lindholm
2017-03-14 9:03 ` Ard Biesheuvel
2017-03-14 10:45 ` Leif Lindholm
2017-03-14 20:29 ` Ard Biesheuvel
2017-03-14 17:13 ` Ryan Harkin
2017-03-14 19:23 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox