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 1040BD8119E for ; Wed, 10 Jan 2024 12:15:45 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=JdXe0c2qdZj3WF2TSK4CGLIUyEGuBelbMEqT7PCe+mM=; 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=1704888944; v=1; b=FwfCTAIKM4kwZlUy4YYyKsPpqTHNS7Q9HZalSXbTJy8vHU6/ok4RdBqbzPt7P0nbpUjV3Xpy lSRAQXWxF38TUCK4ZSLIJigDrn1th6wWkocHikqzYFsqwcXHHni5ZzaysWGPHdxep7G95Jzzi8g qmxkByh/c/MGd7sISLMigvRM= X-Received: by 127.0.0.2 with SMTP id dh6WYY7687511xpTS4uKp8eu; Wed, 10 Jan 2024 04:15:44 -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.web10.10223.1704888944116654597 for ; Wed, 10 Jan 2024 04:15:44 -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-621-pnr6oytpNI6to3yzw7y1dg-1; Wed, 10 Jan 2024 07:15:40 -0500 X-MC-Unique: pnr6oytpNI6to3yzw7y1dg-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (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 AAB2C3C0ED6E; Wed, 10 Jan 2024 12:15:39 +0000 (UTC) X-Received: from [10.39.192.166] (unknown [10.39.192.166]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 50FB23C2E; Wed, 10 Jan 2024 12:15:38 +0000 (UTC) Message-ID: <95163437-75ea-9265-a5e1-35cf01f186eb@redhat.com> Date: Wed, 10 Jan 2024 13:15:37 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap To: Zhiguang Liu , devel@edk2.groups.io Cc: Ray Ni , Rahul Kumar , Gerd Hoffmann , Crystal Lee References: <20240110053828.1473-1-zhiguang.liu@intel.com> From: "Laszlo Ersek" In-Reply-To: <20240110053828.1473-1-zhiguang.liu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 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: oFHy1liGN6TVUFyAv8DQjXBjx7686176AA= 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=FwfCTAIK; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 1/10/24 06:38, Zhiguang Liu wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4614 >=20 > Fix issue that IsModified is wrongly set in PageTableMap. >=20 > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Gerd Hoffmann > Cc: Crystal Lee > Signed-off-by: Zhiguang Liu > --- > UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiC= puPkg/Library/CpuPageTableLib/CpuPageTableMap.c > index 36b2c4e6a3..164187f151 100644 > --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > @@ -567,7 +567,10 @@ PageTableLibMapInLevel ( > OriginalCurrentPagingEntry.Uint64 =3D CurrentPagingEntry->Uint64= ; > PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute= , &CurrentMask); > =20 > - if (OriginalCurrentPagingEntry.Uint64 !=3D CurrentPagingEntry->U= int64) { > + if (Modify && (OriginalCurrentPagingEntry.Uint64 !=3D CurrentPag= ingEntry->Uint64)) { > + // > + // The page table entry can be changed by this function only w= hen Modify is true. > + // > *IsModified =3D TRUE; > } > } > @@ -609,7 +612,10 @@ PageTableLibMapInLevel ( > // Check if ParentPagingEntry entry is modified here is enough. Except= the changes happen in leaf PagingEntry during > // the while loop, if there is any other change happens in page table,= the ParentPagingEntry must has been modified. > // > - if (OriginalParentPagingEntry.Uint64 !=3D ParentPagingEntry->Uint64) { > + if (Modify && (OriginalParentPagingEntry.Uint64 !=3D ParentPagingEntry= ->Uint64)) { > + // > + // The page table entry can be changed by this function only when Mo= dify is true. > + // > *IsModified =3D TRUE; > } > =20 This function is incredibly complicated, so reviewing this patch is hard, even after reading the bugzilla ticket. The commit message is useless. It should contain a brief description of the problem, and how the fix resolves the problem. The documentation of the PageTableLibMapInLevel() function is wrong, even before this patch. It documents the "IsModified" output-only parameter as follows: "TRUE means page table is modified. FALSE means page table is not modified.= " This states that "IsModified" is always set on output, to either FALSE or TRUE. Which is an incorrect statement; in reality the caller is expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel() will (conditionally!) perform a FALSE->TRUE transition only. Now, this patch may fix a bug, but it makes the above-described documentation issue worse, by further restricting the condition for said FALSE->TRUE transition. The fix per se looks vaguely reasonable to me (really the function is so complicated that verifying this change from scratch would take me ages), but minimally, the documentation of "IsModified" should certainly be updated too. To something like this: @param[out] IsModified If "Modify" is TRUE on input and the function has actually modified the page table, then set to TRUE on output. Not overwritten otherwise. 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 (#113528): https://edk2.groups.io/g/devel/message/113528 Mute This Topic: https://groups.io/mt/103636407/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-