public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB
@ 2021-03-16  3:33 Ni, Ray
  2021-03-16  3:33 ` [PATCH 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard Ni, Ray
  2021-03-16  3:33 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB Ni, Ray
  0 siblings, 2 replies; 8+ messages in thread
From: Ni, Ray @ 2021-03-16  3:33 UTC (permalink / raw)
  To: devel

Ray Ni (2):
  UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard
  UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB

 UefiCpuPkg/CpuDxe/CpuGdt.c | 42 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 16 deletions(-)

-- 
2.27.0.windows.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard
  2021-03-16  3:33 [PATCH 0/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB Ni, Ray
@ 2021-03-16  3:33 ` Ni, Ray
  2021-03-16 13:58   ` Dong, Eric
  2021-03-16 16:30   ` Laszlo Ersek
  2021-03-16  3:33 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB Ni, Ray
  1 sibling, 2 replies; 8+ messages in thread
From: Ni, Ray @ 2021-03-16  3:33 UTC (permalink / raw)
  To: devel; +Cc: Eric Dong, Laszlo Ersek, Rahul Kumar

The change doesn't impact any functionality.

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 | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c
index a1ab543f2d..322ce87142 100644
--- a/UefiCpuPkg/CpuDxe/CpuGdt.c
+++ b/UefiCpuPkg/CpuDxe/CpuGdt.c
@@ -2,7 +2,7 @@
   C based implementation of IA32 interrupt handling only
   requiring a minimal assembly interrupt entry point.
 
-  Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -13,7 +13,7 @@
 //
 // Global descriptor table (GDT) Template
 //
