* [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
@ 2024-03-01 2:54 Zhou Jianfeng
2024-03-01 11:50 ` Michael Brown
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Zhou Jianfeng @ 2024-03-01 2:54 UTC (permalink / raw)
To: devel
Cc: Zhou Jianfeng, Ray Ni, Laszlo Ersek, Rahul Kumar, Gerd Hoffmann,
Pedro Falcato, Zhang Di, Tan Dun, Michael Brown
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>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Zhang Di <di.zhang@intel.com>
Cc: Tan Dun <dun.tan@intel.com>
Cc: Michael Brown <mcb30@ipxe.org>
---
.../Library/CpuPageTableLib/CpuPageTableMap.c | 36 +++++++++----------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index c4e46a6d74..0a380a04cb 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -20,17 +20,17 @@
**/
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;
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) {
@@ -94,17 +94,17 @@ PageTableLibSetPte4K (
**/
VOID
PageTableLibSetPleB (
- IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE *PleB,
- IN UINT64 Offset,
- IN IA32_MAP_ATTRIBUTE *Attribute,
- IN IA32_MAP_ATTRIBUTE *Mask
+ IN OUT volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE *PleB,
+ IN UINT64 Offset,
+ IN IA32_MAP_ATTRIBUTE *Attribute,
+ IN IA32_MAP_ATTRIBUTE *Mask
)
{
IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE LocalPleB;
LocalPleB.Uint64 = PleB->Uint64;
if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
- LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
+ LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (LocalPleB.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
}
LocalPleB.Bits.MustBeOne = 1;
@@ -171,11 +171,11 @@ PageTableLibSetPleB (
**/
VOID
PageTableLibSetPle (
- IN UINTN Level,
- IN OUT IA32_PAGING_ENTRY *Ple,
- IN UINT64 Offset,
- IN IA32_MAP_ATTRIBUTE *Attribute,
- IN IA32_MAP_ATTRIBUTE *Mask
+ IN UINTN Level,
+ IN OUT volatile IA32_PAGING_ENTRY *Ple,
+ IN UINT64 Offset,
+ IN IA32_MAP_ATTRIBUTE *Attribute,
+ IN IA32_MAP_ATTRIBUTE *Mask
)
{
if (Level == 1) {
@@ -195,9 +195,9 @@ PageTableLibSetPle (
**/
VOID
PageTableLibSetPnle (
- IN OUT IA32_PAGE_NON_LEAF_ENTRY *Pnle,
- IN IA32_MAP_ATTRIBUTE *Attribute,
- IN IA32_MAP_ATTRIBUTE *Mask
+ IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY *Pnle,
+ IN IA32_MAP_ATTRIBUTE *Attribute,
+ IN IA32_MAP_ATTRIBUTE *Mask
)
{
IA32_PAGE_NON_LEAF_ENTRY LocalPnle;
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116230): https://edk2.groups.io/g/devel/message/116230
Mute This Topic: https://groups.io/mt/104661494/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
2024-03-01 2:54 [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile Zhou Jianfeng
@ 2024-03-01 11:50 ` Michael Brown
2024-03-01 12:21 ` Laszlo Ersek
2024-03-01 18:56 ` Laszlo Ersek
2 siblings, 0 replies; 9+ messages in thread
From: Michael Brown @ 2024-03-01 11:50 UTC (permalink / raw)
To: devel, jianfeng.zhou
Cc: Ray Ni, Laszlo Ersek, Rahul Kumar, Gerd Hoffmann, Pedro Falcato,
Zhang Di, Tan Dun
Reviewed-by: Michael Brown <mcb30@ipxe.org>
Thanks,
Michael
On 01/03/2024 02:54, 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>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Zhang Di <di.zhang@intel.com>
> Cc: Tan Dun <dun.tan@intel.com>
> Cc: Michael Brown <mcb30@ipxe.org>
> ---
> .../Library/CpuPageTableLib/CpuPageTableMap.c | 36 +++++++++----------
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index c4e46a6d74..0a380a04cb 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -20,17 +20,17 @@
> **/
> 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;
>
> 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) {
> @@ -94,17 +94,17 @@ PageTableLibSetPte4K (
> **/
> VOID
> PageTableLibSetPleB (
> - IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE *PleB,
> - IN UINT64 Offset,
> - IN IA32_MAP_ATTRIBUTE *Attribute,
> - IN IA32_MAP_ATTRIBUTE *Mask
> + IN OUT volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE *PleB,
> + IN UINT64 Offset,
> + IN IA32_MAP_ATTRIBUTE *Attribute,
> + IN IA32_MAP_ATTRIBUTE *Mask
> )
> {
> IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE LocalPleB;
>
> LocalPleB.Uint64 = PleB->Uint64;
> if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
> - LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
> + LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (LocalPleB.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
> }
>
> LocalPleB.Bits.MustBeOne = 1;
> @@ -171,11 +171,11 @@ PageTableLibSetPleB (
> **/
> VOID
> PageTableLibSetPle (
> - IN UINTN Level,
> - IN OUT IA32_PAGING_ENTRY *Ple,
> - IN UINT64 Offset,
> - IN IA32_MAP_ATTRIBUTE *Attribute,
> - IN IA32_MAP_ATTRIBUTE *Mask
> + IN UINTN Level,
> + IN OUT volatile IA32_PAGING_ENTRY *Ple,
> + IN UINT64 Offset,
> + IN IA32_MAP_ATTRIBUTE *Attribute,
> + IN IA32_MAP_ATTRIBUTE *Mask
> )
> {
> if (Level == 1) {
> @@ -195,9 +195,9 @@ PageTableLibSetPle (
> **/
> VOID
> PageTableLibSetPnle (
> - IN OUT IA32_PAGE_NON_LEAF_ENTRY *Pnle,
> - IN IA32_MAP_ATTRIBUTE *Attribute,
> - IN IA32_MAP_ATTRIBUTE *Mask
> + IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY *Pnle,
> + IN IA32_MAP_ATTRIBUTE *Attribute,
> + IN IA32_MAP_ATTRIBUTE *Mask
> )
> {
> IA32_PAGE_NON_LEAF_ENTRY LocalPnle;
> --
> 2.31.1.windows.1
>
>
>
> -=-=-=-=-=-=-=-=-=-=-
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#116230): https://edk2.groups.io/g/devel/message/116230
> Mute This Topic: https://groups.io/mt/104661494/1770615
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [mcb30@ipxe.org]
> -=-=-=-=-=-=-=-=-=-=-
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116236): https://edk2.groups.io/g/devel/message/116236
Mute This Topic: https://groups.io/mt/104661494/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
2024-03-01 2:54 [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile Zhou Jianfeng
2024-03-01 11:50 ` Michael Brown
@ 2024-03-01 12:21 ` Laszlo Ersek
2024-03-01 18:56 ` Laszlo Ersek
2 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2024-03-01 12:21 UTC (permalink / raw)
To: devel, jianfeng.zhou
Cc: Ray Ni, Rahul Kumar, Gerd Hoffmann, Pedro Falcato, Zhang Di,
Tan Dun, Michael Brown
On 3/1/24 03:54, 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>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Zhang Di <di.zhang@intel.com>
> Cc: Tan Dun <dun.tan@intel.com>
> Cc: Michael Brown <mcb30@ipxe.org>
> ---
> .../Library/CpuPageTableLib/CpuPageTableMap.c | 36 +++++++++----------
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index c4e46a6d74..0a380a04cb 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -20,17 +20,17 @@
> **/
> 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;
>
> 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) {
> @@ -94,17 +94,17 @@ PageTableLibSetPte4K (
> **/
> VOID
> PageTableLibSetPleB (
> - IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE *PleB,
> - IN UINT64 Offset,
> - IN IA32_MAP_ATTRIBUTE *Attribute,
> - IN IA32_MAP_ATTRIBUTE *Mask
> + IN OUT volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE *PleB,
> + IN UINT64 Offset,
> + IN IA32_MAP_ATTRIBUTE *Attribute,
> + IN IA32_MAP_ATTRIBUTE *Mask
> )
> {
> IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE LocalPleB;
>
> LocalPleB.Uint64 = PleB->Uint64;
> if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
> - LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
> + LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (LocalPleB.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
> }
>
> LocalPleB.Bits.MustBeOne = 1;
> @@ -171,11 +171,11 @@ PageTableLibSetPleB (
> **/
> VOID
> PageTableLibSetPle (
> - IN UINTN Level,
> - IN OUT IA32_PAGING_ENTRY *Ple,
> - IN UINT64 Offset,
> - IN IA32_MAP_ATTRIBUTE *Attribute,
> - IN IA32_MAP_ATTRIBUTE *Mask
> + IN UINTN Level,
> + IN OUT volatile IA32_PAGING_ENTRY *Ple,
> + IN UINT64 Offset,
> + IN IA32_MAP_ATTRIBUTE *Attribute,
> + IN IA32_MAP_ATTRIBUTE *Mask
> )
> {
> if (Level == 1) {
> @@ -195,9 +195,9 @@ PageTableLibSetPle (
> **/
> VOID
> PageTableLibSetPnle (
> - IN OUT IA32_PAGE_NON_LEAF_ENTRY *Pnle,
> - IN IA32_MAP_ATTRIBUTE *Attribute,
> - IN IA32_MAP_ATTRIBUTE *Mask
> + IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY *Pnle,
> + IN IA32_MAP_ATTRIBUTE *Attribute,
> + IN IA32_MAP_ATTRIBUTE *Mask
> )
> {
> IA32_PAGE_NON_LEAF_ENTRY LocalPnle;
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116242): https://edk2.groups.io/g/devel/message/116242
Mute This Topic: https://groups.io/mt/104661494/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
2024-03-01 2:54 [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile Zhou Jianfeng
2024-03-01 11:50 ` Michael Brown
2024-03-01 12:21 ` Laszlo Ersek
@ 2024-03-01 18:56 ` Laszlo Ersek
2 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2024-03-01 18:56 UTC (permalink / raw)
To: devel, jianfeng.zhou
Cc: Ray Ni, Rahul Kumar, Gerd Hoffmann, Pedro Falcato, Zhang Di,
Tan Dun, Michael Brown
On 3/1/24 03:54, 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>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Zhang Di <di.zhang@intel.com>
> Cc: Tan Dun <dun.tan@intel.com>
> Cc: Michael Brown <mcb30@ipxe.org>
> ---
> .../Library/CpuPageTableLib/CpuPageTableMap.c | 36 +++++++++----------
> 1 file changed, 18 insertions(+), 18 deletions(-)
Merged as
2 dcffad2491bc UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
via <https://github.com/tianocore/edk2/pull/5432>.
Unfortunately, the patch was formatted and/or posted incorrectly; git-am complained about "corrupt patch", no matter what I did about the quoted-printable encoding.
Therefore I had to manually reconstruct the patch (I noted that fact on the commit message).
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116281): https://edk2.groups.io/g/devel/message/116281
Mute This Topic: https://groups.io/mt/104661494/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
@ 2024-02-22 8:41 Zhou Jianfeng
2024-02-23 11:59 ` Michael Brown
0 siblings, 1 reply; 9+ messages in thread
From: Zhou Jianfeng @ 2024-02-22 8:41 UTC (permalink / raw)
To: devel
Cc: Zhou Jianfeng, Ray Ni, Laszlo Ersek, Rahul Kumar, Gerd Hoffmann,
Pedro Falcato, Zhang Di, Tan Dun
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>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Zhang Di <di.zhang@intel.com>
Cc: Tan Dun <dun.tan@intel.com>
---
.../Library/CpuPageTableLib/CpuPageTableMap.c | 38 +++++++++----------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 2ea40666cc..996e4001fa 100644
--- 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;
LocalPte4K.Uint64 = Pte4K->Uint64;
if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
@@ -94,13 +94,13 @@ PageTableLibSetPte4K (
**/
VOID
PageTableLibSetPleB (
- IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE *PleB,
- IN UINT64 Offset,
- IN IA32_MAP_ATTRIBUTE *Attribute,
- IN IA32_MAP_ATTRIBUTE *Mask
+ IN OUT volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE *PleB,
+ IN UINT64 Offset,
+ IN IA32_MAP_ATTRIBUTE *Attribute,
+ 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) {
@@ -171,11 +171,11 @@ PageTableLibSetPleB (
**/
VOID
PageTableLibSetPle (
- IN UINTN Level,
- IN OUT IA32_PAGING_ENTRY *Ple,
- IN UINT64 Offset,
- IN IA32_MAP_ATTRIBUTE *Attribute,
- IN IA32_MAP_ATTRIBUTE *Mask
+ IN UINTN Level,
+ IN OUT volatile IA32_PAGING_ENTRY *Ple,
+ IN UINT64 Offset,
+ IN IA32_MAP_ATTRIBUTE *Attribute,
+ IN IA32_MAP_ATTRIBUTE *Mask
)
{
if (Level == 1) {
@@ -195,12 +195,12 @@ PageTableLibSetPle (
**/
VOID
PageTableLibSetPnle (
- IN OUT IA32_PAGE_NON_LEAF_ENTRY *Pnle,
- IN IA32_MAP_ATTRIBUTE *Attribute,
- IN IA32_MAP_ATTRIBUTE *Mask
+ IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY *Pnle,
+ IN IA32_MAP_ATTRIBUTE *Attribute,
+ 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) {
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115873): https://edk2.groups.io/g/devel/message/115873
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
2024-02-22 8:41 Zhou Jianfeng
@ 2024-02-23 11:59 ` Michael Brown
2024-02-23 15:12 ` Zhou, Jianfeng
0 siblings, 1 reply; 9+ messages in thread
From: Michael Brown @ 2024-02-23 11:59 UTC (permalink / raw)
To: devel, jianfeng.zhou
Cc: Ray Ni, Laszlo Ersek, Rahul Kumar, Gerd Hoffmann, Pedro Falcato,
Zhang Di, Tan Dun
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 (#115883): https://edk2.groups.io/g/devel/message/115883
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
2024-02-23 11:59 ` Michael Brown
@ 2024-02-23 15:12 ` Zhou, Jianfeng
2024-02-23 15:51 ` Michael Brown
0 siblings, 1 reply; 9+ messages in thread
From: Zhou, Jianfeng @ 2024-02-23 15:12 UTC (permalink / raw)
To: Michael Brown, devel@edk2.groups.io
Cc: Ni, Ray, Laszlo Ersek, Kumar, Rahul R, Gerd Hoffmann,
Pedro Falcato, Zhang, Di, Tan, Dun
> 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 <mcb30@ipxe.org>
Sent: Friday, February 23, 2024 7:59 PM
To: devel@edk2.groups.io; Zhou, Jianfeng <jianfeng.zhou@intel.com>
Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Pedro Falcato <pedro.falcato@gmail.com>; Zhang, Di <di.zhang@intel.com>; Tan, Dun <dun.tan@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
2024-02-23 15:12 ` Zhou, Jianfeng
@ 2024-02-23 15:51 ` Michael Brown
2024-02-25 13:47 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Michael Brown @ 2024-02-23 15:51 UTC (permalink / raw)
To: Zhou, Jianfeng, devel@edk2.groups.io
Cc: Ni, Ray, Laszlo Ersek, Kumar, Rahul R, Gerd Hoffmann,
Pedro Falcato, Zhang, Di, Tan, Dun
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
2024-02-23 15:51 ` Michael Brown
@ 2024-02-25 13:47 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2024-02-25 13:47 UTC (permalink / raw)
To: Michael Brown, Zhou, Jianfeng, devel@edk2.groups.io
Cc: Ni, Ray, Kumar, Rahul R, Gerd Hoffmann, Pedro Falcato, Zhang, Di,
Tan, Dun
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-01 18:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-01 2:54 [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile Zhou Jianfeng
2024-03-01 11:50 ` Michael Brown
2024-03-01 12:21 ` Laszlo Ersek
2024-03-01 18:56 ` Laszlo Ersek
-- strict thread matches above, loose matches on Subject: below --
2024-02-22 8:41 Zhou Jianfeng
2024-02-23 11:59 ` Michael Brown
2024-02-23 15:12 ` Zhou, Jianfeng
2024-02-23 15:51 ` Michael Brown
2024-02-25 13:47 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox