From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web11.13748.1583154970308153507 for ; Mon, 02 Mar 2020 05:16:10 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=xde089fu; spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f68.google.com with SMTP id v2so12444684wrp.12 for ; Mon, 02 Mar 2020 05:16:10 -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=77EWreEQgwzrQQmWPNg3gqqiJX4MIROdAfz+xyXBRjQ=; b=xde089fu4SO9Hy/7pDi/SUTbPHXjK0+UWLz+TRjGSeki1JwKiqZnigcrb7MugTkC9V iwrfZ5GNQIj/9uPBy9/HXbPvPg7uQcHpA3EE4tCnId7FQlTLO55QySNbW5e2OIhoheQU lEkNbQJJlvlJ0azZ+aQWsdV9lHlwhE8rLjW9TgEH8uHjwCmEQ47S4WGB4sjf7TyHto6S odEEeJow0RKbndelx3gl3ymiDMrSHJp1g91Qp3OBNXKCU6+ouhvDykYoA69V4oZC8KMT GNY3bxdAbB9Wo7AVhPxwsSevwa+77DAJfSjsnAzAEHL1uHkgAtiwwKBEGtl3WPyhh0sA d9kg== 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=77EWreEQgwzrQQmWPNg3gqqiJX4MIROdAfz+xyXBRjQ=; b=pizuNfB5Kk3P2epa2ihfUD5iarmLaj2M+fcYaJPWPRyZbrMwG3+nZYvD4w/A6nlga6 CQloDRRm5LFqRxA2PbzwNs81d4hJvW7lpPf2afQ6dkX6ERrnPftbiB43hMPuO01YMGuS dS/6yyFbEM5wxKmXrHLARDteaitOJTJh2wafyzwIaiPCAPylJB1wvPGNZWV52VOqje0X xHyl+tHA6t57vQ6uqg5C0KV2aOgXCwBltN4KYdt3T/InEX7jFAaT5own6AEM0yUsXbxu ubkQHWO3dHkWXMsBMhz5vRaO+bAeqfgfqVECDJE3JM+JLkZd26jfRUWmhSmui01kWeLI mATQ== X-Gm-Message-State: APjAAAXh3BrjsetZmFUTvQRLTpRbZTe/Qg3mF1WSXcq1ljNfJDz2JfuX kd7jq2oKTZ38yQwqzCh0sduZ5Wk9+28Y3wJ2sWGq4A== X-Google-Smtp-Source: APXvYqxepgKdMBasyUXUrQuGv0/wK2izd1g1GNYIKolUJBBYRxfxh2QyLAV9GJWHWTkwk0vWv8uY6AcSDTw+CgwoUtY= X-Received: by 2002:a05:6000:110b:: with SMTP id z11mr22909325wrw.252.1583154968819; Mon, 02 Mar 2020 05:16:08 -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> <20200302131011.GI23627@bivouac.eciton.net> In-Reply-To: <20200302131011.GI23627@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Mon, 2 Mar 2020 14:15:58 +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 14:10, Leif Lindholm wrote: > > On Mon, Mar 02, 2020 at 13:58:39 +0100, Ard Biesheuvel wrote: > > 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. > > 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. > I don't follow. Getting rid of stale, clean cachelines is the whole point. But to ensure that a speculative fetch doesn't fetch the same stale value again, we need to order the invalidate wrt the store. > > > 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. > It happens later. But remember that all of this code runs with the MMU and caches off. We are just ensuring that our page table changes are not shadowed in the PTW's view by clean but stale cachelines when we turn on the MMU a bit later. > > > > 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 > > > > > > > > > > > > > > > >