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 <ler...@redhat.com>; Ni, Ruiyu <ruiyu...@intel.com>
> Cc: Dong, Eric <eric.d...@intel.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; edk2-devel@lists.01.org; Yao, Jiewen
> <jiewen....@intel.com>; Kinney, Michael D <michael.d.kin...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
