| Message ID | 1460383035-377-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Ard, On Mon, Apr 11, 2016 at 03:57:15PM +0200, Ard Biesheuvel wrote: > On ARM, manipulating live page tables is cumbersome since the architecture > mandates the use of break-before-make, i.e., replacing a block entry with > a table entry requires an intermediate step via an invalid entry, or TLB > conflicts may occur. > > Since it is not generally feasible to decide in the page table manipulation > routines whether such an invalid entry will result in those routines > themselves to become unavailable, and since UEFI uses an identity mapping > anyway, it is far simpler to just disable the MMU, perform the page table > manipulations, flush the TLBs and re-enable the MMU. Perhaps I am missing something, but I suspect that this isn't so simple. More on that below. > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39 ++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > index b7d23c6f3286..2f8f05df8aec 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > @@ -286,6 +286,11 @@ GetBlockEntryListFromAddress ( > } > } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) { > // If we are not at the last level then we need to split this BlockEntry > + // Since splitting live block entries may cause TLB conflicts, we need to > + // enter this function with the MMU disabled and flush the TLBs before > + // re-enabling it. This is the responsibility of the caller. > + ASSERT (!ArmMmuEnabled ()); > + > if (IndexLevel != PageLevel) { > // Retrieve the attributes from the block entry > Attributes = *BlockEntry & TT_ATTRIBUTES_MASK; > @@ -453,6 +458,8 @@ SetMemoryAttributes ( > RETURN_STATUS Status; > ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion; > UINT64 *TranslationTable; > + UINTN SavedInterruptState; > + BOOLEAN SavedMmuState; > > MemoryRegion.PhysicalBase = BaseAddress; > MemoryRegion.VirtualBase = BaseAddress; > @@ -461,6 +468,15 @@ SetMemoryAttributes ( > > TranslationTable = ArmGetTTBR0BaseAddress (); > > + SavedMmuState = ArmMmuEnabled (); > + if (SavedMmuState) { > + SavedInterruptState = ArmGetInterruptState (); > + if (SavedInterruptState) { > + ArmDisableInterrupts(); > + } > + ArmDisableMmu(); > + } Unless ArmDisableMmu() performs a Clean+Invalidate of all memory in use by EDK2 (after the MMU is disabled), this is not safe. For example, the latest version of your stack (which may be dirty in cache) is not guaranteed to be visible after the MMU is disabled. If any of that is dirty it may be naturally evicted at any time, masking/corrupting data written with the MMU off. Any implicit memory accesses generated by the compiler may go wrong. A similar problem applies to anything previously mapped with cacheable attributes. > + > Status = FillTranslationTable (TranslationTable, &MemoryRegion); > if (RETURN_ERROR (Status)) { > return Status; > @@ -468,6 +484,12 @@ SetMemoryAttributes ( > > // Invalidate all TLB entries so changes are synced > ArmInvalidateTlb (); > + if (SavedMmuState) { > + ArmEnableMmu(); > + if (SavedInterruptState) { > + ArmEnableInterrupts (); > + } > + } Likewise, now everything written with the MMU off is not guaranteed to be visible to subsequent cacheable accesses, due to stale lines allocated before the MMU was disabled. That includes the modifications to the page tables, which may become eventually become visible to cacheable accesses in an unpredictable fashion (e.g. you might still end up with the TLBs filling with conflicting entries). > return RETURN_SUCCESS; > } > @@ -483,9 +505,20 @@ SetMemoryRegionAttribute ( > { > RETURN_STATUS Status; > UINT64 *RootTable; > + UINTN SavedInterruptState; > + BOOLEAN SavedMmuState; > > RootTable = ArmGetTTBR0BaseAddress (); > > + SavedMmuState = ArmMmuEnabled (); > + if (SavedMmuState) { > + SavedInterruptState = ArmGetInterruptState (); > + if (SavedInterruptState) { > + ArmDisableInterrupts(); > + } > + ArmDisableMmu(); > + } > + > Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, BlockEntryMask); > if (RETURN_ERROR (Status)) { > return Status; > @@ -493,6 +526,12 @@ SetMemoryRegionAttribute ( > > // Invalidate all TLB entries so changes are synced > ArmInvalidateTlb (); > + if (SavedMmuState) { > + ArmEnableMmu(); > + if (SavedInterruptState) { > + ArmEnableInterrupts (); > + } > + } The same problems apply to both of these. Thanks, Mark. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11 April 2016 at 16:10, Mark Rutland <mark.rutland@arm.com> wrote: > Hi Ard, > > On Mon, Apr 11, 2016 at 03:57:15PM +0200, Ard Biesheuvel wrote: >> On ARM, manipulating live page tables is cumbersome since the architecture >> mandates the use of break-before-make, i.e., replacing a block entry with >> a table entry requires an intermediate step via an invalid entry, or TLB >> conflicts may occur. >> >> Since it is not generally feasible to decide in the page table manipulation >> routines whether such an invalid entry will result in those routines >> themselves to become unavailable, and since UEFI uses an identity mapping >> anyway, it is far simpler to just disable the MMU, perform the page table >> manipulations, flush the TLBs and re-enable the MMU. > > Perhaps I am missing something, but I suspect that this isn't so simple. I am afraid it was I who was missing something. Not having a good day ... :-) > More on that below. > >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39 ++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c >> index b7d23c6f3286..2f8f05df8aec 100644 >> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c >> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c >> @@ -286,6 +286,11 @@ GetBlockEntryListFromAddress ( >> } >> } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) { >> // If we are not at the last level then we need to split this BlockEntry >> + // Since splitting live block entries may cause TLB conflicts, we need to >> + // enter this function with the MMU disabled and flush the TLBs before >> + // re-enabling it. This is the responsibility of the caller. >> + ASSERT (!ArmMmuEnabled ()); >> + >> if (IndexLevel != PageLevel) { >> // Retrieve the attributes from the block entry >> Attributes = *BlockEntry & TT_ATTRIBUTES_MASK; >> @@ -453,6 +458,8 @@ SetMemoryAttributes ( >> RETURN_STATUS Status; >> ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion; >> UINT64 *TranslationTable; >> + UINTN SavedInterruptState; >> + BOOLEAN SavedMmuState; >> >> MemoryRegion.PhysicalBase = BaseAddress; >> MemoryRegion.VirtualBase = BaseAddress; >> @@ -461,6 +468,15 @@ SetMemoryAttributes ( >> >> TranslationTable = ArmGetTTBR0BaseAddress (); >> >> + SavedMmuState = ArmMmuEnabled (); >> + if (SavedMmuState) { >> + SavedInterruptState = ArmGetInterruptState (); >> + if (SavedInterruptState) { >> + ArmDisableInterrupts(); >> + } >> + ArmDisableMmu(); >> + } > > Unless ArmDisableMmu() performs a Clean+Invalidate of all memory in use > by EDK2 (after the MMU is disabled), this is not safe. > > For example, the latest version of your stack (which may be dirty in > cache) is not guaranteed to be visible after the MMU is disabled. If any > of that is dirty it may be naturally evicted at any time, > masking/corrupting data written with the MMU off. Any implicit memory > accesses generated by the compiler may go wrong. > > A similar problem applies to anything previously mapped with cacheable > attributes. > Indeed. I can't believe I didn't spot that. I did have the clarity of mind to put you on cc though :-) >> + >> Status = FillTranslationTable (TranslationTable, &MemoryRegion); >> if (RETURN_ERROR (Status)) { >> return Status; >> @@ -468,6 +484,12 @@ SetMemoryAttributes ( >> >> // Invalidate all TLB entries so changes are synced >> ArmInvalidateTlb (); >> + if (SavedMmuState) { >> + ArmEnableMmu(); >> + if (SavedInterruptState) { >> + ArmEnableInterrupts (); >> + } >> + } > > Likewise, now everything written with the MMU off is not guaranteed to > be visible to subsequent cacheable accesses, due to stale lines > allocated before the MMU was disabled. That includes the modifications > to the page tables, which may become eventually become visible to > cacheable accesses in an unpredictable fashion (e.g. you might still end > up with the TLBs filling with conflicting entries). > >> return RETURN_SUCCESS; >> } >> @@ -483,9 +505,20 @@ SetMemoryRegionAttribute ( >> { >> RETURN_STATUS Status; >> UINT64 *RootTable; >> + UINTN SavedInterruptState; >> + BOOLEAN SavedMmuState; >> >> RootTable = ArmGetTTBR0BaseAddress (); >> >> + SavedMmuState = ArmMmuEnabled (); >> + if (SavedMmuState) { >> + SavedInterruptState = ArmGetInterruptState (); >> + if (SavedInterruptState) { >> + ArmDisableInterrupts(); >> + } >> + ArmDisableMmu(); >> + } >> + >> Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, BlockEntryMask); >> if (RETURN_ERROR (Status)) { >> return Status; >> @@ -493,6 +526,12 @@ SetMemoryRegionAttribute ( >> >> // Invalidate all TLB entries so changes are synced >> ArmInvalidateTlb (); >> + if (SavedMmuState) { >> + ArmEnableMmu(); >> + if (SavedInterruptState) { >> + ArmEnableInterrupts (); >> + } >> + } > > The same problems apply to both of these. > OK. So the bottom line is that keeping track of which memory to clean/invalidate is as intractable as deciding whether break-before-making a certain block entry is going to make some of your code unavailable. Since the code does seem to do the right thing with respect to populating the table entries before overwriting the block entry, there is a single point in the splitting code where I could insert a call to some function that performs the break before make without use of the stack or any data other than register contents. Thanks for the quick feedback _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c index b7d23c6f3286..2f8f05df8aec 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c @@ -286,6 +286,11 @@ GetBlockEntryListFromAddress ( } } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) { // If we are not at the last level then we need to split this BlockEntry + // Since splitting live block entries may cause TLB conflicts, we need to + // enter this function with the MMU disabled and flush the TLBs before + // re-enabling it. This is the responsibility of the caller. + ASSERT (!ArmMmuEnabled ()); + if (IndexLevel != PageLevel) { // Retrieve the attributes from the block entry Attributes = *BlockEntry & TT_ATTRIBUTES_MASK; @@ -453,6 +458,8 @@ SetMemoryAttributes ( RETURN_STATUS Status; ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion; UINT64 *TranslationTable; + UINTN SavedInterruptState; + BOOLEAN SavedMmuState; MemoryRegion.PhysicalBase = BaseAddress; MemoryRegion.VirtualBase = BaseAddress; @@ -461,6 +468,15 @@ SetMemoryAttributes ( TranslationTable = ArmGetTTBR0BaseAddress (); + SavedMmuState = ArmMmuEnabled (); + if (SavedMmuState) { + SavedInterruptState = ArmGetInterruptState (); + if (SavedInterruptState) { + ArmDisableInterrupts(); + } + ArmDisableMmu(); + } + Status = FillTranslationTable (TranslationTable, &MemoryRegion); if (RETURN_ERROR (Status)) { return Status; @@ -468,6 +484,12 @@ SetMemoryAttributes ( // Invalidate all TLB entries so changes are synced ArmInvalidateTlb (); + if (SavedMmuState) { + ArmEnableMmu(); + if (SavedInterruptState) { + ArmEnableInterrupts (); + } + } return RETURN_SUCCESS; } @@ -483,9 +505,20 @@ SetMemoryRegionAttribute ( { RETURN_STATUS Status; UINT64 *RootTable; + UINTN SavedInterruptState; + BOOLEAN SavedMmuState; RootTable = ArmGetTTBR0BaseAddress (); + SavedMmuState = ArmMmuEnabled (); + if (SavedMmuState) { + SavedInterruptState = ArmGetInterruptState (); + if (SavedInterruptState) { + ArmDisableInterrupts(); + } + ArmDisableMmu(); + } + Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, BlockEntryMask); if (RETURN_ERROR (Status)) { return Status; @@ -493,6 +526,12 @@ SetMemoryRegionAttribute ( // Invalidate all TLB entries so changes are synced ArmInvalidateTlb (); + if (SavedMmuState) { + ArmEnableMmu(); + if (SavedInterruptState) { + ArmEnableInterrupts (); + } + } return RETURN_SUCCESS; }
On ARM, manipulating live page tables is cumbersome since the architecture mandates the use of break-before-make, i.e., replacing a block entry with a table entry requires an intermediate step via an invalid entry, or TLB conflicts may occur. Since it is not generally feasible to decide in the page table manipulation routines whether such an invalid entry will result in those routines themselves to become unavailable, and since UEFI uses an identity mapping anyway, it is far simpler to just disable the MMU, perform the page table manipulations, flush the TLBs and re-enable the MMU. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39 ++++++++++++++++++++ 1 file changed, 39 insertions(+) -- 2.5.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel