From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by mx.groups.io with SMTP id smtpd.web09.13158.1583151927949996330 for ; Mon, 02 Mar 2020 04:25:28 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=vuoDMB6B; spf=pass (domain: nuviainc.com, ip: 209.85.128.68, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f68.google.com with SMTP id e26so4884792wme.5 for ; Mon, 02 Mar 2020 04:25:27 -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=GZ5J2W8FLUKV4ZX6z1DA7S6meULuMkmM3NuS/Uohw1k=; b=vuoDMB6B4QvbHOApvuWy6Dshz+bV6CXcGivZ5I61WT7CK1XHClzGpKEUC5mNUsi2nX Ty5lkB/eotCBEkdA9CBwZw1aUlAqbYqfzHdrUH/lxyDz8MwRJHfDTInWF8+no+9ZJQII WA2+c9mgE9jErqq8z0kTVFJ+0F+oVwPdqro8nYdVCq6JbY0VpFgHk42OT0WpmPldLBd6 L2635bNQPpVLDSbDg3ZXmTkrou3z4T2muVx1MM3586l8VWxBEwSbypPp7PKgJVZTs4wc R4vFrGTIxVtjrc32cC1bSzNX8pj/T8NO3mdxqXffOfR4Cl8ucRuqYg27J1DXNSAqDmbp BrFA== 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=GZ5J2W8FLUKV4ZX6z1DA7S6meULuMkmM3NuS/Uohw1k=; b=c/ldx1NPIy8JvK8rU8tFqxHPb1NvnRImu+Lr5KscwDXvzCgfd7CrX0bCm8Jbq6Qa4p hJ1QZt8hBKVerzNh88w47ohKr89FWuDYIeDDNYZMzVa65Ako4jo71TiCITbhWCaAYJsY JgEpnY03LQDE9f8oqdg5rGNzI0J0lKpoykMNaJA5j8VxspRM89ZaPd1C4Om98Jxs9jSc oZTFg4XCxNWnPumK9uQSFg8wHIZSGNXn8myqhjkRDHh+nGhBjctH56o0aMpffLChfNkk 6+1GJ9VZoUU6A5BKJFRcin9YR9NFsp9qaVv8grJ5oaMCFX+F5KeuKsta9T0iu+Fi3urm rSNQ== X-Gm-Message-State: APjAAAUwxsXBdHE8ktIter+WQVs/nh4QB55vxiqBXk0ZOh2nTv7KV6qA LeIdpv1FzCIpE3SOxExSSqbqQrZ0h6jgLkIfuFY64Zop2oh0oeNTGS3wTL6D7JO6VxQQgbpTzfK QJDSpy+Xy1/ZM3IlvedwBkdY7hBbc6BzJ9wvTSpW75GOGBfIk/EJuFahxbQI5oEk= X-Google-Smtp-Source: APXvYqzYMjqLNtdA87OKMX2N1/oa0UXGC4AH0zDRkn6z+9MJkEHpqV+6DGBtDOZ+WtvRz1Sd1k9sOg== X-Received: by 2002:a1c:a502:: with SMTP id o2mr12182979wme.94.1583151926152; Mon, 02 Mar 2020 04:25:26 -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 o16sm12776451wrj.5.2020.03.02.04.25.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 02 Mar 2020 04:25:25 -0800 (PST) Date: Mon, 2 Mar 2020 12:25:22 +0000 From: "Leif Lindholm" To: devel@edk2.groups.io, ard.biesheuvel@linaro.org Cc: lersek@redhat.com, sami.mujawar@arm.com Subject: Re: [edk2-devel] [PATCH 3/6] ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries Message-ID: <20200302122522.GF23627@bivouac.eciton.net> References: <20200226100353.31962-1-ard.biesheuvel@linaro.org> <20200226100353.31962-4-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20200226100353.31962-4-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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? 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? > 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. / 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 > > > >