comments below On 10/29/15 21:35, Michael Kinney wrote: > The PiSmmCpuDxeSmm module makes some assumptions about GDT selectors > that are based on the GDT layout from the DxeIplPeim. This updates the > CpuDxe module to use the same GDT layout as the DxeIplPeim. > > Using a consistent GDT also improves debug experience. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Michael Kinney <[email protected]> > --- > UefiCpuPkg/CpuDxe/CpuGdt.c | 83 > ++++++++++++++++++++++++++-------------------- > UefiCpuPkg/CpuDxe/CpuGdt.h | 10 +++--- > 2 files changed, 53 insertions(+), 40 deletions(-) > > diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.c b/UefiCpuPkg/CpuDxe/CpuGdt.c > index 35a87a6..9ef2fdf 100644 > --- a/UefiCpuPkg/CpuDxe/CpuGdt.c > +++ b/UefiCpuPkg/CpuDxe/CpuGdt.c > @@ -1,10 +1,10 @@ > /** @file > C based implemention of IA32 interrupt handling only > requiring a minimal assembly interrupt entry point. > > - Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license may be > found at > http://opensource.org/licenses/bsd-license.php > > @@ -33,82 +33,93 @@ STATIC GDT_ENTRIES GdtTemplate = { > }, > // > // LINEAR_SEL > // > { > - 0x0FFFF, // limit 0xFFFFF > - 0x0, // base 0 > - 0x0, > - 0x092, // present, ring 0, data, expand-up, writable > + 0x0FFFF, // limit 15:0 > + 0x0, // base 15:0 > + 0x0, // base 23:16 > + 0x092, // present, ring 0, data, read/write > 0x0CF, // page-granular, 32-bit > 0x0, > }, > // > // LINEAR_CODE_SEL > // > { > - 0x0FFFF, // limit 0xFFFFF > - 0x0, // base 0 > - 0x0, > - 0x09A, // present, ring 0, data, expand-up, writable > + 0x0FFFF, // limit 15:0 > + 0x0, // base 15:0 > + 0x0, // base 23:16 > + 0x09F, // present, ring 0, code, execute/read, conforming, > accessed > 0x0CF, // page-granular, 32-bit > 0x0, > }, > // > // SYS_DATA_SEL > // > { > - 0x0FFFF, // limit 0xFFFFF > - 0x0, // base 0 > - 0x0, > - 0x092, // present, ring 0, data, expand-up, writable > + 0x0FFFF, // limit 15:0 > + 0x0, // base 15:0 > + 0x0, // base 23:16 > + 0x093, // present, ring 0, data, read/write, accessed > 0x0CF, // page-granular, 32-bit > 0x0, > }, > // > // SYS_CODE_SEL > // > { > - 0x0FFFF, // limit 0xFFFFF > - 0x0, // base 0 > - 0x0, > - 0x09A, // present, ring 0, data, expand-up, writable > + 0x0FFFF, // limit 15:0 > + 0x0, // base 15:0 > + 0x0, // base 23:16 > + 0x09A, // present, ring 0, code, execute/read > 0x0CF, // page-granular, 32-bit > 0x0, > }, > // > - // LINEAR_CODE64_SEL > + // SPARE4_SEL > // > { > - 0x0FFFF, // limit 0xFFFFF > - 0x0, // base 0 > - 0x0, > - 0x09B, // present, ring 0, code, expand-up, writable > - 0x0AF, // LimitHigh (CS.L=1, CS.D=0) > - 0x0, // base (high) > + 0x0, // limit 15:0 > + 0x0, // base 15:0 > + 0x0, // base 23:16 > + 0x0, // type > + 0x0, // limit 19:16, flags > + 0x0, // base 31:24 > }, > // > - // SPARE4_SEL > + // LINEAR_DATA64_SEL > // > { > - 0x0, // limit 0 > - 0x0, // base 0 > - 0x0, > - 0x0, // present, ring 0, data, expand-up, writable > - 0x0, // page-granular, 32-bit > + 0x0FFFF, // limit 15:0 > + 0x0, // base 15:0 > + 0x0, // base 23:16 > + 0x092, // present, ring 0, data, read/write > + 0x0CF, // page-granular, 32-bit > 0x0, > }, > // > + // LINEAR_CODE64_SEL > + // > + { > + 0x0FFFF, // limit 15:0 > + 0x0, // base 15:0 > + 0x0, // base 23:16 > + 0x09A, // present, ring 0, code, execute/read > + 0x0AF, // page-granular, 64-bit code > + 0x0, // base (high) > + }, > + // > // SPARE5_SEL > // > { > - 0x0, // limit 0 > - 0x0, // base 0 > - 0x0, > - 0x0, // present, ring 0, data, expand-up, writable > - 0x0, // page-granular, 32-bit > - 0x0, > + 0x0, // limit 15:0 > + 0x0, // base 15:0 > + 0x0, // base 23:16 > + 0x0, // type > + 0x0, // limit 19:16, flags > + 0x0, // base 31:24 > }, > };
I wrote a script to convert gGdtEntries from "MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c" to the format visible above (i.e., from IA32_GDT to GDT_ENTRY). I didn't try to verify the semantics (i.e., the comments column) of the various bits here, but I confirm that the post-patch numeric values and the ordering in GdtTemplate are identical to those in gGdtEntries. > > /** > Initialize Global Descriptor Table. > diff --git a/UefiCpuPkg/CpuDxe/CpuGdt.h b/UefiCpuPkg/CpuDxe/CpuGdt.h > index 7ecec5d..2a00751 100644 > --- a/UefiCpuPkg/CpuDxe/CpuGdt.h > +++ b/UefiCpuPkg/CpuDxe/CpuGdt.h > @@ -1,10 +1,10 @@ > /** @file > C based implemention of IA32 interrupt handling only > requiring a minimal assembly interrupt entry point. > > - Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license may be > found at > http://opensource.org/licenses/bsd-license.php > > @@ -40,32 +40,34 @@ struct _GDT_ENTRIES { > GDT_ENTRY Null; > GDT_ENTRY Linear; > GDT_ENTRY LinearCode; > GDT_ENTRY SysData; > GDT_ENTRY SysCode; > - GDT_ENTRY LinearCode64; > GDT_ENTRY Spare4; > + GDT_ENTRY LinearData64; > + GDT_ENTRY LinearCode64; > GDT_ENTRY Spare5; > } GDT_ENTRIES; > > #pragma pack () > > #define NULL_SEL OFFSET_OF (GDT_ENTRIES, Null) > #define LINEAR_SEL OFFSET_OF (GDT_ENTRIES, Linear) > #define LINEAR_CODE_SEL OFFSET_OF (GDT_ENTRIES, LinearCode) > #define SYS_DATA_SEL OFFSET_OF (GDT_ENTRIES, SysData) > #define SYS_CODE_SEL OFFSET_OF (GDT_ENTRIES, SysCode) > -#define LINEAR_CODE64_SEL OFFSET_OF (GDT_ENTRIES, LinearCode64) > #define SPARE4_SEL OFFSET_OF (GDT_ENTRIES, Spare4) > +#define LINEAR_DATA64_SEL OFFSET_OF (GDT_ENTRIES, LinearData64) > +#define LINEAR_CODE64_SEL OFFSET_OF (GDT_ENTRIES, LinearCode64) > #define SPARE5_SEL OFFSET_OF (GDT_ENTRIES, Spare5) Yes, these are consistent with the comments in "MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c", and also with the new comments in GdtTemplate. > > #if defined (MDE_CPU_IA32) > #define CPU_CODE_SEL LINEAR_CODE_SEL > #define CPU_DATA_SEL LINEAR_SEL > #elif defined (MDE_CPU_X64) > #define CPU_CODE_SEL LINEAR_CODE64_SEL > -#define CPU_DATA_SEL LINEAR_SEL > +#define CPU_DATA_SEL LINEAR_DATA64_SEL This gave me a little pause, but it is correct after all, considering that the segment descriptor *contents* for selector LINEAR_SEL (= 0x08) are fully identical to those for selector LINEAR_DATA64_SEL (= 0x30). So in the X64 build the CPU_DATA_SEL selector value will change (from 0x08 to 0x30) wherever it is used, but the effect will be the same. I think that this (cosmetic) change *might* deserve a mention in the commit message too. Summary (of my other email too): - please consider adding the S3 language I proposed there - please add the three tags I asked for there - please consider mentioning that the CPU_DATA_SEL change makes no observable difference With those in place: Reviewed-by: Laszlo Ersek <[email protected]> Clearly, I also tested the patch. SMM S3 works with it in the Ia32 and Ia32X64 cases both. :) Tested-by: Laszlo Ersek <[email protected]> Thank you very much! Laszlo > #else > #error CPU type not supported for CPU GDT initialization! > #endif > > #endif // _CPU_GDT_H_ > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
