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

[Laszlo Ersek <lersek@redhat.com>]

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?

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

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

> +#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.

Please also document the return values (with @retval and/or @return),
even though the explanation is likely trivial.

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

> +#endif
> diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
> new file mode 100644
> index 0000000..2d60b75
> --- /dev/null
> +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c
> @@ -0,0 +1,66 @@
> +/** @file
> +
> +  Copyright (c) 2017, AMD Incorporated. 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.
> +
> +**/
> +
> +#include "Uefi.h"
> +#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.

> +
> +RETURN_STATUS
> +EFIAPI
> +MemcryptSevInitialize (
> +  VOID
> +  )
> +{
> +  UINT32 EBX;

Should be Ebx, IMO.

> +  UINT64 MeMask = 0;

Initialization of local variables is forbidden by the edk2 coding style.

> +
> +  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 ...
//

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

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.

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?

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

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

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

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


> @@ -509,6 +510,9 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>  
> +  # Set memory encryption mask
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|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.PcdPteMemoryEncryptionAddressOrMask|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.

Thanks!
Laszlo