On Mon, Mar 02, 2020 at 13:58:39 +0100, Ard Biesheuvel wrote:
> On Mon, 2 Mar 2020 at 13:25, Leif Lindholm <l...@nuviainc.com> wrote:
> >
> > 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?
>
> The barrier is there to ensure that the write made it to meain memory,
> so we could actually relax this to a DMB.
If there's no risk there could be a stale entry for that line (i.e.,
D-cache has not been enabled since reset). Otherwise, I *think* there
could be a potential race condition in v7.
> > 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?
>
> The MMU enable will issue a DSB to ensure that all the cache
> invalidations have completed.
And that happens on our return path from here?
If so, fine.
> > > 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.
> >
>
> Same answer :-)
Efficient!
/
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 (#55179): https://edk2.groups.io/g/devel/message/55179
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]
-=-=-=-=-=-=-=-=-=-=-=-
