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 22F8F941CF3 for ; Wed, 21 Feb 2024 20:36:33 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=Ptakp9IqtoohDP15BmLjU0q85rdg1V35qNGZ9NsSFds=; 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=1708547792; v=1; b=DieEIOIHUUzhlO+Zr2TdL1CcrGqT18penJbQcssA55uor6pHbJ6WpcJxMDqiTteMQZKHPt27 IvKT8nyHDMTWGhLqY5oFur21qSNhS+Xoc6d6Zw81rNTB9zFMhGsxtiUtarhU/8dAmMgbKLyVP5z jVacaTxvtOW1PqnmFT5eP/U8= X-Received: by 127.0.0.2 with SMTP id 8U2hYY7687511xWKLjG9QYLt; Wed, 21 Feb 2024 12:36:32 -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.1844.1708547792084880832 for ; Wed, 21 Feb 2024 12:36:32 -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-651-U5xBpFtuP-6JAbSW63VU2g-1; Wed, 21 Feb 2024 15:36:28 -0500 X-MC-Unique: U5xBpFtuP-6JAbSW63VU2g-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 C997A185A780; Wed, 21 Feb 2024 20:36:27 +0000 (UTC) X-Received: from [10.39.192.46] (unknown [10.39.192.46]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 111432022AAA; Wed, 21 Feb 2024 20:36:25 +0000 (UTC) Message-ID: <6bf89071-0514-cb97-f639-6bece14cc6d7@redhat.com> Date: Wed, 21 Feb 2024 21:36:24 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: add volatile qualifier to page table related variable To: Zhou Jianfeng , devel@edk2.groups.io Cc: Ray Ni , Rahul Kumar , Gerd Hoffmann References: <20240221012513.27453-1-jianfeng.zhou@intel.com> From: "Laszlo Ersek" In-Reply-To: <20240221012513.27453-1-jianfeng.zhou@intel.com> 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: Mq2iifnsbbGCo8WF3ONjcqLrx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=DieEIOIH; 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 2/21/24 02:25, Zhou Jianfeng wrote: > Add volatile qualifier to page table related variable to prevent > compiler from optimizing away the variables which may lead to > unexpected result. > > Signed-off-by: Zhou Jianfeng > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Gerd Hoffmann > --- > UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) (1) subject should be something like: UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile > > diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > index 2ea40666cc..5cf6e8fea0 100644 > --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c > @@ -26,7 +26,7 @@ PageTableLibSetPte4K ( > IN IA32_MAP_ATTRIBUTE *Mask > ) > { > - IA32_PTE_4K LocalPte4K; > + volatile IA32_PTE_4K LocalPte4K; > > LocalPte4K.Uint64 = Pte4K->Uint64; > if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) { > @@ -78,7 +78,7 @@ PageTableLibSetPte4K ( > } > > if (Pte4K->Uint64 != LocalPte4K.Uint64) { > - Pte4K->Uint64 = LocalPte4K.Uint64; > + *(volatile UINT64 *)&(Pte4K->Uint64) = LocalPte4K.Uint64; > } > } > > @@ -100,7 +100,7 @@ PageTableLibSetPleB ( > IN IA32_MAP_ATTRIBUTE *Mask > ) > { > - IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE LocalPleB; > + volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE LocalPleB; > > LocalPleB.Uint64 = PleB->Uint64; > if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) { > @@ -154,7 +154,7 @@ PageTableLibSetPleB ( > } > > if (PleB->Uint64 != LocalPleB.Uint64) { > - PleB->Uint64 = LocalPleB.Uint64; > + *(volatile UINT64 *)&(PleB->Uint64) = LocalPleB.Uint64; > } > } > > @@ -200,7 +200,7 @@ PageTableLibSetPnle ( > IN IA32_MAP_ATTRIBUTE *Mask > ) > { > - IA32_PAGE_NON_LEAF_ENTRY LocalPnle; > + volatile IA32_PAGE_NON_LEAF_ENTRY LocalPnle; > > LocalPnle.Uint64 = Pnle->Uint64; > if (Mask->Bits.Present) { > @@ -231,7 +231,7 @@ PageTableLibSetPnle ( > LocalPnle.Bits.WriteThrough = 0; > LocalPnle.Bits.CacheDisabled = 0; > if (Pnle->Uint64 != LocalPnle.Uint64) { > - Pnle->Uint64 = LocalPnle.Uint64; > + *(volatile UINT64 *)&(Pnle->Uint64) = LocalPnle.Uint64; > } > } I agree with the idea (I think it's a necessary change, or put differently, an improvement, even though I may not be convinced that it is a *sufficient* improvement; but let's not rehash all that here again); however, I think the implementation is not the greatest. Volatile-qualifying the local variables does not seem useful for anything. It's fine -- actually: it's beneficial -- if the compiler optimizes accesses to those locals -- being on the stack -- as heavily as it can. In other words, those parts of the patch look like a small performance regression. (2) What we want to qualify as volatile here are the *targets* of the Pte4K, PleB and Pnle pointers. Your other patch ("UefiCpuPkg: Fix IN OUT parameters marked as IN") correctly marks those as "IN OUT", so in this patch, we should update them to: IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY *Pnle and similar. Then the existent assignment expressions Pnle->Uint64 = LocalPnle.Uint64; don't have to be changed. Note that call sites will not have to be updated either; see C99 6.3.2.3 Pointers, paragraph 2: For any qualifier q, a pointer to a non-q-qualified type may be converted to a pointer to the q-qualified version of the type; the values stored in the original and converted pointers shall compare equal. and 6.7.3 Type qualifiers, p5-6: If an attempt is made to modify an object defined with a const-qualified type through use of an lvalue with non-const-qualified type, the behavior is undefined. If an attempt is made to refer to an object defined with a volatile-qualified type through use of an lvalue with non-volatile-qualified type, the behavior is undefined. 115) An object that has volatile-qualified type may be modified in ways unknown to the implementation or have other unknown side effects. Therefore any expression referring to such an object shall be evaluated strictly according to the rules of the abstract machine, as described in 5.1.2.3. Furthermore, at every sequence point the value last stored in the object shall agree with that prescribed by the abstract machine, except as modified by the unknown factors mentioned previously. 116) What constitutes an access to an object that has volatile-qualified type is implementation-defined. Footnotes: 115) This applies to those objects that behave as if they were defined with qualified types, even if they are never actually defined as objects in the program (such as an object at a memory-mapped input/output address). 116) A volatile declaration may be used to describe an object corresponding to a memory-mapped input/output port or an object accessed by an asynchronously interrupting function. Actions on objects so declared shall not be ‘‘optimized out’’ by an implementation or reordered except as permitted by the rules for evaluating expressions. (Footnote 116 is quite tricky. It does not speak about access via pointer, but about the declaration of the object itself. What's tricky here is that the page tables we are dealing with are dynamically allocated. And under 6.5 Expressions, paragraph 6, we have this: The effective type of an object for an access to its stored value is the declared type of the object, *if any*. 75) [...] Emphasis mine. And footnote 75 says (!): 75) Allocated objects have no declared type. Which seems to imply that you cannot *declare* any dynamically allocated object as volatile; at best you can access it through pointers-to-volatile.) Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115741): https://edk2.groups.io/g/devel/message/115741 Mute This Topic: https://groups.io/mt/104483610/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-