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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Jordan, Laszlo,

I didn't realize that a platform may have less than 4-page stack before memory is ready.
If I was aware of that, I would change the default scratch buffer size to 2 page, which
should be enough too.

But I do not think we may need to change the scratch buffer size.
Let me clarify about the MtrrLib API usage:
  Though the library is a BASE type, and it's MP-safe, it's not recommended to call
  MtrrSetMemoryAttribute...() in AP. Per IA32 SDM, all processors should use the
  same MTRR settings. In UEFI practice, we always just call the MtrrSetMemoryAttribute...()
  in BSP side, and then use MtrrGetAllMtrrs()/MtrrSetAllMtrrs() to sync the changes to
  all other Aps.


Thanks/Ray

> -----Original Message-----
> From: Justen, Jordan L
> Sent: Thursday, November 9, 2017 9:54 AM
> To: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Yao, Jiewen
> <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to
> calculate optimal settings
> 
> 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