From: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif@nuviainc.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
Laszlo Ersek <lersek@redhat.com>,
Sami Mujawar <sami.mujawar@arm.com>
Subject: Re: [edk2-devel] [PATCH 3/6] ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries
Date: Mon, 2 Mar 2020 13:58:39 +0100 [thread overview]
Message-ID: <CAKv+Gu8LBr6rVLb=ssazXFKY_fEJgo9Sn_4NRiTiENWg_sMM2A@mail.gmail.com> (raw)
In-Reply-To: <20200302122522.GF23627@bivouac.eciton.net>
On Mon, 2 Mar 2020 at 13:25, Leif Lindholm <leif@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.biesheuvel@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.
> 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.
> > 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 :-)
> > // 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
> >
> >
> >
> >
next prev parent reply other threads:[~2020-03-02 12:58 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-26 10:03 [PATCH 0/6] ArmPkg: eradicate and deprecate by set/way cache ops Ard Biesheuvel
2020-02-26 10:03 ` [PATCH 1/6] ArmPkg/ArmMmuLib ARM: remove dummy constructor Ard Biesheuvel
2020-02-26 10:03 ` [PATCH 2/6] ArmPkg/ArmMmuLib ARM: split ArmMmuLibCore.c into core and update code Ard Biesheuvel
2020-02-26 10:03 ` [PATCH 3/6] ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries Ard Biesheuvel
2020-02-26 10:37 ` Ard Biesheuvel
2020-03-02 12:25 ` [edk2-devel] " Leif Lindholm
2020-03-02 12:58 ` Ard Biesheuvel [this message]
2020-03-02 13:10 ` Leif Lindholm
2020-03-02 13:15 ` Ard Biesheuvel
2020-03-04 12:10 ` Leif Lindholm
2020-02-26 10:03 ` [PATCH 4/6] ArmPkg/ArmMmuLib AARCH64: " Ard Biesheuvel
2020-02-26 10:03 ` [PATCH 5/6] ArmPkg/ArmLib: move set/way helper functions into private header Ard Biesheuvel
2020-02-26 10:03 ` [PATCH 6/6] ArmPkg/ArmLib: deprecate set/way cache maintenance routines Ard Biesheuvel
2020-03-02 13:13 ` Leif Lindholm
2020-03-02 13:16 ` Ard Biesheuvel
2020-03-04 12:04 ` Ard Biesheuvel
2020-02-26 10:29 ` [PATCH 0/6] ArmPkg: eradicate and deprecate by set/way cache ops Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKv+Gu8LBr6rVLb=ssazXFKY_fEJgo9Sn_4NRiTiENWg_sMM2A@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox