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

["Duran, Leo" <leo.duran@amd.com>]

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

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
> >>
> >
> >
> >