* [PATCH v2 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection
@ 2017-03-02 10:36 Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily Ard Biesheuvel
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2017-03-02 10:36 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
This series makes the prerequisite modifications to the ARM version of
the CpuDxe driver so we can enable PE/COFF image and NX memory protection
for ARM platforms, including ArmVirtPkg (#4)
Patch #1 refactors CpuSetMemoryAttributes() so it no longer splits section
mappings into page mappings unnecessarily.
Patch #2 removes some unnecessary cache/TLB maintenance, which becomes very
costly when CpuSetMemoryAttributes() is used in anger as is the case with
memory protections enabled.
Patch #3 wires up the EFI_MEMORY_RO/EFI_MEMORY_XP attributes, which were
ignored before.
Patch #4 enables the protection features for ArmVirtPkg platforms when
built for 32-bit ARM.
Changes since v1:
- trigger full TLB flush when UpdatePageEntries() results in a section split
- Make cache maintenance of the remapped regions conditional on whether the
memory type changed. This prevents an inadvertent cache clean/invalidate by
VA of the entire RAM area when the NX attribute is applied to it.
- remove DEBUG_INFO attribute from SetMemoryAttributes DEBUG output
- add Laszlo's R-b to #4
Ard Biesheuvel (4):
ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance
ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()
ArmVirtPkg: enable PE/COFF image and memory protection for ARM
platforms
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 250 ++++++++++----------
ArmVirtPkg/ArmVirt.dsc.inc | 9 +-
2 files changed, 135 insertions(+), 124 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
2017-03-02 10:36 [PATCH v2 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection Ard Biesheuvel
@ 2017-03-02 10:36 ` Ard Biesheuvel
2017-03-06 14:12 ` Leif Lindholm
2017-03-02 10:36 ` [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance Ard Biesheuvel
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2017-03-02 10:36 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
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
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.biesheuvel@linaro.org>
---
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) {
+ 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance
2017-03-02 10:36 [PATCH v2 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily Ard Biesheuvel
@ 2017-03-02 10:36 ` Ard Biesheuvel
2017-03-06 14:27 ` Leif Lindholm
2017-03-02 10:36 ` [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes() Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 4/4] ArmVirtPkg: enable PE/COFF image and memory protection for ARM platforms Ard Biesheuvel
3 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2017-03-02 10:36 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
Page and section entries in the page tables are updated using the
helper ArmUpdateTranslationTableEntry(), which cleans the page
table entry to the PoC, and invalidates the TLB entry covering
the page described by the entry being updated.
Since we may be updating section entries, we might be leaving stale
TLB entries at this point (for all pages in the section except the
first one), which will be invalidated wholesale at the end of
SetMemoryAttributes(). At that point, all caches are cleaned *and*
invalidated as well.
This cache maintenance is costly and unnecessary. The TLB maintenance
is only necessary if we updated any section entries, since any page
by page entries that have been updated will have been invalidated
individually by ArmUpdateTranslationTableEntry().
So drop the clean/invalidate of the caches, and only perform the
full TLB flush if UpdateSectionEntries() was called, or if sections
were split by UpdatePageEntries(). Finally, make the cache maintenance
on the remapped regions themselves conditional on whether any memory
type attributes were modified.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 60 +++++++++++---------
1 file changed, 34 insertions(+), 26 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index ce4d529bda67..26b637e7658f 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -347,10 +347,11 @@ SyncCacheConfig (
EFI_STATUS
UpdatePageEntries (
- IN EFI_PHYSICAL_ADDRESS BaseAddress,
- IN UINT64 Length,
- IN UINT64 Attributes,
- IN EFI_PHYSICAL_ADDRESS VirtualMask
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes,
+ IN EFI_PHYSICAL_ADDRESS VirtualMask,
+ OUT BOOLEAN *FlushTlbs
)
{
EFI_STATUS Status;
@@ -446,6 +447,9 @@ UpdatePageEntries (
// Re-read descriptor
Descriptor = FirstLevelTable[FirstLevelIdx];
+ if (FlushTlbs != NULL) {
+ *FlushTlbs = TRUE;
+ }
}
// Obtain page table base address
@@ -471,15 +475,16 @@ UpdatePageEntries (
if (CurrentPageTableEntry != PageTableEntry) {
Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
- if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
- // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
- // Note assumes switch(Attributes), not ARMv7 possibilities
- WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
- }
// Only need to update if we are changing the entry
PageTable[PageTableIndex] = PageTableEntry;
ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
+
+ // Clean/invalidate the cache for this page, but only
+ // if we are modifying the memory type attributes
+ if (((CurrentPageTableEntry ^ PageTableEntry) & TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK) != 0) {
+ WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
+ }
}
Status = EFI_SUCCESS;
@@ -581,7 +586,12 @@ UpdateSectionEntries (
// has this descriptor already been coverted to pages?
if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
// forward this 1MB range to page table function instead
- Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
+ Status = UpdatePageEntries (
+ (FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT,
+ TT_DESCRIPTOR_SECTION_SIZE,
+ Attributes,
+ VirtualMask,
+ NULL);
} else {
// still a section entry
@@ -596,15 +606,16 @@ UpdateSectionEntries (
if (CurrentDescriptor != Descriptor) {
Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
- if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
- // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
- // Note assumes switch(Attributes), not ARMv7 possabilities
- WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
- }
// Only need to update if we are changing the descriptor
FirstLevelTable[FirstLevelIdx + i] = Descriptor;
ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
+
+ // Clean/invalidate the cache for this section, but only
+ // if we are modifying the memory type attributes
+ if (((CurrentDescriptor ^ Descriptor) & TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK) != 0) {
+ WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
+ }
}
Status = EFI_SUCCESS;
@@ -680,6 +691,7 @@ SetMemoryAttributes (
{
EFI_STATUS Status;
UINT64 ChunkLength;
+ BOOLEAN FlushTlbs;
//
// Ignore invocations that only modify permission bits
@@ -688,6 +700,7 @@ SetMemoryAttributes (
return EFI_SUCCESS;
}
+ FlushTlbs = FALSE;
while (Length > 0) {
if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
Length >= TT_DESCRIPTOR_SECTION_SIZE) {
@@ -700,6 +713,8 @@ SetMemoryAttributes (
Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
VirtualMask);
+
+ FlushTlbs = TRUE;
} else {
//
@@ -717,7 +732,7 @@ SetMemoryAttributes (
BaseAddress, ChunkLength, Attributes));
Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
- VirtualMask);
+ VirtualMask, &FlushTlbs);
}
if (EFI_ERROR (Status)) {
@@ -728,16 +743,9 @@ SetMemoryAttributes (
Length -= ChunkLength;
}
- // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
- // flush and invalidate pages
- //TODO: Do we really need to invalidate the caches everytime we change the memory attributes ?
- ArmCleanInvalidateDataCache ();
-
- ArmInvalidateInstructionCache ();
-
- // Invalidate all TLB entries so changes are synced
- ArmInvalidateTlb ();
-
+ if (FlushTlbs) {
+ ArmInvalidateTlb ();
+ }
return Status;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()
2017-03-02 10:36 [PATCH v2 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance Ard Biesheuvel
@ 2017-03-02 10:36 ` Ard Biesheuvel
2017-03-06 14:48 ` Leif Lindholm
2017-03-02 10:36 ` [PATCH v2 4/4] ArmVirtPkg: enable PE/COFF image and memory protection for ARM platforms Ard Biesheuvel
3 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2017-03-02 10:36 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
Enable the use of strict memory permissions on ARM by processing the
EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,
calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP
bits will preserve the cacheability attributes. Permissions attributes
are not preserved when setting the memory type only: the way the memory
permission attributes are defined does not allows for that, and so this
situation does not deviate from other architectures.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------
1 file changed, 62 insertions(+), 89 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 26b637e7658f..6dd749dadf8b 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -374,50 +374,41 @@ UpdatePageEntries (
// EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
// EntryValue: values at bit positions specified by EntryMask
- EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;
- EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
+ EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
+ if ((Attributes & EFI_MEMORY_XP) != 0) {
+ EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
+ } else {
+ EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
+ }
+
// Although the PI spec is unclear on this the GCD guarantees that only
// one Attribute bit is set at a time, so we can safely use a switch statement
- switch (Attributes) {
- case EFI_MEMORY_UC:
- // modify cacheability attributes
- EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
- // map to strongly ordered
- EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
- break;
-
- case EFI_MEMORY_WC:
- // modify cacheability attributes
- EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
- // map to normal non-cachable
- EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
- break;
-
- case EFI_MEMORY_WT:
- // modify cacheability attributes
- EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
- // write through with no-allocate
- EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
- break;
-
- case EFI_MEMORY_WB:
- // modify cacheability attributes
- EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
- // write back (with allocate)
- EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
- break;
-
- case EFI_MEMORY_WP:
- case EFI_MEMORY_XP:
- case EFI_MEMORY_UCE:
- // cannot be implemented UEFI definition unclear for ARM
- // Cause a page fault if these ranges are accessed.
- EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;
- DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
- break;
+ if ((Attributes & EFI_MEMORY_UC) != 0) {
+ // modify cacheability attributes
+ EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+ // map to strongly ordered
+ EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
+ } else if ((Attributes & EFI_MEMORY_WC) != 0) {
+ // modify cacheability attributes
+ EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+ // map to normal non-cachable
+ EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
+ } else if ((Attributes & EFI_MEMORY_WT) != 0) {
+ // modify cacheability attributes
+ EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+ // write through with no-allocate
+ EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
+ } else if ((Attributes & EFI_MEMORY_WB) != 0) {
+ // modify cacheability attributes
+ EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+ // write back (with allocate)
+ EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
+ }
- default:
- return EFI_UNSUPPORTED;
+ if ((Attributes & EFI_MEMORY_RO) != 0) {
+ EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
+ } else {
+ EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
}
// Obtain page table base
@@ -520,53 +511,42 @@ UpdateSectionEntries (
// EntryValue: values at bit positions specified by EntryMask
// Make sure we handle a section range that is unmapped
- EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;
+ EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
+ TT_DESCRIPTOR_SECTION_AP_MASK;
EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
// Although the PI spec is unclear on this the GCD guarantees that only
// one Attribute bit is set at a time, so we can safely use a switch statement
- switch(Attributes) {
- case EFI_MEMORY_UC:
- // modify cacheability attributes
- EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
- // map to strongly ordered
- EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
- break;
-
- case EFI_MEMORY_WC:
- // modify cacheability attributes
- EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
- // map to normal non-cachable
- EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
- break;
-
- case EFI_MEMORY_WT:
- // modify cacheability attributes
- EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
- // write through with no-allocate
- EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
- break;
-
- case EFI_MEMORY_WB:
- // modify cacheability attributes
- EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
- // write back (with allocate)
- EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
- break;
-
- case EFI_MEMORY_WP:
- case EFI_MEMORY_XP:
- case EFI_MEMORY_RP:
- case EFI_MEMORY_UCE:
- // cannot be implemented UEFI definition unclear for ARM
- // Cause a page fault if these ranges are accessed.
- EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
- DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
- break;
+ if ((Attributes & EFI_MEMORY_UC) != 0) {
+ // modify cacheability attributes
+ EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+ // map to strongly ordered
+ EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
+ } else if ((Attributes & EFI_MEMORY_WC) != 0) {
+ // modify cacheability attributes
+ EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+ // map to normal non-cachable
+ EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
+ } else if ((Attributes & EFI_MEMORY_WT) != 0) {
+ // modify cacheability attributes
+ EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+ // write through with no-allocate
+ EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
+ } else if ((Attributes & EFI_MEMORY_WB) != 0) {
+ // modify cacheability attributes
+ EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+ // write back (with allocate)
+ EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
+ }
+ if ((Attributes & EFI_MEMORY_RO) != 0) {
+ EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
+ } else {
+ EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
+ }
- default:
- return EFI_UNSUPPORTED;
+ if ((Attributes & EFI_MEMORY_XP) != 0) {
+ EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
}
// obtain page table base
@@ -693,13 +673,6 @@ SetMemoryAttributes (
UINT64 ChunkLength;
BOOLEAN FlushTlbs;
- //
- // Ignore invocations that only modify permission bits
- //
- if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
- return EFI_SUCCESS;
- }
-
FlushTlbs = FALSE;
while (Length > 0) {
if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 4/4] ArmVirtPkg: enable PE/COFF image and memory protection for ARM platforms
2017-03-02 10:36 [PATCH v2 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection Ard Biesheuvel
` (2 preceding siblings ...)
2017-03-02 10:36 ` [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes() Ard Biesheuvel
@ 2017-03-02 10:36 ` Ard Biesheuvel
3 siblings, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2017-03-02 10:36 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
Like for AARCH64, enable PE/COFF image and NX memory protection for all
32-bit ARM virt platforms.
Note that this does not [yet] protect EfiLoaderData regions, due to
compatibility issues with GRUB.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/ArmVirt.dsc.inc | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index a91b27f13cf2..acfb71d3ff6c 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -18,7 +18,7 @@ [Defines]
DEFINE TTY_TERMINAL = FALSE
[BuildOptions.common.EDKII.DXE_DRIVER,BuildOptions.common.EDKII.UEFI_DRIVER,BuildOptions.common.EDKII.UEFI_APPLICATION]
- GCC:*_*_AARCH64_DLINK_FLAGS = -z common-page-size=0x1000
+ GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
GCC:*_*_ARM_DLINK_FLAGS = -z common-page-size=0x1000
@@ -373,10 +373,6 @@ [PcdsFixedAtBuild.common]
gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94}
!endif
-[PcdsFixedAtBuild.ARM]
- gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
-
-[PcdsFixedAtBuild.AARCH64]
#
# Enable strict image permissions for all images. (This applies
# only to images that were built with >= 4 KB section alignment.)
@@ -390,6 +386,9 @@ [PcdsFixedAtBuild.AARCH64]
#
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
+[PcdsFixedAtBuild.ARM]
+ gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
+
[Components.common]
#
# Networking stack
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
2017-03-02 10:36 ` [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily Ard Biesheuvel
@ 2017-03-06 14:12 ` Leif Lindholm
2017-03-06 14:55 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Leif Lindholm @ 2017-03-06 14:12 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek
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"?
> 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.biesheuvel@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?
/
Leif
> + 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance
2017-03-02 10:36 ` [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance Ard Biesheuvel
@ 2017-03-06 14:27 ` Leif Lindholm
2017-03-06 15:06 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Leif Lindholm @ 2017-03-06 14:27 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek
On Thu, Mar 02, 2017 at 10:36:14AM +0000, Ard Biesheuvel wrote:
> Page and section entries in the page tables are updated using the
> helper ArmUpdateTranslationTableEntry(), which cleans the page
> table entry to the PoC, and invalidates the TLB entry covering
> the page described by the entry being updated.
>
> Since we may be updating section entries, we might be leaving stale
> TLB entries at this point (for all pages in the section except the
> first one), which will be invalidated wholesale at the end of
> SetMemoryAttributes(). At that point, all caches are cleaned *and*
> invalidated as well.
>
> This cache maintenance is costly and unnecessary. The TLB maintenance
> is only necessary if we updated any section entries, since any page
> by page entries that have been updated will have been invalidated
> individually by ArmUpdateTranslationTableEntry().
>
> So drop the clean/invalidate of the caches, and only perform the
> full TLB flush if UpdateSectionEntries() was called, or if sections
> were split by UpdatePageEntries(). Finally, make the cache maintenance
> on the remapped regions themselves conditional on whether any memory
> type attributes were modified.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 60 +++++++++++---------
> 1 file changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index ce4d529bda67..26b637e7658f 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -347,10 +347,11 @@ SyncCacheConfig (
>
> EFI_STATUS
> UpdatePageEntries (
> - IN EFI_PHYSICAL_ADDRESS BaseAddress,
> - IN UINT64 Length,
> - IN UINT64 Attributes,
> - IN EFI_PHYSICAL_ADDRESS VirtualMask
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> + IN UINT64 Length,
> + IN UINT64 Attributes,
> + IN EFI_PHYSICAL_ADDRESS VirtualMask,
> + OUT BOOLEAN *FlushTlbs
> )
> {
> EFI_STATUS Status;
> @@ -446,6 +447,9 @@ UpdatePageEntries (
>
> // Re-read descriptor
> Descriptor = FirstLevelTable[FirstLevelIdx];
> + if (FlushTlbs != NULL) {
If FlushTlbs is optional, should it not be marked OPTIONAL above?
If you fold that in:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> + *FlushTlbs = TRUE;
> + }
> }
>
> // Obtain page table base address
> @@ -471,15 +475,16 @@ UpdatePageEntries (
>
> if (CurrentPageTableEntry != PageTableEntry) {
> Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
> - if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
> - // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
> - // Note assumes switch(Attributes), not ARMv7 possibilities
> - WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
> - }
>
> // Only need to update if we are changing the entry
> PageTable[PageTableIndex] = PageTableEntry;
> ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
> +
> + // Clean/invalidate the cache for this page, but only
> + // if we are modifying the memory type attributes
> + if (((CurrentPageTableEntry ^ PageTableEntry) & TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK) != 0) {
> + WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
> + }
> }
>
> Status = EFI_SUCCESS;
> @@ -581,7 +586,12 @@ UpdateSectionEntries (
> // has this descriptor already been coverted to pages?
> if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
> // forward this 1MB range to page table function instead
> - Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
> + Status = UpdatePageEntries (
> + (FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT,
> + TT_DESCRIPTOR_SECTION_SIZE,
> + Attributes,
> + VirtualMask,
> + NULL);
> } else {
> // still a section entry
>
> @@ -596,15 +606,16 @@ UpdateSectionEntries (
>
> if (CurrentDescriptor != Descriptor) {
> Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> - if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
> - // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
> - // Note assumes switch(Attributes), not ARMv7 possabilities
> - WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
> - }
>
> // Only need to update if we are changing the descriptor
> FirstLevelTable[FirstLevelIdx + i] = Descriptor;
> ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
> +
> + // Clean/invalidate the cache for this section, but only
> + // if we are modifying the memory type attributes
> + if (((CurrentDescriptor ^ Descriptor) & TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK) != 0) {
> + WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
> + }
> }
>
> Status = EFI_SUCCESS;
> @@ -680,6 +691,7 @@ SetMemoryAttributes (
> {
> EFI_STATUS Status;
> UINT64 ChunkLength;
> + BOOLEAN FlushTlbs;
>
> //
> // Ignore invocations that only modify permission bits
> @@ -688,6 +700,7 @@ SetMemoryAttributes (
> return EFI_SUCCESS;
> }
>
> + FlushTlbs = FALSE;
> while (Length > 0) {
> if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> Length >= TT_DESCRIPTOR_SECTION_SIZE) {
> @@ -700,6 +713,8 @@ SetMemoryAttributes (
>
> Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
> VirtualMask);
> +
> + FlushTlbs = TRUE;
> } else {
>
> //
> @@ -717,7 +732,7 @@ SetMemoryAttributes (
> BaseAddress, ChunkLength, Attributes));
>
> Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
> - VirtualMask);
> + VirtualMask, &FlushTlbs);
> }
>
> if (EFI_ERROR (Status)) {
> @@ -728,16 +743,9 @@ SetMemoryAttributes (
> Length -= ChunkLength;
> }
>
> - // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
> - // flush and invalidate pages
> - //TODO: Do we really need to invalidate the caches everytime we change the memory attributes ?
> - ArmCleanInvalidateDataCache ();
> -
> - ArmInvalidateInstructionCache ();
> -
> - // Invalidate all TLB entries so changes are synced
> - ArmInvalidateTlb ();
> -
> + if (FlushTlbs) {
> + ArmInvalidateTlb ();
> + }
> return Status;
> }
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()
2017-03-02 10:36 ` [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes() Ard Biesheuvel
@ 2017-03-06 14:48 ` Leif Lindholm
2017-03-06 15:11 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Leif Lindholm @ 2017-03-06 14:48 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek
On Thu, Mar 02, 2017 at 10:36:15AM +0000, Ard Biesheuvel wrote:
> Enable the use of strict memory permissions on ARM by processing the
> EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,
> calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP
> bits will preserve the cacheability attributes. Permissions attributes
> are not preserved when setting the memory type only: the way the memory
> permission attributes are defined does not allows for that, and so this
> situation does not deviate from other architectures.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------
> 1 file changed, 62 insertions(+), 89 deletions(-)
>
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 26b637e7658f..6dd749dadf8b 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -374,50 +374,41 @@ UpdatePageEntries (
>
> // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
> // EntryValue: values at bit positions specified by EntryMask
> - EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;
> - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> + EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
> + if ((Attributes & EFI_MEMORY_XP) != 0) {
> + EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
> + } else {
> + EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> + }
> +
> // Although the PI spec is unclear on this the GCD guarantees that only
> // one Attribute bit is set at a time, so we can safely use a switch statement
But the switch statement is going away.
The change effectively introduces a guaranteed priority of
interpretation if the spec is violated. Say something about this order
being arbitrarily decided due to PI spce guarantee instead?
> - switch (Attributes) {
> - case EFI_MEMORY_UC:
> - // modify cacheability attributes
> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> - // map to strongly ordered
> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> - break;
> -
> - case EFI_MEMORY_WC:
> - // modify cacheability attributes
> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> - // map to normal non-cachable
> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> - break;
> -
> - case EFI_MEMORY_WT:
> - // modify cacheability attributes
> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> - // write through with no-allocate
> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> - break;
> -
> - case EFI_MEMORY_WB:
> - // modify cacheability attributes
> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> - // write back (with allocate)
> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> - break;
> -
> - case EFI_MEMORY_WP:
> - case EFI_MEMORY_XP:
> - case EFI_MEMORY_UCE:
> - // cannot be implemented UEFI definition unclear for ARM
> - // Cause a page fault if these ranges are accessed.
> - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;
> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
> - break;
> + if ((Attributes & EFI_MEMORY_UC) != 0) {
Or should these be "Attributes & Mask" == "ATTRIBUTE"?
> + // modify cacheability attributes
> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> + // map to strongly ordered
> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> + } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> + // modify cacheability attributes
> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> + // map to normal non-cachable
> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> + } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> + // modify cacheability attributes
> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> + // write through with no-allocate
> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> + } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> + // modify cacheability attributes
> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> + // write back (with allocate)
> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> + }
>
> - default:
> - return EFI_UNSUPPORTED;
Do we not want a fallback handling for the if-form as well?
> + if ((Attributes & EFI_MEMORY_RO) != 0) {
> + EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
> + } else {
> + EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
> }
>
> // Obtain page table base
> @@ -520,53 +511,42 @@ UpdateSectionEntries (
> // EntryValue: values at bit positions specified by EntryMask
>
> // Make sure we handle a section range that is unmapped
> - EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;
> + EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
> + TT_DESCRIPTOR_SECTION_AP_MASK;
> EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
>
> // Although the PI spec is unclear on this the GCD guarantees that only
> // one Attribute bit is set at a time, so we can safely use a switch statement
Repeat of above.
> - switch(Attributes) {
> - case EFI_MEMORY_UC:
> - // modify cacheability attributes
> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> - // map to strongly ordered
> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> - break;
> -
> - case EFI_MEMORY_WC:
> - // modify cacheability attributes
> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> - // map to normal non-cachable
> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> - break;
> -
> - case EFI_MEMORY_WT:
> - // modify cacheability attributes
> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> - // write through with no-allocate
> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> - break;
> -
> - case EFI_MEMORY_WB:
> - // modify cacheability attributes
> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> - // write back (with allocate)
> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> - break;
> -
> - case EFI_MEMORY_WP:
> - case EFI_MEMORY_XP:
> - case EFI_MEMORY_RP:
> - case EFI_MEMORY_UCE:
> - // cannot be implemented UEFI definition unclear for ARM
> - // Cause a page fault if these ranges are accessed.
> - EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
> - break;
> + if ((Attributes & EFI_MEMORY_UC) != 0) {
> + // modify cacheability attributes
> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> + // map to strongly ordered
> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> + } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> + // modify cacheability attributes
> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> + // map to normal non-cachable
> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> + } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> + // modify cacheability attributes
> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> + // write through with no-allocate
> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> + } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> + // modify cacheability attributes
> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> + // write back (with allocate)
> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> + }
(Again, same questions as above.)
>
> + if ((Attributes & EFI_MEMORY_RO) != 0) {
> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
> + } else {
> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> + }
>
> - default:
> - return EFI_UNSUPPORTED;
> + if ((Attributes & EFI_MEMORY_XP) != 0) {
> + EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
> }
>
> // obtain page table base
> @@ -693,13 +673,6 @@ SetMemoryAttributes (
> UINT64 ChunkLength;
> BOOLEAN FlushTlbs;
>
> - //
> - // Ignore invocations that only modify permission bits
> - //
> - if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
> - return EFI_SUCCESS;
> - }
> -
> FlushTlbs = FALSE;
> while (Length > 0) {
> if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
2017-03-06 14:12 ` Leif Lindholm
@ 2017-03-06 14:55 ` Ard Biesheuvel
2017-03-06 16:24 ` Leif Lindholm
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2017-03-06 14:55 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
On 6 March 2017 at 15:12, Leif Lindholm <leif.lindholm@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.biesheuvel@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
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance
2017-03-06 14:27 ` Leif Lindholm
@ 2017-03-06 15:06 ` Ard Biesheuvel
0 siblings, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2017-03-06 15:06 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
On 6 March 2017 at 15:27, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Mar 02, 2017 at 10:36:14AM +0000, Ard Biesheuvel wrote:
>> Page and section entries in the page tables are updated using the
>> helper ArmUpdateTranslationTableEntry(), which cleans the page
>> table entry to the PoC, and invalidates the TLB entry covering
>> the page described by the entry being updated.
>>
>> Since we may be updating section entries, we might be leaving stale
>> TLB entries at this point (for all pages in the section except the
>> first one), which will be invalidated wholesale at the end of
>> SetMemoryAttributes(). At that point, all caches are cleaned *and*
>> invalidated as well.
>>
>> This cache maintenance is costly and unnecessary. The TLB maintenance
>> is only necessary if we updated any section entries, since any page
>> by page entries that have been updated will have been invalidated
>> individually by ArmUpdateTranslationTableEntry().
>>
>> So drop the clean/invalidate of the caches, and only perform the
>> full TLB flush if UpdateSectionEntries() was called, or if sections
>> were split by UpdatePageEntries(). Finally, make the cache maintenance
>> on the remapped regions themselves conditional on whether any memory
>> type attributes were modified.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 60 +++++++++++---------
>> 1 file changed, 34 insertions(+), 26 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> index ce4d529bda67..26b637e7658f 100644
>> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> @@ -347,10 +347,11 @@ SyncCacheConfig (
>>
>> EFI_STATUS
>> UpdatePageEntries (
>> - IN EFI_PHYSICAL_ADDRESS BaseAddress,
>> - IN UINT64 Length,
>> - IN UINT64 Attributes,
>> - IN EFI_PHYSICAL_ADDRESS VirtualMask
>> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>> + IN UINT64 Length,
>> + IN UINT64 Attributes,
>> + IN EFI_PHYSICAL_ADDRESS VirtualMask,
>> + OUT BOOLEAN *FlushTlbs
>> )
>> {
>> EFI_STATUS Status;
>> @@ -446,6 +447,9 @@ UpdatePageEntries (
>>
>> // Re-read descriptor
>> Descriptor = FirstLevelTable[FirstLevelIdx];
>> + if (FlushTlbs != NULL) {
>
> If FlushTlbs is optional, should it not be marked OPTIONAL above?
Yes
> If you fold that in:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
Thanks
>> + *FlushTlbs = TRUE;
>> + }
>> }
>>
>> // Obtain page table base address
>> @@ -471,15 +475,16 @@ UpdatePageEntries (
>>
>> if (CurrentPageTableEntry != PageTableEntry) {
>> Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
>> - if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
>> - // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
>> - // Note assumes switch(Attributes), not ARMv7 possibilities
>> - WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
>> - }
>>
>> // Only need to update if we are changing the entry
>> PageTable[PageTableIndex] = PageTableEntry;
>> ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
>> +
>> + // Clean/invalidate the cache for this page, but only
>> + // if we are modifying the memory type attributes
>> + if (((CurrentPageTableEntry ^ PageTableEntry) & TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK) != 0) {
>> + WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
>> + }
>> }
>>
>> Status = EFI_SUCCESS;
>> @@ -581,7 +586,12 @@ UpdateSectionEntries (
>> // has this descriptor already been coverted to pages?
>> if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
>> // forward this 1MB range to page table function instead
>> - Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
>> + Status = UpdatePageEntries (
>> + (FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT,
>> + TT_DESCRIPTOR_SECTION_SIZE,
>> + Attributes,
>> + VirtualMask,
>> + NULL);
>> } else {
>> // still a section entry
>>
>> @@ -596,15 +606,16 @@ UpdateSectionEntries (
>>
>> if (CurrentDescriptor != Descriptor) {
>> Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
>> - if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
>> - // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
>> - // Note assumes switch(Attributes), not ARMv7 possabilities
>> - WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
>> - }
>>
>> // Only need to update if we are changing the descriptor
>> FirstLevelTable[FirstLevelIdx + i] = Descriptor;
>> ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
>> +
>> + // Clean/invalidate the cache for this section, but only
>> + // if we are modifying the memory type attributes
>> + if (((CurrentDescriptor ^ Descriptor) & TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK) != 0) {
>> + WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
>> + }
>> }
>>
>> Status = EFI_SUCCESS;
>> @@ -680,6 +691,7 @@ SetMemoryAttributes (
>> {
>> EFI_STATUS Status;
>> UINT64 ChunkLength;
>> + BOOLEAN FlushTlbs;
>>
>> //
>> // Ignore invocations that only modify permission bits
>> @@ -688,6 +700,7 @@ SetMemoryAttributes (
>> return EFI_SUCCESS;
>> }
>>
>> + FlushTlbs = FALSE;
>> while (Length > 0) {
>> if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
>> Length >= TT_DESCRIPTOR_SECTION_SIZE) {
>> @@ -700,6 +713,8 @@ SetMemoryAttributes (
>>
>> Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
>> VirtualMask);
>> +
>> + FlushTlbs = TRUE;
>> } else {
>>
>> //
>> @@ -717,7 +732,7 @@ SetMemoryAttributes (
>> BaseAddress, ChunkLength, Attributes));
>>
>> Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
>> - VirtualMask);
>> + VirtualMask, &FlushTlbs);
>> }
>>
>> if (EFI_ERROR (Status)) {
>> @@ -728,16 +743,9 @@ SetMemoryAttributes (
>> Length -= ChunkLength;
>> }
>>
>> - // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
>> - // flush and invalidate pages
>> - //TODO: Do we really need to invalidate the caches everytime we change the memory attributes ?
>> - ArmCleanInvalidateDataCache ();
>> -
>> - ArmInvalidateInstructionCache ();
>> -
>> - // Invalidate all TLB entries so changes are synced
>> - ArmInvalidateTlb ();
>> -
>> + if (FlushTlbs) {
>> + ArmInvalidateTlb ();
>> + }
>> return Status;
>> }
>>
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()
2017-03-06 14:48 ` Leif Lindholm
@ 2017-03-06 15:11 ` Ard Biesheuvel
2017-03-06 16:40 ` Leif Lindholm
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2017-03-06 15:11 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
On 6 March 2017 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Mar 02, 2017 at 10:36:15AM +0000, Ard Biesheuvel wrote:
>> Enable the use of strict memory permissions on ARM by processing the
>> EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,
>> calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP
>> bits will preserve the cacheability attributes. Permissions attributes
>> are not preserved when setting the memory type only: the way the memory
>> permission attributes are defined does not allows for that, and so this
>> situation does not deviate from other architectures.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------
>> 1 file changed, 62 insertions(+), 89 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> index 26b637e7658f..6dd749dadf8b 100644
>> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> @@ -374,50 +374,41 @@ UpdatePageEntries (
>>
>> // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
>> // EntryValue: values at bit positions specified by EntryMask
>> - EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;
>> - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
>> + EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
>> + if ((Attributes & EFI_MEMORY_XP) != 0) {
>> + EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
>> + } else {
>> + EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
>> + }
>> +
>> // Although the PI spec is unclear on this the GCD guarantees that only
>> // one Attribute bit is set at a time, so we can safely use a switch statement
>
> But the switch statement is going away.
>
> The change effectively introduces a guaranteed priority of
> interpretation if the spec is violated. Say something about this order
> being arbitrarily decided due to PI spce guarantee instead?
>
Indeed.
>> - switch (Attributes) {
>> - case EFI_MEMORY_UC:
>> - // modify cacheability attributes
>> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> - // map to strongly ordered
>> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
>> - break;
>> -
>> - case EFI_MEMORY_WC:
>> - // modify cacheability attributes
>> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> - // map to normal non-cachable
>> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
>> - break;
>> -
>> - case EFI_MEMORY_WT:
>> - // modify cacheability attributes
>> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> - // write through with no-allocate
>> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
>> - break;
>> -
>> - case EFI_MEMORY_WB:
>> - // modify cacheability attributes
>> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> - // write back (with allocate)
>> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
>> - break;
>> -
>> - case EFI_MEMORY_WP:
>> - case EFI_MEMORY_XP:
>> - case EFI_MEMORY_UCE:
>> - // cannot be implemented UEFI definition unclear for ARM
>> - // Cause a page fault if these ranges are accessed.
>> - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;
>> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
>> - break;
>> + if ((Attributes & EFI_MEMORY_UC) != 0) {
>
> Or should these be "Attributes & Mask" == "ATTRIBUTE"?
>
I know this is more idiomatic for EDK2, but the mask *is* the
attribute in this case, and so the former implies the latter. If the
mask were
(EFI_MEMORY_WB | EFI_MEMORY_WT | etc
it would make more sense do to the latter I think
>> + // modify cacheability attributes
>> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> + // map to strongly ordered
>> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
>> + } else if ((Attributes & EFI_MEMORY_WC) != 0) {
>> + // modify cacheability attributes
>> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> + // map to normal non-cachable
>> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
>> + } else if ((Attributes & EFI_MEMORY_WT) != 0) {
>> + // modify cacheability attributes
>> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> + // write through with no-allocate
>> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
>> + } else if ((Attributes & EFI_MEMORY_WB) != 0) {
>> + // modify cacheability attributes
>> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> + // write back (with allocate)
>> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
>> + }
>>
>> - default:
>> - return EFI_UNSUPPORTED;
>
> Do we not want a fallback handling for the if-form as well?
>
No, and that is actually the point of this patch. SetMemoryAttributes
may be called without a memory type argument, in which case it only
modifies permission attributes, and leaves the memory type attributes
only.
>> + if ((Attributes & EFI_MEMORY_RO) != 0) {
>> + EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
>> + } else {
>> + EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
>> }
>>
>> // Obtain page table base
>> @@ -520,53 +511,42 @@ UpdateSectionEntries (
>> // EntryValue: values at bit positions specified by EntryMask
>>
>> // Make sure we handle a section range that is unmapped
>> - EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;
>> + EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
>> + TT_DESCRIPTOR_SECTION_AP_MASK;
>> EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
>>
>> // Although the PI spec is unclear on this the GCD guarantees that only
>> // one Attribute bit is set at a time, so we can safely use a switch statement
>
> Repeat of above.
>
>> - switch(Attributes) {
>> - case EFI_MEMORY_UC:
>> - // modify cacheability attributes
>> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> - // map to strongly ordered
>> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
>> - break;
>> -
>> - case EFI_MEMORY_WC:
>> - // modify cacheability attributes
>> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> - // map to normal non-cachable
>> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
>> - break;
>> -
>> - case EFI_MEMORY_WT:
>> - // modify cacheability attributes
>> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> - // write through with no-allocate
>> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
>> - break;
>> -
>> - case EFI_MEMORY_WB:
>> - // modify cacheability attributes
>> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> - // write back (with allocate)
>> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
>> - break;
>> -
>> - case EFI_MEMORY_WP:
>> - case EFI_MEMORY_XP:
>> - case EFI_MEMORY_RP:
>> - case EFI_MEMORY_UCE:
>> - // cannot be implemented UEFI definition unclear for ARM
>> - // Cause a page fault if these ranges are accessed.
>> - EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
>> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
>> - break;
>> + if ((Attributes & EFI_MEMORY_UC) != 0) {
>> + // modify cacheability attributes
>> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> + // map to strongly ordered
>> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
>> + } else if ((Attributes & EFI_MEMORY_WC) != 0) {
>> + // modify cacheability attributes
>> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> + // map to normal non-cachable
>> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
>> + } else if ((Attributes & EFI_MEMORY_WT) != 0) {
>> + // modify cacheability attributes
>> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> + // write through with no-allocate
>> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
>> + } else if ((Attributes & EFI_MEMORY_WB) != 0) {
>> + // modify cacheability attributes
>> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> + // write back (with allocate)
>> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
>> + }
>
> (Again, same questions as above.)
>
>>
>> + if ((Attributes & EFI_MEMORY_RO) != 0) {
>> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
>> + } else {
>> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
>> + }
>>
>> - default:
>> - return EFI_UNSUPPORTED;
>> + if ((Attributes & EFI_MEMORY_XP) != 0) {
>> + EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
>> }
>>
>> // obtain page table base
>> @@ -693,13 +673,6 @@ SetMemoryAttributes (
>> UINT64 ChunkLength;
>> BOOLEAN FlushTlbs;
>>
>> - //
>> - // Ignore invocations that only modify permission bits
>> - //
>> - if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
>> - return EFI_SUCCESS;
>> - }
>> -
>> FlushTlbs = FALSE;
>> while (Length > 0) {
>> if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
2017-03-06 14:55 ` Ard Biesheuvel
@ 2017-03-06 16:24 ` Leif Lindholm
0 siblings, 0 replies; 13+ messages in thread
From: Leif Lindholm @ 2017-03-06 16:24 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
On Mon, Mar 06, 2017 at 03:55:42PM +0100, Ard Biesheuvel wrote:
> On 6 March 2017 at 15:12, Leif Lindholm <leif.lindholm@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
OK, can you expand the statement to say "hurt performance" then?
> >> 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.biesheuvel@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.
Works for me. If you also tweak the commit message:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >> + 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
> >>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()
2017-03-06 15:11 ` Ard Biesheuvel
@ 2017-03-06 16:40 ` Leif Lindholm
0 siblings, 0 replies; 13+ messages in thread
From: Leif Lindholm @ 2017-03-06 16:40 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
On Mon, Mar 06, 2017 at 04:11:50PM +0100, Ard Biesheuvel wrote:
> On 6 March 2017 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Mar 02, 2017 at 10:36:15AM +0000, Ard Biesheuvel wrote:
> >> Enable the use of strict memory permissions on ARM by processing the
> >> EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,
> >> calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP
> >> bits will preserve the cacheability attributes. Permissions attributes
> >> are not preserved when setting the memory type only: the way the memory
> >> permission attributes are defined does not allows for that, and so this
> >> situation does not deviate from other architectures.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------
> >> 1 file changed, 62 insertions(+), 89 deletions(-)
> >>
> >> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> >> index 26b637e7658f..6dd749dadf8b 100644
> >> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> >> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> >> @@ -374,50 +374,41 @@ UpdatePageEntries (
> >>
> >> // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
> >> // EntryValue: values at bit positions specified by EntryMask
> >> - EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;
> >> - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> >> + EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
> >> + if ((Attributes & EFI_MEMORY_XP) != 0) {
> >> + EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
> >> + } else {
> >> + EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> >> + }
> >> +
> >> // Although the PI spec is unclear on this the GCD guarantees that only
> >> // one Attribute bit is set at a time, so we can safely use a switch statement
> >
> > But the switch statement is going away.
> >
> > The change effectively introduces a guaranteed priority of
> > interpretation if the spec is violated. Say something about this order
> > being arbitrarily decided due to PI spce guarantee instead?
> >
>
> Indeed.
OK, I'll hold back to see what you come up with :)
> >> - switch (Attributes) {
> >> - case EFI_MEMORY_UC:
> >> - // modify cacheability attributes
> >> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> - // map to strongly ordered
> >> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> - break;
> >> -
> >> - case EFI_MEMORY_WC:
> >> - // modify cacheability attributes
> >> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> - // map to normal non-cachable
> >> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> - break;
> >> -
> >> - case EFI_MEMORY_WT:
> >> - // modify cacheability attributes
> >> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> - // write through with no-allocate
> >> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> - break;
> >> -
> >> - case EFI_MEMORY_WB:
> >> - // modify cacheability attributes
> >> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> - // write back (with allocate)
> >> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> - break;
> >> -
> >> - case EFI_MEMORY_WP:
> >> - case EFI_MEMORY_XP:
> >> - case EFI_MEMORY_UCE:
> >> - // cannot be implemented UEFI definition unclear for ARM
> >> - // Cause a page fault if these ranges are accessed.
> >> - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;
> >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
> >> - break;
> >> + if ((Attributes & EFI_MEMORY_UC) != 0) {
> >
> > Or should these be "Attributes & Mask" == "ATTRIBUTE"?
> >
>
> I know this is more idiomatic for EDK2, but the mask *is* the
> attribute in this case, and so the former implies the latter. If the
> mask were
OK ... in that case, could you just drop the != 0?
> (EFI_MEMORY_WB | EFI_MEMORY_WT | etc
>
> it would make more sense do to the latter I think
>
> >> + // modify cacheability attributes
> >> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> + // map to strongly ordered
> >> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> + } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> >> + // modify cacheability attributes
> >> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> + // map to normal non-cachable
> >> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> + } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> >> + // modify cacheability attributes
> >> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> + // write through with no-allocate
> >> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> + } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> >> + // modify cacheability attributes
> >> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> + // write back (with allocate)
> >> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> + }
> >>
> >> - default:
> >> - return EFI_UNSUPPORTED;
> >
> > Do we not want a fallback handling for the if-form as well?
> >
>
> No, and that is actually the point of this patch. SetMemoryAttributes
> may be called without a memory type argument, in which case it only
> modifies permission attributes, and leaves the memory type attributes
> only.
But should there not be a final "else if (Attributes &
CACHE_ATTRIBUTE_MASK)" error path then?
/
Leif
> >> + if ((Attributes & EFI_MEMORY_RO) != 0) {
> >> + EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
> >> + } else {
> >> + EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
> >> }
> >>
> >> // Obtain page table base
> >> @@ -520,53 +511,42 @@ UpdateSectionEntries (
> >> // EntryValue: values at bit positions specified by EntryMask
> >>
> >> // Make sure we handle a section range that is unmapped
> >> - EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;
> >> + EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
> >> + TT_DESCRIPTOR_SECTION_AP_MASK;
> >> EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
> >>
> >> // Although the PI spec is unclear on this the GCD guarantees that only
> >> // one Attribute bit is set at a time, so we can safely use a switch statement
> >
> > Repeat of above.
> >
> >> - switch(Attributes) {
> >> - case EFI_MEMORY_UC:
> >> - // modify cacheability attributes
> >> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> - // map to strongly ordered
> >> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> - break;
> >> -
> >> - case EFI_MEMORY_WC:
> >> - // modify cacheability attributes
> >> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> - // map to normal non-cachable
> >> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> - break;
> >> -
> >> - case EFI_MEMORY_WT:
> >> - // modify cacheability attributes
> >> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> - // write through with no-allocate
> >> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> - break;
> >> -
> >> - case EFI_MEMORY_WB:
> >> - // modify cacheability attributes
> >> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> - // write back (with allocate)
> >> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> - break;
> >> -
> >> - case EFI_MEMORY_WP:
> >> - case EFI_MEMORY_XP:
> >> - case EFI_MEMORY_RP:
> >> - case EFI_MEMORY_UCE:
> >> - // cannot be implemented UEFI definition unclear for ARM
> >> - // Cause a page fault if these ranges are accessed.
> >> - EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
> >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
> >> - break;
> >> + if ((Attributes & EFI_MEMORY_UC) != 0) {
> >> + // modify cacheability attributes
> >> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> + // map to strongly ordered
> >> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> + } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> >> + // modify cacheability attributes
> >> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> + // map to normal non-cachable
> >> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> + } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> >> + // modify cacheability attributes
> >> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> + // write through with no-allocate
> >> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> + } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> >> + // modify cacheability attributes
> >> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> + // write back (with allocate)
> >> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> + }
> >
> > (Again, same questions as above.)
> >
> >>
> >> + if ((Attributes & EFI_MEMORY_RO) != 0) {
> >> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
> >> + } else {
> >> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> >> + }
> >>
> >> - default:
> >> - return EFI_UNSUPPORTED;
> >> + if ((Attributes & EFI_MEMORY_XP) != 0) {
> >> + EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
> >> }
> >>
> >> // obtain page table base
> >> @@ -693,13 +673,6 @@ SetMemoryAttributes (
> >> UINT64 ChunkLength;
> >> BOOLEAN FlushTlbs;
> >>
> >> - //
> >> - // Ignore invocations that only modify permission bits
> >> - //
> >> - if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
> >> - return EFI_SUCCESS;
> >> - }
> >> -
> >> FlushTlbs = FALSE;
> >> while (Length > 0) {
> >> if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> >> --
> >> 2.7.4
> >>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-03-06 16:40 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-02 10:36 [PATCH v2 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily Ard Biesheuvel
2017-03-06 14:12 ` Leif Lindholm
2017-03-06 14:55 ` Ard Biesheuvel
2017-03-06 16:24 ` Leif Lindholm
2017-03-02 10:36 ` [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance Ard Biesheuvel
2017-03-06 14:27 ` Leif Lindholm
2017-03-06 15:06 ` Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes() Ard Biesheuvel
2017-03-06 14:48 ` Leif Lindholm
2017-03-06 15:11 ` Ard Biesheuvel
2017-03-06 16:40 ` Leif Lindholm
2017-03-02 10:36 ` [PATCH v2 4/4] ArmVirtPkg: enable PE/COFF image and memory protection for ARM platforms Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox