From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22f.google.com (mail-wm0-x22f.google.com [IPv6:2a00:1450:400c:c09::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A529280313 for ; Mon, 6 Mar 2017 06:28:03 -0800 (PST) Received: by mail-wm0-x22f.google.com with SMTP id n11so65361868wma.1 for ; Mon, 06 Mar 2017 06:28:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=xm+scwm89Xp0iyFT7ucLJfrhfLPKHd64OX+vATL3Y3Q=; b=Knt/z7quqHgvJZYUvZ28yjf8kMDoSUytr+CZbDeHRWrBSbRZvyDKg/HX/Y2UEknQy9 kKsPqjtYVuo1QAZl8RVwxd/9TzcJ6fbeyt+4UZydFZPSUdMPBFTp/54FCahSAFrk/weC WagZDYM9DfvGsb1nsTA4XvLxNQamFzV82bBYM= 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=xm+scwm89Xp0iyFT7ucLJfrhfLPKHd64OX+vATL3Y3Q=; b=sdt+nhg1MshEvTWG51Hf27ZSwHJEmuiw6mUsEcS9JWzaMI+SIQu04Dmww1KlPJ0z8b wHQkJn5NN22snDNG1pQPj38QHy4JBbyY29yUXTPz9XLU1GBn2EhwOzoF2bib9Ws0DzWW +nFt4zyCqp0iIdzd56R9kY6HDjfsO8bQNC3L/zM4qoSFIODFze/rBP87Bfqni22x2ayQ cUKozeIUGLpEkd319p5/3TxqxOPHF65GlUAIdsT8pjAgy7y1vmx3wwVRgybuIvCqujlE 2VxnzA1BfB85Dqv7JIra1pSNKFfhxfJtIDTKibGkmHaefC2Gap6GmiwTCvcCMw8+k4Dy z3cw== X-Gm-Message-State: AMke39kcVJgHYLCZjGKN7N/JUQjTHQtQ1DKP8d508xMy1UB+zOU4PHKTNOMscg8SdjKEI8dz X-Received: by 10.28.84.18 with SMTP id i18mr13500919wmb.130.1488810482107; Mon, 06 Mar 2017 06:28:02 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id t194sm15078801wmd.13.2017.03.06.06.28.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Mar 2017 06:28:01 -0800 (PST) Date: Mon, 6 Mar 2017 14:27:59 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, lersek@redhat.com Message-ID: <20170306142759.GU16034@bivouac.eciton.net> References: <1488450976-16257-1-git-send-email-ard.biesheuvel@linaro.org> <1488450976-16257-3-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <1488450976-16257-3-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Mar 2017 14:28:04 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Mar 02, 2017 at 10:36:14AM +0000, Ard Biesheuvel wrote: > Page and section entries in the page tables are updated using the > helper ArmUpdateTranslationTableEntry(), which cleans the page > table entry to the PoC, and invalidates the TLB entry covering > the page described by the entry being updated. > > Since we may be updating section entries, we might be leaving stale > TLB entries at this point (for all pages in the section except the > first one), which will be invalidated wholesale at the end of > SetMemoryAttributes(). At that point, all caches are cleaned *and* > invalidated as well. > > This cache maintenance is costly and unnecessary. The TLB maintenance > is only necessary if we updated any section entries, since any page > by page entries that have been updated will have been invalidated > individually by ArmUpdateTranslationTableEntry(). > > So drop the clean/invalidate of the caches, and only perform the > full TLB flush if UpdateSectionEntries() was called, or if sections > were split by UpdatePageEntries(). Finally, make the cache maintenance > on the remapped regions themselves conditional on whether any memory > type attributes were modified. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 60 +++++++++++--------- > 1 file changed, 34 insertions(+), 26 deletions(-) > > diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > index ce4d529bda67..26b637e7658f 100644 > --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > @@ -347,10 +347,11 @@ SyncCacheConfig ( > > EFI_STATUS > UpdatePageEntries ( > - IN EFI_PHYSICAL_ADDRESS BaseAddress, > - IN UINT64 Length, > - IN UINT64 Attributes, > - IN EFI_PHYSICAL_ADDRESS VirtualMask > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > + IN UINT64 Length, > + IN UINT64 Attributes, > + IN EFI_PHYSICAL_ADDRESS VirtualMask, > + OUT BOOLEAN *FlushTlbs > ) > { > EFI_STATUS Status; > @@ -446,6 +447,9 @@ UpdatePageEntries ( > > // Re-read descriptor > Descriptor = FirstLevelTable[FirstLevelIdx]; > + if (FlushTlbs != NULL) { If FlushTlbs is optional, should it not be marked OPTIONAL above? If you fold that in: Reviewed-by: Leif Lindholm > + *FlushTlbs = TRUE; > + } > } > > // Obtain page table base address > @@ -471,15 +475,16 @@ UpdatePageEntries ( > > if (CurrentPageTableEntry != PageTableEntry) { > Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT)); > - if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) { > - // The current section mapping is cacheable so Clean/Invalidate the MVA of the page > - // Note assumes switch(Attributes), not ARMv7 possibilities > - WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE); > - } > > // Only need to update if we are changing the entry > PageTable[PageTableIndex] = PageTableEntry; > ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva); > + > + // Clean/invalidate the cache for this page, but only > + // if we are modifying the memory type attributes > + if (((CurrentPageTableEntry ^ PageTableEntry) & TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK) != 0) { > + WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE); > + } > } > > Status = EFI_SUCCESS; > @@ -581,7 +586,12 @@ UpdateSectionEntries ( > // has this descriptor already been coverted to pages? > if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) { > // forward this 1MB range to page table function instead > - Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask); > + Status = UpdatePageEntries ( > + (FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, > + TT_DESCRIPTOR_SECTION_SIZE, > + Attributes, > + VirtualMask, > + NULL); > } else { > // still a section entry > > @@ -596,15 +606,16 @@ UpdateSectionEntries ( > > if (CurrentDescriptor != Descriptor) { > Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT); > - if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) { > - // The current section mapping is cacheable so Clean/Invalidate the MVA of the section > - // Note assumes switch(Attributes), not ARMv7 possabilities > - WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB); > - } > > // Only need to update if we are changing the descriptor > FirstLevelTable[FirstLevelIdx + i] = Descriptor; > ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva); > + > + // Clean/invalidate the cache for this section, but only > + // if we are modifying the memory type attributes > + if (((CurrentDescriptor ^ Descriptor) & TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK) != 0) { > + WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB); > + } > } > > Status = EFI_SUCCESS; > @@ -680,6 +691,7 @@ SetMemoryAttributes ( > { > EFI_STATUS Status; > UINT64 ChunkLength; > + BOOLEAN FlushTlbs; > > // > // Ignore invocations that only modify permission bits > @@ -688,6 +700,7 @@ SetMemoryAttributes ( > return EFI_SUCCESS; > } > > + FlushTlbs = FALSE; > while (Length > 0) { > if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) && > Length >= TT_DESCRIPTOR_SECTION_SIZE) { > @@ -700,6 +713,8 @@ SetMemoryAttributes ( > > Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes, > VirtualMask); > + > + FlushTlbs = TRUE; > } else { > > // > @@ -717,7 +732,7 @@ SetMemoryAttributes ( > BaseAddress, ChunkLength, Attributes)); > > Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes, > - VirtualMask); > + VirtualMask, &FlushTlbs); > } > > if (EFI_ERROR (Status)) { > @@ -728,16 +743,9 @@ SetMemoryAttributes ( > Length -= ChunkLength; > } > > - // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks > - // flush and invalidate pages > - //TODO: Do we really need to invalidate the caches everytime we change the memory attributes ? > - ArmCleanInvalidateDataCache (); > - > - ArmInvalidateInstructionCache (); > - > - // Invalidate all TLB entries so changes are synced > - ArmInvalidateTlb (); > - > + if (FlushTlbs) { > + ArmInvalidateTlb (); > + } > return Status; > } > > -- > 2.7.4 >