Good day, Thanks for the updates!
> On 22. Mar 2022, at 08:23, Kuo, Ted <ted....@intel.com> wrote: > > Hi Marvin, > > Good day. Thanks for your valuable comments. After checking all of your > comments, I decide to drop the patches and close the bugzilla ticket since > the changes should be specific to X64 in IntelFspPkg. You still can find my > inline comments with [Ted] for your questions. > > Thanks, > Ted > > -----Original Message----- > From: Marvin Häuser <mhaeu...@posteo.de> > Sent: Tuesday, March 22, 2022 3:46 AM > To: devel@edk2.groups.io; Kuo, Ted <ted....@intel.com> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Bi, Dandan > <dandan...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; De, Debkumar > <debkumar...@intel.com>; Han, Harry <harry....@intel.com>; West, Catharine > <catharine.w...@intel.com>; Wang, Jian J <jian.j.w...@intel.com> > Subject: Re: [edk2-devel][PATCH 1/2] MdeModulePkg: StackOffset must be > aligned to a 16-byte boundary in X64 > > Good day, > > Thanks for the update! > >> On 21.03.22 13:43, Kuo, Ted wrote: >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3865 >> For X64, StackOffset must be aligned to a 16-byte boundary as well as >> old stack and new stack. Otherwise, it'll get wrong data from Private >> pointer after switching from old stack to new stack. >> >> Cc: Michael D Kinney <michael.d.kin...@intel.com> >> Cc: Dandan Bi <dandan...@intel.com> >> Cc: Liming Gao <gaolim...@byosoft.com.cn> >> Cc: Debkumar De <debkumar...@intel.com> >> Cc: Harry Han <harry....@intel.com> >> Cc: Catharine West <catharine.w...@intel.com> >> Cc: Jian J Wang <jian.j.w...@intel.com> >> Cc: Marvin Häuser <mhaeu...@posteo.de> >> Signed-off-by: Ted Kuo <ted....@intel.com> >> --- >> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c >> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c >> index 3552feda8f..8a2c1ec779 100644 >> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c >> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c >> @@ -823,6 +823,19 @@ PeiCheckAndSwitchStack ( >> (VOID **)&TemporaryRamSupportPpi >> ); >> if (!EFI_ERROR (Status)) { >> + // >> + // For X64, StackOffset must be aligned to a 16-byte boundary. >> Otherwise, it'll get wrong data >> + // from Private pointer after switching to new stack. >> + // >> + if ((sizeof (UINTN) == sizeof (UINT64)) && ((StackOffset & 0x0F) == >> 8)) { >> + if (StackOffsetPositive == TRUE) { >> + StackOffset -= 8; >> + } else { >> + StackOffset += 8; >> + } >> + Private->StackOffset = StackOffset; >> + } >> + > > Hmm, the overall design (not your patch) looks very broken to me. So, if the > PPI exists, it's responsible for the migration of the stack, but it is > neither passed where to migrate the stack to, nor does it return where it did > migrate it to. This means the StackOffset calculated here may be out-of-sync > with what actually happens in the PPI, e.g., if the PPI code is changed. > There also is no detailed explanation for the memory layout with FSP separate > stack vs bootloader shared stack, so I cannot really give detailed comments > quickly. *Sigh*. > > Anyhow, as for the patch, I do not understand a few things: > > 1) Maybe most important of all, what even is broken? Which address is not > 16-Byte-aligned to cause this issue in the first place? > [Ted]: CPU will generate exception when running some X64 instructions which > need input/output memory address to be 16-Byte-aligned. Yes, I understood as much. I built a chain of reasoning for alignment in the response for the first revision, because a proper fix needs knowledge of which assumption is wrong in the first place. The question is, which *exact* value in the chain is not 16-Byte aligned, and should it be? Did you ever print all involved values, like PhysicalMemoryBegin (or whatever its name was, sorry, I’m responding from mobile)? > > 2) Why do you align StackOffset? Like yes, if the old top of the stack and > the offset to the new top of the stack are both 16-Byte-aligned, then the new > top of the stack is 16-Byte-aligned too. However, StackOffset is more of a > by-product and TopOfNewStack remains holding the old value. I just don't > really understand the idea of this approach. > [Ted]: Since new stack must keep the original stack alignment as old stack, > it means stack offset must be 16-Byte-aligned too. Yes, I agreed above. But StackOffset is not the definition of where the new stack is located, but only a consequence from it. It is not the primary descriptor is what I’m trying to say. > And the OldStack/NewStack in the fsp patch indicates the *current* old/new > stack. The fsp patch just makes left shift 8-byte of whole used stack data > when new stack not aligning with old stack. Hence I think no need to update > TopOfNewStack. Yes, I understand what is happening, I don’t understand why it is happening. My comment here was separate from the FSP patch and only concerned the code here. If the top of the stack (as described by StackOffset) is aligned from its original value, why can TopOfNewStack remain unchanged, when it all points to the old, unaligned top, where no data should ever be stored? > e.g. > case1: > old stack: 0xfef5e000 > new stack: 0x49c8f3b0 > stack: 0x9c8f3b0 -> 16-Byte-aligned > case2: > old stack: 0xfef5e008 Is this the top or bottom? If it’s the top, why can it *not* be 16-Byte-aligned? If it’s the bottom, how is it relevant here? We are only dealing with the top addresses in this patch, no? > new stack: 0x49c8f3b8 > stack: 0x9c8f3b0 -> 16-Byte-aligned > > 3) This only works when StackOffset is guaranteed to be 8-Byte-aligned (is > it?). As we are dealing with the *top* of the stack (which should be > 4K-aligned even for memory protection!), what would be wrong with just > aligning down and up instead? > (Same question for the second patch to the FSP code) > As my answer in Q2, what we adjust in the fsp patch is the new "current" > stack in order to keep the same stack alignment as old stack after switching > stack. Top of the new stack remains unchanged. If StackOffset is not adjusted > accordingly, bios will get wrong data from Private pointer after switching to > new stack. But StackOffset describes the offset from the top of the old stack to the top of the new stack. How does the top “remain unchanged”? Top alignment is the root for the transitive alignment chain, if it is aligned, and it must be, everything is aligned. > > 4) The next patch performs a similar alignment operation (as mentioned > before). However, while this patch aligns the *top* of the stack, the FSP > patch aligns the *bottom* of the stack. This may or may not be correct based > on your premises. Can you maybe document why this is correct, or even better, > try to align the top of the stack in FSP as well? (By transitivity, if you > align the top correctly, the bottom should be aligned correctly as well, as > every nested call must preserve the alignment invariant) > [Ted]: Actually the new stack region (from top to bottom) is not changed with > the patches. It just adjusted the *current* new stack to align with the > *current* old stack. Confused, sorry. Maybe explaining the rest will make this clearer. > > 5) Why does this only happen when the PPI is found? Would that not risk an > unaligned stack if the PPI is not provided, the same way it is unaligned now? > [Ted]: I didn't observe the unaligned stack issue in the else case (the PPI > is not found). This is why I asked about 1). Things like this are fundamental and should not be changed without understanding both the full scope of the issue and the full scope of the changes. > > 6) The comment explicitly mentions X64, but checks only for 64-bit pointer > size. So this should affect AArch64 and RISC-V-64 as well. Are they > guaranteed to function correctly after this patch (especially with the PPI > sync guarantees mentioned earlier)? > [Ted]: Good point. The changes should only be needed for X64. I shall make > the changes specific to X64 only in IntelFspPkg. I'll drop the patch and > close the bugzilla ticket. But how will StackOffset be in-sync with the changes in FSP SecCore then? If I’m not mistaken with my intro from last mail, both calculate the stack address separately from each other, and their calculations must be in-sync. If I’m mistaken, how does it work then? > > 7) This only updates FSP code, similar to 5), but to QEMU and friends > continue to work? > [Ted]: The changes should be specific to X64 architecture in IntelFspPkg > without any impact to QEMU and other architectures. Should be solved by 6). Thanks! Best regards, Marvin > > > Thanks! > > Best regards, > Marvin > >> // >> // Heap Offset >> // >> @@ -852,7 +865,10 @@ PeiCheckAndSwitchStack ( >> // Temporary Ram Support PPI is provided by platform, it will copy >> // temporary memory to permanent memory and do stack switching. >> // After invoking Temporary Ram Support PPI, the following code's >> - // stack is in permanent memory. >> + // stack is in permanent memory. For X64, the bit3:0 of the new stack >> + // produced by TemporaryRamMigration must be aligned with the bit3:0 >> of >> + // the old stack. Otherwise, it'll break the original stack alignment >> + // after switching to new stack. >> // >> TemporaryRamSupportPpi->TemporaryRamMigration ( >> PeiServices, > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#87829): https://edk2.groups.io/g/devel/message/87829 Mute This Topic: https://groups.io/mt/89926603/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
