Hi Ray, On 11/10/17 01:52, Ni, Ruiyu wrote: > > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, November 9, 2017 9:16 PM >> To: Ni, Ruiyu ; Justen, Jordan L >> ; Jeff Fan >> Cc: Kinney, Michael D ; edk2-devel@lists.01.org; >> Yao, Jiewen ; Dong, Eric ; Ard >> Biesheuvel >> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to >> calculate optimal settings >> (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. >> 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. Thank you very much for the explanation! I have an untested prototype for (1b), using a 16KB static array as MtrrLib scratch space, in OvmfPkg/PlatformPei. In the compressed FVMAIN_COMPACT volume, its size impact is 320 bytes only. (For illustration, I'm attaching this "proof of concept".) However, after some more thinking, I dislike this approach. First, I'd like to keep this added complexity out of PlatformPei, if possible. Second, a 16KB growth in PEIFV (current total size: 896 KB) just to preserve the "status quo" is not really nice; we could use that space for including executable code and related static data. Third, this scratch space cannot be used for any other purpose. A larger temp stack is generally available to other functions in PlatformPei, and to all other PEIMs as well. In particular, if OVMF included a PEIM in the future that used the traditional MtrrSetMemoryAttribute() API, then PlatformPei's dedicated scratch space could not be shared by that PEIM. So, I'll go ahead and post the variant that grows the temp SEC/PEI RAM for OVMF. Thanks! Laszlo