| Message ID | 1489478464-5737-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 6ac97ad31edbd9c8cdea778f452273974c589bf1 |
| Headers | show |
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> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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> > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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, _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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);