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 <jianfeng.z...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> ---
> 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
