Re: [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-11-08 17:36:01, Laszlo Ersek wrote:
> Hi Ray,
> 
> On 10/12/17 10:48, Ruiyu Ni wrote:
> > The new algorithm converts the problem calculating optimal
> > MTRR settings (using least MTRR registers) to the problem finding
> > the shortest path in a graph.
> > The memory required in extreme but rare case can be up to 256KB,
> > so using local stack buffer is impossible considering current
> > DxeIpl only allocates 128KB stack.
> > 
> > The patch changes existing MtrrSetMemoryAttributeInMtrrSettings() and
> > MtrrSetMemoryAttribute() to use the 4-page stack buffer for
> > calculation. The two APIs return BUFFER_TOO_SMALL when the buffer
> > is too small for calculation.
> 
> [snip]
> 
> > +#define SCRATCH_BUFFER_SIZE           (4 * SIZE_4KB)
> 
> [snip]
> 
> >  RETURN_STATUS
> >  EFIAPI
> > -MtrrSetMemoryAttribute (
> > +MtrrSetMemoryAttributeInMtrrSettings (
> > +  IN OUT MTRR_SETTINGS       *MtrrSetting,
> >    IN PHYSICAL_ADDRESS        BaseAddress,
> >    IN UINT64                  Length,
> >    IN MTRR_MEMORY_CACHE_TYPE  Attribute
> >    )
> >  {
> >    RETURN_STATUS              Status;
> > +  UINT8                      Scratch[SCRATCH_BUFFER_SIZE];
> 
> [snip]
> 
> (This patch is now commit 2bbd7e2fbd4b.)
> 
> Today I managed to spend time on
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=747
> 
> (which is in turn based on the earlier mailing list thread
> 
>   [edk2] dynamic PCD impact on temporary PEI memory
>   https://lists.01.org/pipermail/edk2-devel/2017-October/016213.html
> ).
> 
> While writing the patches, I found the root cause of BZ#747:
> "OvmfPkg/PlatformPei" calls MtrrLib APIs, and due to the above 16KB
> stack allocation, MtrrLib overflow's OVMF's 16KB (total) temp SEC/PEI
> stack.

I thought it was considered bad form to use a significant portion of
the stack (> ~100 bytes) via local variables. This used to
occasionally break MSVC builds as MS would insert a stack check call
if the locals size exceeded some threshold.

For a BASE library, I think this should go beyond "bad form" and into
not allowed.

-Jordan