[edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

[edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

[duntan]

When creating smm page table, limit maximum
supported physical address bits returned by
CalculateMaximumSupportAddress() to 47 if
5-Level Paging is disabled.
When 5-Level Paging is disabled and the
PhysicalAddressBits retrived from CPU HOB or
CpuId is bigger than 47, and since virtual
addresses are sign-extended, only [0, 2^47-1]
range in 52-bit physical address is mapped
in page table.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Reviewed-by: 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>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index ddd9be66b5..35c282a771 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -137,11 +137,13 @@ GetSubEntriesNum (
 /**
   Calculate the maximum support address.
 
+  @param[in] Is5LevelPagingNeeded    If 5-level paging enabling is needed.
+
   @return the maximum support address.
 **/
 UINT8
 CalculateMaximumSupportAddress (
-  VOID
+  BOOLEAN  Is5LevelPagingNeeded
   )
 {
   UINT32  RegEax;
@@ -164,6 +166,15 @@ CalculateMaximumSupportAddress (
     }
   }
 
+  //
+  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page table
+  // when 5-Level Paging is disabled.
+  //
+  ASSERT (PhysicalAddressBits <= 52);
+  if (!Is5LevelPagingNeeded && (PhysicalAddressBits > 47)) {
+    PhysicalAddressBits = 47;
+  }
+
   return PhysicalAddressBits;
 }
 
@@ -197,7 +208,7 @@ SmmInitPageTable (
   mCpuSmmRestrictedMemoryAccess = PcdGetBool (PcdCpuSmmRestrictedMemoryAccess);
   m1GPageTableSupport           = Is1GPageSupport ();
   m5LevelPagingNeeded           = Is5LevelPagingNeeded ();
-  mPhysicalAddressBits          = CalculateMaximumSupportAddress ();
+  mPhysicalAddressBits          = CalculateMaximumSupportAddress (m5LevelPagingNeeded);
   PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded, 1);
   if (m5LevelPagingNeeded) {
     mPagingMode = m1GPageTableSupport ? Paging5Level1GB : Paging5Level;
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113600): https://edk2.groups.io/g/devel/message/113600
Mute This Topic: https://groups.io/mt/103658816/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

[Gerd Hoffmann]

On Thu, Jan 11, 2024 at 04:59:47PM +0800, Dun Tan wrote:
> When creating smm page table, limit maximum
> supported physical address bits returned by
> CalculateMaximumSupportAddress() to 47 if
> 5-Level Paging is disabled.
> When 5-Level Paging is disabled and the
> PhysicalAddressBits retrived from CPU HOB or
> CpuId is bigger than 47, and since virtual
> addresses are sign-extended, only [0, 2^47-1]
> range in 52-bit physical address is mapped
> in page table.

> +  //
> +  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page table
> +  // when 5-Level Paging is disabled.
> +  //
> +  ASSERT (PhysicalAddressBits <= 52);
> +  if (!Is5LevelPagingNeeded && (PhysicalAddressBits > 47)) {
> +    PhysicalAddressBits = 47;
> +  }

The code change is fine but the comment should be more verbose and
explain the why 47 not 48 is used here.  The discussion on the patch
clearly showed that the technical background is not obvious ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113613): https://edk2.groups.io/g/devel/message/113613
Mute This Topic: https://groups.io/mt/103658816/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

[duntan]

Hi Gerd,

I explain the reason why 47 here is since virtual addresses are sign-extended in the commit message.
About the technical background, I also mentioned in the commit message " When 5-Level Paging is disabled and the PhysicalAddressBits retrived  from CPU HOB or CpuId is bigger than 47". Could you please provide more detailed suggestion about the commit message?

Or can we merge the code firstly? Then I'll raise another PR to make the comments around the code more detailed as we want.

Thanks,
Dun

-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com> 
Sent: Thursday, January 11, 2024 6:21 PM
To: Tan, Dun <dun.tan@intel.com>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: Re: [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

On Thu, Jan 11, 2024 at 04:59:47PM +0800, Dun Tan wrote:
> When creating smm page table, limit maximum supported physical address 
> bits returned by
> CalculateMaximumSupportAddress() to 47 if 5-Level Paging is disabled.
> When 5-Level Paging is disabled and the PhysicalAddressBits retrived 
> from CPU HOB or CpuId is bigger than 47, and since virtual addresses 
> are sign-extended, only [0, 2^47-1] range in 52-bit physical address 
> is mapped in page table.

> +  //
> +  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page 
> + table  // when 5-Level Paging is disabled.
> +  //
> +  ASSERT (PhysicalAddressBits <= 52);  if (!Is5LevelPagingNeeded && 
> + (PhysicalAddressBits > 47)) {
> +    PhysicalAddressBits = 47;
> +  }

The code change is fine but the comment should be more verbose and explain the why 47 not 48 is used here.  The discussion on the patch clearly showed that the technical background is not obvious ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113636): https://edk2.groups.io/g/devel/message/113636
Mute This Topic: https://groups.io/mt/103658816/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

[duntan]

Hi Gerd,

Could you help to review this V3 patch? The related code and comments has been modified based on your previous comments.
Please ignore the V2 patch set. There was a typo error in the V2 patch and was corrected in V3 patch.

Thanks,
Dun

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
Sent: Thursday, January 11, 2024 5:00 PM
To: devel@edk2.groups.io
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>
Subject: [edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

When creating smm page table, limit maximum supported physical address bits returned by
CalculateMaximumSupportAddress() to 47 if 5-Level Paging is disabled.
When 5-Level Paging is disabled and the
PhysicalAddressBits retrived from CPU HOB or CpuId is bigger than 47, and since virtual addresses are sign-extended, only [0, 2^47-1] range in 52-bit physical address is mapped in page table.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Reviewed-by: 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>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index ddd9be66b5..35c282a771 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -137,11 +137,13 @@ GetSubEntriesNum (
 /**
   Calculate the maximum support address.
 
+  @param[in] Is5LevelPagingNeeded    If 5-level paging enabling is needed.
+
   @return the maximum support address.
 **/
 UINT8
 CalculateMaximumSupportAddress (
-  VOID
+  BOOLEAN  Is5LevelPagingNeeded
   )
 {
   UINT32  RegEax;
@@ -164,6 +166,15 @@ CalculateMaximumSupportAddress (
     }
   }
 
+  //
+  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page 
+ table  // when 5-Level Paging is disabled.
+  //
+  ASSERT (PhysicalAddressBits <= 52);
+  if (!Is5LevelPagingNeeded && (PhysicalAddressBits > 47)) {
+    PhysicalAddressBits = 47;
+  }
+
   return PhysicalAddressBits;
 }
 
@@ -197,7 +208,7 @@ SmmInitPageTable (
   mCpuSmmRestrictedMemoryAccess = PcdGetBool (PcdCpuSmmRestrictedMemoryAccess);
   m1GPageTableSupport           = Is1GPageSupport ();
   m5LevelPagingNeeded           = Is5LevelPagingNeeded ();
-  mPhysicalAddressBits          = CalculateMaximumSupportAddress ();
+  mPhysicalAddressBits          = CalculateMaximumSupportAddress (m5LevelPagingNeeded);
   PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded, 1);
   if (m5LevelPagingNeeded) {
     mPagingMode = m1GPageTableSupport ? Paging5Level1GB : Paging5Level;
--
2.31.1.windows.1








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113605): https://edk2.groups.io/g/devel/message/113605
Mute This Topic: https://groups.io/mt/103658816/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-