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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
