Re: [edk2-devel] [RFC PATCH v2 16/44] OvmfPkg/MemEncryptSevLib: Make MemEncryptSevLib available during SEC

["Laszlo Ersek" <lersek@redhat.com>]

On 10/02/19 14:24, Laszlo Ersek wrote:
> On 09/19/19 21:52, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>
>> The SEC phase of OVMF will need access to the MemEncryptSevLib library,
>> so make the library available during SEC.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
>> index 7c44d0952815..755d49cc22dc 100644
>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
>> @@ -14,7 +14,7 @@ [Defines]
>>    FILE_GUID                      = c1594631-3888-4be4-949f-9c630dbc842b
>>    MODULE_TYPE                    = BASE
>>    VERSION_STRING                 = 1.0
>> -  LIBRARY_CLASS                  = MemEncryptSevLib|PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER
>> +  LIBRARY_CLASS                  = MemEncryptSevLib|SEC PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER
>>  
>>  #
>>  # The following information is for reference only and not required by the build
>>
> 
> This is not a good idea, at least in this form.
> 
> This library instance uses multiple variables with static storage
> duration, such as:
> - mSevStatus
> - mSevEsStatus
> - mSevStatusChecked
> - mAddressEncMaskChecked [X64]
> - mAddressEncMask [X64]
> - mPageTablePool [X64]
> 
> SEC runs from pflash, so such variables are read-only (writes to them
> won't trap, but will have no effect).
> 
> What are the functions from "OvmfPkg/Include/Library/MemEncryptSevLib.h"
> that you need in the SEC phase?
> 
> Because, maybe we should split the library class in two classes. One lib
> class would declare (for example):
> - MemEncryptSevEsIsEnabled
> - MemEncryptSevIsEnabled
> 
> The other lib class would declare:
> - MemEncryptSevClearPageEncMask
> - MemEncryptSevSetPageEncMask
> - MemEncryptSevLocateInitialSmramSaveStateMapPages
> 
> The first lib class could have two instances, one for SEC (using no
> global variables for caching / speeding up the checks), and another
> instance for PEI and later (with the global variables used for caching
> the detection results).
> 
> The second lib class would have only one instance (the current one), and
> it would not be usable in the SEC phase. (The main issue is that, for
> manipulating PTEs, MemEncryptSevSetPageEncMask and
> MemEncryptSevClearPageEncMask sometimes need to split page tables, and
> for that, they need to allocate pages dynamically. We can't do that in SEC.)
> 
> The [LibraryClasses] sections of some INF files that currently list
> "MemEncryptSevLib" may have to be updated, corresponding to the split.
> 
> Again, all of this depends on the exact subset of APIs you need in SEC.
> If it's just one API, e.g. MemEncryptSevEsIsEnabled(), needed in just
> one place in SEC, then it might not necessarily be tragic to simply
> open-code (duplicate) the CPUID logic in SEC.

Ah right, so if it's just for patch#18 ("OvmfPkg/Sec: Enable cache early
to speed up booting"), then I'd definitely opt for open-coding the two
AsmCpuid(), plus one AsmReadMsr32(), calls, in SecCoreStartupWithStack().

Later on, *if* we decide to call AsmEnableCache() in
SecCoreStartupWithStack() unconditionally -- and that's a big "if" --,
then it's going to be easier to remove those CPUID+MSR checks, than to
undo the MemEncryptSevLib class split as well.

Thanks
Laszlo