From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web12.13443.1583153931417275825 for ; Mon, 02 Mar 2020 04:58:51 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=Qka/ASno; spf=pass (domain: linaro.org, ip: 209.85.221.65, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f65.google.com with SMTP id r7so12474389wro.2 for ; Mon, 02 Mar 2020 04:58:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9+ZiAI7JWqysZ4ZjG9gD3bAL2qvb81b9d0wWKiI3w50=; b=Qka/ASnoEhRSchSTnwvafuiFTAqPpumIyzna6wtiysbwDDSssOaeK1ksgh7EfPmFhv NKMgQmMR/JMuOLF8e/Q7t5BH+OaCq4NgEMmNZbTDL8xo5KZCY/UtM7SJ5CTUGwsqVSA6 dVoAcEXvOgNgtgpT2Db28yZCqu8F3AeDOBlHyyxpEgWo5XAOGOgWhJJC7ismgesuS9M6 OEluHbN+5ivGRYckt/Al6ScBF7wzT3nK87hw9YTH9fpdtM7c50l5wtNPQ2xZkaQBxeuX tqrM6OlDC583GYkq9XMtNgN+99nOLnnvH2mJ1S69lqnRhjB0E2F+0A0LiHozB+aXBgaR OrkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9+ZiAI7JWqysZ4ZjG9gD3bAL2qvb81b9d0wWKiI3w50=; b=G3r/0tuULwRwfy1Nff3LXhWxXsgVCnmFHzAYl1nFfdnCrg+Q367w7iWjTKxYcoGuYB 6fMkZjdiq0myRYM6X1VktmYUrUD+qcqJ7HBPMvrSb1EoweZfqFy+zuCNlHVq6+qY+iNe NQ8KNUaR3Qcc6wXysJGR9LC9632liWzo8EpP9HB04BmWV5CiDw+BLm6YceliZAg/vZab cxOwCsVmOLkJqWbCDQSbsBTaa3AFcIYQxIfwBXzQWOfUXFji9yGNZ+0XHY0R5sxLMmmx 3kHQjT3HaILHAX6pHqmszo9j82qQPv94VPYf+XUmgaD2n8OsBh0oBtJ40jIiIshqNqSb ceFA== X-Gm-Message-State: APjAAAVy3C9LlBf0Bv2dtBizqnTv3u3/bhiDMICF5GhszkujxYlL6m7B 83trE0Ahp6gUZq+tQ0D8tZARY9yD+pgFlGhgmbfmbZUpGn8= X-Google-Smtp-Source: APXvYqxt4D1n0fB4DudBIJm1Dxzbl+TZgNQpjLjmzHS38eoZT08/pgHIyz0QJUL+WqyVXLElPIeUc4lY87pRilUiIvc= X-Received: by 2002:adf:f84a:: with SMTP id d10mr22700301wrq.208.1583153929957; Mon, 02 Mar 2020 04:58:49 -0800 (PST) MIME-Version: 1.0 References: <20200226100353.31962-1-ard.biesheuvel@linaro.org> <20200226100353.31962-4-ard.biesheuvel@linaro.org> <20200302122522.GF23627@bivouac.eciton.net> In-Reply-To: <20200302122522.GF23627@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Mon, 2 Mar 2020 13:58:39 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH 3/6] ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries To: Leif Lindholm Cc: edk2-devel-groups-io , Laszlo Ersek , Sami Mujawar Content-Type: text/plain; charset="UTF-8" On Mon, 2 Mar 2020 at 13:25, Leif Lindholm 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 > > --- > > 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 > > > > > > > >