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

[Laszlo Ersek <lersek@redhat.com>]

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