[edk2-devel][PATCH 0/2] Ensure RSP is aligned to a 16-byte boundary for PEI 64bit

[edk2-devel][PATCH 0/2] Ensure RSP is aligned to a 16-byte boundary for PEI 64bit

[Kuo, Ted]

The changes ensure the same stack alignment is kept before and after
swithing stack in X64.

Ted Kuo (2):
  MdeModulePkg: StackOffset must be aligned to a 16-byte boundary in X64
  IntelFsp2Pkg: Ensure new stack is aligned to old stack for X64

 IntelFsp2Pkg/FspSecCore/SecMain.c             |  8 ++++++++
 MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 18 +++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

-- 
2.16.2.windows.1

[edk2-devel][PATCH 1/2] MdeModulePkg: StackOffset must be aligned to a 16-byte boundary in X64

[Kuo, Ted]

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.kinney@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Debkumar De <debkumar.de@intel.com>
Cc: Harry Han <harry.han@intel.com>
Cc: Catharine West <catharine.west@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Signed-off-by: Ted Kuo <ted.kuo@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;
+      }
+
       //
       // 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,
-- 
2.16.2.windows.1

[edk2-devel][PATCH 2/2] IntelFsp2Pkg: Ensure new stack is aligned to old stack for X64

[Kuo, Ted]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3865
Ensure bit3:0 of NewStack is aligned with bit3:0 of OldStack for X64
before switching stack. Otherwise, RSP may not be aligned to a 16-byte
boundary after returning from SecTemporaryRamSupport.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
Signed-off-by: Ted Kuo <ted.kuo@intel.com>
---
 IntelFsp2Pkg/FspSecCore/SecMain.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c b/IntelFsp2Pkg/FspSecCore/SecMain.c
index d376fb8361..f93e2d2ff7 100644
--- a/IntelFsp2Pkg/FspSecCore/SecMain.c
+++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
@@ -258,6 +258,14 @@ SecTemporaryRamSupport (
     NewStack = (VOID *)(UINTN)PermanentMemoryBase;
   }
 
+  //
+  // Ensure bit3:0 of NewStack is aligned with bit3:0 of OldStack for X64 before switching stack.
+  // Otherwise, RSP may not be aligned to a 16-byte boundary after returning from SecTemporaryRamSupport.
+  //
+  if ((sizeof (UINTN) == sizeof (UINT64)) && (((UINTN)NewStack & 0x0F) != ((UINTN)OldStack & 0x0F))) {
+    NewStack = (VOID *)((UINTN)NewStack - 8);
+  }
+
   //
   // Migrate Heap
   //
-- 
2.16.2.windows.1

Re: [edk2-devel][PATCH 1/2] MdeModulePkg: StackOffset must be aligned to a 16-byte boundary in X64

[Marvin Häuser]

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.kinney@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Debkumar De <debkumar.de@intel.com>
> Cc: Harry Han <harry.han@intel.com>
> Cc: Catharine West <catharine.west@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Signed-off-by: Ted Kuo <ted.kuo@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?

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.

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)

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)

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?

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)?

7) This only updates FSP code, similar to 5), but to QEMU and friends 
continue to work?

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,

Re: [edk2-devel][PATCH 1/2] MdeModulePkg: StackOffset must be aligned to a 16-byte boundary in X64

[Kuo, Ted]

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 <mhaeuser@posteo.de> 
Sent: Tuesday, March 22, 2022 3:46 AM
To: devel@edk2.groups.io; Kuo, Ted <ted.kuo@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; De, Debkumar <debkumar.de@intel.com>; Han, Harry <harry.han@intel.com>; West, Catharine <catharine.west@intel.com>; Wang, Jian J <jian.j.wang@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.kinney@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Debkumar De <debkumar.de@intel.com>
> Cc: Harry Han <harry.han@intel.com>
> Cc: Catharine West <catharine.west@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Signed-off-by: Ted Kuo <ted.kuo@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.

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. 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.
e.g.
case1:
old stack: 0xfef5e000
new stack: 0x49c8f3b0
stack: 0x9c8f3b0 -> 16-Byte-aligned
case2: 
old stack: 0xfef5e008
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.

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.

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).

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.

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.


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,

