Initialize Secure Encrypted Virtualization support and set the memory encryption mask PCD.
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
+
!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
@@ -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
+ 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
+
+ **/
+STATIC
+BOOLEAN
+SevIsEnabled (
+ VOID
+ )
+{
+ 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;
+
+ //
+ // 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);
+
+ DEBUG ((EFI_D_INFO, "SEV support is enabled (mask 0x%lx)\n", MeMask));
+}
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 ();
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
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]
IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
@@ -98,6 +99,7 @@
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.