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 C9E33D811D0 for ; Tue, 6 Feb 2024 13:32:40 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=jNyF3xPwtjBPRIYnlIRH9R+5qKOEaxXF37ijYWDuWZA=; 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=1707226359; v=1; b=LkVcHvYn9UG9bYxmJ0g+QyhqeecW5GJhH8J4TpsbsLOeVlsIOxwvy00OP6ogh9m/rGOIEPKD 3Mk86UQXZAT2Ch2CSb6KLfzsgBQkIirSs6gDmy+8H1rVtpy9uOq+2UK2Vr6CkpKQ0rHO2H+GlsT qeeK0ZDd9Zqi3Ps1SXN719fI= X-Received: by 127.0.0.2 with SMTP id QoYxYY7687511x1fA4yZ0SME; Tue, 06 Feb 2024 05:32:39 -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.21402.1707226358627857921 for ; Tue, 06 Feb 2024 05:32:38 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-552-57j8sP8oNSCdKaSpTAwHuw-1; Tue, 06 Feb 2024 08:32:33 -0500 X-MC-Unique: 57j8sP8oNSCdKaSpTAwHuw-1 X-Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (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 6A9B583722E; Tue, 6 Feb 2024 13:32:32 +0000 (UTC) X-Received: from [10.39.195.129] (unknown [10.39.195.129]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 65B48492BF0; Tue, 6 Feb 2024 13:32:31 +0000 (UTC) Message-ID: Date: Tue, 6 Feb 2024 14:32:30 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute To: devel@edk2.groups.io, dun.tan@intel.com Cc: Zhou Jianfeng , Ray Ni , Rahul Kumar , Gerd Hoffmann References: <20240205140345.1437-1-dun.tan@intel.com> <20240205140345.1437-2-dun.tan@intel.com> From: "Laszlo Ersek" In-Reply-To: <20240205140345.1437-2-dun.tan@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10 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: iZCQN4rrodJ6sAOaXiSHgDFCx7686176AA= 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=LkVcHvYn; 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 2/5/24 15:03, duntan wrote: > From: Zhou Jianfeng >=20 > This commit is to reduce and optimize access to > attribute in CpuPageTableLib. >=20 > Unreasonable writing to attribute of page table may > leads to expection. > The assembly code for C code Pnle->Bits.Present =3D > Attribute->Bits.Present looks like: > and dword [rcx], 0xfffffffe > and eax, 0x1 > or [rcx], eax > In case Pnle->Bits.Present and Attribute->Bits.Present > is 1, Pnle->Bits.Present will be set to 0 for short > time(2 instructions) which is unexpected. If some other > core is accessing the page, it may leads to expection. > This change reduce and optimize access to attribute of > page table, attribute of page table is set only when it > need to be changed. This patch does nothing to eliminate the actual race condition, it only shrinks the window of potential corruption. The PTEs continue to be overwritten without any kind of synchronization with the other processors. Feel free to merge this with Ray's R-b. Laszlo >=20 > Signed-off-by: Zhou Jianfeng > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Gerd Hoffmann > --- > UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 86 ++++++++++++++= +++++++++++++++++++++++++++++++++++++++--------------------------------- > 1 file changed, 53 insertions(+), 33 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiC= puPkg/Library/CpuPageTableLib/CpuPageTableMap.c > index 36b2c4e6a3..ae4caf8dfe 100644 > --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > @@ -26,52 +26,59 @@ PageTableLibSetPte4K ( > IN IA32_MAP_ATTRIBUTE *Mask > ) > { > + IA32_PTE_4K LocalPte4K; > + > + LocalPte4K.Uint64 =3D Pte4K->Uint64; > if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddr= essHigh) { > - Pte4K->Uint64 =3D (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attri= bute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40); > + LocalPte4K.Uint64 =3D (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (A= ttribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40); > } > =20 > if (Mask->Bits.Present) { > - Pte4K->Bits.Present =3D Attribute->Bits.Present; > + LocalPte4K.Bits.Present =3D Attribute->Bits.Present; > } > =20 > if (Mask->Bits.ReadWrite) { > - Pte4K->Bits.ReadWrite =3D Attribute->Bits.ReadWrite; > + LocalPte4K.Bits.ReadWrite =3D Attribute->Bits.ReadWrite; > } > =20 > if (Mask->Bits.UserSupervisor) { > - Pte4K->Bits.UserSupervisor =3D Attribute->Bits.UserSupervisor; > + LocalPte4K.Bits.UserSupervisor =3D Attribute->Bits.UserSupervisor; > } > =20 > if (Mask->Bits.WriteThrough) { > - Pte4K->Bits.WriteThrough =3D Attribute->Bits.WriteThrough; > + LocalPte4K.Bits.WriteThrough =3D Attribute->Bits.WriteThrough; > } > =20 > if (Mask->Bits.CacheDisabled) { > - Pte4K->Bits.CacheDisabled =3D Attribute->Bits.CacheDisabled; > + LocalPte4K.Bits.CacheDisabled =3D Attribute->Bits.CacheDisabled; > } > =20 > if (Mask->Bits.Accessed) { > - Pte4K->Bits.Accessed =3D Attribute->Bits.Accessed; > + LocalPte4K.Bits.Accessed =3D Attribute->Bits.Accessed; > } > =20 > if (Mask->Bits.Dirty) { > - Pte4K->Bits.Dirty =3D Attribute->Bits.Dirty; > + LocalPte4K.Bits.Dirty =3D Attribute->Bits.Dirty; > } > =20 > if (Mask->Bits.Pat) { > - Pte4K->Bits.Pat =3D Attribute->Bits.Pat; > + LocalPte4K.Bits.Pat =3D Attribute->Bits.Pat; > } > =20 > if (Mask->Bits.Global) { > - Pte4K->Bits.Global =3D Attribute->Bits.Global; > + LocalPte4K.Bits.Global =3D Attribute->Bits.Global; > } > =20 > if (Mask->Bits.ProtectionKey) { > - Pte4K->Bits.ProtectionKey =3D Attribute->Bits.ProtectionKey; > + LocalPte4K.Bits.ProtectionKey =3D Attribute->Bits.ProtectionKey; > } > =20 > if (Mask->Bits.Nx) { > - Pte4K->Bits.Nx =3D Attribute->Bits.Nx; > + LocalPte4K.Bits.Nx =3D Attribute->Bits.Nx; > + } > + > + if (Pte4K->Uint64 !=3D LocalPte4K.Uint64) { > + Pte4K->Uint64 =3D LocalPte4K.Uint64; > } > } > =20 > @@ -93,54 +100,61 @@ PageTableLibSetPleB ( > IN IA32_MAP_ATTRIBUTE *Mask > ) > { > + IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE LocalPleB; > + > + LocalPleB.Uint64 =3D PleB->Uint64; > if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddr= essHigh) { > - PleB->Uint64 =3D (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attrib= ute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39); > + LocalPleB.Uint64 =3D (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (At= tribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39); > } > =20 > - PleB->Bits.MustBeOne =3D 1; > + LocalPleB.Bits.MustBeOne =3D 1; > =20 > if (Mask->Bits.Present) { > - PleB->Bits.Present =3D Attribute->Bits.Present; > + LocalPleB.Bits.Present =3D Attribute->Bits.Present; > } > =20 > if (Mask->Bits.ReadWrite) { > - PleB->Bits.ReadWrite =3D Attribute->Bits.ReadWrite; > + LocalPleB.Bits.ReadWrite =3D Attribute->Bits.ReadWrite; > } > =20 > if (Mask->Bits.UserSupervisor) { > - PleB->Bits.UserSupervisor =3D Attribute->Bits.UserSupervisor; > + LocalPleB.Bits.UserSupervisor =3D Attribute->Bits.UserSupervisor; > } > =20 > if (Mask->Bits.WriteThrough) { > - PleB->Bits.WriteThrough =3D Attribute->Bits.WriteThrough; > + LocalPleB.Bits.WriteThrough =3D Attribute->Bits.WriteThrough; > } > =20 > if (Mask->Bits.CacheDisabled) { > - PleB->Bits.CacheDisabled =3D Attribute->Bits.CacheDisabled; > + LocalPleB.Bits.CacheDisabled =3D Attribute->Bits.CacheDisabled; > } > =20 > if (Mask->Bits.Accessed) { > - PleB->Bits.Accessed =3D Attribute->Bits.Accessed; > + LocalPleB.Bits.Accessed =3D Attribute->Bits.Accessed; > } > =20 > if (Mask->Bits.Dirty) { > - PleB->Bits.Dirty =3D Attribute->Bits.Dirty; > + LocalPleB.Bits.Dirty =3D Attribute->Bits.Dirty; > } > =20 > if (Mask->Bits.Pat) { > - PleB->Bits.Pat =3D Attribute->Bits.Pat; > + LocalPleB.Bits.Pat =3D Attribute->Bits.Pat; > } > =20 > if (Mask->Bits.Global) { > - PleB->Bits.Global =3D Attribute->Bits.Global; > + LocalPleB.Bits.Global =3D Attribute->Bits.Global; > } > =20 > if (Mask->Bits.ProtectionKey) { > - PleB->Bits.ProtectionKey =3D Attribute->Bits.ProtectionKey; > + LocalPleB.Bits.ProtectionKey =3D Attribute->Bits.ProtectionKey; > } > =20 > if (Mask->Bits.Nx) { > - PleB->Bits.Nx =3D Attribute->Bits.Nx; > + LocalPleB.Bits.Nx =3D Attribute->Bits.Nx; > + } > + > + if (PleB->Uint64 !=3D LocalPleB.Uint64) { > + PleB->Uint64 =3D LocalPleB.Uint64; > } > } > =20 > @@ -186,24 +200,27 @@ PageTableLibSetPnle ( > IN IA32_MAP_ATTRIBUTE *Mask > ) > { > + IA32_PAGE_NON_LEAF_ENTRY LocalPnle; > + > + LocalPnle.Uint64 =3D Pnle->Uint64; > if (Mask->Bits.Present) { > - Pnle->Bits.Present =3D Attribute->Bits.Present; > + LocalPnle.Bits.Present =3D Attribute->Bits.Present; > } > =20 > if (Mask->Bits.ReadWrite) { > - Pnle->Bits.ReadWrite =3D Attribute->Bits.ReadWrite; > + LocalPnle.Bits.ReadWrite =3D Attribute->Bits.ReadWrite; > } > =20 > if (Mask->Bits.UserSupervisor) { > - Pnle->Bits.UserSupervisor =3D Attribute->Bits.UserSupervisor; > + LocalPnle.Bits.UserSupervisor =3D Attribute->Bits.UserSupervisor; > } > =20 > if (Mask->Bits.Nx) { > - Pnle->Bits.Nx =3D Attribute->Bits.Nx; > + LocalPnle.Bits.Nx =3D Attribute->Bits.Nx; > } > =20 > - Pnle->Bits.Accessed =3D 0; > - Pnle->Bits.MustBeZero =3D 0; > + LocalPnle.Bits.Accessed =3D 0; > + LocalPnle.Bits.MustBeZero =3D 0; > =20 > // > // Set the attributes (WT, CD, A) to 0. > @@ -211,8 +228,11 @@ PageTableLibSetPnle ( > // So, it implictly requires PAT[0] is Write Back. > // Create a new parameter if caller requires to use a different memory= type for accessing page directories. > // > - Pnle->Bits.WriteThrough =3D 0; > - Pnle->Bits.CacheDisabled =3D 0; > + LocalPnle.Bits.WriteThrough =3D 0; > + LocalPnle.Bits.CacheDisabled =3D 0; > + if (Pnle->Uint64 !=3D LocalPnle.Uint64) { > + Pnle->Uint64 =3D LocalPnle.Uint64; > + } > } > =20 > /** -=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 (#115162): https://edk2.groups.io/g/devel/message/115162 Mute This Topic: https://groups.io/mt/104176232/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-