Re: [RFC PATCH v2 03/10] OvmfPkg/PlatformPei: Add Secure Encrypted Virutualization (SEV) support

[Laszlo Ersek <lersek@redhat.com>]

Please fix the typo ("Virutualization") in the subject.

On 03/21/17 22:13, Brijesh Singh wrote:
> Initialize Secure Encrypted Virtualization support and set the memory encryption mask PCD.

Please wrap commit messages at 74 characters.

> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc             |    3 +
>  OvmfPkg/OvmfPkgIa32X64.dsc          |    3 +
>  OvmfPkg/OvmfPkgX64.dsc              |    3 +
>  OvmfPkg/PlatformPei/AmdSev.c        |   97 +++++++++++++++++++++++++++++++++++
>  OvmfPkg/PlatformPei/Platform.c      |    1 
>  OvmfPkg/PlatformPei/Platform.h      |    5 ++
>  OvmfPkg/PlatformPei/PlatformPei.inf |    2 +
>  7 files changed, 114 insertions(+)
>  create mode 100644 OvmfPkg/PlatformPei/AmdSev.c
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 546cdf7..769251d 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -506,6 +506,9 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>  
> +  # Set memory encryption mask
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
> +

Please update your git config as described here:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05

(the "xfuncname" setting in particular,) and here:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09

The point of these settings is that the diff hunk header marked with @@
above will display the DSC section that the hunk modifies (and I'll know
immediately that you are adding a dynamic PCD default).

>  !if $(SMM_REQUIRE) == TRUE
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 383c8d3..3874c35 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -514,6 +514,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 0b7533c..fe7f086 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -513,6 +513,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/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> new file mode 100644
> index 0000000..7f05a9a
> --- /dev/null
> +++ b/OvmfPkg/PlatformPei/AmdSev.c

New file -- can you please double check it is CRLF terminated? Hm,
looking at your "sev-rfc-2" branch, the file does use CRLF. Good.

> @@ -0,0 +1,97 @@
> +/**@file
> +  Initialize Secure Encrypted Virtualization (SEV) support
> +
> +  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

This line is too wide. Please make sure no line is wider than 79 chars.

Please check your patches with

  python BaseTools/Scripts/PatchCheck.py

before submission.

> +  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.
> +
> +**/
> +//
> +// The package level header files this module uses
> +//
> +#include <PiPei.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Register/Cpuid.h>
> +#include <Register/AmdSevMap.h>
> +
> +/**
> +
> +  Function returns 'TRUE' when SEV is enabled otherwise FALSE
> +
> +  **/

One idiomatic way to write this comment is:

/**
  Query whether SEV is enabled.

  @retval TRUE   SEV is enabled.
  @retval FALSE  Otherwise.
**/

> +STATIC
> +BOOLEAN
> +SevIsEnabled (
> +  VOID
> +  )
> +{
> +  UINT32 RegEax;
> +  MSR_SEV_STATUS_REGISTER Msr;
> +  CPUID_MEMORY_ENCRYPTION_INFO_EAX  Eax;

Please align the definitions like this:

  UINT32                           RegEax;
  MSR_SEV_STATUS_REGISTER          Msr;
  CPUID_MEMORY_ENCRYPTION_INFO_EAX Eax;


> +
> +  //
> +  // Check if memory encryption leaf exist
> +  //
> +  AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> +  if (RegEax >= CPUID_MEMORY_ENCRYPTION_INFO) {
> +    //
> +    // CPUID Fn8000_001F[EAX] Bit 1 (Sev supported)
> +    //
> +    AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> +
> +    if (Eax.Bits.SevBit) {
> +      //
> +      // Check MSR_0xC0010131 Bit 0 (Sev Enabled)
> +      //
> +      Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
> +      if (Msr.Bits.SevBit) {
> +        return TRUE;
> +      }
> +    }
> +  }
> +
> +  return FALSE;
> +}
> +
> +/**
> +  Function checks if SEV support is available, if present then it updates
> +  the dynamic PcdPteMemoryEncryptionAddressOrMask with memory encryption mask.
> +
> +  **/
> +VOID
> +EFIAPI
> +AmdSevInitialize (
> +  VOID
> +  )
> +{
> +  UINT64 MeMask;
> +  CPUID_MEMORY_ENCRYPTION_INFO_EBX  Ebx;

Same alignment / layout comment.

Also, I suggest renaming "MeMask" to "EncryptionMask".

> +
> +  //
> +  // Check if SEV is enabled
> +  //
> +  if (!SevIsEnabled ()) {
> +    return;
> +  }
> +
> +  //
> +  // CPUID Fn8000_001F[EBX] Bit 0:5 (memory encryption bit position)
> +  //
> +  AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, NULL, &Ebx.Uint32, NULL, NULL);
> +  MeMask = LShiftU64 (1, Ebx.Bits.PtePosBits);
> +
> +  //
> +  // Set Memory Encryption Mask PCD
> +  //
> +  PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask);

Please save the status of the PcdSet64S() function call, and assert that
it works:

  RETURN_ERROR PcdStatus;

  PcdStatus = PcdSet64S (...);
  ASSERT_RETURN_ERROR (PcdStatus);

> +
> +  DEBUG ((EFI_D_INFO, "SEV support is enabled (mask 0x%lx)\n", MeMask));

We no longer use EFI_D_* macros in new code, please use DEBUG_* instead.

> +}
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 77a8a16..49e6c66 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -667,6 +667,7 @@ InitializePlatform (
>      NoexecDxeInitialization ();
>    }
>  
> +  AmdSevInitialize ();
>    MiscInitialization ();
>    InstallFeatureControlCallback ();
>  

OK, so this is something we do on S3 resume as well... Yes, S3Resume2Pei
consumes this PCD.

> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
> index 18f42c3..a7729b9 100644
> --- a/OvmfPkg/PlatformPei/Platform.h
> +++ b/OvmfPkg/PlatformPei/Platform.h
> @@ -88,6 +88,11 @@ XenDetect (
>    VOID
>    );
>  
> +VOID
> +AmdSevInitialize (
> +  VOID
> +  );
> +
>  extern BOOLEAN mXen;
>  
>  VOID

Again, I request that you please configure your edk2 working tree as
described in the above wiki article. In particular,

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-10

and

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23

will ensure that .h files are formatted before .c files into the
patches, which helps quite a bit with review.

(With newer git, you can set -O permanently for git-format-patch,
through "diff.orderFile".)

> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 53c6dd4..2cf4ac876 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -35,6 +35,7 @@
>    MemDetect.c
>    Platform.c
>    Xen.c
> +  AmdSev.c
>  
>  [Packages]

If possible please try to keep the list of files alphabetically sorted.

>    IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
> @@ -98,6 +99,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask

Ditto for the list of PCDs.

Functionally the patch looks okay to me.

Thanks,
Laszlo

>  
>  [FixedPcd]
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>