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

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

[Zhou Jianfeng]

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(-)

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;
   }
 }

--
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115747): https://edk2.groups.io/g/devel/message/115747
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Zhou, Jianfeng <jianfeng.zhou@intel.com>
> Sent: Wednesday, February 21, 2024 9:25 AM
> To: devel@edk2.groups.io
> Cc: Zhou, Jianfeng <jianfeng.zhou@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH] UefiCpuPkg: add volatile qualifier to page table related
> variable
> 
> 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(-)
> 
> 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;
> 
>    }
> 
>  }
> 
> 
> 
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115712): https://edk2.groups.io/g/devel/message/115712
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Pedro Falcato]

On Wed, Feb 21, 2024 at 8:36 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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>

I'd appreciate getting CC'd on my own suggestion....

> > ---
> >  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.

I echo these comments :)

>
> 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.

Ugh, honestly converting to volatile implicitly is kind-of yucky, but
I guess it works; personally I'd rather have explicit conversion, but
it's just a matter of taste.
What I *really* prefer in these cases (when we're not dealing with
MMIO) is something like READ_ONCE and WRITE_ONCE, where the
"volatility points" are very well annotated, but oh well :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115743): https://edk2.groups.io/g/devel/message/115743
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Zhou, Jianfeng]

Thank all for review.

> 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.

Obviously, local variables like LocalPleB just a temporary variable. 
From optimization perspective, it may be replaced by function parameter PleB.
Qualifying local variable is for preventing it is optimized. In this case, code like "LocalPleB.Bits.Present = Attribute->Bits.Present" may lead to unexpected result, as it is not atomic.
It might lead to performance degradation, as it can't be optimized to register. I think such code will not be called in large quantities and slight drop in performance is acceptable.


>   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.

agree that such improvement would have the same effect, and the code would be a little more graceful.
Will update this patch.

Thanks & Regards,
Zhou Jianfeng

-----Original Message-----
From: Pedro Falcato <pedro.falcato@gmail.com> 
Sent: Thursday, February 22, 2024 5:44 AM
To: devel@edk2.groups.io; lersek@redhat.com
Cc: Zhou, Jianfeng <jianfeng.zhou@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: add volatile qualifier to page table related variable

On Wed, Feb 21, 2024 at 8:36 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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>

I'd appreciate getting CC'd on my own suggestion....

> > ---
> >  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.

I echo these comments :)

>
> 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.

Ugh, honestly converting to volatile implicitly is kind-of yucky, but I guess it works; personally I'd rather have explicit conversion, but it's just a matter of taste.
What I *really* prefer in these cases (when we're not dealing with
MMIO) is something like READ_ONCE and WRITE_ONCE, where the "volatility points" are very well annotated, but oh well :)

--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115872): https://edk2.groups.io/g/devel/message/115872
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

> > 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.

I did experiment using MSVC compiler with below code:
  int main () {
    int x;
    x = 3;
    return 0;
  }

If building the above code in optimized mode, the disassembly does not contain
any reference to local variable x.

But if I changed "int x" to "volatile int x", the compiler does not optimize out the
assignment of x.

So, it means the "volatile"  matters even when it applies local variables.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115795): https://edk2.groups.io/g/devel/message/115795
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 2/22/24 11:23, Ni, Ray wrote:
>>> 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.
> 
> I did experiment using MSVC compiler with below code:
>   int main () {
>     int x;
>     x = 3;
>     return 0;
>   }
> 
> If building the above code in optimized mode, the disassembly does not contain
> any reference to local variable x.
> 
> But if I changed "int x" to "volatile int x", the compiler does not optimize out the
> assignment of x.
> 
> So, it means the "volatile"  matters even when it applies local variables.

Yes, but that's beside the point. It does not matter what kinds of
accesses the compiler generates for the local variables, as those
variables exist on the stack, and are not seen by anything else
(certainly not by other processors). The accesses that matter are to the
page tables, in dynamically allocated memory, so if anything, *those*
accesses should be "volatile".

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115924): https://edk2.groups.io/g/devel/message/115924
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]
-=-=-=-=-=-=-=-=-=-=-=-