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

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

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.

Thanks,
Laszlo