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

[Brijesh Singh <brijesh.ksingh@gmail.com>]

On Mon, Mar 27, 2017 at 3:23 AM, Laszlo Ersek <lersek@redhat.com> wrote:

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



> >
> > 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.PcdPteMemoryEncryptionAddressO
> rMask|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).
>
>
Thanks for the pointer, I will configure our project based on the settings
described in the wiki.



> >  !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.PcdPteMemoryEncryptionAddressO
> rMask|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.PcdPteMemoryEncryptionAddressO
> rMask|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.
> **/
>
>

I will update comment.



> > +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;
>
>
>
Will do



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

Will fix both alignment/layout and rename the variable.



> > +
> > +  //
> > +  // 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);
>
>

Okay, will update the code.



> > +
> > +  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.
>
>
Ah, I was not aware of this. I may have used this macro in other patches. I
will go and fix them.



> > +}
> > 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.
>
>
I did run a S3 resume and could see the code was kicking in and we were
able to resume fine.



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


Sure I will update gitconfig file with those settings.


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


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




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


-- 
Confusion is always the most honest response.