* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-04 5:41 [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging Zhiguang Liu
@ 2023-01-12 12:11 ` Zeng, Star
2023-01-17 8:44 ` Wu, Jiaxin
2023-01-17 9:01 ` Ni, Ray
2023-01-17 9:02 ` Ni, Ray
2 siblings, 1 reply; 12+ messages in thread
From: Zeng, Star @ 2023-01-12 12:11 UTC (permalink / raw)
To: devel@edk2.groups.io, Liu, Zhiguang
Cc: Ni, Ray, Kumar, Rahul R, Dong, Eric, Tan, Dun, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhiguang Liu
Sent: Wednesday, January 4, 2023 1:41 PM
To: devel@edk2.groups.io
Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4246
In function InitPaging, NumberOfPml5Entries is calculated by below code NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48); If the SizeOfMemorySpace is larger than 48, NumberOfPml5Entries will be larger than 1. However, this doesn't make sense if the hardware doesn't support 5 level page table.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index c1efda7126..c597b39b8c 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -1,7 +1,7 @@
/** @file
Enable SMM profile.
-Copyright (c) 2012 - 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2012 - 2023, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent @@ -587,15 +587,17 @@ InitPaging (
}
SizeOfMemorySpace = HighBitSet64 (gPhyMask) + 1;
+ ASSERT (SizeOfMemorySpace <= 52);
+
//
- // Calculate the table entries of PML4E and PDPTE.
+ // Calculate the table entries of PML5E, PML4E and PDPTE.
//
NumberOfPml5Entries = 1;
- if (SizeOfMemorySpace > 48) {
+ if (Enable5LevelPaging && (SizeOfMemorySpace > 48)) {
NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48);
- SizeOfMemorySpace = 48;
}
+ SizeOfMemorySpace = SizeOfMemorySpace > 48 ? 48 : SizeOfMemorySpace;
NumberOfPml4Entries = 1;
if (SizeOfMemorySpace > 39) {
NumberOfPml4Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 39);
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-12 12:11 ` [edk2-devel] " Zeng, Star
@ 2023-01-17 8:44 ` Wu, Jiaxin
0 siblings, 0 replies; 12+ messages in thread
From: Wu, Jiaxin @ 2023-01-17 8:44 UTC (permalink / raw)
To: devel@edk2.groups.io, Zeng, Star, Liu, Zhiguang
Cc: Ni, Ray, Kumar, Rahul R, Dong, Eric, Tan, Dun
Reviewed-by: Wu, Jiaxin <jiaxin.wu@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zeng,
> Star
> Sent: Thursday, January 12, 2023 8:12 PM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Tan, Dun <dun.tan@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when
> InitPaging
>
> Reviewed-by: Star Zeng <star.zeng@intel.com>
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Zhiguang Liu
> Sent: Wednesday, January 4, 2023 1:41 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when
> InitPaging
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4246
>
> In function InitPaging, NumberOfPml5Entries is calculated by below code
> NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48); If
> the SizeOfMemorySpace is larger than 48, NumberOfPml5Entries will be
> larger than 1. However, this doesn't make sense if the hardware doesn't
> support 5 level page table.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> index c1efda7126..c597b39b8c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> @@ -1,7 +1,7 @@
> /** @file
> Enable SMM profile.
>
> -Copyright (c) 2012 - 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2012 - 2023, Intel Corporation. All rights reserved.<BR>
> Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent @@ -587,15 +587,17 @@
> InitPaging (
> }
>
> SizeOfMemorySpace = HighBitSet64 (gPhyMask) + 1;
> + ASSERT (SizeOfMemorySpace <= 52);
> +
> //
> - // Calculate the table entries of PML4E and PDPTE.
> + // Calculate the table entries of PML5E, PML4E and PDPTE.
> //
> NumberOfPml5Entries = 1;
> - if (SizeOfMemorySpace > 48) {
> + if (Enable5LevelPaging && (SizeOfMemorySpace > 48)) {
> NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48);
> - SizeOfMemorySpace = 48;
> }
>
> + SizeOfMemorySpace = SizeOfMemorySpace > 48 ? 48 :
> SizeOfMemorySpace;
> NumberOfPml4Entries = 1;
> if (SizeOfMemorySpace > 39) {
> NumberOfPml4Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 39);
> --
> 2.31.1.windows.1
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-04 5:41 [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging Zhiguang Liu
2023-01-12 12:11 ` [edk2-devel] " Zeng, Star
@ 2023-01-17 9:01 ` Ni, Ray
2023-01-17 9:02 ` Ni, Ray
2 siblings, 0 replies; 12+ messages in thread
From: Ni, Ray @ 2023-01-17 9:01 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Kumar, Rahul R, Dong, Eric
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Wednesday, January 4, 2023 1:41 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Dong,
> Eric <eric.dong@intel.com>
> Subject: [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4246
>
> In function InitPaging, NumberOfPml5Entries is calculated by below code
> NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48);
> If the SizeOfMemorySpace is larger than 48, NumberOfPml5Entries will be
> larger than 1. However, this doesn't make sense if the hardware doesn't
> support 5 level page table.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> index c1efda7126..c597b39b8c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> @@ -1,7 +1,7 @@
> /** @file
> Enable SMM profile.
>
> -Copyright (c) 2012 - 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2012 - 2023, Intel Corporation. All rights reserved.<BR>
> Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -587,15 +587,17 @@ InitPaging (
> }
>
> SizeOfMemorySpace = HighBitSet64 (gPhyMask) + 1;
> + ASSERT (SizeOfMemorySpace <= 52);
> +
> //
> - // Calculate the table entries of PML4E and PDPTE.
> + // Calculate the table entries of PML5E, PML4E and PDPTE.
> //
> NumberOfPml5Entries = 1;
> - if (SizeOfMemorySpace > 48) {
> + if (Enable5LevelPaging && (SizeOfMemorySpace > 48)) {
> NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48);
> - SizeOfMemorySpace = 48;
> }
>
> + SizeOfMemorySpace = SizeOfMemorySpace > 48 ? 48 : SizeOfMemorySpace;
> NumberOfPml4Entries = 1;
> if (SizeOfMemorySpace > 39) {
> NumberOfPml4Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 39);
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-04 5:41 [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging Zhiguang Liu
2023-01-12 12:11 ` [edk2-devel] " Zeng, Star
2023-01-17 9:01 ` Ni, Ray
@ 2023-01-17 9:02 ` Ni, Ray
2023-01-17 12:13 ` Gerd Hoffmann
2 siblings, 1 reply; 12+ messages in thread
From: Ni, Ray @ 2023-01-17 9:02 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io
Cc: Kumar, Rahul R, Dong, Eric, 'Gerd Hoffmann'
+ Gerd.
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Wednesday, January 4, 2023 1:41 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Dong,
> Eric <eric.dong@intel.com>
> Subject: [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4246
>
> In function InitPaging, NumberOfPml5Entries is calculated by below code
> NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48);
> If the SizeOfMemorySpace is larger than 48, NumberOfPml5Entries will be
> larger than 1. However, this doesn't make sense if the hardware doesn't
> support 5 level page table.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> index c1efda7126..c597b39b8c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> @@ -1,7 +1,7 @@
> /** @file
> Enable SMM profile.
>
> -Copyright (c) 2012 - 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2012 - 2023, Intel Corporation. All rights reserved.<BR>
> Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -587,15 +587,17 @@ InitPaging (
> }
>
> SizeOfMemorySpace = HighBitSet64 (gPhyMask) + 1;
> + ASSERT (SizeOfMemorySpace <= 52);
> +
> //
> - // Calculate the table entries of PML4E and PDPTE.
> + // Calculate the table entries of PML5E, PML4E and PDPTE.
> //
> NumberOfPml5Entries = 1;
> - if (SizeOfMemorySpace > 48) {
> + if (Enable5LevelPaging && (SizeOfMemorySpace > 48)) {
> NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48);
> - SizeOfMemorySpace = 48;
> }
>
> + SizeOfMemorySpace = SizeOfMemorySpace > 48 ? 48 : SizeOfMemorySpace;
> NumberOfPml4Entries = 1;
> if (SizeOfMemorySpace > 39) {
> NumberOfPml4Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 39);
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-17 9:02 ` Ni, Ray
@ 2023-01-17 12:13 ` Gerd Hoffmann
2023-01-17 12:48 ` [edk2-devel] " Ni, Ray
0 siblings, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2023-01-17 12:13 UTC (permalink / raw)
To: Ni, Ray; +Cc: Liu, Zhiguang, devel@edk2.groups.io, Kumar, Rahul R, Dong, Eric
On Tue, Jan 17, 2023 at 09:02:01AM +0000, Ni, Ray wrote:
> + Gerd.
>
> > -----Original Message-----
> > From: Liu, Zhiguang <zhiguang.liu@intel.com>
> > Sent: Wednesday, January 4, 2023 1:41 PM
> > To: devel@edk2.groups.io
> > Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Dong,
> > Eric <eric.dong@intel.com>
> > Subject: [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4246
> >
> > In function InitPaging, NumberOfPml5Entries is calculated by below code
> > NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48);
> > If the SizeOfMemorySpace is larger than 48, NumberOfPml5Entries will be
> > larger than 1. However, this doesn't make sense if the hardware doesn't
> > support 5 level page table.
> > + ASSERT (SizeOfMemorySpace <= 52);
> > +
> > //
> > - // Calculate the table entries of PML4E and PDPTE.
> > + // Calculate the table entries of PML5E, PML4E and PDPTE.
> > //
> > NumberOfPml5Entries = 1;
> > - if (SizeOfMemorySpace > 48) {
> > + if (Enable5LevelPaging && (SizeOfMemorySpace > 48)) {
> > NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48);
> > - SizeOfMemorySpace = 48;
> > }
> >
> > + SizeOfMemorySpace = SizeOfMemorySpace > 48 ? 48 : SizeOfMemorySpace;
if (SizeOfMemorySpace > 48) {
if (Enable5LevelPaging) {
NumberOfPml5Entries = ...
}
SizeOfMemorySpace = 48
}
That is a much more readable version.
The only effect I can see is that this avoids creating page tables which
would not be used anyway.
Can you explain where the hangs mentioned in the subject line are coming
from and why the patch fixes them?
take care,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-17 12:13 ` Gerd Hoffmann
@ 2023-01-17 12:48 ` Ni, Ray
2023-01-18 1:13 ` Zhiguang Liu
0 siblings, 1 reply; 12+ messages in thread
From: Ni, Ray @ 2023-01-17 12:48 UTC (permalink / raw)
To: devel@edk2.groups.io, kraxel@redhat.com
Cc: Liu, Zhiguang, Kumar, Rahul R, Dong, Eric
> > > + ASSERT (SizeOfMemorySpace <= 52);
> > > +
> > > //
> > > - // Calculate the table entries of PML4E and PDPTE.
> > > + // Calculate the table entries of PML5E, PML4E and PDPTE.
> > > //
> > > NumberOfPml5Entries = 1;
> > > - if (SizeOfMemorySpace > 48) {
> > > + if (Enable5LevelPaging && (SizeOfMemorySpace > 48)) {
> > > NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace - 48);
> > > - SizeOfMemorySpace = 48;
> > > }
> > >
> > > + SizeOfMemorySpace = SizeOfMemorySpace > 48 ? 48 : SizeOfMemorySpace;
>
> if (SizeOfMemorySpace > 48) {
> if (Enable5LevelPaging) {
> NumberOfPml5Entries = ...
> }
> SizeOfMemorySpace = 48
> }
>
> That is a much more readable version.
I had the same thought. New version is consistent with the logic below.
>
> The only effect I can see is that this avoids creating page tables which
> would not be used anyway.
>
> Can you explain where the hangs mentioned in the subject line are coming
> from and why the patch fixes them?
>
> take care,
> Gerd
>
>
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-17 12:48 ` [edk2-devel] " Ni, Ray
@ 2023-01-18 1:13 ` Zhiguang Liu
2023-01-18 8:53 ` Gerd Hoffmann
0 siblings, 1 reply; 12+ messages in thread
From: Zhiguang Liu @ 2023-01-18 1:13 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, kraxel@redhat.com
Cc: Kumar, Rahul R, Dong, Eric, Zeng, Star, Wu, Jiaxin
Thanks all for reviewing, and I will send a new version to address the comment.
As for Gerd's question, let me explain.
Let's see one example, that the CPU has SizeOfMemorySpace >48, but the CPU doesn't enable 5 level paging.
The purpose of the current function InitPaging is to modify existing page table. To use the same logic to handle both 5 level and 4 level paging, for 4 level paging, the logic will create a false 5 level paging entry to treat it like a 5 level page table. This way, the number of 5 level paging should always be one. If we use SizeOfMemorySpace to calculate the 5 level paging entry count, we will get number more than one. However, as I just mentioned, we only create one false 5 level paging entry, system may hang when we try to access the second 5 level paging entry. This patch fixes the issue by always letting the number of 5 level paging entry as one if 5 level paging is disabled.
Thanks
Zhiguang
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, January 17, 2023 8:49 PM
> To: devel@edk2.groups.io; kraxel@redhat.com
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when
> InitPaging
>
> > > > + ASSERT (SizeOfMemorySpace <= 52);
> > > > +
> > > > //
> > > > - // Calculate the table entries of PML4E and PDPTE.
> > > > + // Calculate the table entries of PML5E, PML4E and PDPTE.
> > > > //
> > > > NumberOfPml5Entries = 1;
> > > > - if (SizeOfMemorySpace > 48) {
> > > > + if (Enable5LevelPaging && (SizeOfMemorySpace > 48)) {
> > > > NumberOfPml5Entries = (UINTN)LShiftU64 (1, SizeOfMemorySpace -
> 48);
> > > > - SizeOfMemorySpace = 48;
> > > > }
> > > >
> > > > + SizeOfMemorySpace = SizeOfMemorySpace > 48 ? 48 :
> SizeOfMemorySpace;
> >
> > if (SizeOfMemorySpace > 48) {
> > if (Enable5LevelPaging) {
> > NumberOfPml5Entries = ...
> > }
> > SizeOfMemorySpace = 48
> > }
> >
> > That is a much more readable version.
>
> I had the same thought. New version is consistent with the logic below.
>
>
> >
> > The only effect I can see is that this avoids creating page tables
> > which would not be used anyway.
> >
> > Can you explain where the hangs mentioned in the subject line are
> > coming from and why the patch fixes them?
>
>
> >
> > take care,
> > Gerd
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-18 1:13 ` Zhiguang Liu
@ 2023-01-18 8:53 ` Gerd Hoffmann
2023-01-18 9:12 ` Zhiguang Liu
0 siblings, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2023-01-18 8:53 UTC (permalink / raw)
To: devel, zhiguang.liu
Cc: Ni, Ray, Kumar, Rahul R, Dong, Eric, Zeng, Star, Wu, Jiaxin
On Wed, Jan 18, 2023 at 01:13:43AM +0000, Zhiguang Liu wrote:
> Thanks all for reviewing, and I will send a new version to address the comment.
>
> As for Gerd's question, let me explain.
> Let's see one example, that the CPU has SizeOfMemorySpace >48, but the CPU doesn't enable 5 level paging.
> The purpose of the current function InitPaging is to modify existing
> page table. To use the same logic to handle both 5 level and 4 level
> paging, for 4 level paging, the logic will create a false 5 level
> paging entry to treat it like a 5 level page table.
Yes. Same for 3-level paging btw. There are always page tables for 5
levels, but the higher levels might be unused.
> This way, the
> number of 5 level paging should always be one. If we use
> SizeOfMemorySpace to calculate the 5 level paging entry count, we will
> get number more than one. However, as I just mentioned, we only
> create one false 5 level paging entry, system may hang when we try to
> access the second 5 level paging entry.
If 5-level paging is turned off the CPU should not see what you are
doing with the page tables for the second (and higher) 5-level entry.
So, limiting the number of 5-level entries does make sense. Higher
entries are not used, so it's pointless work.
But that doesn't answer the question: Why does that fix the system
hanging? I just can't see a reason for that when looking through the
InitPaging code. I suspect this might hide a bug somewhere else.
Related: We got UefiCpuPkg/Library/CpuPageTableLib last year, can this
be used instead?
take care,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-18 8:53 ` Gerd Hoffmann
@ 2023-01-18 9:12 ` Zhiguang Liu
2023-01-18 10:10 ` Gerd Hoffmann
0 siblings, 1 reply; 12+ messages in thread
From: Zhiguang Liu @ 2023-01-18 9:12 UTC (permalink / raw)
To: kraxel@redhat.com, devel@edk2.groups.io
Cc: Ni, Ray, Kumar, Rahul R, Dong, Eric, Zeng, Star, Wu, Jiaxin
Hi Gerd,
Let's check the code in InitPaging.
If 5LevelPaging is disabled, Pml5 points to a local variable. Pml5[1] shouldn't be used.
UINT64 Pml5Entry;
UINT64 *Pml5;
if (!Enable5LevelPaging) {
Pml5Entry = (UINTN)mSmmProfileCr3 | IA32_PG_P;
Pml5 = &Pml5Entry;
However, if NumberOfPml5Entries is larger than 1, below code will access Pml5[1], which may cause unexpected future code flow.
for (Pml5Index = 0; Pml5Index < NumberOfPml5Entries; Pml5Index++) {
if ((Pml5[Pml5Index] & IA32_PG_P) == 0) {
Could this can answer your question? Please let me know if you still have concern.
And for the CpuPageTableLib, I think the API don't provide the interface to split 2MB-page page table into 4KB-page, which is the function wants to do.
Thanks
Zhiguang
> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Wednesday, January 18, 2023 4:54 PM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when
> InitPaging
>
> On Wed, Jan 18, 2023 at 01:13:43AM +0000, Zhiguang Liu wrote:
> > Thanks all for reviewing, and I will send a new version to address the comment.
> >
> > As for Gerd's question, let me explain.
> > Let's see one example, that the CPU has SizeOfMemorySpace >48, but the CPU
> doesn't enable 5 level paging.
> > The purpose of the current function InitPaging is to modify existing
> > page table. To use the same logic to handle both 5 level and 4 level
> > paging, for 4 level paging, the logic will create a false 5 level
> > paging entry to treat it like a 5 level page table.
>
> Yes. Same for 3-level paging btw. There are always page tables for 5 levels, but
> the higher levels might be unused.
>
> > This way, the
> > number of 5 level paging should always be one. If we use
> > SizeOfMemorySpace to calculate the 5 level paging entry count, we will
> > get number more than one. However, as I just mentioned, we only
> > create one false 5 level paging entry, system may hang when we try to
> > access the second 5 level paging entry.
>
> If 5-level paging is turned off the CPU should not see what you are doing with
> the page tables for the second (and higher) 5-level entry.
>
> So, limiting the number of 5-level entries does make sense. Higher entries are
> not used, so it's pointless work.
>
> But that doesn't answer the question: Why does that fix the system hanging? I
> just can't see a reason for that when looking through the InitPaging code. I
> suspect this might hide a bug somewhere else.
>
> Related: We got UefiCpuPkg/Library/CpuPageTableLib last year, can this be
> used instead?
>
> take care,
> Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-18 9:12 ` Zhiguang Liu
@ 2023-01-18 10:10 ` Gerd Hoffmann
2023-01-18 15:27 ` Ni, Ray
0 siblings, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2023-01-18 10:10 UTC (permalink / raw)
To: devel, zhiguang.liu
Cc: Ni, Ray, Kumar, Rahul R, Dong, Eric, Zeng, Star, Wu, Jiaxin
On Wed, Jan 18, 2023 at 09:12:09AM +0000, Zhiguang Liu wrote:
> Hi Gerd,
>
> Let's check the code in InitPaging.
> If 5LevelPaging is disabled, Pml5 points to a local variable. Pml5[1] shouldn't be used.
>
> UINT64 Pml5Entry;
> UINT64 *Pml5;
> if (!Enable5LevelPaging) {
> Pml5Entry = (UINTN)mSmmProfileCr3 | IA32_PG_P;
> Pml5 = &Pml5Entry;
Oh, it's just a dummy entry on the stack, not an dummy page table.
Missed that detail.
So writing entry #2 and higher smashes the stack. That certainly
explains why the code hangs.
> And for the CpuPageTableLib, I think the API don't provide the
> interface to split 2MB-page page table into 4KB-page, which is the
> function wants to do.
I think that is handled by the library automatically. You can request
address ranges being mapped with specific attributes (such as NX set),
and the library will transparently split pages for you if needed.
take care,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix SMM code hangs when InitPaging
2023-01-18 10:10 ` Gerd Hoffmann
@ 2023-01-18 15:27 ` Ni, Ray
0 siblings, 0 replies; 12+ messages in thread
From: Ni, Ray @ 2023-01-18 15:27 UTC (permalink / raw)
To: kraxel@redhat.com, devel@edk2.groups.io, Liu, Zhiguang
Cc: Kumar, Rahul R, Dong, Eric, Zeng, Star, Wu, Jiaxin
>
> > And for the CpuPageTableLib, I think the API don't provide the
> > interface to split 2MB-page page table into 4KB-page, which is the
> > function wants to do.
>
> I think that is handled by the library automatically. You can request
> address ranges being mapped with specific attributes (such as NX set),
> and the library will transparently split pages for you if needed.
>
Replacing today's duplicated page table manipulation logic with PageTableLib
is in the todo list (as I said earlier creating a page table library was in the todo
list).
The bug is critical and needs to be fixed asap.
Using lib will be done in future.
That will be great if the someone from the community can help on that part.
Thanks,
Ray
^ permalink raw reply [flat|nested] 12+ messages in thread