Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile

["Michael Brown" <mcb30@ipxe.org>]

On 23/02/2024 15:12, Zhou, Jianfeng wrote:
>> While it may well cause the compiler to generate less optimised code, there is absolutely no way that this volatile declaration on a local stack variable can possibly change the outcome of the code.
>> There can never be any meaningful side-effects from reading or writing a stack variable.
>> I would suggest dropping the volatile on LocalPte4K, since its *only* possible impact is to confuse a future reader of the code.
> 
> The change is for preventing compiler from optimizing.
> As a temporary variable,  LocalPte4K may be replaced by function parameter Pte4K.

No, it can't.  If Pte4K is marked as a volatile pointer, then the 
compiler is not allowed to unilaterally decide to treat it as a 
non-volatile pointer.

> In this case, code like "LocalPte4K.Bits.Present = Attribute->Bits.Present" may lead to unexpected result, as it is not atomic. Assembly code look like:
>     mov eax, [r8]
>     and dword [rcx], 0xfffffffe  // this instruction clear the present bit and may leads to unexpected result.
>     and eax, 0x1
>     or [rcx], eax

Please test with Pte4K marked as volatile and LocalPte4K marked as 
non-volatile.  If you can still generate assembly code that writes to 
*Pte4K more than once, then that would be a serious compiler bug.


As a separate note, I would also suggest removing the unnecessary second 
read through Pte4K, since once Pte4K is marked as volatile the compiler 
will generate an extra read from that address:

--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -30,7 +30,7 @@ PageTableLibSetPte4K (

    LocalPte4K.Uint64 = Pte4K->Uint64;
    if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
-    LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
(Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
+    LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
(Attribute) + Offset) | (LocalPte4K.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
    }

    if (Mask->Bits.Present) {

Michael




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115888): https://edk2.groups.io/g/devel/message/115888
Mute This Topic: https://groups.io/mt/104524857/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-