[edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

[edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

[Agyeman, Prince]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2409

Updated WhiskeylakeURvp PCDs to enable FSP/BL stack sharing.
This fixes the boot failure seen with the latest Coffee Lake (CFL)
FSP binary (v 7.0.68.41)

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Michael Kubacki <michael.a.kubacki@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../WhiskeylakeURvp/OpenBoardPkgPcd.dsc               | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
index 906f7b7ade..b3e1da3970 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
@@ -54,15 +54,14 @@
   gSiPkgTokenSpaceGuid.PcdTsegSize|0x1000000
 
   #
-  # FSP API mode does not share stack with the boot loader,
-  # so FSP needs more temporary memory for FSP heap + stack size.
+  # When sharing stack with boot loader, FSP only needs small temp ram for heap
   #
-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000
+
   #
-  # FSP API mode does not need to enlarge the boot loader stack size
-  # since the stacks are separate.
+  # Boot loader stack size has to be big enough to executing FSP
   #
-  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
+  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
   gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
-- 
2.19.1.windows.1

Re: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

[Chiu, Chasel]

Reviewed-by: Chasel Chiu <chasel.chiu@intel.com>

> -----Original Message-----
> From: Agyeman, Prince <prince.agyeman@intel.com>
> Sent: Thursday, December 12, 2019 9:13 AM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Kubacki, Michael A
> <michael.a.kubacki@intel.com>
> Subject: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs
> to enable stack sharing
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2409
> 
> Updated WhiskeylakeURvp PCDs to enable FSP/BL stack sharing.
> This fixes the boot failure seen with the latest Coffee Lake (CFL) FSP binary (v
> 7.0.68.41)
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> 
> Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
> ---
>  .../WhiskeylakeURvp/OpenBoardPkgPcd.dsc               | 11
> +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git
> a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardP
> kgPcd.dsc
> b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardP
> kgPcd.dsc
> index 906f7b7ade..b3e1da3970 100644
> ---
> a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardP
> kgPcd.dsc
> +++
> b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardP
> k
> +++ gPcd.dsc
> @@ -54,15 +54,14 @@
>    gSiPkgTokenSpaceGuid.PcdTsegSize|0x1000000
> 
>    #
> -  # FSP API mode does not share stack with the boot loader,
> -  # so FSP needs more temporary memory for FSP heap + stack size.
> +  # When sharing stack with boot loader, FSP only needs small temp ram
> + for heap
>    #
> -  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
> +  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000
> +
>    #
> -  # FSP API mode does not need to enlarge the boot loader stack size
> -  # since the stacks are separate.
> +  # Boot loader stack size has to be big enough to executing FSP
>    #
> -  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
> +  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000
> 
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
> 
> gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
> --
> 2.19.1.windows.1

Re: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

[Kubacki, Michael A]

Reviewed-by: Michael Kubacki <michael.a.kubacki@intel.com>

> -----Original Message-----
> From: Agyeman, Prince <prince.agyeman@intel.com>
> Sent: Wednesday, December 11, 2019 5:13 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Kubacki, Michael A
> <michael.a.kubacki@intel.com>
> Subject: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs
> to enable stack sharing
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2409
> 
> Updated WhiskeylakeURvp PCDs to enable FSP/BL stack sharing.
> This fixes the boot failure seen with the latest Coffee Lake (CFL) FSP binary (v
> 7.0.68.41)
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> 
> Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
> ---
>  .../WhiskeylakeURvp/OpenBoardPkgPcd.dsc               | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git
> a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> PkgPcd.dsc
> b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> PkgPcd.dsc
> index 906f7b7ade..b3e1da3970 100644
> ---
> a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> PkgPcd.dsc
> +++
> b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> Pk
> +++ gPcd.dsc
> @@ -54,15 +54,14 @@
>    gSiPkgTokenSpaceGuid.PcdTsegSize|0x1000000
> 
>    #
> -  # FSP API mode does not share stack with the boot loader,
> -  # so FSP needs more temporary memory for FSP heap + stack size.
> +  # When sharing stack with boot loader, FSP only needs small temp ram
> + for heap
>    #
> -  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
> +  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000
> +
>    #
> -  # FSP API mode does not need to enlarge the boot loader stack size
> -  # since the stacks are separate.
> +  # Boot loader stack size has to be big enough to executing FSP
>    #
> -  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
> +  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000
> 
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
>    gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
> --
> 2.19.1.windows.1

Re: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

[Nate DeSimone]

Hi Prince,

Is 256KB really necessary? Could you try the 152KB (0x26000) that we had previously?

Thanks,
Nate

-----Original Message-----
From: Agyeman, Prince <prince.agyeman@intel.com> 
Sent: Wednesday, December 11, 2019 5:13 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2409

Updated WhiskeylakeURvp PCDs to enable FSP/BL stack sharing.
This fixes the boot failure seen with the latest Coffee Lake (CFL) FSP binary (v 7.0.68.41)

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Michael Kubacki <michael.a.kubacki@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../WhiskeylakeURvp/OpenBoardPkgPcd.dsc               | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
index 906f7b7ade..b3e1da3970 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPk
+++ gPcd.dsc
@@ -54,15 +54,14 @@
   gSiPkgTokenSpaceGuid.PcdTsegSize|0x1000000
 
   #
-  # FSP API mode does not share stack with the boot loader,
-  # so FSP needs more temporary memory for FSP heap + stack size.
+  # When sharing stack with boot loader, FSP only needs small temp ram 
+ for heap
   #
-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000
+
   #
-  # FSP API mode does not need to enlarge the boot loader stack size
-  # since the stacks are separate.
+  # Boot loader stack size has to be big enough to executing FSP
   #
-  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
+  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
   gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
--
2.19.1.windows.1

Re: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

[Agyeman, Prince]

Hi Nate,

Which 256KB are you referring to ?

The temporary ram size was reduced from  152KB (0x26000) to 64KB (0x10000)

With stack sharing enabled in FSP,  temp ram size of 0x26000 causes system to hang during memory training.

-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize |0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> 
Sent: Thursday, December 12, 2019 3:07 PM
To: Agyeman, Prince <prince.agyeman@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

Hi Prince,

Is 256KB really necessary? Could you try the 152KB (0x26000) that we had previously?

Thanks,
Nate

-----Original Message-----
From: Agyeman, Prince <prince.agyeman@intel.com>
Sent: Wednesday, December 11, 2019 5:13 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2409

Updated WhiskeylakeURvp PCDs to enable FSP/BL stack sharing.
This fixes the boot failure seen with the latest Coffee Lake (CFL) FSP binary (v 7.0.68.41)

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Michael Kubacki <michael.a.kubacki@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../WhiskeylakeURvp/OpenBoardPkgPcd.dsc               | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
index 906f7b7ade..b3e1da3970 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPk
+++ gPcd.dsc
@@ -54,15 +54,14 @@
   gSiPkgTokenSpaceGuid.PcdTsegSize|0x1000000
 
   #
-  # FSP API mode does not share stack with the boot loader,
-  # so FSP needs more temporary memory for FSP heap + stack size.
+  # When sharing stack with boot loader, FSP only needs small temp ram 
+ for heap
   #
-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000
+
   #
-  # FSP API mode does not need to enlarge the boot loader stack size
-  # since the stacks are separate.
+  # Boot loader stack size has to be big enough to executing FSP
   #
-  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
+  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
   gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
--
2.19.1.windows.1

Re: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

[Nate DeSimone]

Hi Prince,

Looking at your patch I see the following two changes:

1. Reduce FSP Temp Ram size from 152KB to 64KB:

-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000

This makes sense because now that the FSP is using the single stack it does not need as much temp ram.

2. Increase the size of the platform's PEI phase stack from 128KB to 256KB:

-  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
+  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000

This also makes sense because the FSP is now running on the same stack as the rest of the platform code, so more space will be needed to run MRC. What I am questioning is if we really need 256KB? Looking at the numbers from before, it seems like MRC was running OK with only 152KB of stack space, does platform code really use that much stack space _at_the_same_time_ that MRC is running? Or maybe, the newer version of MRC uses more stack space now?

The basic summary is... have you tried booting with gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x26000?

Thanks,
Nate

-----Original Message-----
From: Agyeman, Prince <prince.agyeman@intel.com> 
Sent: Friday, December 13, 2019 8:25 AM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing


Hi Nate,

Which 256KB are you referring to ?

The temporary ram size was reduced from  152KB (0x26000) to 64KB (0x10000)

With stack sharing enabled in FSP,  temp ram size of 0x26000 causes system to hang during memory training.

-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize |0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: Thursday, December 12, 2019 3:07 PM
To: Agyeman, Prince <prince.agyeman@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

Hi Prince,

Is 256KB really necessary? Could you try the 152KB (0x26000) that we had previously?

Thanks,
Nate

-----Original Message-----
From: Agyeman, Prince <prince.agyeman@intel.com>
Sent: Wednesday, December 11, 2019 5:13 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2409

Updated WhiskeylakeURvp PCDs to enable FSP/BL stack sharing.
This fixes the boot failure seen with the latest Coffee Lake (CFL) FSP binary (v 7.0.68.41)

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Michael Kubacki <michael.a.kubacki@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../WhiskeylakeURvp/OpenBoardPkgPcd.dsc               | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
index 906f7b7ade..b3e1da3970 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPk
+++ gPcd.dsc
@@ -54,15 +54,14 @@
   gSiPkgTokenSpaceGuid.PcdTsegSize|0x1000000
 
   #
-  # FSP API mode does not share stack with the boot loader,
-  # so FSP needs more temporary memory for FSP heap + stack size.
+  # When sharing stack with boot loader, FSP only needs small temp ram 
+ for heap
   #
-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000
+
   #
-  # FSP API mode does not need to enlarge the boot loader stack size
-  # since the stacks are separate.
+  # Boot loader stack size has to be big enough to executing FSP
   #
-  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
+  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
   gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
--
2.19.1.windows.1

Re: [edk2-devel] [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

[Nate DeSimone]

Actually looking at the CoffeeLake FSP Integration Guide it states the minimum stack size to be 160KB (0x28000) so I guess a slight revision to my previous statement:

gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x28000

Thanks,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Nate DeSimone
Sent: Friday, December 13, 2019 2:14 PM
To: Agyeman, Prince <prince.agyeman@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

Hi Prince,

Looking at your patch I see the following two changes:

1. Reduce FSP Temp Ram size from 152KB to 64KB:

-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000

This makes sense because now that the FSP is using the single stack it does not need as much temp ram.

2. Increase the size of the platform's PEI phase stack from 128KB to 256KB:

-  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
+  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000

This also makes sense because the FSP is now running on the same stack as the rest of the platform code, so more space will be needed to run MRC. What I am questioning is if we really need 256KB? Looking at the numbers from before, it seems like MRC was running OK with only 152KB of stack space, does platform code really use that much stack space _at_the_same_time_ that MRC is running? Or maybe, the newer version of MRC uses more stack space now?

The basic summary is... have you tried booting with gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x26000?

Thanks,
Nate

-----Original Message-----
From: Agyeman, Prince <prince.agyeman@intel.com> 
Sent: Friday, December 13, 2019 8:25 AM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing


Hi Nate,

Which 256KB are you referring to ?

The temporary ram size was reduced from  152KB (0x26000) to 64KB (0x10000)

With stack sharing enabled in FSP,  temp ram size of 0x26000 causes system to hang during memory training.

-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize |0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: Thursday, December 12, 2019 3:07 PM
To: Agyeman, Prince <prince.agyeman@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

Hi Prince,

Is 256KB really necessary? Could you try the 152KB (0x26000) that we had previously?

Thanks,
Nate

-----Original Message-----
From: Agyeman, Prince <prince.agyeman@intel.com>
Sent: Wednesday, December 11, 2019 5:13 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2409

Updated WhiskeylakeURvp PCDs to enable FSP/BL stack sharing.
This fixes the boot failure seen with the latest Coffee Lake (CFL) FSP binary (v 7.0.68.41)

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Michael Kubacki <michael.a.kubacki@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../WhiskeylakeURvp/OpenBoardPkgPcd.dsc               | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
index 906f7b7ade..b3e1da3970 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPk
+++ gPcd.dsc
@@ -54,15 +54,14 @@
   gSiPkgTokenSpaceGuid.PcdTsegSize|0x1000000
 
   #
-  # FSP API mode does not share stack with the boot loader,
-  # so FSP needs more temporary memory for FSP heap + stack size.
+  # When sharing stack with boot loader, FSP only needs small temp ram 
+ for heap
   #
-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000
+
   #
-  # FSP API mode does not need to enlarge the boot loader stack size
-  # since the stacks are separate.
+  # Boot loader stack size has to be big enough to executing FSP
   #
-  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
+  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
   gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
--
2.19.1.windows.1

Re: [edk2-devel] [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

[Agyeman, Prince]

Thanks for clarifying. 

Yes the minimum stack size of 160KB works as well.

I will update this patch

Thanks
Prince

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> 
Sent: Friday, December 13, 2019 2:27 PM
To: devel@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-devel] [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

Actually looking at the CoffeeLake FSP Integration Guide it states the minimum stack size to be 160KB (0x28000) so I guess a slight revision to my previous statement:

gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x28000

Thanks,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Nate DeSimone
Sent: Friday, December 13, 2019 2:14 PM
To: Agyeman, Prince <prince.agyeman@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

Hi Prince,

Looking at your patch I see the following two changes:

1. Reduce FSP Temp Ram size from 152KB to 64KB:

-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000

This makes sense because now that the FSP is using the single stack it does not need as much temp ram.

2. Increase the size of the platform's PEI phase stack from 128KB to 256KB:

-  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
+  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000

This also makes sense because the FSP is now running on the same stack as the rest of the platform code, so more space will be needed to run MRC. What I am questioning is if we really need 256KB? Looking at the numbers from before, it seems like MRC was running OK with only 152KB of stack space, does platform code really use that much stack space _at_the_same_time_ that MRC is running? Or maybe, the newer version of MRC uses more stack space now?

The basic summary is... have you tried booting with gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x26000?

Thanks,
Nate

-----Original Message-----
From: Agyeman, Prince <prince.agyeman@intel.com> 
Sent: Friday, December 13, 2019 8:25 AM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing


Hi Nate,

Which 256KB are you referring to ?

The temporary ram size was reduced from  152KB (0x26000) to 64KB (0x10000)

With stack sharing enabled in FSP,  temp ram size of 0x26000 causes system to hang during memory training.

-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize |0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: Thursday, December 12, 2019 3:07 PM
To: Agyeman, Prince <prince.agyeman@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

Hi Prince,

Is 256KB really necessary? Could you try the 152KB (0x26000) that we had previously?

Thanks,
Nate

-----Original Message-----
From: Agyeman, Prince <prince.agyeman@intel.com>
Sent: Wednesday, December 11, 2019 5:13 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: [edk2-platforms] [PATCH] WhiskeylakeOpenBoardPkg: Update PCDs to enable stack sharing

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2409

Updated WhiskeylakeURvp PCDs to enable FSP/BL stack sharing.
This fixes the boot failure seen with the latest Coffee Lake (CFL) FSP binary (v 7.0.68.41)

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Michael Kubacki <michael.a.kubacki@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../WhiskeylakeURvp/OpenBoardPkgPcd.dsc               | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
index 906f7b7ade..b3e1da3970 100644
--- a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPkgPcd.dsc
+++ b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoardPk
+++ gPcd.dsc
@@ -54,15 +54,14 @@
   gSiPkgTokenSpaceGuid.PcdTsegSize|0x1000000
 
   #
-  # FSP API mode does not share stack with the boot loader,
-  # so FSP needs more temporary memory for FSP heap + stack size.
+  # When sharing stack with boot loader, FSP only needs small temp ram 
+ for heap
   #
-  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x26000
+  gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize|0x10000
+
   #
-  # FSP API mode does not need to enlarge the boot loader stack size
-  # since the stacks are separate.
+  # Boot loader stack size has to be big enough to executing FSP
   #
-  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x20000
+  gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0x40000
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
   gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
--
2.19.1.windows.1