On 6 March 2017 at 15:12, Leif Lindholm <leif.lindh...@linaro.org> wrote: > On Thu, Mar 02, 2017 at 10:36:13AM +0000, Ard Biesheuvel wrote: >> Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is >> fully broken down into page mappings if the start or the size of the >> region happens to be misaliged relative to the section size of 1 MB. >> >> This is going to hurt when we enable strict memory permissions, given > > "Hurt" -> "cause unnecessary performance penalties" or "hurt" -> > "break everything"? >
The former. It will map all of RAM using page mappings, which uses more space unnecessarily >> that we remap the entire RAM space non-executable (modulo the code >> bits) when the CpuArchProtocol is installed. >> >> So refactor the code to iterate over the range in a way that ensures >> that all naturally aligned section sized subregions are not broken up. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> > > Many thanks for getting rid of the magic values, and in general making > the code more logical. One question below: > >> --- >> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++---- >> 1 file changed, 39 insertions(+), 8 deletions(-) >> >> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> index 89e429925ba9..ce4d529bda67 100644 >> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> @@ -679,6 +679,7 @@ SetMemoryAttributes ( >> ) >> { >> EFI_STATUS Status; >> + UINT64 ChunkLength; >> >> // >> // Ignore invocations that only modify permission bits >> @@ -687,14 +688,44 @@ SetMemoryAttributes ( >> return EFI_SUCCESS; >> } >> >> - if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) { >> - // Is the base and length a multiple of 1 MB? >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length >> 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); >> - Status = UpdateSectionEntries (BaseAddress, Length, Attributes, >> VirtualMask); >> - } else { >> - // Base and/or length is not a multiple of 1 MB >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x >> to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes)); >> - Status = UpdatePageEntries (BaseAddress, Length, Attributes, >> VirtualMask); >> + while (Length > 0) { > > Would this not end up returning an uninitialized Status if called with > Length == 0? > Yes, well spotted. I will just add an early 'return EFI_SUCCESS' for this case. >> + if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) && >> + Length >= TT_DESCRIPTOR_SECTION_SIZE) { >> + >> + ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE; >> + >> + DEBUG ((DEBUG_PAGE, >> + "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n", >> + BaseAddress, ChunkLength, Attributes)); >> + >> + Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes, >> + VirtualMask); >> + } else { >> + >> + // >> + // Process page by page until the next section boundary, but only if >> + // we have more than a section's worth of area to deal with after >> that. >> + // >> + ChunkLength = TT_DESCRIPTOR_SECTION_SIZE - >> + (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE); >> + if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) { >> + ChunkLength = Length; >> + } >> + >> + DEBUG ((DEBUG_PAGE, >> + "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n", >> + BaseAddress, ChunkLength, Attributes)); >> + >> + Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes, >> + VirtualMask); >> + } >> + >> + if (EFI_ERROR (Status)) { >> + break; >> + } >> + >> + BaseAddress += ChunkLength; >> + Length -= ChunkLength; >> } >> >> // Flush d-cache so descriptors make it back to uncached memory for >> subsequent table walks >> -- >> 2.7.4 >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel