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

[edk2-platforms] [PATCH v2] 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..cfe42883be 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|0x28000
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
   gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
-- 
2.19.1.windows.1

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

[Chiu, Chasel]

Hi Nate, Prince,

I would recommend that we set larger gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize so we do not have to update this again and again later when required temporary ram increased by enabling some boot loader features.
How about set it to 0x30000?

Thanks,
Chasel

> -----Original Message-----
> From: Agyeman, Prince <prince.agyeman@intel.com>
> Sent: Saturday, December 14, 2019 9:29 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 v2] 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..cfe42883be 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|0x28000
> 
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
> 
> gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
> --
> 2.19.1.windows.1

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

[Kubacki, Michael A]

I agree with Chasel. There should be enough T-RAM available to expand this to 0x30000 without a problem and reduce potential thrash in the future.

Thanks,
Michael

> -----Original Message-----
> From: Chiu, Chasel <chasel.chiu@intel.com>
> Sent: Sunday, December 15, 2019 4:32 PM
> To: Agyeman, Prince <prince.agyeman@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> devel@edk2.groups.io
> Subject: RE: [edk2-platforms] [PATCH v2] WhiskeylakeOpenBoardPkg:
> Update PCDs to enable stack sharing
> 
> 
> Hi Nate, Prince,
> 
> I would recommend that we set larger
> gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize so we do not have to
> update this again and again later when required temporary ram increased by
> enabling some boot loader features.
> How about set it to 0x30000?
> 
> Thanks,
> Chasel
> 
> > -----Original Message-----
> > From: Agyeman, Prince <prince.agyeman@intel.com>
> > Sent: Saturday, December 14, 2019 9:29 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 v2] 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
> P
> > kgPcd.dsc
> >
> b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> P
> > kgPcd.dsc
> > index 906f7b7ade..cfe42883be 100644
> > ---
> >
> a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> P
> > kgPcd.dsc
> > +++
> >
> b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> P
> > 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|0x28000
> >
> >    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
> >
> >
> gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
> > --
> > 2.19.1.windows.1
> 

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

[Nate DeSimone]

It's a careful balancing act. The more you allocate to temp ram the less will be available to cache the SPI flash, which can impact responsiveness depending on the size of your IBB. I agree that 192KB is probably a good compromise.

Thanks,
Nate

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

I agree with Chasel. There should be enough T-RAM available to expand this to 0x30000 without a problem and reduce potential thrash in the future.

Thanks,
Michael

> -----Original Message-----
> From: Chiu, Chasel <chasel.chiu@intel.com>
> Sent: Sunday, December 15, 2019 4:32 PM
> To: Agyeman, Prince <prince.agyeman@intel.com>; Desimone, Nathaniel L 
> <nathaniel.l.desimone@intel.com>
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; 
> devel@edk2.groups.io
> Subject: RE: [edk2-platforms] [PATCH v2] WhiskeylakeOpenBoardPkg:
> Update PCDs to enable stack sharing
> 
> 
> Hi Nate, Prince,
> 
> I would recommend that we set larger
> gSiPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize so we do not have to 
> update this again and again later when required temporary ram 
> increased by enabling some boot loader features.
> How about set it to 0x30000?
> 
> Thanks,
> Chasel
> 
> > -----Original Message-----
> > From: Agyeman, Prince <prince.agyeman@intel.com>
> > Sent: Saturday, December 14, 2019 9:29 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 v2] 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
> P
> > kgPcd.dsc
> >
> b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> P
> > kgPcd.dsc
> > index 906f7b7ade..cfe42883be 100644
> > ---
> >
> a/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> P
> > kgPcd.dsc
> > +++
> >
> b/Platform/Intel/WhiskeylakeOpenBoardPkg/WhiskeylakeURvp/OpenBoard
> P
> > 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|0x28000
> >
> >    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000
> >
> >
> gMinPlatformPkgTokenSpaceGuid.PcdPciExpressRegionLength|0x10000000
> > --
> > 2.19.1.windows.1
>