Re: [edk2-devel] [PATCH] UefiCpuPkg: add volatile qualifier to page table related variable

["Laszlo Ersek" <lersek@redhat.com>]

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.zhou@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-