Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library

[Laszlo Ersek <lersek@redhat.com>]

On 03/08/17 15:56, Duran, Leo wrote:
> For libraries, I've noticed this usage pattern:
> 
> INF File:
> - Name: <BASE_NAME>.inf
> - Path: xxxPkg/Library/<BASE_NAME>/<BASE_NAME>.inf
> 
> INCLUDE File:
> - Name: <LIBRARY_CLASS>.h
> - Path: xxxPkg/Include/Library/<LIBRARY_CLASS>.h

Correct.

What I described earlier was the relationship between <LIBRARY_CLASS>
and <BASE_NAME>. Namely, <BASE_NAME> is specific to the library
instance, and if you have multiple instances for the same library class,
then there is a convention for composing <BASE_NAME> from <LIBRARY_CLASS>:

  <BASE_NAME> :=  <PHASE><LIBRARY_CLASS><VARIANT>

Laszlo

> 
> Leo
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, March 07, 2017 4:08 PM
>> To: Brijesh Singh <brijesh.ksingh@gmail.com>
>> Cc: jordan.l.justen@intel.com; edk2-devel@ml01.01.org; Duran, Leo
>> <leo.duran@amd.com>; Singh, Brijesh <brijesh.singh@amd.com>; Lendacky,
>> Thomas <Thomas.Lendacky@amd.com>
>> Subject: Re: [edk2] [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV
>> helper library
>>
>> On 03/07/17 20:14, Brijesh Singh wrote:
>>> On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>> On 03/07/17 00:27, Brijesh Singh wrote:
>>>>> The library contain common helper routines for SEV feature.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>>>> ---
>>>>>  OvmfPkg/Include/Library/MemcryptSevLib.h          |   42
>> +++++++++++++
>>>>>  OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c   |   66
>>>> +++++++++++++++++++++
>>>>>  OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf |   44
>> ++++++++++++++
>>>>>  OvmfPkg/OvmfPkgIa32X64.dsc                        |    4 +
>>>>>  OvmfPkg/OvmfPkgX64.dsc                            |    4 +
>>>>>  5 files changed, 160 insertions(+)
>>>>>  create mode 100644 OvmfPkg/Include/Library/MemcryptSevLib.h
>>>>>  create mode 100644
>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
>>>>>  create mode 100644
>>>>> OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>
>>>> New files -- can you please double-check they are CRLF terminated?
>>>>
>>>>
>>> This version does not have CRLF, I will ensure that nex rev contains CRLF.
>>>
>>>
>>>>>
>>>>> diff --git a/OvmfPkg/Include/Library/MemcryptSevLib.h
>>>> b/OvmfPkg/Include/Library/MemcryptSevLib.h
>>>>> new file mode 100644
>>>>> index 0000000..89f9c86
>>>>> --- /dev/null
>>>>> +++ b/OvmfPkg/Include/Library/MemcryptSevLib.h
>>>>> @@ -0,0 +1,42 @@
>>>>> +/** @file
>>>>
>>>> Please add one or two sentences about the purpose of this library class.
>>>
>>>
>>> Will do.
>>>
>>>
>>>>
>>>>
>>>> +  Copyright (C) 2017 Advanced Micro Devices.
>>>>> +
>>>>> +  This program and the accompanying materials are licensed and made
>>>> available
>>>>> +  under the terms and conditions of the BSD License which
>>>>> + accompanies
>>>> this
>>>>> +  distribution.  The full text of the license may be found at
>>>>> + http://opensource.org/licenses/bsd-license.php
>>>>> +
>>>>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS
>> IS"
>>>>> + BASIS,
>>>> WITHOUT
>>>>> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
>> OR IMPLIED.
>>>>> +**/
>>>>> +
>>>>> +#ifndef __MEMCRYPT_SEV_LIB_H__
>>>>> +#define __MEMCRYPT_SEV_LIB_H__
>>>>> +
>>>>> +#include <Uefi/UefiBaseType.h>
>>>>
>>>> I think it shouldn't be necessary to include this, especially not for
>>>> a BASE library (class). What type exactly did you need this include for?
>>>>
>>>>
>>> I should be able to remove the inclusion of this header.
>>>
>>>
>>>>> +#include <Base.h>
>>>>> +
>>>>> +/**
>>>>> +
>>>>> + Initialize SEV memory encryption
>>>>> +
>>>>> +**/
>>>>> +
>>>>> +RETURN_STATUS
>>>>> +EFIAPI
>>>>> +MemcryptSevInitialize (
>>>>> +  VOID
>>>>> +  );
>>>>> +
>>>>> +/**
>>>>> +
>>>>> + Return TRUE when SEV is active otherwise FALSE
>>>>> +
>>>>> + **/
>>>>
>>>> Is this function restricted to code that runs after a successful
>>>> invocation of MemcryptSevInitialize()? If so, please document that.
>>>>
>>>>
>>> Both are independent functions, SevActive() function returns whether
>>> SEV is active or not. Whereas SevInitialize(), calls SevActive() and
>>> if SEV is active then it sets the dynamic PtePcdMemoryEncryptionMask.
>>>
>>>
>>> Please also document the return values (with @retval and/or @return),
>>>> even though the explanation is likely trivial.
>>>>
>>>> I will document it.
>>>
>>>
>>>
>>>>> +BOOLEAN
>>>>> +EFIAPI
>>>>> +SevActive (
>>>>> +  VOID
>>>>> +  );
>>>>> +
>>>>
>>>> I'd prefer if we could use a common prefix for the two functions,
>>>> something that is unique to / characteristic of the new library class.
>>>>
>>>>
>>> Will fix it.  In general, are we are okay with MemcryptSevLib naming
>>> convensio ?
>>
>> Regarding library naming conventions, we can consider functions, and library
>> instance names.
>>
>> For functions, I tend to prefer a common prefix, although I don't believe this
>> is a hard requirement in edk2.
>>
>> For library instance names, the naming convention seems to be:
>>
>> <PHASE>LibClassName<VARIANT>
>>
>> Where <PHASE> describes the phases the library instance can be used in (so,
>> Base, Dxe, Pei, PeiDxe, and so on), and <VARIANT> describes, very tersely,
>> the underlying implementation (for example, Null means the library does
>> nothing). I think <VARIANT> can be omitted if there's only one library
>> instance.
>>
>>
>>> If we are okay with that then I was going to prefix all the function
>>> with Sev e.g
>>>
>>> SevInitialize()  : this sets the dynamic PCD
>>> SevActive()     : returns TRUE when SEV is active
>>>
>>>> +#endif
>>>>> +#include <Library/BaseLib.h>
>>>>> +#include <Library/DebugLib.h>
>>>>> +#include <Library/PcdLib.h>
>>>>> +#include <Library/MemcryptSevLib.h>
>>>>> +
>>>>> +#define KVM_FEATURE_MEMORY_ENCRYPTION 0x100
>>>>
>>>> Can you move all these magic constants (CPUID leaves as well) to
>>>> OvmfPkg/Include/IndustryStandard/? I guess the filename could be
>>>> "AmdSev.h", or something similar.
>>>>
>>>> Such header files are also prime locations to capture the great
>>>> standards references that you added to the blurb.
>>>>
>>>>
>>> Will do it.
>>>
>>>
>>>>> +
>>>>> +RETURN_STATUS
>>>>> +EFIAPI
>>>>> +MemcryptSevInitialize (
>>>>> +  VOID
>>>>> +  )
>>>>> +{
>>>>> +  UINT32 EBX;
>>>>
>>>> Should be Ebx, IMO.
>>>>
>>>>
>>> Will fix it
>>>
>>>> +  UINT64 MeMask = 0;
>>>>
>>>> Initialization of local variables is forbidden by the edk2 coding style.
>>>>
>>>>
>>> Will fix it.
>>>
>>>
>>>>> +
>>>>> +  if (SevActive ()) {
>>>>
>>>> Aha! So it's the other way around than I expected.
>>>>
>>>>> +     // CPUID Fn8000_001f[EBX] - Bit 5:0 (memory encryption bit
>>>> position)
>>>>
>>>> Comment style is
>>>>
>>>> //
>>>> // CPUID ...
>>>> //
>>>>
>>>>
>>> Will fix it.
>>>
>>>
>>>>> +     AsmCpuid(0x8000001f, NULL, &EBX, NULL, NULL);
>>>>> +     MeMask = (1ULL << (EBX & 0x3f));
>>>>
>>>> You can't bit shift 64-bit values in edk2 with the C-language
>>>> operator; it won't build for Ia32.
>>>>
>>>> Please either use LShiftU64() here, or -- if you are sure the code
>>>> will never be reached in Ia32 guests -- use (UINTN)1, and shift that with
>> <<.
>>>>
>>>>
>>> I will fix it to use LShiftU64() version.
>>>
>>>
>>>> BTW, in what PI phases do you intend to use this library? For
>>>> example, in the Ia32X64 build of OVMF, the PEI phase runs as 32-bit,
>>>> and the DXE phase runs in 64-bit.
>>>>
>>>>
>>> So my rational behind creating a new library was to have a flexibilty
>>> of adding more SEV specifc functions. The functions which I have mind is:
>>>
>>> SevChangeMemoryAttributeEncrypted(Address, Length) : set the
>>> encryption attribute SevChangeMemoryAttributeDecrypted(Address,
>>> Length) :  clear the encryption attribute
>>>
>>> SevChangeMemoryAttribute*() are not implemented in this RFC and I am
>>> working to add in next rev. This will be mainly execute in Dxe phase.
>>> These functions will be update the pagetable to clear/set encryption
>>> masks. It will be mainly used for Dma libraries and possibly
>>> QemuVideoDxe (since framebuffer is shared between HV and Guest
>> hence
>>> we will need to clear the encryption attribute of framebuffer memory).
>>>
>>> I would potentially add two libraries:
>>>
>>> * SevBaseLib: it provides SevActive() and SevInitialize() routines
>>> which can be called anytime
>>> * SevDxeLib: it will provide routines which can be called used by Dxe
>>> drivers.
>>>
>>> Does it make sense to you. I am open to suggestions.
>>
>> I think SevActive() and SevInitialize() should become part of PlatformPei only
>> (if that's possible).
>>
>> The upcoming PTE massaging functions could become part of the DMA lib
>> stuff that you mention (as functions with external linkage), and then you
>> could pull the DMA lib into QemuVideoDxe just to make these functions
>> available.
>>
>> Presently the suggested functions don't seem to justify two (or even
>> one) new libclass.
>>
>> Thanks
>> Laszlo
>>
>>>
>>> Hm, in the next patch, it seems that the library is put to use in
>>>> PlatformPei (and only there). Since these functions are really small,
>>>> I think I would prefer having a new file under OvmfPkg/PlatformPei only.
>>>>
>>>> More on the Ia32X64 build of OVMF -- assuming the PEI phase runs in
>>>> 32-bit, but the DXE phase (and the OS) run in 64-bit, does SEV appear
>>>> active when the 32-bit PlatformPei module queries it?
>>>>
>>>> Yes SEV will appear active on both Ia32X64 and X64. I have tried
>>>> running
>>> the OVMF build with both version.
>>>
>>>
>>>> * If it doesn't, that's a problem, because then the PCD will be set
>>>> incorrectly. In this case, it might make sense to limit SEV only to
>>>> the
>>>> X64 build of OVMF.
>>>>
>>>> * If SEV does appear active when the 32-bit PlatformPei module
>>>> queries it, then we definitely need to use LShiftU64 here.
>>>>
>>>> Have you tested this series in all three builds of OVMF? (Ia32,
>>>> Ia32X64, and X64?) I'm not asking about SMM, I can see it's on the TODO
>> list.
>>>>
>>>> I have tried Ia3264 and X64 but have not tried Ia32. I will try to
>>>> and let
>>> you know If I find any issues.
>>>
>>>
>>>>> +     DEBUG ((DEBUG_INFO, "KVM Secure Encrypted Virtualization (SEV)
>>>>> + is
>>>> enabled\n"));
>>>>> +     DEBUG ((DEBUG_INFO, "MemEncryptionMask 0x%lx\n", MeMask));
>> }
>>>>> +
>>>>> +  PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask);
>>>>
>>>> Whiile we do exped PcdSet64S to succeed, please add a separate Status
>>>> variable, and add an ASSERT_RETURN_ERROR on that.
>>>>
>>>>
>>> Will do
>>>
>>>> +
>>>>> +  return RETURN_SUCCESS;
>>>>> +}
>>>>> +
>>>>> +BOOLEAN
>>>>> +EFIAPI
>>>>> +SevActive (
>>>>> +  VOID
>>>>> +  )
>>>>> +{
>>>>> +  UINT32 KVMFeatures, EAX;
>>>>
>>>> Should be KvmFeatures and Eax.
>>>>
>>>>> +
>>>>> +  // Check if KVM memory encyption feature is set
>>>>
>>>> comment style
>>>>
>>>>> +  AsmCpuid(0x40000001, &KVMFeatures, NULL, NULL, NULL);  if
>>>>> + (KVMFeatures & KVM_FEATURE_MEMORY_ENCRYPTION) {
>>>>> +
>>>>> +     // Check whether SEV is enabled
>>>>> +     // CPUID Fn8000_001f[EAX] - Bit 0  (SEV is enabled)
>>>>> +     AsmCpuid(0x8000001f, &EAX, NULL, NULL, NULL);
>>>>
>>>> Tom commented on this -- the result is not checked.
>>>>
>>>>
>>> Will fix it.
>>>
>>>
>>>>> +
>>>>> +     return TRUE;
>>>>> +  }
>>>>> +
>>>>> +  return FALSE;
>>>>> +}
>>>>> +
>>>>> diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>> b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>> new file mode 100644
>>>>> index 0000000..8e8d7e0
>>>>> --- /dev/null
>>>>> +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>> @@ -0,0 +1,44 @@
>>>>> +## @file
>>>>> +#
>>>>> +# Copyright (c) 2017 Advanced Micro Devices. All rights
>>>>> +reserved.<BR> # #  This program and the accompanying materials #
>>>>> +are licensed and made available under the terms and conditions of
>>>> the BSD License
>>>>> +#  which accompanies this distribution. The full text of the
>>>>> +license
>>>> may be found at
>>>>> +#  http://opensource.org/licenses/bsd-license.php
>>>>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS
>> IS"
>>>>> +BASIS, #  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> EITHER
>>>>> +EXPRESS OR
>>>> IMPLIED.
>>>>> +#
>>>>> +#
>>>>> +##
>>>>> +
>>>>> +[Defines]
>>>>> +  INF_VERSION                    = 0x00010005
>>>>> +  BASE_NAME                      = MemcryptSevLib
>>>>> +  FILE_GUID                      = c1594631-3888-4be4-949f-9c630dbc842b
>>>>> +  MODULE_TYPE                    = BASE
>>>>> +  VERSION_STRING                 = 1.0
>>>>> +  LIBRARY_CLASS                  = MemcryptSevLib
>>>>> +
>>>>> +#
>>>>> +# The following information is for reference only and not required
>>>>> +by
>>>> the build tools.
>>>>> +#
>>>>> +# VALID_ARCHITECTURES           = IA32 X64
>>>>> +#
>>>>> +
>>>>> +[Packages]
>>>>> +  MdePkg/MdePkg.dec
>>>>> +  MdeModulePkg/MdeModulePkg.dec
>>>>> +  OvmfPkg/OvmfPkg.dec
>>>>> +  UefiCpuPkg/UefiCpuPkg.dec
>>>>> +
>>>>> +[Sources]
>>>>> +  MemcryptSevLib.c
>>>>> +
>>>>> +[LibraryClasses]
>>>>> +  BaseLib
>>>>> +  DebugLib
>>>>> +  PcdLib
>>>>> +
>>>>> +[Pcd]
>>>>> +
>>>>>
>> +gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOr
>> Mask
>>>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc
>> b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>>> index 56f7ff9..a35e1d2 100644
>>>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>>> @@ -129,6 +129,7 @@
>>>>>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>>>>>    VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>>>>>    LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>>>>> +
>> MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>>  !if $(SMM_REQUIRE) == FALSE
>>>>>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>>>>>  !endif
>>>>
>>>> Please configure your git instance so that it includes the section
>>>> names of INI-style files in hunk headers:
>>>>
>>>> https://github.com/tianocore/tianocore.github.io/wiki/
>>>> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
>>>> maintainers#contrib-05
>>>>
>>>> see xfuncname there, and:
>>>>
>>>> https://github.com/tianocore/tianocore.github.io/wiki/
>>>> Laszlo's-unkempt-git-guide-for-edk2-contributors-and-
>>>> maintainers#contrib-09
>>>>
>>>>
>>>
>>> I will go through INI-Style file and git setting etc.
>>>
>>>
>>>>
>>>>> @@ -509,6 +510,9 @@
>>>>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>>>>>
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>>>>>
>>>>> +  # Set memory encryption mask
>>>>> +
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
>>>> rMask|0x0
>>>>> +
>>>>>  !if $(SMM_REQUIRE) == TRUE
>>>>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
>>>>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
>>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index
>>>>> d0b0b0e..5d853d6 100644
>>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>>> @@ -129,6 +129,7 @@
>>>>>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>>>>>    VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>>>>>    LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
>>>>> +
>> MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf
>>>>>  !if $(SMM_REQUIRE) == FALSE
>>>>>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>>>>>  !endif
>>>>> @@ -508,6 +509,9 @@
>>>>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>>>>>
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>>>>>
>>>>> +  # Set memory encryption mask
>>>>> +
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO
>>>> rMask|0x0
>>>>> +
>>>>>  !if $(SMM_REQUIRE) == TRUE
>>>>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
>>>>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
>>>>>
>>>>
>>>> The OvmfPkgIa32.dsc file is not modified. The MemcryptSevLib class is
>>>> not resolved for the Ia32 build, hence PlatformPei will fail to build
>>>> in the next patch.
>>>>
>>>> Please build and regression-test all three builds of OVMF (SMM
>>>> disabled, for now) in the development of this feature.
>>>>
>>>> Regression-testing should in particular include S3 suspend/resume
>>>> (again, with SMM disabled, for now). If it breaks (and it is expected
>>>> to break), please extend the S3Verification() function so that it
>>>> prevents the firmware from booting (with a reasonable error message)
>>>> if it sees that SEV is active and S3 was *not* disabled on the QEMU
>> command line.
>>>> Losing a guest at S3 resume is very annoying, it's better not to boot
>>>> in that case (and to ask the user to disable S3 on the QEMU command
>> line).
>>>>
>>>> Anyhow, I think these functions should go directly under
>>>> OvmfPkg/PlatformPei, into a new file.
>>>>
>>>> I may have missed a few things, but I'm (theoretically) at a
>>>> conference, and right now it seems more correct for me to give
>>>> (perhaps spotty) feedback *quickly*, than to let my backlog pile up.
>>>>
>>>>
>>> Appriciate your quick feedbacks, I will go through each of them and
>>> will try to address in v2.
>>>
>>>
>>>> Thanks!
>>>> Laszlo
>>>>
>>>
>>>
>>>
>