Re: [edk2-devel] [PATCH 08/12] OvmfPkg/MemEncryptSevLib: Make the MemEncryptSevLib available for SEC

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

On 01/05/21 16:38, Lendacky, Thomas wrote:
> On 1/5/21 8:34 AM, Tom Lendacky wrote:
>> On 1/5/21 3:40 AM, Laszlo Ersek wrote:
>>> On 12/15/20 21:51, Lendacky, Thomas wrote:
>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>>
>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3108&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C1440a9afd7f1450ba93d08d8b15e02a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637454364641627971%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=P52g0gS3SEhdgkF2qRY6l1J8%2FLjJm1DNR3LLlEmKSBk%3D&amp;reserved=0
>>>>
>>>> In preparation for a new interface to be added to the MemEncryptSevLib
>>>> library that will be used in SEC, create an SEC version of the library.
>>>>
>>>> This requires the creation of SEC specific files.
>>>>
>>>> Some of the current MemEncryptSevLib functions perform memory allocations
>>>> which cannot be performed in SEC, so these interfaces will return an error
>>>> during SEC. Also, the current MemEncryptSevLib library uses some static
>>>> variables to optimize access to variables, which cannot be used in SEC.
>>>>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>> ---
>>>>  .../DxeBaseMemEncryptSevLib.inf               |   2 +-
>>>>  .../PeiBaseMemEncryptSevLib.inf               |   2 +-
>>>>  .../SecBaseMemEncryptSevLib.inf               |  54 ++++++++
>>>>  .../SecMemEncryptSevLibInternal.c             | 130 ++++++++++++++++++
>>>>  ...{VirtualMemory.c => PeiDxeVirtualMemory.c} |  12 +-
>>>>  .../X64/SecVirtualMemory.c                    |  80 +++++++++++
>>>>  6 files changed, 272 insertions(+), 8 deletions(-)
>>>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf
>>>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
>>>>  rename OvmfPkg/Library/BaseMemEncryptSevLib/X64/{VirtualMemory.c => PeiDxeVirtualMemory.c} (95%)
>>>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
>>>
>>> (1) /s/SecBase/Sec/ (in filenames and in filename references; the
>>> BASE_NAME is OK)
>>
>> Yup, I'll fix that.
>>
>>>
>>>>
>>>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf
>>>> index 2be6ca1fa737..390f2d60677f 100644
>>>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf
>>>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf
>>>> @@ -33,7 +33,7 @@ [Sources.X64]
>>>>    DxeMemEncryptSevLibInternal.c
>>>>    MemEncryptSevLibInternal.c
>>>>    X64/MemEncryptSevLib.c
>>>> -  X64/VirtualMemory.c
>>>> +  X64/PeiDxeVirtualMemory.c
>>>>    X64/VirtualMemory.h
>>>>  
>>>>  [Sources.IA32]
>>>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiBaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiBaseMemEncryptSevLib.inf
>>>> index 7bdf8cb5210d..cb973fdeb868 100644
>>>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiBaseMemEncryptSevLib.inf
>>>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiBaseMemEncryptSevLib.inf
>>>> @@ -33,7 +33,7 @@ [Sources.X64]
>>>>    PeiMemEncryptSevLibInternal.c
>>>>    MemEncryptSevLibInternal.c
>>>>    X64/MemEncryptSevLib.c
>>>> -  X64/VirtualMemory.c
>>>> +  X64/PeiDxeVirtualMemory.c
>>>>    X64/VirtualMemory.h
>>>>  
>>>>  [Sources.IA32]
>>>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf
>>>> new file mode 100644
>>>> index 000000000000..b26f739d69fd
>>>> --- /dev/null
>>>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf
>>>> @@ -0,0 +1,54 @@
>>>> +## @file
>>>> +#  Library provides the helper functions for SEV guest
>>>> +#
>>>> +# Copyright (c) 2020 Advanced Micro Devices. All rights reserved.<BR>
>>>> +#
>>>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>> +#
>>>> +#
>>>> +##
>>>> +
>>>> +[Defines]
>>>> +  INF_VERSION                    = 1.25
>>>> +  BASE_NAME                      = SecMemEncryptSevLib
>>>> +  FILE_GUID                      = 046388b4-430e-4e61-88f6-51ea21db2632
>>>> +  MODULE_TYPE                    = BASE
>>>> +  VERSION_STRING                 = 1.0
>>>> +  LIBRARY_CLASS                  = MemEncryptSevLib|SEC
>>>> +
>>>> +#
>>>> +# The following information is for reference only and not required by the build
>>>> +# tools.
>>>> +#
>>>> +# VALID_ARCHITECTURES           = IA32 X64
>>>> +#
>>>> +
>>>> +[Packages]
>>>> +  MdeModulePkg/MdeModulePkg.dec
>>>> +  MdePkg/MdePkg.dec
>>>> +  OvmfPkg/OvmfPkg.dec
>>>> +  UefiCpuPkg/UefiCpuPkg.dec
>>>> +
>>>> +[Sources.X64]
>>>> +  SecMemEncryptSevLibInternal.c
>>>> +  MemEncryptSevLibInternal.c
>>>> +  X64/MemEncryptSevLib.c
>>>> +  X64/SecVirtualMemory.c
>>>> +  X64/VirtualMemory.h
>>>> +
>>>> +[Sources.IA32]
>>>> +  SecMemEncryptSevLibInternal.c
>>>> +  MemEncryptSevLibInternal.c
>>>> +  Ia32/MemEncryptSevLib.c
>>>> +
>>>> +[LibraryClasses]
>>>> +  BaseLib
>>>> +  CpuLib
>>>> +  DebugLib
>>>> +  PcdLib
>>>> +
>>>> +[FeaturePcd]
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
>>>> +
>>>
>>> (2) This PCD does not look useful for the new library instance (at least
>>> at this stage).
>>
>> The PCD is used in MemEncryptSevLocateInitialSmramSaveStateMapPages() in
>> the MemEncryptSevLibInternal.c file, which is part of the library. Because
>> of that, I assumed that it needed to be added even though the function
>> that uses it isn't called during SEC.
>>
>> I'll remove it.
> 
> Removing it does cause an error.
> 
> If we really don't want to include this PCD, I can create SEC and PEI/DXE
> specific versions of the MemEncryptSevLibInternal.c file and just return
> RETURN_UNSUPPORTED for the SEC version of
> MemEncryptSevLocateInitialSmramSaveStateMapPages().

This one seems like the best way forward.

Thanks!
Laszlo

> 
> Alternatively, I can just remove the MemEncryptSevLibInternal.c file from
> the build of the SEC library. This should be ok during SEC because there
> are no calls to MemEncryptSevLocateInitialSmramSaveStateMapPages(). If,
> for some reason a call is added later, then the build will fail, but it
> should be obvious why it failed.
> 
> Or I can just leave the FeaturePcd section in the SEC inf file.
> 
> Thoughts?