On 14 April 2016 at 13:17, Mark Rutland <mark.rutl...@arm.com> wrote:
> On Tue, Apr 12, 2016 at 08:57:46AM +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, use a function that is callable with
>> the MMU off (i.e., a leaf function that does not access the stack) to
>> perform the change of a block entry into a table entry.
>>
>> Note that the opposite should never occur, i.e., table entries are never
>> coalesced into block entries.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> ---
>> ArmPkg/Include/Library/ArmLib.h | 6 +++
>> ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 5 +-
>> ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c | 36 +++++++++++++
>> ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 17 +++++-
>> ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 57
>> ++++++++++++++++++++
>> 5 files changed, 119 insertions(+), 2 deletions(-)
>
>
>> +RETURN_STATUS
>> +EFIAPI
>> +AArch64LibConstructor (
>> + VOID
>> + )
>> +{
>> + extern UINT32 ArmReplaceLiveTranslationEntrySize;
>> +
>> + //
>> + // The ArmReplaceLiveTranslationEntry () helper function may be invoked
>> + // with the MMU off so we have to ensure that it gets cleaned to the PoC
>> + //
>> + WriteBackDataCacheRange (ArmReplaceLiveTranslationEntry,
>> + ArmReplaceLiveTranslationEntrySize);
>> +
>> + return RETURN_SUCCESS;
>> +}
>
> I take it that ArmReplaceLiveTranslationEntry isn't part of the static
> image (e.g. it gets relocated or copied after the MMU is enabled for the
> first time)?
>
> Otherwise I'm not following why the code wouldn't be clean to the PoC.
>
Yes, the various DXE modules are loaded from a compressed firmware
volume by a PE/COFF loader which may clean to the PoU only.
>> + // write an entry and invalidate it in the caches
>> + .macro __set_entry, reg
>> + str \reg, [x0]
>> + dmb sy
>> + dc ivac, x0
>> + dsb sy
>> + .endm
>> +
>> + .macro __replace_entry, el
>> + mrs x8, sctlr_el\el
>> + bic x9, x8, #CTRL_M_BIT // Clear MMU enable bit
>> + msr sctlr_el\el, x9
>> + isb
>> +
>> + __set_entry xzr // write invalid entry
>
> The MMU is off, and thus there are no TLB walks, so I don't see the
> point in storing+invalidating the invalid entry here.
>
Do you mean storing is sufficient? Or that we don't need bbm in the
first place when disabling the MMU? Because it seems to me that one
implies the other: if we can flush the TLBs without the risk of them
refetching the stale entries, we can just write the new value here and
be done with it.
> It might be better to hoist the DC CIVAC from
> ArmReplaceLiveTranslationEntry to here, to keep the maintenance
> together, and avoid the redundant invalidate.
>
> With the __set_entry macro folded in here, it would be a little clearer,
> too.
>
> Other than that, the general strategey seems sound to me.
>
Thanks.
>> + .if \el == 1
>> + tlbi vmalle1
>> + .else
>> + tlbi alle\el
>> + .endif
>> + dsb sy
>> + __set_entry x1 // write updated entry
>> + msr sctlr_el\el, x8
>> + isb
>> + .endm
>> +
>> +//VOID
>> +//ArmReplaceLiveTranslationEntry (
>> +// IN UINT64 *Entry,
>> +// IN UINT64 Value
>> +// )
>> +ASM_PFX(ArmReplaceLiveTranslationEntry):
>> +
>> + // disable interrupts
>> + mrs x2, daif
>> + msr daifset, #0xf
>> + isb
>> +
>> + // clean and invalidate first so that we don't clobber adjacent entries
>> + // that are dirty in the caches
>> + dc civac, x0
>> + dsb ish
>> +
>> + EL1_OR_EL2_OR_EL3(x3)
>> +1:__replace_entry 1
>> + b 4f
>> +2:__replace_entry 2
>> + b 4f
>> +3:__replace_entry 3
>> +4:msr daif, x2
>> + ret
>> +
>> +ASM_PFX(ArmReplaceLiveTranslationEntrySize):
>> + .long . - ArmReplaceLiveTranslationEntry
>> +
>> ASM_FUNCTION_REMOVE_IF_UNREFERENCED
>> --
>> 2.5.0
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
