[edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB

Ni, Ray posted 2 patches 4 years, 11 months ago
There is a newer version of this series
[edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB
Posted by Ni, Ray 4 years, 11 months ago
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3233

GDT needs to be allocated below 4GB in 64bit environment
because AP needs it for entering to protected mode.
CPU running in big real mode cannot access above 4GB GDT.

But CpuDxe driver contains below code:
  gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
  .....
  gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;

The AllocateRuntimePool() may allocate memory above 4GB.
Thus, we cannot use AllocateRuntimePool (), instead,
we should use AllocatePages() to make sure GDT is below 4GB space.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuGdt.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c
index 322ce87142..98d5551702 100644
--- a/UefiCpuPkg/CpuDxe/CpuGdt.c
+++ b/UefiCpuPkg/CpuDxe/CpuGdt.c
@@ -124,15 +124,26 @@ InitGlobalDescriptorTable (
   VOID
   )
 {
+  EFI_STATUS            Status;
   GDT_ENTRIES           *Gdt;
   IA32_DESCRIPTOR       Gdtr;
+  EFI_PHYSICAL_ADDRESS  Memory;
 
   //
-  // Allocate Runtime Data for the GDT
-  //
-  Gdt = AllocateRuntimePool (sizeof (gGdtTemplate) + 8);
-  ASSERT (Gdt != NULL);
-  Gdt = ALIGN_POINTER (Gdt, 8);
+  // Allocate Runtime Data below 4GB for the GDT
+  // AP uses the same GDT when it's waken up from real mode so
+  // the GDT needs to be below 4GB.
+  //
+  Memory = SIZE_4GB - 1;
+  Status = gBS->AllocatePages (
+                  AllocateMaxAddress,
+                  EfiRuntimeServicesData,
+                  EFI_SIZE_TO_PAGES (sizeof (gGdtTemplate)),
+                  &Memory
+                  );
+  ASSERT_EFI_ERROR (Status);
+  ASSERT ((Memory != 0) && (Memory < SIZE_4GB));
+  Gdt = (GDT_ENTRIES *) (UINTN) Memory;
 
   //
   // Initialize all GDT entries
-- 
2.27.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72856): https://edk2.groups.io/g/devel/message/72856
Mute This Topic: https://groups.io/mt/81368665/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB
Posted by Dong, Eric 4 years, 10 months ago
Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Tuesday, March 16, 2021 11:34 AM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3233

GDT needs to be allocated below 4GB in 64bit environment
because AP needs it for entering to protected mode.
CPU running in big real mode cannot access above 4GB GDT.

But CpuDxe driver contains below code:
  gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
  .....
  gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;

The AllocateRuntimePool() may allocate memory above 4GB.
Thus, we cannot use AllocateRuntimePool (), instead,
we should use AllocatePages() to make sure GDT is below 4GB space.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuGdt.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c
index 322ce87142..98d5551702 100644
--- a/UefiCpuPkg/CpuDxe/CpuGdt.c
+++ b/UefiCpuPkg/CpuDxe/CpuGdt.c
@@ -124,15 +124,26 @@ InitGlobalDescriptorTable (
   VOID

   )

 {

+  EFI_STATUS            Status;

   GDT_ENTRIES           *Gdt;

   IA32_DESCRIPTOR       Gdtr;

+  EFI_PHYSICAL_ADDRESS  Memory;

 

   //

-  // Allocate Runtime Data for the GDT

-  //

-  Gdt = AllocateRuntimePool (sizeof (gGdtTemplate) + 8);

-  ASSERT (Gdt != NULL);

-  Gdt = ALIGN_POINTER (Gdt, 8);

+  // Allocate Runtime Data below 4GB for the GDT

+  // AP uses the same GDT when it's waken up from real mode so

+  // the GDT needs to be below 4GB.

+  //

+  Memory = SIZE_4GB - 1;

+  Status = gBS->AllocatePages (

+                  AllocateMaxAddress,

+                  EfiRuntimeServicesData,

+                  EFI_SIZE_TO_PAGES (sizeof (gGdtTemplate)),

+                  &Memory

+                  );

+  ASSERT_EFI_ERROR (Status);

+  ASSERT ((Memory != 0) && (Memory < SIZE_4GB));

+  Gdt = (GDT_ENTRIES *) (UINTN) Memory;

 

   //

   // Initialize all GDT entries

-- 
2.27.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72873): https://edk2.groups.io/g/devel/message/72873
Mute This Topic: https://groups.io/mt/81368665/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB
Posted by Laszlo Ersek 4 years, 10 months ago
On 03/16/21 04:33, Ray Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3233
> 
> GDT needs to be allocated below 4GB in 64bit environment
> because AP needs it for entering to protected mode.
> CPU running in big real mode cannot access above 4GB GDT.
> 
> But CpuDxe driver contains below code:
>   gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
>   .....
>   gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
> 
> The AllocateRuntimePool() may allocate memory above 4GB.
> Thus, we cannot use AllocateRuntimePool (), instead,
> we should use AllocatePages() to make sure GDT is below 4GB space.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuGdt.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c
> index 322ce87142..98d5551702 100644
> --- a/UefiCpuPkg/CpuDxe/CpuGdt.c
> +++ b/UefiCpuPkg/CpuDxe/CpuGdt.c
> @@ -124,15 +124,26 @@ InitGlobalDescriptorTable (
>    VOID
>    )
>  {
> +  EFI_STATUS            Status;
>    GDT_ENTRIES           *Gdt;
>    IA32_DESCRIPTOR       Gdtr;
> +  EFI_PHYSICAL_ADDRESS  Memory;
>  
>    //
> -  // Allocate Runtime Data for the GDT
> -  //
> -  Gdt = AllocateRuntimePool (sizeof (gGdtTemplate) + 8);
> -  ASSERT (Gdt != NULL);
> -  Gdt = ALIGN_POINTER (Gdt, 8);
> +  // Allocate Runtime Data below 4GB for the GDT
> +  // AP uses the same GDT when it's waken up from real mode so

(1) s/waken/woken/

> +  // the GDT needs to be below 4GB.
> +  //
> +  Memory = SIZE_4GB - 1;
> +  Status = gBS->AllocatePages (
> +                  AllocateMaxAddress,
> +                  EfiRuntimeServicesData,
> +                  EFI_SIZE_TO_PAGES (sizeof (gGdtTemplate)),
> +                  &Memory
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +  ASSERT ((Memory != 0) && (Memory < SIZE_4GB));

(2) Can we drop the (Memory < SIZE_4GB) sub-condition? That should be
guaranteed by the UEFI spec.

> +  Gdt = (GDT_ENTRIES *) (UINTN) Memory;
>  
>    //
>    // Initialize all GDT entries
> 

Anyway...

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72920): https://edk2.groups.io/g/devel/message/72920
Mute This Topic: https://groups.io/mt/81368665/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-