Re: [edk2-devel][PATCH 1/2] MdeModulePkg: StackOffset must be aligned to a 16-byte boundary in X64

[Marvin Häuser]

Good day,

Thanks for the updates!

> On 22. Mar 2022, at 08:23, Kuo, Ted <ted.kuo@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 <mhaeuser@posteo.de> 
> Sent: Tuesday, March 22, 2022 3:46 AM
> To: devel@edk2.groups.io; Kuo, Ted <ted.kuo@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; De, Debkumar <debkumar.de@intel.com>; Han, Harry <harry.han@intel.com>; West, Catharine <catharine.west@intel.com>; Wang, Jian J <jian.j.wang@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.kinney@intel.com>
>> Cc: Dandan Bi <dandan.bi@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Debkumar De <debkumar.de@intel.com>
>> Cc: Harry Han <harry.han@intel.com>
>> Cc: Catharine West <catharine.west@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Signed-off-by: Ted Kuo <ted.kuo@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,
> 

Re: [edk2-devel][PATCH 1/2] MdeModulePkg: StackOffset must be aligned to a 16-byte boundary in X64

[Kuo, Ted]

Hi Marvin,

Please find my inline comments with [Ted2].

Thanks,
Ted

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de> 
Sent: Tuesday, March 22, 2022 5:27 PM
To: Kuo, Ted <ted.kuo@intel.com>
Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; De, Debkumar <debkumar.de@intel.com>; Han, Harry <harry.han@intel.com>; West, Catharine <catharine.west@intel.com>; Wang, Jian J <jian.j.wang@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 updates!

> On 22. Mar 2022, at 08:23, Kuo, Ted <ted.kuo@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 <mhaeuser@posteo.de>
> Sent: Tuesday, March 22, 2022 3:46 AM
> To: devel@edk2.groups.io; Kuo, Ted <ted.kuo@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Bi, Dandan 
> <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; De, 
> Debkumar <debkumar.de@intel.com>; Han, Harry <harry.han@intel.com>; 
> West, Catharine <catharine.west@intel.com>; Wang, Jian J 
> <jian.j.wang@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.kinney@intel.com>
>> Cc: Dandan Bi <dandan.bi@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Debkumar De <debkumar.de@intel.com>
>> Cc: Harry Han <harry.han@intel.com>
>> Cc: Catharine West <catharine.west@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Signed-off-by: Ted Kuo <ted.kuo@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)?
[Ted2]: Yes, I confirmed that no issue with the current design in Dispatcher.c. The unaligned stack offset I observed was caused by the unaligned TopOfOldStack which is produced by sum of SecCoreData->StackBase and SecCoreData->StackSize. In my case, SecCoreData is provided by FspSecCore. Hence we need to ensure SecCoreData->StackBase and SecCoreData->StackSize provided by FspSecCore are 16-byte-aligned instead of making changes in Dispatcher.c in MdeModulePkg. The correct fix should be making the size of CONTEXT_STACK to be 16-byte-aligned for X64 so that FspSecCore can produce 16-byte-aligned stack base and save it in SecCoreData->StackBase. The fix will be included in the patch of enabling FspSecCore support for X64.

> 
> 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?
[Ted2]: It's current stack before and after switching stack, not top or bottom. I just wanted to show why stack offset must be 16-byte-aligned in this patch which is no longer needed since the root cause is already identified in FspSecCore. We can drop the two patches.

> 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.
[Ted2]: Totally agreed. We can drop the two patches as not required.

> 
> 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?
[Ted2]: Agreed. We can drop the two patches.

> 
> 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,
>