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 D01137803CE for ; Wed, 10 Jan 2024 11:57:27 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=GQ1WmKVRcG+5sIqB9rOLci7X6MbZsYvJNNOiwfO3358=; 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=1704887846; v=1; b=Sh9YNLMHXVRnKoyxTVq+1hooIosI97oWqCq1fFE4FCKO+SAIX+ukgnip9ycglYEv9thdD+du A42sM0QXtc3NL/Ip1xGkFQU0gT97Mt8c+FU21hrLuG+59a4kfd33yN0LyR3CrhIrOhpAu2WZfhO 5b9GbRRpHeGyYi3G/ir71tMs= X-Received: by 127.0.0.2 with SMTP id QpdEYY7687511x5LksjeUYf2; Wed, 10 Jan 2024 03:57:26 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.9984.1704887845782898648 for ; Wed, 10 Jan 2024 03:57:25 -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-330-CDYIwpNOMf6lISJ74hQb6w-1; Wed, 10 Jan 2024 06:57:21 -0500 X-MC-Unique: CDYIwpNOMf6lISJ74hQb6w-1 X-Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (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 B5B6B3C0ED6E; Wed, 10 Jan 2024 11:57:20 +0000 (UTC) X-Received: from [10.39.192.166] (unknown [10.39.192.166]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D6479492BC9; Wed, 10 Jan 2024 11:57:19 +0000 (UTC) Message-ID: <8206c15e-8cc0-1969-e4dc-bd9c6b5df202@redhat.com> Date: Wed, 10 Jan 2024 12:57:18 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent To: Zhiguang Liu , devel@edk2.groups.io Cc: Ray Ni , Rahul Kumar , Gerd Hoffmann References: <20240110054318.997-1-zhiguang.liu@intel.com> From: "Laszlo Ersek" In-Reply-To: <20240110054318.997-1-zhiguang.liu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 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: 8HlngZ7cCQ9M5OgF59gAo7Whx7686176AA= 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=Sh9YNLMH; 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/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 >=20 > 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(-) >=20 > diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPag= ing.c > index 15c7015fb8..c6894458f7 100644 > --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c > +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c > @@ -167,7 +167,6 @@ ConvertMemoryPageToNotPresent ( > } > =20 > ASSERT_EFI_ERROR (Status); > - AsmWriteCr3 (PageTable); > return Status; > } > =20 (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. 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 (#113527): https://edk2.groups.io/g/devel/message/113527 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-