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.web10.13779.1583154615406525088 for ; Mon, 02 Mar 2020 05:10:15 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=L9JnWJMZ; spf=pass (domain: nuviainc.com, ip: 209.85.221.65, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f65.google.com with SMTP id y17so12527023wrn.6 for ; Mon, 02 Mar 2020 05:10:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=BAsV2iQ2UzVmI1zqH1nskbW/hvZIx8+dU4p9VOfwNP8=; b=L9JnWJMZRBiIN1UltWHCr62KtdIrudtKuY3QlD6oTFU6O5JnlvC1r+lb+ONlCDeGtj PPBGw6pX6NMTn7D8qpDHar/WsTg45TH0pP5XSBd4R5fhvP8kORnXnlNrCS7hHy2If7vq SC+VQCMqdZituVzQsQvNGpoJXrpa6u0xXEVwokQwFRLIwnAvpMs9AfFFM1bdAJje3PeO WlDnsX3f+byw1YcGbIgY6Ub+yWInnJ3aO0NF+9SJYm6Q62W2WQsgeNho6aZO8FLK8uiU BHJNw0itTx/zo+x5C1w7SF9jctFnwb4B4pSUzUKQtMD+dCFP7Swch1AbYTXc3Sdp20vT hKyA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=BAsV2iQ2UzVmI1zqH1nskbW/hvZIx8+dU4p9VOfwNP8=; b=g7kQ3dEM0D9GqizsYEaMl2VmYhHP/Lbxs6ep6LYUfbEawMYQXyNw9xLQEYloEGFZas RDbkrqecvaK9JOJHPckDL/kDziYr90/SfHBNzTKxkUTIYdf7HKoiE6WfA+QyCDu2hQ9T 8woE7NAfKxJhkaY+P2+tgPqmWXvCT4qNXNwhHpBNLqwl2saaiIkHWQmKiu7kqi3UfIUE +K1Cb3lkaa+R1GmybrwADwuvN80nIXo8/MAKVJbUxJ+4b6eSmujo88aW/PhDTFMurOiX 1CT1rS+j9d9M2i0FbP0fE4gftXi1MLpfMC8vxrEzf/PCAKDYCvXuiLGFO/3Rytgl3Kk/ hvQw== X-Gm-Message-State: ANhLgQ3n5voTapTN/Q727BNTqj6p4C5O5b+czEwNyrzRVfts6tqsi/zw p1LbZvQ3ixZdYW4WgtBiF5phSg== X-Google-Smtp-Source: ADFU+vuJTAswbdJIQ4gb+zmJEvL0URtBmcpul6US+zBKZCuCxElKD0WjJB5mp49o27S5FCN9wY8mEg== X-Received: by 2002:a05:6000:189:: with SMTP id p9mr12498505wrx.391.1583154613826; Mon, 02 Mar 2020 05:10:13 -0800 (PST) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id m22sm15909248wmc.41.2020.03.02.05.10.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 02 Mar 2020 05:10:13 -0800 (PST) Date: Mon, 2 Mar 2020 13:10:11 +0000 From: "Leif Lindholm" To: Ard Biesheuvel Cc: edk2-devel-groups-io , Laszlo Ersek , Sami Mujawar Subject: Re: [edk2-devel] [PATCH 3/6] ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries Message-ID: <20200302131011.GI23627@bivouac.eciton.net> References: <20200226100353.31962-1-ard.biesheuvel@linaro.org> <20200226100353.31962-4-ard.biesheuvel@linaro.org> <20200302122522.GF23627@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. > > 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 > > > > > > > > > > > >