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

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

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 9, 2017 9:16 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Jeff Fan <vanjeff_919@hotmail.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org;
> Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to
> calculate optimal settings
> 
> On 11/09/17 08:11, Ni, Ruiyu wrote:
> > The old version is not just slow.
> > It cannot work in certain cases. That's why the new version was developed.
> >
> > The new version adds a new API which allows caller to pass in the
> > scratch buffer instead of using the stack. If a platform has limited
> > stack, it can use that API.
> 
> (1) OK, so let me summarize:
> 
> (1a) Ray and Jeff confirmed that the AP stacks should be affected in
> *neither* PiSmmCpuDxeSmm *nor* the MpInitLib client modules (CpuMpPei,
> CpuDxe).
> 
> (1b) There is a new MtrrLib class API that allows the client module to pass in a
> preallocated scratch buffer.
> 
> (1a) sounds great.
> 
> (1b) is an option we may or may not want to exercise in OVMF. I have the
> patches ready for enlarging the temp SEC/PEI RAM (and as part of that, the
> temp stack), which is one alternative. However, because OVMF's PEI phase runs
> from RAM (and not flash), the other alternative is just to add a sufficiently large
> static UINT8 array to PlatformPei, and pass that as scratch space to MtrrLib.
> 
> 
> (2) However: I don't know *how* the new API --
> MtrrSetMemoryAttributesInMtrrSettings() -- is supposed to be used. In
> OvmfPkg/PlatformPei, we have the following MtrrLib calls:
> 
> * OvmfPkg/PlatformPei/Xen.c:
> 
> - MtrrSetMemoryAttribute()
> 
> (in a loop, basically)
> 
> * OvmfPkg/PlatformPei/MemDetect.c:
> 
> - IsMtrrSupported()
> - MtrrGetAllMtrrs()
> - MtrrSetAllMtrrs()
> - MtrrSetMemoryAttribute()
> 
> Is my understanding correct that MtrrSetMemoryAttribute() is the only function
> that is affected?

1. yes. Only MtrrSetMemoryAttribute() call in OVMF is affected.

> 
> 
> (3) Is my understanding correct that
> MtrrSetMemoryAttributesInMtrrSettings() should be used like this:
> 
> (3a) start with MtrrGetAllMtrrs()
> 
> (3b) collect all *foreseeable* MtrrSetMemoryAttribute() calls into an
>      array of MTRR_MEMORY_RANGE elements
> 
> (3c) Perform a batch update on the output of (3a) by calling
>      MtrrSetMemoryAttributesInMtrrSettings(). For this, the array from
>      (3b), plus a caller-allocated scratch space, have to be passed in,.
> 
> (3d) Finally, call MtrrSetAllMtrrs().
> 
> Is this correct?

2. yes. In summary, there are three ways to call this new API. The first way is what
    you described. The second way is a bit change to (3a), ZeroMem() is called 
    instead of MtrrGetAllMtrrs() to initialize the MTRR. The third way is to call
    this new API using NULL MtrrSetting, which cause the API itself to retrieve
    the current MTRR settings from CPU, apply the new setting, write to CPU.
    But the third way doesn't support batch setting.

> 
> I think we could use this. Jordan, which alternative do you prefer; larger stack
> and unchanged code in PlatformPei, or same stack and updated code in
> PlatformPei?
> 
> 
> (4) Ray: would it be possible to expose SCRATCH_BUFFER_SIZE (with a new
> name MTRR_SCRATCH_BUFFER_SIZE) in the library class header? I see the new
> RETURN_BUFFER_TOO_SMALL status codes, and I don't really want to deal with
> them. The library class header should provide clients with a size macro that
> *guarantees* that RETURN_BUFFER_TOO_SMALL will not occur.
> 
> Practically speaking, I would use MTRR_SCRATCH_BUFFER_SIZE in the definition
> of the static UINT8 array in PlatformPei. (If Jordan prefers this alternative to the
> larger temp stack.)

3. Not quite correct. Because even when pass in the scratch buffer whose size equal
    to MTRR_SCRATCH_BUFFER_SIZE, the BUFFER_TOO_SMALL could be returned.
    That's why the BUFFER_TOO_SMALL status is invented. It requires caller to re-supply
    the enough scratch buffer for calculation.
    As such, I do not think exposing SCRATCH_BUFFER_SIZE helps.
    When implementing the code, I tried to find out the maximum scratch buffer size but
    found that the maximum could be up to 256KB. I cannot use such large stack because
    as Jordan said, MSVC will inject some code results in unresolved symbol in EDKII code.
    And DxeIpl only allocates 128KB stack for whole DXE phase.


> 
> Thanks!
> Laszlo
> 
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >> Of Jordan Justen
> >> Sent: Thursday, November 9, 2017 2:56 PM
> >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
> >> devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric
> >> <eric.dong@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm
> >> to calculate optimal settings
> >>
> >> On 2017-11-08 19:04:35, Ni, Ruiyu wrote:
> >>> 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.
> >>
> >> This does not sound much better. I'm saying that the BASE library
> >> should only use at most a few hundred bytes of stack.
> >>
> >> Apparently the old algorithm did not use much memory, but perhaps was
> >> slow? Can we put it back in place for the BASE version of the library?
> >> Then, we can add a DXE specific version that uses a large buffer
> >> which it can allocate, and potentially free.
> >>
> >> -Jordan
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel