On Wed, Feb 26, 2020 at 11:03:50 +0100, Ard Biesheuvel wrote: > In the ARM version of ArmMmuLib, we are currently relying on set/way > invalidation to ensure that the caches are in a consistent state with > respect to main memory once we turn the MMU on. Even if set/way > operations were the appropriate method to achieve this, doing an > invalidate-all first and then populating the page table entries creates > a window where page table entries could be loaded speculatively into > the caches before we modify them, and shadow the new values that we > write there. > > So let's get rid of the blanket clean/invalidate operations, and > instead, update ArmUpdateTranslationTableEntry () to invalidate each > page table entry *after* it is written if the MMU is still disabled > at this point. > > On ARMv7, cache maintenance may be required also when the MMU is > enabled, in case the page table walker is not cache coherent. However, > the code being updated here is guaranteed to run only when the MMU is > still off, and so we can disregard the case when the MMU and caches > are on. > > Since the MMU and D-cache are already off when we reach this point, we > can drop the MMU and D-cache disables as well. Maintenance of the I-cache > is unnecessary, since we are not modifying any code, and the installed > mapping is guaranteed to be 1:1. This means we can also leave it enabled > while the page table population code is running. > > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> > --- > ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 25 +++++++++----------- > 1 file changed, 11 insertions(+), 14 deletions(-) > > diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c > b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c > index aca7a37facac..c5906b4310cc 100644 > --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c > +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c > @@ -183,6 +183,8 @@ PopulateLevel2PageTable ( > PhysicalBase += TT_DESCRIPTOR_PAGE_SIZE; > } > > + InvalidateDataCacheRange ((UINT32 *)TranslationTable + FirstPageOffset, > + RemainLength / TT_DESCRIPTOR_PAGE_SIZE * sizeof (*PageEntry)); > } > > STATIC > @@ -257,7 +259,11 @@ FillTranslationTable ( > RemainLength >= TT_DESCRIPTOR_SECTION_SIZE) { > // Case: Physical address aligned on the Section Size (1MB) && the > length > // is greater than the Section Size > - *SectionEntry++ = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | > Attributes; > + *SectionEntry = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | > Attributes; > + > + ArmDataSynchronizationBarrier (); > + ArmInvalidateDataCacheEntryByMVA ((UINTN)SectionEntry++); > +
Since the sequence is somewhat conterintuitive, could we add a comment to the extent that // Force subsequent acces to fetch from main memory? Obnoxious question: do we need another DSB here? Or are we reasonably guaranteed that one will appear in the instruction stream between here and anything else that would touch the same line? > PhysicalBase += TT_DESCRIPTOR_SECTION_SIZE; > RemainLength -= TT_DESCRIPTOR_SECTION_SIZE; > } else { > @@ -267,9 +273,12 @@ FillTranslationTable ( > // Case: Physical address aligned on the Section Size (1MB) && the > length > // does not fill a section > // Case: Physical address NOT aligned on the Section Size (1MB) > - PopulateLevel2PageTable (SectionEntry++, PhysicalBase, PageMapLength, > + PopulateLevel2PageTable (SectionEntry, PhysicalBase, PageMapLength, > MemoryRegion->Attributes); > > + ArmDataSynchronizationBarrier (); > + ArmInvalidateDataCacheEntryByMVA ((UINTN)SectionEntry++); > + Same pattern, so same questions. / Leif > // If it is the last entry > if (RemainLength < TT_DESCRIPTOR_SECTION_SIZE) { > break; > @@ -349,18 +358,6 @@ ArmConfigureMmu ( > } > } > > - ArmCleanInvalidateDataCache (); > - ArmInvalidateInstructionCache (); > - > - ArmDisableDataCache (); > - ArmDisableInstructionCache(); > - // TLBs are also invalidated when calling ArmDisableMmu() > - ArmDisableMmu (); > - > - // Make sure nothing sneaked into the cache > - ArmCleanInvalidateDataCache (); > - ArmInvalidateInstructionCache (); > - > ArmSetTTBR0 ((VOID *)(UINTN)(((UINTN)TranslationTable & > ~TRANSLATION_TABLE_SECTION_ALIGNMENT_MASK) | (TTBRAttributes & 0x7F))); > > // > -- > 2.17.1 > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55168): https://edk2.groups.io/g/devel/message/55168 Mute This Topic: https://groups.io/mt/71562847/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-