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

[Laszlo Ersek <lersek@redhat.com>]

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?


(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?

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.)

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