From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22e.google.com (mail-io0-x22e.google.com [IPv6:2607:f8b0:4001:c06::22e]) (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 7F57D802A1 for ; Mon, 6 Mar 2017 07:06:08 -0800 (PST) Received: by mail-io0-x22e.google.com with SMTP id 90so114853861ios.1 for ; Mon, 06 Mar 2017 07:06:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=iNPKX0yBiAju9dsJ7MoTqy5XVxwbnYTwtapxxdvLz54=; b=aMMM5XjtrWaWKFTtptguY8yLXdVVTkocrtMSTtrYNAwiNBbmIv8aeGd0xdKEgRbliV qhJb3c0gLJD1GVGvfOsqfe4zwBs3YFUu53qE/g0JGpI3kcs9CSP7VN7JlsxbaDS2QFO5 hnW2M0r8r35eiYCqW8BzuDIIWePS5+cs7RWz0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=iNPKX0yBiAju9dsJ7MoTqy5XVxwbnYTwtapxxdvLz54=; b=cvD4FDBCMVv5WzJQ8+3aQBhdnf2k3DXJlcsS/auY8meIUbbS1lk2q/j8WHfCZLel3+ DX0DCtQIIVfbfz1KNS1+xUzzHY8v1iFpPo/99UQhGMy5ai9wvMaC6yCHYDjS6FMFPSyY GAud2j18qMDUcb4VgaPZT37sgNbHCf4ZoDkblHEMJxgff9ftOiLEvL36EUWF/is+IDmz +d596+74pii/ie1Nys5fLHIsinN5howrRN+EQE71sMlwVhKIzUBLrLOdaRn1gTENiZ8a 3qWYYW0KDDlqb1EI5iZazfTo5r+/sn1BPAwLCcY8lgTputZtsDFiUOvfV/j+U28rZhdm Hb+Q== X-Gm-Message-State: AMke39kuoWvE5wWgSAcDzSZRYhh+3w5rF1ttB4vtYIYERf1nIb8/b6Gyjnzqxz/sgDMg/6OfsKWvKQ1unIYIdtA4 X-Received: by 10.107.132.155 with SMTP id o27mr14117440ioi.138.1488812767752; Mon, 06 Mar 2017 07:06:07 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Mon, 6 Mar 2017 07:06:07 -0800 (PST) In-Reply-To: <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> <20170306142759.GU16034@bivouac.eciton.net> From: Ard Biesheuvel Date: Mon, 6 Mar 2017 16:06:07 +0100 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Laszlo Ersek 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 15:06:08 -0000 Content-Type: text/plain; charset=UTF-8 On 6 March 2017 at 15:27, Leif Lindholm wrote: > 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? Yes > If you fold that in: > Reviewed-by: Leif Lindholm > Thanks >> + *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 >>