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
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
+ 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>
+#include <Base.h>
+
+/**
+
+ Initialize SEV memory encryption
+
+**/
+
+RETURN_STATUS
+EFIAPI
+MemcryptSevInitialize (
+ VOID
+ );
+
+/**
+
+ Return TRUE when SEV is active otherwise FALSE
+
+ **/
+BOOLEAN
+EFIAPI
+SevActive (
+ VOID
+ );
+
+#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
+
+RETURN_STATUS
+EFIAPI
+MemcryptSevInitialize (
+ VOID
+ )
+{
+ UINT32 EBX;
+ UINT64 MeMask = 0;
+
+ if (SevActive ()) {
+ // CPUID Fn8000_001f[EBX] - Bit 5:0 (memory encryption bit position)
+ AsmCpuid(0x8000001f, NULL, &EBX, NULL, NULL);
+ MeMask = (1ULL << (EBX & 0x3f));
+ DEBUG ((DEBUG_INFO, "KVM Secure Encrypted Virtualization (SEV) is enabled\n"));
+ DEBUG ((DEBUG_INFO, "MemEncryptionMask 0x%lx\n", MeMask));
+ }
+
+ PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask);
+
+ return RETURN_SUCCESS;
+}
+
+BOOLEAN
+EFIAPI
+SevActive (
+ VOID
+ )
+{
+ UINT32 KVMFeatures, EAX;
+
+ // Check if KVM memory encyption feature is set
+ 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);
+
+ 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
@@ -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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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 ? 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.
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
>
--
Confusion is always the most honest response.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>>
>
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/07/17 20:14, Brijesh Singh wrote: > > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com> wrote: > 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. > I think I should be able to accomate SevInitialize() and SevActive() function inside the PlatformPei. I will drop MemcryptSevLib library in next rev. I will go with your idea for adding PTE massaging function directly inside the DMA library and will link that into QemuVideoDxe. Only part which I have not yet figured out, how to deal with Qemu FW_CFG DMA support, I believe some of FW_CFG DMA read and write happens fairly early (PEI stage). The PTE massaging code may need to allocate memory and not sure how to allocate dynamic memory in early stages. Any pointers ? Thanks Brijesh > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/07/17 23:36, Brijesh Singh wrote: > > > On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com > <mailto:lersek@redhat.com>> wrote: > > On 03/07/17 20:14, Brijesh Singh wrote: > > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com > <mailto:lersek@redhat.com>> wrote: > 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. > > > > I think I should be able to accomate SevInitialize() and SevActive() > function inside the PlatformPei. I will drop MemcryptSevLib library in > next rev. > > I will go with your idea for adding PTE massaging function directly > inside the DMA library and will link that into QemuVideoDxe. > > Only part which I have not yet figured out, how to deal with Qemu > FW_CFG DMA support, I believe some of FW_CFG DMA read and write > happens fairly early (PEI stage). That's right, off the top of my head, minimally PlatformPei uses fw_cfg heavily during PEI. > The PTE massaging code may need to > allocate memory and not sure how to allocate dynamic memory in early stages. > Any pointers ? You can use MemoryAllocationLib functions for that (such as AllocatePool() and AllocatePages()). The OVMF DSC files resolve the lib class for the PEI phase like this: MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf and the PeiMemoryAllocationLib instance maps those functions to the PEI services. A few important things about this: - AllocatePool() works up to only ~64KB in size, and the allocation is backed by a new HOB. Generally speaking, the HOB may be moved to a different spot in memory before entering the DXE phase, so pointers returned by such AllocatePool() calls (in PEI) are not safe to dereference in the DXE phase. - FreePool() does nothing, the allocated memory (the HOB, see above) is only released when the guest OS starts (and it drops all boot services data type memory). - AllocatePages() works as it says on the tin, and the pointer returned by it is safe to dereference in DXE. - FreePages() however is again a no-op, it practically leaks the memory, and only the guest OS will be able to release it (see FreePool() above). One workaround for this could be to stash the address of the PEI-phase allocation in a GUID HOB or a PCD, and then let some DXE driver in the DXE phase release the memory with gBS->FreePages(). I'm not sure though if this complexity is worth it. - Note that it is PlatformPei itself that installs the permanent PEI RAM. Before that happens, PEIMs (including PlatformPei itself) can only allocate memory from the temporary SEC/PEI heap, which is very very small, and only AllocatePool() would work at that point (AllocatePages() wouldn't). However, if you place the AllocatePages() function call after PublishPeiMemory(), then things should work. As far as I can see, you added MemcryptSevInitialize() to the end of InitializePlatform(); allocating pages at that point should be fine. - During S3 resume, a different (pre-reserved) memory area is used as permanent PEI RAM, which is quite smaller than the one used during normal boot. It means that, if you need a lot of memory for setting up SEV during S3 resume, AllocatePages() might run out of memory, and we might have to resize the pre-reservation mentioned above. - If you could reasonably bound the allocation size with a constant, it might be simplest to use static arrays / variables. Those would be dropped as soon as the PEI phase was exited. As one quirk however, you should not rely on such variables being zero-initialized during S3 resume. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, Mar 8, 2017 at 2:40 AM, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/07/17 23:36, Brijesh Singh wrote: > > > > > > On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com > > <mailto:lersek@redhat.com>> wrote: > > > > On 03/07/17 20:14, Brijesh Singh wrote: > > > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com > > <mailto:lersek@redhat.com>> wrote: > > 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. > > > > > > > > I think I should be able to accomate SevInitialize() and SevActive() > > function inside the PlatformPei. I will drop MemcryptSevLib library in > > next rev. > > > > I will go with your idea for adding PTE massaging function directly > > inside the DMA library and will link that into QemuVideoDxe. > > > > Only part which I have not yet figured out, how to deal with Qemu > > FW_CFG DMA support, I believe some of FW_CFG DMA read and write > > happens fairly early (PEI stage). > > That's right, off the top of my head, minimally PlatformPei uses fw_cfg > heavily during PEI. > > > The PTE massaging code may need to > > allocate memory and not sure how to allocate dynamic memory in early > stages. > > Any pointers ? > > You can use MemoryAllocationLib functions for that (such as > AllocatePool() and AllocatePages()). The OVMF DSC files resolve the lib > class for the PEI phase like this: > > > MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/ > PeiMemoryAllocationLib.inf > > and the PeiMemoryAllocationLib instance maps those functions to the PEI > services. > > A few important things about this: > > - AllocatePool() works up to only ~64KB in size, and the allocation is > backed by a new HOB. Generally speaking, the HOB may be moved to a > different spot in memory before entering the DXE phase, so pointers > returned by such AllocatePool() calls (in PEI) are not safe to > dereference in the DXE phase. > > - FreePool() does nothing, the allocated memory (the HOB, see above) is > only released when the guest OS starts (and it drops all boot services > data type memory). > > - AllocatePages() works as it says on the tin, and the pointer returned > by it is safe to dereference in DXE. > > I took a stab at implementing SEV specific DMA hooks into QemuFwCfgLib. But I found that QemuFWCfgLib is used in both PEI and Dxe phases. It makes things interesting, in SEV guest we can perform DMA operation only when processor is either in 32-bit PAE or long mode (mainly because C-bit is not accessiable in 32 or 16-bit mode). It limits us to using OvmfX64.dsc. Ideally, I would prefer to support both OvmfPkgIa32X64.dsc and OvmfPkgX64.dsc. In my code browsing so far I have found that only QemuFwCfgLib does DMA operations in PEI phase and other packages perform the DMA operation in DXE phase. If we can somehow manage to not require the DMA support in PEI phase then we should be able to support both OvmfPkIa32X64.dsc and OvmfPkgX64.dsc. How about defaulting to I/O operations in PEI stage for SEV guest ? I do understand that QEMU prefers us to use DMA interface for FwCfg write. Additionally, I found that some FwCfg DMA access happens before PublishPeiMemory() hence AllocatePages() was failing to allocate the bounce buffer for SEV DMA. I was thinking that for SEV guest if we get a request to perform smaller reads or writes (maybe < 64 bytes) then silently fallback to IO else perform DMA operations. Thoughts ? -Brijesh - FreePages() however is again a no-op, it practically leaks the memory, > and only the guest OS will be able to release it (see FreePool() above). > One workaround for this could be to stash the address of the PEI-phase > allocation in a GUID HOB or a PCD, and then let some DXE driver in the > DXE phase release the memory with gBS->FreePages(). I'm not sure though > if this complexity is worth it. > > - Note that it is PlatformPei itself that installs the permanent PEI > RAM. Before that happens, PEIMs (including PlatformPei itself) can only > allocate memory from the temporary SEC/PEI heap, which is very very > small, and only AllocatePool() would work at that point (AllocatePages() > wouldn't). However, if you place the AllocatePages() function call after > PublishPeiMemory(), then things should work. > > As far as I can see, you added MemcryptSevInitialize() to the end of > InitializePlatform(); allocating pages at that point should be fine. > > - During S3 resume, a different (pre-reserved) memory area is used as > permanent PEI RAM, which is quite smaller than the one used during > normal boot. It means that, if you need a lot of memory for setting up > SEV during S3 resume, AllocatePages() might run out of memory, and we > might have to resize the pre-reservation mentioned above. > > - If you could reasonably bound the allocation size with a constant, it > might be simplest to use static arrays / variables. Those would be > dropped as soon as the PEI phase was exited. As one quirk however, you > should not rely on such variables being zero-initialized during S3 resume. > > Thanks > Laszlo > -- Confusion is always the most honest response. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/17/17 03:02, Brijesh Singh wrote: > > > On Wed, Mar 8, 2017 at 2:40 AM, Laszlo Ersek <lersek@redhat.com > <mailto:lersek@redhat.com>> wrote: > > On 03/07/17 23:36, Brijesh Singh wrote: > > > > > > On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com> > > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote: > > > > On 03/07/17 20:14, Brijesh Singh wrote: > > > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com> > > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote: > > 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. > > > > > > > > I think I should be able to accomate SevInitialize() and SevActive() > > function inside the PlatformPei. I will drop MemcryptSevLib library in > > next rev. > > > > I will go with your idea for adding PTE massaging function directly > > inside the DMA library and will link that into QemuVideoDxe. > > > > Only part which I have not yet figured out, how to deal with Qemu > > FW_CFG DMA support, I believe some of FW_CFG DMA read and write > > happens fairly early (PEI stage). > > That's right, off the top of my head, minimally PlatformPei uses fw_cfg > heavily during PEI. > > > The PTE massaging code may need to > > allocate memory and not sure how to allocate dynamic memory in early stages. > > Any pointers ? > > You can use MemoryAllocationLib functions for that (such as > AllocatePool() and AllocatePages()). The OVMF DSC files resolve the lib > class for the PEI phase like this: > > > MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf > > and the PeiMemoryAllocationLib instance maps those functions to the PEI > services. > > A few important things about this: > > - AllocatePool() works up to only ~64KB in size, and the allocation is > backed by a new HOB. Generally speaking, the HOB may be moved to a > different spot in memory before entering the DXE phase, so pointers > returned by such AllocatePool() calls (in PEI) are not safe to > dereference in the DXE phase. > > - FreePool() does nothing, the allocated memory (the HOB, see above) is > only released when the guest OS starts (and it drops all boot services > data type memory). > > - AllocatePages() works as it says on the tin, and the pointer returned > by it is safe to dereference in DXE. > > > > I took a stab at implementing SEV specific DMA hooks into QemuFwCfgLib. > But I found that QemuFWCfgLib is used in both PEI and Dxe phases. > It makes things interesting, in SEV guest we can perform DMA operation only > when processor is either in 32-bit PAE or long mode (mainly because C-bit > is not accessiable in 32 or 16-bit mode). It limits us to using OvmfX64.dsc. > > Ideally, I would prefer to support both OvmfPkgIa32X64.dsc and > OvmfPkgX64.dsc. > In my code browsing so far I have found that only QemuFwCfgLib does DMA > operations in PEI phase and other packages perform the DMA operation in > DXE phase. > If we can somehow manage to not require the DMA support in PEI phase then we > should be able to support both OvmfPkIa32X64.dsc and OvmfPkgX64.dsc. > How about defaulting to I/O operations in PEI stage for SEV guest ? I suggest the following: (1) Introduce an internal API to "QemuFwCfgLibInternal.h", called InternalQemuFwCfgSevIsEnabled(). (2) Implement InternalQemuFwCfgSevIsEnabled() in "QemuFwCfgSec.c" without using global variables (i.e., with a CPUID on each call, on AMD processors, and return constant FALSE on Intel processors). (3) Create two copies of "QemuFwCfgPeiDxe.c", using the following names: QemuFwCfgPei.c, QemuFwCfgDxe.c, and then delete the original. (4) Accordingly, create two copies of "QemuFwCfgLib.inf", QemuFwCfgPeiLib.inf and QemuFwCfgDxeLib.inf, then delete the original. Don't forget to update the FILE_GUID defines. Additionally, update the library client module type restrictions in each (near LIBRARY_CLASS). (5) In QemuFwCfgDxeLib.inf / QemuFwCfgDxe.c, the constructor function should detect SEV, and store the result in a global variable. (As far as I remember, this detection should only happen on AMD processors, on Intel processors, the CPUID should not even be attempted -- is that right?) The InternalQemuFwCfgSevIsEnabled() function should be implemented to return the global variable. (6) In QemuFwCfgPeiLib.inf / QemuFwCfgPei.c, InternalQemuFwCfgSevIsEnabled() should be implemented the same way, but the constructor function should also avoid setting mQemuFwCfgDmaSupported to TRUE if SEV is detected. (7) In "QemuFwCfgLib.c", the InternalQemuFwCfgDmaBytes() function should use a bounce buffer if InternalQemuFwCfgSevIsEnabled() returns TRUE. (Unfortunately, InternalQemuFwCfgDmaBytes() is not capable of returning an error, so if the bounce buffer allocation fails, you'll have to call ASSERT_EFI_ERROR (Status), and then an explicit CpuDeadLoop() too.) Note that skip operations never need a bounce buffer. The end result is that PEIMs will fall back to "IO only" if SEV is enabled (*), while DXE clients will use bounce buffers with DMA if SEV is enabled. (*) It's OK to use less performant solutions in PEI. > I do > understand > that QEMU prefers us to use DMA interface for FwCfg write. ... It's also OK to use less functional solutions in PEI. Normally, no fw_cfg write occurs in the PEI phase. Unfortunately, during S3 resume, fw_cfg writes (with DMA) do occur in PEI, performed by the stashed BootScriptExecutorDxe driver. And, in the S3 boot script, you cannot allocate memory (for bounce buffering) *dynamically*. However, in the S3 boot script, you cannot allocate memory dynamically for any other purpose either! Which is why QemuFwCfgS3Lib already pre-allocates reserved scratch space anyway, for the boot script opcodes (and QEMU) to operate upon. Thus, it should be possible to customize that too -- find the AllocateReservedPool() call in "QemuFwCfgS3Dxe.c", and make it allocate non-encrypted full pages instead, if SEV is enabled (you can call CPUID right there, on AMD processors). This way, whenever a DXE driver saves fw_cfg DMA writes into the ACPI S3 boot script, to be run on S3 resume, the scratch space that it allocates in advance (for both the DMA control struct and the data to transfer), via QemuFwCfgS3CallWhenBootScriptReady(), will be plain-text. > > Additionally, I found that some FwCfg DMA access happens before > PublishPeiMemory() > hence AllocatePages() was failing to allocate the bounce buffer for SEV DMA. > I was thinking that for SEV guest if we get a request to perform smaller > reads or writes > (maybe < 64 bytes) then silently fallback to IO else perform DMA operations. This should not be necessary with the above approach. (In general, AllocatePages() should not be used for temporary purposes in PEI (for bounce buffering or anything else), because those pages will be leaked for the rest of the firmware (unless something in DXE explicitly frees them up).) Thanks! Laszlo > > Thoughts ? > > -Brijesh > > - FreePages() however is again a no-op, it practically leaks the memory, > > and only the guest OS will be able to release it (see FreePool() above). > One workaround for this could be to stash the address of the PEI-phase > allocation in a GUID HOB or a PCD, and then let some DXE driver in the > DXE phase release the memory with gBS->FreePages(). I'm not sure though > if this complexity is worth it. > > - Note that it is PlatformPei itself that installs the permanent PEI > RAM. Before that happens, PEIMs (including PlatformPei itself) can only > allocate memory from the temporary SEC/PEI heap, which is very very > small, and only AllocatePool() would work at that point (AllocatePages() > wouldn't). However, if you place the AllocatePages() function call after > PublishPeiMemory(), then things should work. > > As far as I can see, you added MemcryptSevInitialize() to the end of > InitializePlatform(); allocating pages at that point should be fine. > > - During S3 resume, a different (pre-reserved) memory area is used as > permanent PEI RAM, which is quite smaller than the one used during > normal boot. It means that, if you need a lot of memory for setting up > SEV during S3 resume, AllocatePages() might run out of memory, and we > might have to resize the pre-reservation mentioned above. > > - If you could reasonably bound the allocation size with a constant, it > might be simplest to use static arrays / variables. Those would be > dropped as soon as the PEI phase was exited. As one quirk however, you > should not rely on such variables being zero-initialized during S3 > resume. > > Thanks > Laszlo > > > > > -- > Confusion is always the most honest response. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Fri, Mar 17, 2017 at 5:29 AM, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/17/17 03:02, Brijesh Singh wrote: > > > > > > On Wed, Mar 8, 2017 at 2:40 AM, Laszlo Ersek <lersek@redhat.com > > <mailto:lersek@redhat.com>> wrote: > > > > On 03/07/17 23:36, Brijesh Singh wrote: > > > > > > > > > On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com > <mailto:lersek@redhat.com> > > > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote: > > > > > > On 03/07/17 20:14, Brijesh Singh wrote: > > > > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek < > lersek@redhat.com <mailto:lersek@redhat.com> > > > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote: > > > 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. > > > > > > > > > > > > I think I should be able to accomate SevInitialize() and > SevActive() > > > function inside the PlatformPei. I will drop MemcryptSevLib > library in > > > next rev. > > > > > > I will go with your idea for adding PTE massaging function directly > > > inside the DMA library and will link that into QemuVideoDxe. > > > > > > Only part which I have not yet figured out, how to deal with Qemu > > > FW_CFG DMA support, I believe some of FW_CFG DMA read and write > > > happens fairly early (PEI stage). > > > > That's right, off the top of my head, minimally PlatformPei uses > fw_cfg > > heavily during PEI. > > > > > The PTE massaging code may need to > > > allocate memory and not sure how to allocate dynamic memory in > early stages. > > > Any pointers ? > > > > You can use MemoryAllocationLib functions for that (such as > > AllocatePool() and AllocatePages()). The OVMF DSC files resolve the > lib > > class for the PEI phase like this: > > > > > > MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/ > PeiMemoryAllocationLib.inf > > > > and the PeiMemoryAllocationLib instance maps those functions to the > PEI > > services. > > > > A few important things about this: > > > > - AllocatePool() works up to only ~64KB in size, and the allocation > is > > backed by a new HOB. Generally speaking, the HOB may be moved to a > > different spot in memory before entering the DXE phase, so pointers > > returned by such AllocatePool() calls (in PEI) are not safe to > > dereference in the DXE phase. > > > > - FreePool() does nothing, the allocated memory (the HOB, see above) > is > > only released when the guest OS starts (and it drops all boot > services > > data type memory). > > > > - AllocatePages() works as it says on the tin, and the pointer > returned > > by it is safe to dereference in DXE. > > > > > > > > I took a stab at implementing SEV specific DMA hooks into QemuFwCfgLib. > > But I found that QemuFWCfgLib is used in both PEI and Dxe phases. > > It makes things interesting, in SEV guest we can perform DMA operation > only > > when processor is either in 32-bit PAE or long mode (mainly because C-bit > > is not accessiable in 32 or 16-bit mode). It limits us to using > OvmfX64.dsc. > > > > Ideally, I would prefer to support both OvmfPkgIa32X64.dsc and > > OvmfPkgX64.dsc. > > In my code browsing so far I have found that only QemuFwCfgLib does DMA > > operations in PEI phase and other packages perform the DMA operation in > > DXE phase. > > If we can somehow manage to not require the DMA support in PEI phase > then we > > should be able to support both OvmfPkIa32X64.dsc and OvmfPkgX64.dsc. > > How about defaulting to I/O operations in PEI stage for SEV guest ? > > I suggest the following: > > (1) Introduce an internal API to "QemuFwCfgLibInternal.h", called > InternalQemuFwCfgSevIsEnabled(). > > (2) Implement InternalQemuFwCfgSevIsEnabled() in "QemuFwCfgSec.c" > without using global variables (i.e., with a CPUID on each call, on AMD > processors, and return constant FALSE on Intel processors). > > (3) Create two copies of "QemuFwCfgPeiDxe.c", using the following names: > QemuFwCfgPei.c, QemuFwCfgDxe.c, and then delete the original. > > (4) Accordingly, create two copies of "QemuFwCfgLib.inf", > QemuFwCfgPeiLib.inf and QemuFwCfgDxeLib.inf, then delete the original. > > Don't forget to update the FILE_GUID defines. > > Additionally, update the library client module type restrictions in each > (near LIBRARY_CLASS). > > (5) In QemuFwCfgDxeLib.inf / QemuFwCfgDxe.c, the constructor function > should detect SEV, and store the result in a global variable. (As far as > I remember, this detection should only happen on AMD processors, on > Intel processors, the CPUID should not even be attempted -- is that right?) > > Yes that is right. The CPUID detection function will return TRUE only when SEV is enabled on AMD processor. On Intel it will return FALSE. > The InternalQemuFwCfgSevIsEnabled() function should be implemented to > return the global variable. > > (6) In QemuFwCfgPeiLib.inf / QemuFwCfgPei.c, > InternalQemuFwCfgSevIsEnabled() should be implemented the same way, but > the constructor function should also avoid setting > mQemuFwCfgDmaSupported to TRUE if SEV is detected. > > (7) In "QemuFwCfgLib.c", the InternalQemuFwCfgDmaBytes() function should > use a bounce buffer if InternalQemuFwCfgSevIsEnabled() returns TRUE. > > (Unfortunately, InternalQemuFwCfgDmaBytes() is not capable of returning > an error, so if the bounce buffer allocation fails, you'll have to call > ASSERT_EFI_ERROR (Status), and then an explicit CpuDeadLoop() too.) > > Note that skip operations never need a bounce buffer. > > > The end result is that PEIMs will fall back to "IO only" if SEV is > enabled (*), while DXE clients will use bounce buffers with DMA if SEV > is enabled. > > (*) It's OK to use less performant solutions in PEI. > > > I do > > understand > > that QEMU prefers us to use DMA interface for FwCfg write. > > ... It's also OK to use less functional solutions in PEI. Normally, no > fw_cfg write occurs in the PEI phase. > > Unfortunately, during S3 resume, fw_cfg writes (with DMA) do occur in > PEI, performed by the stashed BootScriptExecutorDxe driver. And, in the > S3 boot script, you cannot allocate memory (for bounce buffering) > *dynamically*. > > However, in the S3 boot script, you cannot allocate memory dynamically > for any other purpose either! > > Which is why QemuFwCfgS3Lib already pre-allocates reserved scratch space > anyway, for the boot script opcodes (and QEMU) to operate upon. > > Thus, it should be possible to customize that too -- find the > AllocateReservedPool() call in "QemuFwCfgS3Dxe.c", and make it allocate > non-encrypted full pages instead, if SEV is enabled (you can call CPUID > right there, on AMD processors). > > This way, whenever a DXE driver saves fw_cfg DMA writes into the ACPI S3 > boot script, to be run on S3 resume, the scratch space that it allocates > in advance (for both the DMA control struct and the data to transfer), > via QemuFwCfgS3CallWhenBootScriptReady(), will be plain-text. > > > > > Additionally, I found that some FwCfg DMA access happens before > > PublishPeiMemory() > > hence AllocatePages() was failing to allocate the bounce buffer for SEV > DMA. > > I was thinking that for SEV guest if we get a request to perform smaller > > reads or writes > > (maybe < 64 bytes) then silently fallback to IO else perform DMA > operations. > > This should not be necessary with the above approach. > > (In general, AllocatePages() should not be used for temporary purposes > in PEI (for bounce buffering or anything else), because those pages will > be leaked for the rest of the firmware (unless something in DXE > explicitly frees them up).) > > Thanks! > Laszlo > > > > > > Thoughts ? > > > > > > -Brijesh > > > > - FreePages() however is again a no-op, it practically leaks the memory, > > > > and only the guest OS will be able to release it (see FreePool() > above). > > One workaround for this could be to stash the address of the > PEI-phase > > allocation in a GUID HOB or a PCD, and then let some DXE driver in > the > > DXE phase release the memory with gBS->FreePages(). I'm not sure > though > > if this complexity is worth it. > > > > - Note that it is PlatformPei itself that installs the permanent PEI > > RAM. Before that happens, PEIMs (including PlatformPei itself) can > only > > allocate memory from the temporary SEC/PEI heap, which is very very > > small, and only AllocatePool() would work at that point > (AllocatePages() > > wouldn't). However, if you place the AllocatePages() function call > after > > PublishPeiMemory(), then things should work. > > > > As far as I can see, you added MemcryptSevInitialize() to the end of > > InitializePlatform(); allocating pages at that point should be fine. > > > > - During S3 resume, a different (pre-reserved) memory area is used as > > permanent PEI RAM, which is quite smaller than the one used during > > normal boot. It means that, if you need a lot of memory for setting > up > > SEV during S3 resume, AllocatePages() might run out of memory, and we > > might have to resize the pre-reservation mentioned above. > > > > - If you could reasonably bound the allocation size with a constant, > it > > might be simplest to use static arrays / variables. Those would be > > dropped as soon as the PEI phase was exited. As one quirk however, > you > > should not rely on such variables being zero-initialized during S3 > > resume. > > > > Thanks > > Laszlo > > > > > > > > > > -- > > Confusion is always the most honest response. > > -- Confusion is always the most honest response. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
For libraries, I've noticed this usage pattern:
INF File:
- Name: <BASE_NAME>.inf
- Path: xxxPkg/Library/<BASE_NAME>/<BASE_NAME>.inf
INCLUDE File:
- Name: <LIBRARY_CLASS>.h
- Path: xxxPkg/Include/Library/<LIBRARY_CLASS>.h
Leo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, March 07, 2017 4:08 PM
> To: Brijesh Singh <brijesh.ksingh@gmail.com>
> Cc: jordan.l.justen@intel.com; edk2-devel@ml01.01.org; Duran, Leo
> <leo.duran@amd.com>; Singh, Brijesh <brijesh.singh@amd.com>; Lendacky,
> Thomas <Thomas.Lendacky@amd.com>
> Subject: Re: [edk2] [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV
> helper library
>
> 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.PcdPteMemoryEncryptionAddressOr
> Mask
> >>> 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
> >>
> >
> >
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 03/08/17 15:56, Duran, Leo wrote:
> For libraries, I've noticed this usage pattern:
>
> INF File:
> - Name: <BASE_NAME>.inf
> - Path: xxxPkg/Library/<BASE_NAME>/<BASE_NAME>.inf
>
> INCLUDE File:
> - Name: <LIBRARY_CLASS>.h
> - Path: xxxPkg/Include/Library/<LIBRARY_CLASS>.h
Correct.
What I described earlier was the relationship between <LIBRARY_CLASS>
and <BASE_NAME>. Namely, <BASE_NAME> is specific to the library
instance, and if you have multiple instances for the same library class,
then there is a convention for composing <BASE_NAME> from <LIBRARY_CLASS>:
<BASE_NAME> := <PHASE><LIBRARY_CLASS><VARIANT>
Laszlo
>
> Leo
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, March 07, 2017 4:08 PM
>> To: Brijesh Singh <brijesh.ksingh@gmail.com>
>> Cc: jordan.l.justen@intel.com; edk2-devel@ml01.01.org; Duran, Leo
>> <leo.duran@amd.com>; Singh, Brijesh <brijesh.singh@amd.com>; Lendacky,
>> Thomas <Thomas.Lendacky@amd.com>
>> Subject: Re: [edk2] [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV
>> helper library
>>
>> 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.PcdPteMemoryEncryptionAddressOr
>> Mask
>>>>> 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
>>>>
>>>
>>>
>>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.