Hi Mike, I am aware of this possibility, but it feels unnecessary ugly in my opinion. Marvin has already sent a patch alignment-related patches not so long ago[1], updating this with V3 and using as a ground feels like a much better choice.
Best wishes, Vitaly [1] https://edk2.groups.io/g/devel/message/78887?p=%2C%2C%2C100%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2CALIGNOF%2C100%2C2%2C0%2C84754061 > On 6 Nov 2021, at 00:39, Kinney, Michael D <michael.d.kin...@intel.com> wrote: > > Hi Vitaly, > > So IA32 CLANGPDB is likely putting the UINTT8 array global mNewGdt > on a 4-byte boundary, when it would have to be on an 8-byte boundary > to meet the GDT alignment requirements. > > I would expect the X64 CLANGPDB build to use an 8-byte boundary. > > Another option to align to 8-byte boundary is to make the array an > array of UINT64 values. To be more correct, we could use an array > of IA32_SEGMENT_DESCRIPTOR structures that is a union with a > UINT64 to guarantee 8-byte alignment. > > typedef union { > struct { > UINT32 LimitLow:16; > UINT32 BaseLow:16; > UINT32 BaseMid:8; > UINT32 Type:4; > UINT32 S:1; > UINT32 DPL:2; > UINT32 P:1; > UINT32 LimitHigh:4; > UINT32 AVL:1; > UINT32 L:1; > UINT32 DB:1; > UINT32 G:1; > UINT32 BaseHigh:8; > } Bits; > UINT64 Uint64; > } IA32_SEGMENT_DESCRIPTOR; > > IA32_SEGMENT_DESCRIPTOR mNewGdt[(CPU_TSS_GDT_SIZE / sizeof > (IA32_SEGMENT_DESCRIPTOR)) + 1]; > > Add 1 in case CPU_TSS_GDT_SIZE is not 8-byte aligned in size. > > > For come of the logic, you would still need to introduce a UINT8 * local > variable > to compute the start of each component in this array. > > I think in other places, the GDT is allocated using AllocatePool() > which guarantees an 8-byte alignment. > > Are you suggesting that the ALIGNAS macro would map to the compiler > specific alignment directives? We have not had to introduce that yet > and would like to figure out how to address this one issues without > adding those directives. > > Mike > >> -----Original Message----- >> From: Vitaly Cheptsov <chept...@ispras.ru> >> Sent: Friday, November 5, 2021 1:36 PM >> To: Kinney, Michael D <michael.d.kin...@intel.com> >> Cc: Leif Lindholm <l...@nuviainc.com>; devel@edk2.groups.io; Yao, Jiewen >> <jiewen....@intel.com>; Dong, Eric >> <eric.d...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Jeff Fan >> <vanjeff_...@hotmail.com>; Mikhail Krichanov >> <kricha...@ispras.ru>; Marvin Häuser <mhaeu...@posteo.de> >> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by >> aligning GDT buffer >> >> Hi Mike, >> >> The command is: >> build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -t CLANGPDB -b NOOPT -D >> DEBUG_ON_SERIAL_PORT >> >> But I obviously needed to add >> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|TRUE >> to OvmfPkgIa32.dsc. >> >> The compiler is clang 12.0.1, because clang 13 is badly broken[1]. It would >> then crash like this: >> >> HandOffToDxeCore() Stack Base: 0x7FE24000, Stack Size: 0x20000 >> >> ASSERT_EFI_ERROR (Status = Invalid Parameter) >> ASSERT [DxeCore] DxeMain.c(256): !EFI_ERROR (Status) >> >> As for aligning the array, I can submit a V2 which introduces the ALIGNAS >> macro. I also like this solution a lot more. >> >> Best wishes, >> Vitaly >> >> [1] >> https://bugzilla.tianocore.org/buglist.cgi?quicksearch=clang%2013&list_id$276 >> >> >>> On 5 Nov 2021, at 22:42, Kinney, Michael D <michael.d.kin...@intel.com> >>> wrote: >>> >>> Hi Vitaly, >>> >>> Can you please provide some details on the compiler/build command that did >>> not align the array >>> correctly. >>> >>> I agree that the GDT must have the correct alignment. >>> >>> I do not like the idea of unused bytes at the beginning of the array. I >>> would prefer to see >>> an array that is aligned correctly by declaration. >>> >>> >>> Mike >>> >>>> -----Original Message----- >>>> From: Leif Lindholm <l...@nuviainc.com> >>>> Sent: Friday, November 5, 2021 12:28 PM >>>> To: devel@edk2.groups.io; chept...@ispras.ru >>>> Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>; >>>> Kinney, Michael D >> <michael.d.kin...@intel.com>; >>>> Wang, Jian J <jian.j.w...@intel.com>; Jeff Fan <vanjeff_...@hotmail.com>; >>>> Mikhail Krichanov <kricha...@ispras.ru>; >> Marvin >>>> Häuser <mhaeu...@posteo.de> >>>> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support >>>> by aligning GDT buffer >>>> >>>> UefiCpuPkg maintainers - please respond. >>>> >>>> Meanwhile, Vitaly, could you please provide a commit message? >>>> The BZ link is needed, but it's not a substitute. >>>> >>>> / >>>> Leif >>>> >>>> On Mon, Sep 20, 2021 at 17:13:47 +0300, Vitaly Cheptsov wrote: >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id639 >>>>> >>>>> >>>>> >>>>> Cc: Jiewen Yao <jiewen....@intel.com> >>>>> >>>>> Cc: Eric Dong <eric.d...@intel.com> >>>>> >>>>> Cc: Michael Kinney <michael.d.kin...@intel.com> >>>>> >>>>> Cc: Jian J Wang <jian.j.w...@intel.com> >>>>> >>>>> Cc: Jeff Fan <vanjeff_...@hotmail.com> >>>>> >>>>> Cc: Mikhail Krichanov <kricha...@ispras.ru> >>>>> >>>>> Cc: Marvin Häuser <mhaeu...@posteo.de> >>>>> >>>>> Signed-off-by: Vitaly Cheptsov <chept...@ispras.ru> >>>>> >>>>> --- >>>>> >>>>> .../Library/CpuExceptionHandlerLib/DxeException.c | 12 +++++++----- >>>>> >>>>> 1 file changed, 7 insertions(+), 5 deletions(-) >>>>> >>>>> >>>>> >>>>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c >>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c >>>>> >>>>> index fd59f09ecd..12874811e1 100644 >>>>> >>>>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c >>>>> >>>>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c >>>>> >>>>> @@ -22,7 +22,7 @@ EXCEPTION_HANDLER_DATA mExceptionHandlerData; >>>>> >>>>> >>>>> >>>>> UINT8 mNewStack[CPU_STACK_SWITCH_EXCEPTION_NUMBER * >>>>> >>>>> CPU_KNOWN_GOOD_STACK_SIZE]; >>>>> >>>>> -UINT8 mNewGdt[CPU_TSS_GDT_SIZE]; >>>>> >>>>> +UINT8 mNewGdt[CPU_TSS_GDT_SIZE + >>>>> IA32_GDT_ALIGNMENT]; >>>>> >>>>> >>>>> >>>>> /** >>>>> >>>>> Common exception handler. >>>>> >>>>> @@ -238,6 +238,7 @@ InitializeCpuExceptionHandlersEx ( >>>>> >>>>> CPU_EXCEPTION_INIT_DATA EssData; >>>>> >>>>> IA32_DESCRIPTOR Idtr; >>>>> >>>>> IA32_DESCRIPTOR Gdtr; >>>>> >>>>> + UINT8 *Gdt; >>>>> >>>>> >>>>> >>>>> // >>>>> >>>>> // To avoid repeat initialization of default handlers, the caller should >>>>> pass >>>>> >>>>> @@ -259,6 +260,7 @@ InitializeCpuExceptionHandlersEx ( >>>>> >>>>> if (PcdGetBool (PcdCpuStackGuard)) { >>>>> >>>>> if (InitData == NULL) { >>>>> >>>>> SetMem (mNewGdt, sizeof (mNewGdt), 0); >>>>> >>>>> + Gdt = ALIGN_POINTER (mNewGdt, IA32_GDT_ALIGNMENT); >>>>> >>>>> >>>>> >>>>> AsmReadIdtr (&Idtr); >>>>> >>>>> AsmReadGdtr (&Gdtr); >>>>> >>>>> @@ -270,11 +272,11 @@ InitializeCpuExceptionHandlersEx ( >>>>> >>>>> EssData.X64.StackSwitchExceptionNumber = >>>>> CPU_STACK_SWITCH_EXCEPTION_NUMBER; >>>>> >>>>> EssData.X64.IdtTable = (VOID *)Idtr.Base; >>>>> >>>>> EssData.X64.IdtTableSize = Idtr.Limit + 1; >>>>> >>>>> - EssData.X64.GdtTable = mNewGdt; >>>>> >>>>> - EssData.X64.GdtTableSize = sizeof (mNewGdt); >>>>> >>>>> - EssData.X64.ExceptionTssDesc = mNewGdt + Gdtr.Limit + 1; >>>>> >>>>> + EssData.X64.GdtTable = Gdt; >>>>> >>>>> + EssData.X64.GdtTableSize = CPU_TSS_GDT_SIZE; >>>>> >>>>> + EssData.X64.ExceptionTssDesc = Gdt + Gdtr.Limit + 1; >>>>> >>>>> EssData.X64.ExceptionTssDescSize = CPU_TSS_DESC_SIZE; >>>>> >>>>> - EssData.X64.ExceptionTss = mNewGdt + Gdtr.Limit + 1 + >>>>> CPU_TSS_DESC_SIZE; >>>>> >>>>> + EssData.X64.ExceptionTss = Gdt + Gdtr.Limit + 1 + >>>>> CPU_TSS_DESC_SIZE; >>>>> >>>>> EssData.X64.ExceptionTssSize = CPU_TSS_SIZE; >>>>> >>>>> >>>>> >>>>> InitData = &EssData; >>>>> >>>>> -- >>>>> >>>>> 2.30.1 (Apple Git-130) >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#83413): https://edk2.groups.io/g/devel/message/83413 Mute This Topic: https://groups.io/mt/85741694/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: Message signed with OpenPGP
