From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id C808FAC0C65 for ; Thu, 11 Jan 2024 08:49:32 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=UL33GwJk4c/mmgjrVDfmahX7QxX2dex1i1ptZQgUdyk=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1704962971; v=1; b=OPrHXCuMAcDFEGMQA9H/80TRmnn945793swNqYLba/kTLqOm2KPkQ/MvmeVQL5QwFZ+5HLih J62PGy0wHq7uPEvI2SWY+VxFaycdvha0VjBKAS8YiBV4xwW/gSB2aaze76a9u0YSZf1TruSpBMA tWszse4I2SmaMEXM3WHBm8TE= X-Received: by 127.0.0.2 with SMTP id OsInYY7687511xrorF138YoY; Thu, 11 Jan 2024 00:49:31 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.7466.1704962970847148397 for ; Thu, 11 Jan 2024 00:49:31 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-137-PyJ2lM2qPeGstYKu3x-rMw-1; Thu, 11 Jan 2024 03:49:26 -0500 X-MC-Unique: PyJ2lM2qPeGstYKu3x-rMw-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 787A0380407B; Thu, 11 Jan 2024 08:49:26 +0000 (UTC) X-Received: from [10.39.193.20] (unknown [10.39.193.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 930792026D66; Thu, 11 Jan 2024 08:49:25 +0000 (UTC) Message-ID: <470d9f50-1568-9fb5-3af3-4a19eb681701@redhat.com> Date: Thu, 11 Jan 2024 09:49:24 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent To: "Ni, Ray" , "Liu, Zhiguang" , "devel@edk2.groups.io" Cc: "Kumar, Rahul R" , Gerd Hoffmann References: <20240110054318.997-1-zhiguang.liu@intel.com> <8206c15e-8cc0-1969-e4dc-bd9c6b5df202@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: pa5Pjg72e3D5DZL0dbf2wljWx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=OPrHXCuM; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 1/11/24 03:08, Ni, Ray wrote: >=20 >=20 > Thanks, > Ray >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Wednesday, January 10, 2024 7:57 PM >> To: Liu, Zhiguang ; devel@edk2.groups.io >> Cc: Ni, Ray ; Kumar, Rahul R = ; >> Gerd Hoffmann >> Subject: Re: [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in >> ConvertMemoryPageToNotPresent >> >> On 1/10/24 06:43, Zhiguang Liu wrote: >>> After ConvertMemoryPageToNotPresent, there is always a flush TLB >>> function. So, to improve performance, there is no need to write CR3 >>> inside ConvertMemoryPageToNotPresent >>> >>> Cc: Ray Ni >>> Cc: Laszlo Ersek >>> Cc: Rahul Kumar >>> Cc: Gerd Hoffmann >>> Signed-off-by: Zhiguang Liu >>> --- >>> UefiCpuPkg/CpuMpPei/CpuPaging.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c >> b/UefiCpuPkg/CpuMpPei/CpuPaging.c >>> index 15c7015fb8..c6894458f7 100644 >>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c >>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c >>> @@ -167,7 +167,6 @@ ConvertMemoryPageToNotPresent ( >>> } >>> >>> ASSERT_EFI_ERROR (Status); >>> - AsmWriteCr3 (PageTable); >>> return Status; >>> } >>> >> >> (1) I mostly understand the point that the commit message makes, but the >> message is not clear enough. The real point is that we have two >> ConvertMemoryPageToNotPresent() calls altogether, and each of those >> takes place in a *loop*, and each of those loops is followed by a >> CpuFlushTlb() call. >> >> The loops matter. If there were no loops, then we might be motivated to >> choose a different solution (for example, to move centralize the >> CpuFlushTlb() calls *inside* ConvertMemoryPageToNotPresent(), or >> something similar). >> >> So, please update the commit message; mention the loops. >> >> (2) I can't easily see why this change is actually correct. IIRC, >> writing CR3 has a "side effect" of flushing the TLB. But obviously, >> that's not the *only* effect of writing CR3. You could say that >> CpuFlushTlb() performs a *strict subset* of what AsmWriteCr3() performs. >> Therefore, in order to replace AsmWriteCr3() with just CpuFlushTlb(), >> you need to demonstrate that the functionality that now is *not* going >> to be done has always been superfluous. In more direct terms, you need >> to show that the AsmWriteCr3() write call that's being removed here >> never actuall changes the *value* of CR3. >> >> And I cannot show that myself very easily. >> ConvertMemoryPageToNotPresent(). In ConvertMemoryPageToNotPresent(), >> "PageTable" is first set from AsmReadCr3(), then passed twice to >> PageTableMap() by reference (!), and then it is written back to CR3. If >> at least one of those PageTableMap() calls change "PageTable", then the >> AsmWriteCr3() call at the end that's now being removed actually changes >> the value of CR3, and then the patch would be wrong. >> >> It's possible that PageTableMap() never changes the value of >> "PageTable", but it must be proved, and the evidence should be included >> in the commit message. >> >> (3) Furthermore, with the patch applied, ConvertMemoryPageToNotPresent() >> will no longer flush the TLB itself -- that will always be left to the >> caller. This new caller responsibility should be documented in the >> leading comment of ConvertMemoryPageToNotPresent(). We already have a >> paragraph there starting with "Caller should make sure..." >> >> Sorry for making such a big deal out of this, but such simple-looking >> changes can sometimes case obscure (and rarely occurring) bugs down the >> road. If you already have evidence that CR3 does not change here, that's >> great, but then please don't think it's "obvious"; just go ahead and >> document it. >=20 > Laszlo, > Happy to see these comments! All make sense! >=20 > PageTableMap() only modifies the PageTable root pointer when creating fro= m zero. > Since there is only one instance of the PageTableLib, I think we could up= date the > PageTableLib API comments to explicitly mention that. So point (2) will b= e resolved. That should work, thanks! Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113597): https://edk2.groups.io/g/devel/message/113597 Mute This Topic: https://groups.io/mt/103636435/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-