> 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.
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
Thanks & Regards,
Zhou Jianfeng
-----Original Message-----
From: Michael Brown <mc...@ipxe.org>
Sent: Friday, February 23, 2024 7:59 PM
To: devel@edk2.groups.io; Zhou, Jianfeng <jianfeng.z...@intel.com>
Cc: Ni, Ray <ray...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Kumar, Rahul
R <rahul.r.ku...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Pedro Falcato
<pedro.falc...@gmail.com>; Zhang, Di <di.zh...@intel.com>; Tan, Dun
<dun....@intel.com>
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page
table accesses as volatile
On 22/02/2024 08:41, Zhou Jianfeng wrote:
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -20,13 +20,13 @@
> **/
> VOID
> PageTableLibSetPte4K (
> - IN OUT IA32_PTE_4K *Pte4K,
> - IN UINT64 Offset,
> - IN IA32_MAP_ATTRIBUTE *Attribute,
> - IN IA32_MAP_ATTRIBUTE *Mask
> + IN OUT volatile IA32_PTE_4K *Pte4K,
> + IN UINT64 Offset,
> + IN IA32_MAP_ATTRIBUTE *Attribute,
> + IN IA32_MAP_ATTRIBUTE *Mask
> )
> {
> - IA32_PTE_4K LocalPte4K;
> + volatile IA32_PTE_4K LocalPte4K;
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.
> - IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE LocalPleB;
> + volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE LocalPleB;
Same comment.
> - IA32_PAGE_NON_LEAF_ENTRY LocalPnle;
> + volatile IA32_PAGE_NON_LEAF_ENTRY LocalPnle;
Same comment.
Thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115933): https://edk2.groups.io/g/devel/message/115933
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]
-=-=-=-=-=-=-=-=-=-=-=-
