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

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

On 2/23/24 16:51, Michael Brown wrote:
> 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) {

Agreed on each point; couldn't have expressed them any better. (I didn't
even notice the second read through Pte4K.)

Laszlo



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