-STATIC GDT_ENTRIES GdtTemplate = {
+STATIC GDT_ENTRIES gGdtTemplate = {
   //
   // NULL_SEL
   //
@@ -124,32 +124,31 @@ InitGlobalDescriptorTable (
   VOID
   )
 {
-  GDT_ENTRIES *gdt;
-  IA32_DESCRIPTOR gdtPtr;
+  GDT_ENTRIES           *Gdt;
+  IA32_DESCRIPTOR       Gdtr;
 
   //
   // Allocate Runtime Data for the GDT
   //
-  gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
-  ASSERT (gdt != NULL);
-  gdt = ALIGN_POINTER (gdt, 8);
+  Gdt = AllocateRuntimePool (sizeof (gGdtTemplate) + 8);
+  ASSERT (Gdt != NULL);
+  Gdt = ALIGN_POINTER (Gdt, 8);
 
   //
   // Initialize all GDT entries
   //
-  CopyMem (gdt, &GdtTemplate, sizeof (GdtTemplate));
+  CopyMem (Gdt, &gGdtTemplate, sizeof (gGdtTemplate));
 
   //
   // Write GDT register
   //
-  gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
-  gdtPtr.Limit = (UINT16) (sizeof (GdtTemplate) - 1);
-  AsmWriteGdtr (&gdtPtr);
+  Gdtr.Base  = (UINT32) (UINTN) Gdt;
+  Gdtr.Limit = (UINT16) (sizeof (gGdtTemplate) - 1);
+  AsmWriteGdtr (&Gdtr);
 
   //
   // Update selector (segment) registers base on new GDT
   //
-  SetCodeSelector ((UINT16)CPU_CODE_SEL);
-  SetDataSelectors ((UINT16)CPU_DATA_SEL);
+  SetCodeSelector  ((UINT16) CPU_CODE_SEL);
+  SetDataSelectors ((UINT16) CPU_DATA_SEL);
 }
-
-- 
2.27.0.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB
  2021-03-16  3:33 [PATCH 0/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB Ni, Ray
  2021-03-16  3:33 ` [PATCH 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard Ni, Ray
@ 2021-03-16  3:33 ` Ni, Ray
  2021-03-16 13:59   ` Dong, Eric
  2021-03-16 16:35   ` Laszlo Ersek
  1 sibling, 2 replies; 8+ messages in thread
From: Ni, Ray @ 2021-03-16  3:33 UTC (permalink / raw)
  To: devel; +Cc: Eric Dong, Laszlo Ersek, Rahul Kumar

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard
  2021-03-16  3:33 ` [PATCH 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard Ni, Ray
@ 2021-03-16 13:58   ` Dong, Eric
  2021-03-16 16:30   ` Laszlo Ersek
  1 sibling, 0 replies; 8+ messages in thread
From: Dong, Eric @ 2021-03-16 13:58 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek, Kumar, Rahul1

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 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard

The change doesn't impact any functionality.

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 | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c
index a1ab543f2d..322ce87142 100644
--- a/UefiCpuPkg/CpuDxe/CpuGdt.c
+++ b/UefiCpuPkg/CpuDxe/CpuGdt.c
@@ -2,7 +2,7 @@
   C based implementation of IA32 interrupt handling only

   requiring a minimal assembly interrupt entry point.

 

-  Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>

+  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 

 **/

@@ -13,7 +13,7 @@
 //

 // Global descriptor table (GDT) Template

 //

-STATIC GDT_ENTRIES GdtTemplate = {

+STATIC GDT_ENTRIES gGdtTemplate = {

   //

   // NULL_SEL

   //

@@ -124,32 +124,31 @@ InitGlobalDescriptorTable (
   VOID

   )

 {

-  GDT_ENTRIES *gdt;

-  IA32_DESCRIPTOR gdtPtr;

+  GDT_ENTRIES           *Gdt;

+  IA32_DESCRIPTOR       Gdtr;

 

   //

   // Allocate Runtime Data for the GDT

   //

-  gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);

-  ASSERT (gdt != NULL);

-  gdt = ALIGN_POINTER (gdt, 8);

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

+  ASSERT (Gdt != NULL);

+  Gdt = ALIGN_POINTER (Gdt, 8);

 

   //

   // Initialize all GDT entries

   //

-  CopyMem (gdt, &GdtTemplate, sizeof (GdtTemplate));

+  CopyMem (Gdt, &gGdtTemplate, sizeof (gGdtTemplate));

 

   //

   // Write GDT register

   //

-  gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;

-  gdtPtr.Limit = (UINT16) (sizeof (GdtTemplate) - 1);

-  AsmWriteGdtr (&gdtPtr);

+  Gdtr.Base  = (UINT32) (UINTN) Gdt;

+  Gdtr.Limit = (UINT16) (sizeof (gGdtTemplate) - 1);

+  AsmWriteGdtr (&Gdtr);

 

   //

   // Update selector (segment) registers base on new GDT

   //

-  SetCodeSelector ((UINT16)CPU_CODE_SEL);

-  SetDataSelectors ((UINT16)CPU_DATA_SEL);

+  SetCodeSelector  ((UINT16) CPU_CODE_SEL);

+  SetDataSelectors ((UINT16) CPU_DATA_SEL);

 }

-

-- 
2.27.0.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB
  2021-03-16  3:33 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB Ni, Ray
@ 2021-03-16 13:59   ` Dong, Eric
  2021-03-16 16:35   ` Laszlo Ersek
  1 sibling, 0 replies; 8+ messages in thread
From: Dong, Eric @ 2021-03-16 13:59 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek, Kumar, Rahul1

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard
  2021-03-16  3:33 ` [PATCH 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard Ni, Ray
  2021-03-16 13:58   ` Dong, Eric
@ 2021-03-16 16:30   ` Laszlo Ersek
  2021-03-17  2:47     ` Ni, Ray
  1 sibling, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2021-03-16 16:30 UTC (permalink / raw)
  To: Ray Ni, devel; +Cc: Eric Dong, Rahul Kumar

On 03/16/21 04:33, Ray Ni wrote:
> The change doesn't impact any functionality.
> 
> 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 | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c
> index a1ab543f2d..322ce87142 100644
> --- a/UefiCpuPkg/CpuDxe/CpuGdt.c
> +++ b/UefiCpuPkg/CpuDxe/CpuGdt.c
> @@ -2,7 +2,7 @@
>    C based implementation of IA32 interrupt handling only
>    requiring a minimal assembly interrupt entry point.
>  
> -  Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -13,7 +13,7 @@
>  //
>  // Global descriptor table (GDT) Template
>  //
> -STATIC GDT_ENTRIES GdtTemplate = {
> +STATIC GDT_ENTRIES gGdtTemplate = {
>    //
>    // NULL_SEL
>    //
> @@ -124,32 +124,31 @@ InitGlobalDescriptorTable (
>    VOID
>    )
>  {
> -  GDT_ENTRIES *gdt;
> -  IA32_DESCRIPTOR gdtPtr;
> +  GDT_ENTRIES           *Gdt;
> +  IA32_DESCRIPTOR       Gdtr;
>  
>    //
>    // Allocate Runtime Data for the GDT
>    //
> -  gdt = AllocateRuntimePool (sizeof (GdtTemplate) + 8);
> -  ASSERT (gdt != NULL);
> -  gdt = ALIGN_POINTER (gdt, 8);
> +  Gdt = AllocateRuntimePool (sizeof (gGdtTemplate) + 8);
> +  ASSERT (Gdt != NULL);
> +  Gdt = ALIGN_POINTER (Gdt, 8);
>  
>    //
>    // Initialize all GDT entries
>    //
> -  CopyMem (gdt, &GdtTemplate, sizeof (GdtTemplate));
> +  CopyMem (Gdt, &gGdtTemplate, sizeof (gGdtTemplate));
>  
>    //
>    // Write GDT register
>    //
> -  gdtPtr.Base = (UINT32)(UINTN)(VOID*) gdt;
> -  gdtPtr.Limit = (UINT16) (sizeof (GdtTemplate) - 1);
> -  AsmWriteGdtr (&gdtPtr);
> +  Gdtr.Base  = (UINT32) (UINTN) Gdt;
> +  Gdtr.Limit = (UINT16) (sizeof (gGdtTemplate) - 1);
> +  AsmWriteGdtr (&Gdtr);
>  
>    //
>    // Update selector (segment) registers base on new GDT
>    //
> -  SetCodeSelector ((UINT16)CPU_CODE_SEL);
> -  SetDataSelectors ((UINT16)CPU_DATA_SEL);
> +  SetCodeSelector  ((UINT16) CPU_CODE_SEL);
> +  SetDataSelectors ((UINT16) CPU_DATA_SEL);
>  }
> -
> 

(1) I think "mGdtTemplate" would be a better name than "gGdtTemplate". I
think the "g" prefix is used when an object is identical for all
firmware modules (such as named GUIDs, for example).

(2) I think the last hunk does not belong in this patch -- more
precisely, I *disagree* with the last hunk. Inserting a space in the
middle of a typecast, after the parenthesized typename, is a bad
practice in edk2; it is error prone and suggests that typecasts have low
binding power (when in reality they have almost the strongest binding).

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB
  2021-03-16  3:33 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB Ni, Ray
  2021-03-16 13:59   ` Dong, Eric
@ 2021-03-16 16:35   ` Laszlo Ersek
  1 sibling, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2021-03-16 16:35 UTC (permalink / raw)
  To: Ray Ni, devel; +Cc: Eric Dong, Rahul Kumar

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard
  2021-03-16 16:30   ` Laszlo Ersek
@ 2021-03-17  2:47     ` Ni, Ray
  0 siblings, 0 replies; 8+ messages in thread
From: Ni, Ray @ 2021-03-17  2:47 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul1

> (1) I think "mGdtTemplate" would be a better name than "gGdtTemplate". I
> think the "g" prefix is used when an object is identical for all
> firmware modules (such as named GUIDs, for example).
Agree! I will change in v2.


> 
> (2) I think the last hunk does not belong in this patch -- more
> precisely, I *disagree* with the last hunk. Inserting a space in the
> middle of a typecast, after the parenthesized typename, is a bad
> practice in edk2; it is error prone and suggests that typecasts have low
> binding power (when in reality they have almost the strongest binding).

I double checked the edk2 coding standard and did find a rule for this.
That might be just my personal preference. Since I need your Ack or Rb, I will remove
this change in v2.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-03-17  2:47 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-16  3:33 [PATCH 0/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB Ni, Ray
2021-03-16  3:33 ` [PATCH 1/2] UefiCpuPkg/CpuDxe: Rename variables to follow EDKII coding standard Ni, Ray
2021-03-16 13:58   ` Dong, Eric
2021-03-16 16:30   ` Laszlo Ersek
2021-03-17  2:47     ` Ni, Ray
2021-03-16  3:33 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Guarantee GDT is below 4GB Ni, Ray
2021-03-16 13:59   ` Dong, Eric
2021-03-16 16:35   ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox