From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::142; helo=mail-it1-x142.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x142.google.com (mail-it1-x142.google.com [IPv6:2607:f8b0:4864:20::142]) (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 2AD4C20971733 for ; Mon, 28 Jan 2019 04:30:05 -0800 (PST) Received: by mail-it1-x142.google.com with SMTP id p197so19095544itp.0 for ; Mon, 28 Jan 2019 04:30:05 -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=HM2BiQeeNwynoGbM3wLBLcCjGl160jc/NtmRr7mW3Zg=; b=BO8E5jJmXiu/UJtE6ZHKLUpzkl5B6AthfrkabmcmwCVosiOKwV3oifjQtyCwCHr0vV I+sntem734WVrlaJ78yZPh4pzf+utpRrCWHOJzYdAuN/ANp/zlVZozJPmsl57R9xhMwa o90elX10SLTx4y7Cfolo0/CZy02/PxdVEmYsQ= 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=HM2BiQeeNwynoGbM3wLBLcCjGl160jc/NtmRr7mW3Zg=; b=i9bhdm64txyEsOAfF5DdmNNIrvqB9Ew9lPnnOcHG64b1lMjUKMqYGkHRZbNXNSMOBT lUNxtQ5PM1pHAOY6md2XQYr4I7ntHizyhq2y322AklrhiZjipZVvgQRm4NF+c/YX0fcc bxwNa5TOdp66xvZPRpFksrra81ToFe7hF+KyXc86SNpGZ6x1vJetDmc5OK9/jFvbKfwQ ZwiZQ/LIlPQjOmwo3noZERywfschfme3Q9MBDoFc7rjRkWoYMY+UWTrGvi+uHLrPDyL1 Alwa4BZtPwD3gZTBC+KC7wh+3wFEwDhNaSIdDLXMaWwemYA3RrJv/PvIvc1UMqAwmz/W 7hpw== X-Gm-Message-State: AJcUukcg9ypPyOgLYnec/lmSU9io+6S00bZ8XzGSMOusjPNwL4awg9nH bTG3EMA4P29YY+IsAyPCkFwqBC3EFahUXTOHRHbI4g== X-Google-Smtp-Source: ALg8bN6ZBOKokew+khwbzS3nRVc6tR1sDBFesM8s43hIvsT1gDjSZTrLmU7M00STrwWFne/ry9Jlt91VfrYIljnId/M= X-Received: by 2002:a24:edc4:: with SMTP id r187mr11192136ith.158.1548678605155; Mon, 28 Jan 2019 04:30:05 -0800 (PST) MIME-Version: 1.0 References: <20190107071504.2431-1-ard.biesheuvel@linaro.org> <20190107071504.2431-3-ard.biesheuvel@linaro.org> <20190123154622.pibw6mi5gh6ywb26@bivouac.eciton.net> <20190123161257.nydm3s5c27oibn6e@bivouac.eciton.net> <20190123162026.i3id7zabzmo52sin@bivouac.eciton.net> In-Reply-To: <20190123162026.i3id7zabzmo52sin@bivouac.eciton.net> From: Ard Biesheuvel Date: Mon, 28 Jan 2019 13:29:54 +0100 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH 2/5] ArmPkg/ArmMmuLib AARCH64: get rid of needless TLB invalidation X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jan 2019 12:30:06 -0000 Content-Type: text/plain; charset="UTF-8" On Wed, 23 Jan 2019 at 17:20, Leif Lindholm wrote: > > On Wed, Jan 23, 2019 at 05:16:57PM +0100, Ard Biesheuvel wrote: > > On Wed, 23 Jan 2019 at 17:13, Leif Lindholm wrote: > > > > > > On Wed, Jan 23, 2019 at 04:55:28PM +0100, Ard Biesheuvel wrote: > > > > On Wed, 23 Jan 2019 at 16:46, Leif Lindholm wrote: > > > > > > > > > > On Mon, Jan 07, 2019 at 08:15:01AM +0100, Ard Biesheuvel wrote: > > > > > > Currently, we always invalidate the TLBs entirely after making > > > > > > any modification to the page tables. Now that we have introduced > > > > > > strict memory permissions in quite a number of places, such > > > > > > modifications occur much more often, and it is better for performance > > > > > > to flush only those TLB entries that are actually affected by > > > > > > the changes. > > > > > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > > Signed-off-by: Ard Biesheuvel > > > > > > --- > > > > > > ArmPkg/Include/Library/ArmMmuLib.h | 3 ++- > > > > > > ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 6 +++--- > > > > > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 16 +++++++--------- > > > > > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 14 ++++++++------ > > > > > > 4 files changed, 20 insertions(+), 19 deletions(-) > > > > > > > > > > > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > > > > > index d66df3e17a02..e1fabfcbea14 100644 > > > > > > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > > > > > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > > > > > @@ -129,13 +129,14 @@ STATIC > > > > > > VOID > > > > > > ReplaceLiveEntry ( > > > > > > IN UINT64 *Entry, > > > > > > - IN UINT64 Value > > > > > > + IN UINT64 Value, > > > > > > + IN UINT64 Address > > > > > > ) > > > > > > { > > > > > > if (!ArmMmuEnabled ()) { > > > > > > *Entry = Value; > > > > > > } else { > > > > > > - ArmReplaceLiveTranslationEntry (Entry, Value); > > > > > > + ArmReplaceLiveTranslationEntry (Entry, Value, Address); > > > > > > } > > > > > > } > > > > > > > > > > > > @@ -296,7 +297,8 @@ GetBlockEntryListFromAddress ( > > > > > > > > > > > > // Fill the BlockEntry with the new TranslationTable > > > > > > ReplaceLiveEntry (BlockEntry, > > > > > > - ((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY); > > > > > > + (UINTN)TranslationTable | TableAttributes | TT_TYPE_TABLE_ENTRY, > > > > > > + RegionStart); > > > > > > > > > > > > > /me pages in the data ... > > > > > > > > > OK, this whole patch took a few times around the loop before I think I > > > > > caught on what was happening. > > > > > > > > > > I think I'm down to the only things confusing me being: > > > > > - The name "Address" to refer to something that is always the start > > > > > address of a 4KB-aligned translation region. > > > > > Is this because the function will be usable in other contexts in > > > > > later patches? > > > > > > > > I could change it to VirtualAddress if you prefer. > > > > It is only passed > > > > for TLB maintenance which is only needed at page granularity, and the > > > > low bits are shifted out anyway. > > > > > > Yeah, exactly. It would just be nice if the name reflected that. Not > > > sure VirtualAddress does. VirtualBase? PageBase? > > > > > > > Well, the argument passed in is called RegionStart, so let's just > > stick with that. > > Sure. With that: > Reviewed-by: Leif Lindholm > Thanks GIven the discussion in the other thread regarding shareability upgrades of barriers and TLB maintenance instructions when running under virt, mind if I fold in the hunk below? (and add a mention to the commit log) --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S @@ -35,7 +35,7 @@ // flush translations for the target address from the TLBs lsr x2, x2, #12 tlbi vae\el, x2 - dsb sy + dsb nsh // re-enable the MMU msr sctlr_el\el, x8 @@ -58,7 +58,7 @@ ASM_FUNC(ArmReplaceLiveTranslationEntry) // clean and invalidate first so that we don't clobber // adjacent entries that are dirty in the caches dc civac, x0 - dsb ish + dsb nsh EL1_OR_EL2_OR_EL3(x3) 1:__replace_entry 1 The first one reduces the scope of the tlb maintenance of a live entry to the current CPU (and when running under virt, the hypervisor will upgrade this to inner shareable) The second one prevents the cache maintenance from being broadcast unnecessarily, and will be upgraded back to ish in the same way when running under virt.