public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Leif Lindholm <leif@nuviainc.com>,
	Laszlo Ersek <lersek@redhat.com>,
	 Sami Mujawar <sami.mujawar@arm.com>
Subject: Re: [PATCH 3/6] ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries
Date: Wed, 26 Feb 2020 11:37:05 +0100	[thread overview]
Message-ID: <CAKv+Gu84zCi=-qyrkMCvUuu5v=r7e7E6eSDDHHSGqvvOey2GNA@mail.gmail.com> (raw)
In-Reply-To: <20200226100353.31962-4-ard.biesheuvel@linaro.org>

On Wed, 26 Feb 2020 at 11:04, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.
>

Uhm, apologies. This paragraph was copy-pasted from the AARCH64
version (along with the preceding one), but it doesn't apply here.
Instead, it should read,

"""
So let's get rid of the blanket clean/invalidate operations, and
instead, invalidate each section entry right after it is updated, and
invalidate sets of level 2 entries in blocks, using the generic
invalidation routine from CacheMaintenanceLib.
"""

> 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++);
> +
>        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++);
> +
>        // 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
>

  reply	other threads:[~2020-02-26 10:37 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 [this message]
2020-03-02 12:25   ` [edk2-devel] " Leif Lindholm
2020-03-02 12:58     ` Ard Biesheuvel
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+Gu84zCi=-qyrkMCvUuu5v=r7e7E6eSDDHHSGqvvOey2GNA@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