* [PATCH v2 00/13] Add GHCBv2 macro and helpers
@ 2021-05-12 23:46 Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition Brijesh Singh
` (12 more replies)
0 siblings, 13 replies; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas,
Michael D Kinney, Liming Gao, Zhiguang Liu
This series is taken from the SNP RFC. This series defines the GHCBv2
macros and NAE events. Additionally, it also introduces a helper to
clear the page encryption mask from the Mmio region.
The series is based on the commit:
5531fd48ded1 BaseTools: Add support for version 3 of FMP Image Header structure
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Change since v1:
- address the review comments
- drop the Mmio specific changes from 'remove Flush .." patch
Brijesh Singh (11):
MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition
MdePkg/Register/Amd: realign macros with more space for future
expansion
MdePkg/Register/Amd: define GHCB macros for hypervisor feature
detection
MdePkg/Register/Amd: define GHCB macro for Register GPA structure
MdePkg/Register/Amd: define GHCB macro for the Page State Change
MdePkg/BaseLib: add support for PVALIDATE instruction
OvmfPkg/BaseMemEncryptSevLib: introduce
MemEncryptSevClearMmioPageEncMask()
OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear
EncMask
OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc
mask
OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask()
OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
Tom Lendacky (2):
MdePkg/Register/Amd: define GHCB macros for SNP AP creation
MdePkg/BaseLib: add support for RMPADJUST instruction
MdePkg/Library/BaseLib/BaseLib.inf | 2 +
MdePkg/Include/Library/BaseLib.h | 85 ++++++++++++
MdePkg/Include/Register/Amd/Fam17Msr.h | 46 ++++++-
MdePkg/Include/Register/Amd/Ghcb.h | 129 +++++++++++++++++-
OvmfPkg/Include/Library/MemEncryptSevLib.h | 35 +++--
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 33 +++--
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 13 +-
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 6 +-
.../Ia32/MemEncryptSevLib.c | 41 ++++--
.../X64/MemEncryptSevLib.c | 49 +++++--
.../X64/PeiDxeVirtualMemory.c | 47 +++++--
.../X64/SecVirtualMemory.c | 38 +++++-
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +-
OvmfPkg/PlatformPei/AmdSev.c | 3 +-
.../FwBlockServiceDxe.c | 5 +-
.../QemuFlashSmm.c | 5 +-
.../TpmMmioSevDecryptPeim.c | 5 +-
MdePkg/Include/X64/Nasm.inc | 16 +++
MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 42 ++++++
MdePkg/Library/BaseLib/X64/RmpAdjust.nasm | 40 ++++++
20 files changed, 554 insertions(+), 89 deletions(-)
create mode 100644 MdePkg/Library/BaseLib/X64/Pvalidate.nasm
create mode 100644 MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
--
2.17.1
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 02/13] MdePkg/Register/Amd: realign macros with more space for future expansion Brijesh Singh
` (11 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas,
Michael D Kinney, Liming Gao, Zhiguang Liu
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
Define the SEV-SNP MSR bits.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Register/Amd/Fam17Msr.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index e4db09c5184c..716d52fd508d 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -87,7 +87,12 @@ typedef union {
///
UINT32 SevEsBit:1;
- UINT32 Reserved:30;
+ ///
+ /// [Bit 2] Secure Nested Paging (SevSnp) is enabled
+ ///
+ UINT32 SevSnpBit:1;
+
+ UINT32 Reserved2:29;
} Bits;
///
/// All bit fields as a 32-bit value
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 02/13] MdePkg/Register/Amd: realign macros with more space for future expansion
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection Brijesh Singh
` (10 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas,
Michael D Kinney, Liming Gao, Zhiguang Liu
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
Version 2 of the GHCB spec introduces several new SNP-specific NAEs.
Unfortunately, the names for those NAEs break the alignment. Add some
white spaces so that the SNP support patches do not break the alignment.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Register/Amd/Fam17Msr.h | 10 +++++-----
MdePkg/Include/Register/Amd/Ghcb.h | 12 ++++++------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index 716d52fd508d..7368ce7af02a 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -53,11 +53,11 @@ typedef union {
UINT64 GhcbPhysicalAddress;
} MSR_SEV_ES_GHCB_REGISTER;
-#define GHCB_INFO_SEV_INFO 1
-#define GHCB_INFO_SEV_INFO_GET 2
-#define GHCB_INFO_CPUID_REQUEST 4
-#define GHCB_INFO_CPUID_RESPONSE 5
-#define GHCB_INFO_TERMINATE_REQUEST 256
+#define GHCB_INFO_SEV_INFO 1
+#define GHCB_INFO_SEV_INFO_GET 2
+#define GHCB_INFO_CPUID_REQUEST 4
+#define GHCB_INFO_CPUID_RESPONSE 5
+#define GHCB_INFO_TERMINATE_REQUEST 256
#define GHCB_TERMINATE_GHCB 0
#define GHCB_TERMINATE_GHCB_GENERAL 0
diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index ccdb662af7a7..712dc8e769c0 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -49,12 +49,12 @@
//
// VMG Special Exit Codes
//
-#define SVM_EXIT_MMIO_READ 0x80000001ULL
-#define SVM_EXIT_MMIO_WRITE 0x80000002ULL
-#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
-#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
-#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
-#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
+#define SVM_EXIT_MMIO_READ 0x80000001ULL
+#define SVM_EXIT_MMIO_WRITE 0x80000002ULL
+#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
+#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
+#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
+#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
//
// IOIO Exit Information
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 02/13] MdePkg/Register/Amd: realign macros with more space for future expansion Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-17 18:19 ` Erdem Aktas
2021-05-12 23:46 ` [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure Brijesh Singh
` (9 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas,
Michael D Kinney, Liming Gao, Zhiguang Liu
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
Version 2 of GHCB introduces advertisement of features that are supported
by the hypervisor. See the GHCB spec section 2.2 for an additional details.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++++++
MdePkg/Include/Register/Amd/Ghcb.h | 8 ++++++++
2 files changed, 15 insertions(+)
diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index 7368ce7af02a..cdb8f588ccf8 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -48,6 +48,11 @@ typedef union {
UINT32 Reserved2:32;
} GhcbTerminate;
+ struct {
+ UINT64 Function:12;
+ UINT64 Features:52;
+ } GhcbHypervisorFeatures;
+
VOID *Ghcb;
UINT64 GhcbPhysicalAddress;
@@ -57,6 +62,8 @@ typedef union {
#define GHCB_INFO_SEV_INFO_GET 2
#define GHCB_INFO_CPUID_REQUEST 4
#define GHCB_INFO_CPUID_RESPONSE 5
+#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
+#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
#define GHCB_TERMINATE_GHCB 0
diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 712dc8e769c0..ec232ebd3807 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -54,6 +54,7 @@
#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
+#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
//
@@ -154,4 +155,11 @@ typedef union {
#define GHCB_EVENT_INJECTION_TYPE_EXCEPTION 3
#define GHCB_EVENT_INJECTION_TYPE_SOFT_INT 4
+//
+// Hypervisor features
+//
+#define GHCB_HV_FEATURES_SNP BIT0
+#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
+#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
+#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (2 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-17 18:25 ` Erdem Aktas
2021-05-12 23:46 ` [PATCH v2 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change Brijesh Singh
` (8 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas,
Michael D Kinney, Liming Gao, Zhiguang Liu
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
An SEV-SNP guest is required to perform the GHCB GPA registration. See
the GHCB specification for further details.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index cdb8f588ccf8..542e4cdf4782 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -53,6 +53,11 @@ typedef union {
UINT64 Features:52;
} GhcbHypervisorFeatures;
+ struct {
+ UINT64 Function:12;
+ UINT64 GuestFrameNumber:52;
+ } GhcbGpaRegister;
+
VOID *Ghcb;
UINT64 GhcbPhysicalAddress;
@@ -62,6 +67,8 @@ typedef union {
#define GHCB_INFO_SEV_INFO_GET 2
#define GHCB_INFO_CPUID_REQUEST 4
#define GHCB_INFO_CPUID_RESPONSE 5
+#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
+#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (3 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-17 19:20 ` Erdem Aktas
2021-05-12 23:46 ` [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation Brijesh Singh
` (7 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas,
Michael D Kinney, Liming Gao, Zhiguang Liu
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
The Page State Change NAE exit will be used by the SEV-SNP guest to
request a page state change using the GHCB protocol. See the GHCB
spec section 4.1.6 and 2.3.1 for more detail on the structure
definitions.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Register/Amd/Fam17Msr.h | 15 ++++++++++++
MdePkg/Include/Register/Amd/Ghcb.h | 33 ++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index 542e4cdf4782..62014854d9b7 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -58,6 +58,19 @@ typedef union {
UINT64 GuestFrameNumber:52;
} GhcbGpaRegister;
+ struct {
+ UINT64 Function:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Operation:4;
+ UINT64 Reserved:8;
+ } SnpPageStateChangeRequest;
+
+ struct {
+ UINT32 Function:12;
+ UINT32 Reserved:20;
+ UINT32 ErrorCode;
+ } SnpPageStateChangeResponse;
+
VOID *Ghcb;
UINT64 GhcbPhysicalAddress;
@@ -69,6 +82,8 @@ typedef union {
#define GHCB_INFO_CPUID_RESPONSE 5
#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE 21
#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index ec232ebd3807..029904b1c63a 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -54,6 +54,7 @@
#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
+#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
@@ -162,4 +163,36 @@ typedef union {
#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
+
+//
+// SNP Page State Change.
+//
+// Note that the PSMASH and UNSMASH operations are not supported when using the MSR protocol.
+//
+#define SNP_PAGE_STATE_PRIVATE 1
+#define SNP_PAGE_STATE_SHARED 2
+#define SNP_PAGE_STATE_PSMASH 3
+#define SNP_PAGE_STATE_UNSMASH 4
+
+typedef struct {
+ UINT64 CurrentPage:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Operation:4;
+ UINT64 PageSize:1;
+ UINT64 Reserved:7;
+} SNP_PAGE_STATE_ENTRY;
+
+typedef struct {
+ UINT16 CurrentEntry;
+ UINT16 EndEntry;
+ UINT32 Reserved;
+} SNP_PAGE_STATE_HEADER;
+
+#define SNP_PAGE_STATE_MAX_ENTRY 253
+
+typedef struct {
+ SNP_PAGE_STATE_HEADER Header;
+ SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
+} SNP_PAGE_STATE_CHANGE_INFO;
+
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (4 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-17 3:08 ` [edk2-devel] " Laszlo Ersek
2021-05-12 23:46 ` [PATCH v2 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction Brijesh Singh
` (6 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Tom Lendacky, James Bottomley, Min Xu, Jiewen Yao, Jordan Justen,
Ard Biesheuvel, Laszlo Ersek, Erdem Aktas, Michael D Kinney,
Liming Gao, Zhiguang Liu, Brijesh Singh
From: Tom Lendacky <thomas.lendacky@amd.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3275
Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is enabled
in the guest VM. See the GHCB specification, Table 5 "List of Supported
Non-Automatic Events" and sections 4.1.9 and 4.3.2, for further details.
While at it, define the VMSA state save area that is required for creating
the AP. The save area format is defined in AMD APM volume 2, Table B-4
(there is a mistake in the table that defines the size of the reserved
area at offset 0xc8 as a dword, when it is actually a word). The format of
the save area segment registers is further defined in AMD APM volume 2,
sections 10 and 15.5.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Register/Amd/Ghcb.h | 76 ++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 029904b1c63a..4d1ee29e0a5e 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -55,6 +55,7 @@
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
+#define SVM_EXIT_SNP_AP_CREATION 0x80000013ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
@@ -83,6 +84,12 @@
#define IOIO_SEG_ES 0
#define IOIO_SEG_DS (BIT11 | BIT10)
+//
+// AP Creation Information
+//
+#define SVM_VMGEXIT_SNP_AP_CREATE_ON_INIT 0
+#define SVM_VMGEXIT_SNP_AP_CREATE 1
+#define SVM_VMGEXIT_SNP_AP_DESTROY 2
typedef PACKED struct {
UINT8 Reserved1[203];
@@ -195,4 +202,73 @@ typedef struct {
SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
} SNP_PAGE_STATE_CHANGE_INFO;
+//
+// SEV-ES save area mapping structures used for SEV-SNP AP Creation.
+// Only the fields required to be set to a non-zero value are defined.
+//
+#define SEV_ES_RESET_CODE_SEGMENT_TYPE 0xA
+#define SEV_ES_RESET_DATA_SEGMENT_TYPE 0x2
+
+#define SEV_ES_RESET_LDT_TYPE 0x2
+#define SEV_ES_RESET_TSS_TYPE 0x3
+
+#pragma pack (1)
+typedef union {
+ struct {
+ UINT16 Type:4;
+ UINT16 Sbit:1;
+ UINT16 Dpl:2;
+ UINT16 Present:1;
+ UINT16 Avl:1;
+ UINT16 Reserved1:1;
+ UINT16 Db:1;
+ UINT16 Granularity:1;
+ } Bits;
+ UINT16 Uint16;
+} SEV_ES_SEGMENT_REGISTER_ATTRIBUTES;
+
+typedef struct {
+ UINT16 Selector;
+ SEV_ES_SEGMENT_REGISTER_ATTRIBUTES Attributes;
+ UINT32 Limit;
+ UINT64 Base;
+} SEV_ES_SEGMENT_REGISTER;
+
+typedef struct {
+ SEV_ES_SEGMENT_REGISTER Es;
+ SEV_ES_SEGMENT_REGISTER Cs;
+ SEV_ES_SEGMENT_REGISTER Ss;
+ SEV_ES_SEGMENT_REGISTER Ds;
+ SEV_ES_SEGMENT_REGISTER Fs;
+ SEV_ES_SEGMENT_REGISTER Gs;
+ SEV_ES_SEGMENT_REGISTER Gdtr;
+ SEV_ES_SEGMENT_REGISTER Ldtr;
+ SEV_ES_SEGMENT_REGISTER Idtr;
+ SEV_ES_SEGMENT_REGISTER Tr;
+ UINT8 Reserved1[42];
+ UINT8 Vmpl;
+ UINT8 Reserved2[5];
+ UINT64 Efer;
+ UINT8 Reserved3[112];
+ UINT64 Cr4;
+ UINT8 Reserved4[8];
+ UINT64 Cr0;
+ UINT64 Dr7;
+ UINT64 Dr6;
+ UINT64 Rflags;
+ UINT64 Rip;
+ UINT8 Reserved5[232];
+ UINT64 GPat;
+ UINT8 Reserved6[320];
+ UINT64 SevFeatures;
+ UINT8 Reserved7[48];
+ UINT64 XCr0;
+ UINT8 Reserved8[24];
+ UINT32 Mxcsr;
+ UINT16 X87Ftw;
+ UINT8 Reserved9[2];
+ UINT16 X87Fcw;
+} SEV_ES_SAVE_AREA;
+#pragma pack ()
+
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (5 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-17 3:16 ` [edk2-devel] " Laszlo Ersek
2021-05-12 23:46 ` [PATCH v2 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction Brijesh Singh
` (5 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas,
Michael D Kinney, Liming Gao, Zhiguang Liu
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
The PVALIDATE instruction validates or rescinds validation of a guest
page RMP entry. Upon completion, a return code is stored in EAX, rFLAGS
bits OF, ZF, AF, PF and SF are set based on this return code. If the
instruction completed succesfully, the rFLAGS bit CF indicates if the
contents of the RMP entry were changed or not.
For more information about the instruction see AMD APM volume 3.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Include/Library/BaseLib.h | 50 +++++++++++++++++++++++
MdePkg/Include/X64/Nasm.inc | 8 ++++
MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 42 +++++++++++++++++++
4 files changed, 101 insertions(+)
create mode 100644 MdePkg/Library/BaseLib/X64/Pvalidate.nasm
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index b76f3af380ea..89a52f72c08a 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -317,6 +317,7 @@ [Sources.X64]
X64/GccInlinePriv.c | GCC
X64/EnableDisableInterrupts.nasm
X64/DisablePaging64.nasm
+ X64/Pvalidate.nasm
X64/RdRand.nasm
X64/XGetBv.nasm
X64/XSetBv.nasm
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 7253997a6f8c..a2cd134bea9a 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4813,6 +4813,56 @@ SpeculationBarrier (
VOID
);
+#if defined (MDE_CPU_X64)
+//
+// The page size for the PVALIDATE instruction
+//
+typedef enum {
+ PvalidatePageSize4K = 0,
+ PvalidatePageSize2MB,
+} PVALIDATE_PAGE_SIZE;
+
+//
+// PVALIDATE Return Code.
+//
+#define PVALIDATE_RET_SUCCESS 0
+#define PVALIDATE_RET_FAIL_INPUT 1
+#define PVALIDATE_RET_SIZE_MISMATCH 6
+
+//
+// The PVALIDATE instruction did not make any changes to the RMP entry.
+//
+#define PVALIDATE_RET_NO_RMPUPDATE 255
+
+/**
+ Execute a PVALIDATE instruction to validate or to rescinds validation of a guest
+ page's RMP entry.
+
+ The instruction is available only when CPUID Fn8000_001F_EAX[SNP]=1.
+
+ The function is available on X64.
+
+ @param[in] PageSize The page size to use.
+ @param[in] Validate If TRUE, validate the guest virtual address
+ otherwise invalidate the guest virtual address.
+ @param[in] Address The guest virtual address.
+
+ @retval PVALIDATE_RET_SUCCESS The PVALIDATE instruction succeeded, and
+ updated the RMP entry.
+ @retval PVALIDATE_RET_NO_RMPUPDATE The PVALIDATE instruction succeeded, but
+ did not update the RMP entry.
+ @return Failure code from the PVALIDATE
+ instruction.
+**/
+UINT32
+EFIAPI
+AsmPvalidate (
+ IN PVALIDATE_PAGE_SIZE PageSize,
+ IN BOOLEAN Validate,
+ IN PHYSICAL_ADDRESS Address
+ );
+#endif
+
#if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
///
diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc
index 527f71e9eb4d..528bb3385609 100644
--- a/MdePkg/Include/X64/Nasm.inc
+++ b/MdePkg/Include/X64/Nasm.inc
@@ -33,6 +33,14 @@
DB 0xF3, 0x48, 0x0F, 0xAE, 0xE8
%endmacro
+;
+; Macro for the PVALIDATE instruction, defined in AMD APM volume 3.
+; NASM feature request URL: https://bugzilla.nasm.us/show_bug.cgi?id=3392753
+;
+%macro PVALIDATE 0
+ DB 0xF2, 0x0F, 0x01, 0xFF
+%endmacro
+
; NASM provides built-in macros STRUC and ENDSTRUC for structure definition.
; For example, to define a structure called mytype containing a longword,
; a word, a byte and a string of bytes, you might code
diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
new file mode 100644
index 000000000000..a7d177913405
--- /dev/null
+++ b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
@@ -0,0 +1,42 @@
+;-----------------------------------------------------------------------------
+;
+; Copyright (c) 2021, AMD. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;-----------------------------------------------------------------------------
+
+%include "Nasm.inc"
+
+ SECTION .text
+
+;-----------------------------------------------------------------------------
+; UINT32
+; EFIAPI
+; AsmPvalidate (
+; IN UINT32 PageSize
+; IN UINT32 Validate,
+; IN UINT64 Address
+; )
+;-----------------------------------------------------------------------------
+global ASM_PFX(AsmPvalidate)
+ASM_PFX(AsmPvalidate):
+ mov rax, r8
+
+ PVALIDATE
+
+ ; Save the carry flag.
+ setc dl
+
+ ; The PVALIDATE instruction returns the status in rax register.
+ cmp rax, 0
+ jne PvalidateExit
+
+ ; Check the carry flag to determine if RMP entry was updated.
+ cmp dl, 0
+ je PvalidateExit
+
+ ; Return the PVALIDATE_RET_NO_RMPUPDATE.
+ mov rax, 255
+
+PvalidateExit:
+ ret
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (6 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-17 3:25 ` [edk2-devel] " Laszlo Ersek
2021-05-12 23:46 ` [PATCH v2 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask() Brijesh Singh
` (4 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Tom Lendacky, James Bottomley, Min Xu, Jiewen Yao, Jordan Justen,
Ard Biesheuvel, Laszlo Ersek, Erdem Aktas, Michael D Kinney,
Liming Gao, Zhiguang Liu, Brijesh Singh
From: Tom Lendacky <thomas.lendacky@amd.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
The RMPADJUST instruction will be used by the SEV-SNP guest to modify the
RMP permissions for a guest page. See AMD APM volume 3 for further
details.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Include/Library/BaseLib.h | 35 ++++++++++++++++++++
MdePkg/Include/X64/Nasm.inc | 8 +++++
MdePkg/Library/BaseLib/X64/RmpAdjust.nasm | 40 +++++++++++++++++++++++
4 files changed, 84 insertions(+)
create mode 100644 MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index 89a52f72c08a..6ccb8997b7e8 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -319,6 +319,7 @@ [Sources.X64]
X64/DisablePaging64.nasm
X64/Pvalidate.nasm
X64/RdRand.nasm
+ X64/RmpAdjust.nasm
X64/XGetBv.nasm
X64/XSetBv.nasm
X64/VmgExit.nasm
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index a2cd134bea9a..e8eff32716b8 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4861,6 +4861,41 @@ AsmPvalidate (
IN BOOLEAN Validate,
IN PHYSICAL_ADDRESS Address
);
+
+//
+// RDX settings for RMPADJUST
+//
+#define RMPADJUST_VMPL_MAX 3
+#define RMPADJUST_VMPL_MASK 0xFF
+#define RMPADJUST_VMPL_SHIFT 0
+#define RMPADJUST_PERMISSION_MASK_MASK 0xFF
+#define RMPADJUST_PERMISSION_MASK_SHIFT 8
+#define RMPADJUST_VMSA_PAGE_BIT BIT16
+
+/**
+ Adjusts the permissions of an SEV-SNP guest page.
+
+ Executes a RMPADJUST instruction with the register state specified by Rax,
+ Rcx, and Rdx. Returns Rax. This function is only available on X64.
+
+ The instruction is available only when CPUID Fn8000_001F_EAX[SNP]=1.
+
+ @param[in] Rax The value to load into RAX before executing the RMPADJUST
+ instruction.
+ @param[in] Rcx The value to load into RCX before executing the RMPADJUST
+ instruction.
+ @param[in] Rdx The value to load into RDX before executing the RMPADJUST
+ instruction.
+
+ @return Rax
+**/
+UINT32
+EFIAPI
+AsmRmpAdjust (
+ IN UINT64 Rax,
+ IN UINT64 Rcx,
+ IN UINT64 Rdx
+ );
#endif
diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc
index 528bb3385609..cfb14edc9449 100644
--- a/MdePkg/Include/X64/Nasm.inc
+++ b/MdePkg/Include/X64/Nasm.inc
@@ -41,6 +41,14 @@
DB 0xF2, 0x0F, 0x01, 0xFF
%endmacro
+;
+; Macro for the RMPADJUST instruction, defined in AMD APM volume 3.
+; NASM feature request URL: https://bugzilla.nasm.us/show_bug.cgi?id=3392754
+;
+%macro RMPADJUST 0
+ DB 0xF3, 0x0F, 0x01, 0xFE
+%endmacro
+
; NASM provides built-in macros STRUC and ENDSTRUC for structure definition.
; For example, to define a structure called mytype containing a longword,
; a word, a byte and a string of bytes, you might code
diff --git a/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm b/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
new file mode 100644
index 000000000000..c307f64b518a
--- /dev/null
+++ b/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
@@ -0,0 +1,40 @@
+;-----------------------------------------------------------------------------
+;
+; Copyright (c) 2021, Advanced Micro Devices, Inc. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Module Name:
+;
+; RmpAdjust.Asm
+;
+; Abstract:
+;
+; AsmRmpAdjust function
+;
+; Notes:
+;
+;-----------------------------------------------------------------------------
+
+%include "Nasm.inc"
+
+ SECTION .text
+
+;-----------------------------------------------------------------------------
+; UINT32
+; EFIAPI
+; AsmRmpAdjust (
+; IN UINT64 Rax,
+; IN UINT64 Rcx,
+; IN UINT64 Rdx
+; )
+;-----------------------------------------------------------------------------
+global ASM_PFX(AsmRmpAdjust)
+ASM_PFX(AsmRmpAdjust):
+ mov rax, rcx ; Input Rax is in RCX by calling convention
+ mov rcx, rdx ; Input Rcx is in RDX by calling convention
+ mov rdx, r8 ; Input Rdx is in R8 by calling convention
+
+ RMPADJUST
+
+ ; RMPADJUST returns the status in the EAX register.
+ ret
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask()
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (7 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-17 3:32 ` [edk2-devel] " Laszlo Ersek
2021-05-12 23:46 ` [PATCH v2 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask Brijesh Singh
` (3 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
The MemEncryptSevClearMmioPageEncMask() helper can be used for clearing
the memory encryption mask for the Mmio region.
The MemEncryptSevClearMmioPageEncMask() is a simplified version of
MemEncryptSevClearPageEncMask() -- it does not flush the caches after
clearing the page encryption mask.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/Include/Library/MemEncryptSevLib.h | 25 ++++++++++++++
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 23 +++++++++++++
.../Ia32/MemEncryptSevLib.c | 31 +++++++++++++++++
.../X64/MemEncryptSevLib.c | 33 +++++++++++++++++++
.../X64/PeiDxeVirtualMemory.c | 33 +++++++++++++++++++
.../X64/SecVirtualMemory.c | 30 +++++++++++++++++
6 files changed, 175 insertions(+)
diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index 99f15a7d1271..b91490d5d44d 100644
--- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
+++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
@@ -203,4 +203,29 @@ MemEncryptSevGetAddressRangeState (
IN UINTN Length
);
+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ BaseAddress and NumPages.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] BaseAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] NumPages The number of pages from start memory
+ region.
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Number of pages is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+MemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS BaseAddress,
+ IN UINTN NumPages
+ );
+
#endif // _MEM_ENCRYPT_SEV_LIB_H_
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
index fe2a0b2826cd..8dc39e647b90 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
@@ -126,4 +126,27 @@ InternalMemEncryptSevGetAddressRangeState (
IN UINTN Length
);
+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ PhysicalAddress and Length.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] PhysicalAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] Length The length of memory region
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Length is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+InternalMemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS PhysicalAddress,
+ IN UINTN Length
+ );
#endif
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
index 12a5bf495bd7..169d3118e44f 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
@@ -111,3 +111,34 @@ MemEncryptSevGetAddressRangeState (
//
return MemEncryptSevAddressRangeEncrypted;
}
+
+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ BaseAddress and NumPages.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] BaseAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] NumPages The number of pages from start memory
+ region.
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Number of pages is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+MemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS BaseAddress,
+ IN UINTN NumPages
+ )
+{
+ //
+ // Memory encryption bit is not accessible in 32-bit mode
+ //
+ return RETURN_UNSUPPORTED;
+}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
index 4fea6a6be0ac..a2bf698bcde7 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
@@ -118,3 +118,36 @@ MemEncryptSevGetAddressRangeState (
Length
);
}
+
+/**
+ This function clears memory encryption bit for the mmio region specified by
+ BaseAddress and NumPages.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] BaseAddress The physical address that is the start
+ address of a mmio region.
+ @param[in] NumPages The number of pages from start memory
+ region.
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Number of pages is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+MemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS BaseAddress,
+ IN UINTN NumPages
+ )
+{
+ return InternalMemEncryptSevClearMmioPageEncMask (
+ Cr3BaseAddress,
+ BaseAddress,
+ EFI_PAGES_TO_SIZE (NumPages)
+ );
+
+}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
index d3455e812bd1..a18d336a8789 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -891,3 +891,36 @@ InternalMemEncryptSevSetMemoryEncrypted (
Flush
);
}
+
+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ PhysicalAddress and Length.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] PhysicalAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] Length The length of memory region
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Length is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+InternalMemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS PhysicalAddress,
+ IN UINTN Length
+ )
+{
+ return SetMemoryEncDec (
+ Cr3BaseAddress,
+ PhysicalAddress,
+ Length,
+ ClearCBit,
+ FALSE
+ );
+}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
index bca5e3febb1b..e0d3a15e8503 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
@@ -98,3 +98,33 @@ InternalMemEncryptSevSetMemoryEncrypted (
//
return RETURN_UNSUPPORTED;
}
+
+/**
+ This function clears memory encryption bit for the MMIO region specified by
+ PhysicalAddress and Length.
+
+ @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
+ current CR3)
+ @param[in] PhysicalAddress The physical address that is the start
+ address of a MMIO region.
+ @param[in] Length The length of memory region
+
+ @retval RETURN_SUCCESS The attributes were cleared for the
+ memory region.
+ @retval RETURN_INVALID_PARAMETER Length is zero.
+ @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute
+ is not supported
+**/
+RETURN_STATUS
+EFIAPI
+InternalMemEncryptSevClearMmioPageEncMask (
+ IN PHYSICAL_ADDRESS Cr3BaseAddress,
+ IN PHYSICAL_ADDRESS PhysicalAddress,
+ IN UINTN Length
+ )
+{
+ //
+ // This function is not available during SEC.
+ //
+ return RETURN_UNSUPPORTED;
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (8 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask() Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask Brijesh Singh
` (2 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
Use the MemEncryptSevClearMmioPageEncMask() to clear memory encryption mask
for the Mmio and NonExistent address range.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 689bfb376d03..80831b81facf 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -53,11 +53,10 @@ AmdSevDxeEntryPoint (
Desc = &AllDescMap[Index];
if (Desc->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo ||
Desc->GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
Desc->BaseAddress,
- EFI_SIZE_TO_PAGES (Desc->Length),
- FALSE
+ EFI_SIZE_TO_PAGES (Desc->Length)
);
ASSERT_EFI_ERROR (Status);
}
@@ -73,11 +72,10 @@ AmdSevDxeEntryPoint (
// the range.
//
if (PcdGet16 (PcdOvmfHostBridgePciDevId) == INTEL_Q35_MCH_DEVICE_ID) {
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
FixedPcdGet64 (PcdPciExpressBaseAddress),
- EFI_SIZE_TO_PAGES (SIZE_256MB),
- FALSE
+ EFI_SIZE_TO_PAGES (SIZE_256MB)
);
ASSERT_EFI_ERROR (Status);
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (9 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask() Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter Brijesh Singh
12 siblings, 0 replies; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
Use the MemEncryptSevClearMmioPageEncMask() to clear memory encryption mask
for the Mmio address range.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 5 ++---
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c | 5 ++---
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
index 1f285e008372..ab40087a8408 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -205,11 +205,10 @@ MarkIoMemoryRangeForRuntimeAccess (
// memory range.
//
if (MemEncryptSevIsEnabled ()) {
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
BaseAddress,
- EFI_SIZE_TO_PAGES (Length),
- FALSE
+ EFI_SIZE_TO_PAGES (Length)
);
ASSERT_EFI_ERROR (Status);
}
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
index 7eb80bfeffae..ea75b489c7fd 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
@@ -38,11 +38,10 @@ QemuFlashBeforeProbe (
// C-bit on flash ranges from SMM page table.
//
- Status = MemEncryptSevClearPageEncMask (
+ Status = MemEncryptSevClearMmioPageEncMask (
0,
BaseAddress,
- EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount),
- FALSE
+ EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount)
);
ASSERT_EFI_ERROR (Status);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask()
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (10 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter Brijesh Singh
12 siblings, 0 replies; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
Use the MemEncryptSevClearMmioPageEncMask() to clear memory encryption mask
for the Mmio address range.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
index df2ad623308d..570c8467a673 100644
--- a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
+++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
@@ -62,11 +62,10 @@ TpmMmioSevDecryptPeimEntryPoint (
"%a: mapping TPM MMIO address range unencrypted\n",
__FUNCTION__));
- DecryptStatus = MemEncryptSevClearPageEncMask (
+ DecryptStatus = MemEncryptSevClearMmioPageEncMask (
0,
FixedPcdGet64 (PcdTpmBaseAddress),
- EFI_SIZE_TO_PAGES ((UINTN) 0x5000),
- FALSE
+ EFI_SIZE_TO_PAGES ((UINTN) 0x5000)
);
if (RETURN_ERROR (DecryptStatus)) {
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (11 preceding siblings ...)
2021-05-12 23:46 ` [PATCH v2 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask() Brijesh Singh
@ 2021-05-12 23:46 ` Brijesh Singh
2021-05-17 3:46 ` [edk2-devel] " Laszlo Ersek
12 siblings, 1 reply; 27+ messages in thread
From: Brijesh Singh @ 2021-05-12 23:46 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Erdem Aktas
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
The Flush parameter is used to provide a hint whether the specified range
is Mmio address. Now that we have a dedicated helper to clear the
memory encryption mask for the Mmio address range, its safe to remove the
Flush parameter from MemEncryptSev{Set,Clear}PageEncMask().
Since the address specified in the MemEncryptSev{Set,Clear}PageEncMask()
points to a system RAM, thus a cache flush is required during the
encryption mask update.
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/Include/Library/MemEncryptSevLib.h | 10 ++--------
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 10 ++--------
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 3 +--
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 6 ++----
.../BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 10 ++--------
.../BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 16 ++++------------
.../X64/PeiDxeVirtualMemory.c | 14 ++++----------
.../BaseMemEncryptSevLib/X64/SecVirtualMemory.c | 8 ++------
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +--
OvmfPkg/PlatformPei/AmdSev.c | 3 +--
10 files changed, 21 insertions(+), 62 deletions(-)
diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index b91490d5d44d..76d06c206c8b 100644
--- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
+++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
@@ -100,8 +100,6 @@ MemEncryptSevIsEnabled (
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before clearing the bit
- (mostly TRUE except MMIO addresses)
@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -114,8 +112,7 @@ EFIAPI
MemEncryptSevClearPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
);
/**
@@ -128,8 +125,6 @@ MemEncryptSevClearPageEncMask (
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before setting the bit
- (mostly TRUE except MMIO addresses)
@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -142,8 +137,7 @@ EFIAPI
MemEncryptSevSetPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
);
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
index 8dc39e647b90..21bbbd1c4f9c 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
@@ -58,8 +58,6 @@ InternalGetMemEncryptionAddressMask (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask
@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -72,8 +70,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryDecrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
);
/**
@@ -85,8 +82,6 @@ InternalMemEncryptSevSetMemoryDecrypted (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask
@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -99,8 +94,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryEncrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
);
/**
diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 80831b81facf..c66c4e9b9272 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -120,8 +120,7 @@ AmdSevDxeEntryPoint (
Status = MemEncryptSevClearPageEncMask (
0, // Cr3BaseAddress -- use current CR3
MapPagesBase, // BaseAddress
- MapPagesCount, // NumPages
- TRUE // Flush
+ MapPagesCount // NumPages
);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevClearPageEncMask(): %r\n",
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 49ffa2448811..b30628078f73 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -252,8 +252,7 @@ IoMmuMap (
Status = MemEncryptSevClearPageEncMask (
0,
MapInfo->PlainTextAddress,
- MapInfo->NumberOfPages,
- TRUE
+ MapInfo->NumberOfPages
);
ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
@@ -407,8 +406,7 @@ IoMmuUnmapWorker (
Status = MemEncryptSevSetPageEncMask (
0,
MapInfo->PlainTextAddress,
- MapInfo->NumberOfPages,
- TRUE
+ MapInfo->NumberOfPages
);
ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
index 169d3118e44f..be260e0d1014 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
@@ -25,8 +25,6 @@
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before clearing the bit
- (mostly TRUE except MMIO addresses)
@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -39,8 +37,7 @@ EFIAPI
MemEncryptSevClearPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
)
{
//
@@ -59,8 +56,6 @@ MemEncryptSevClearPageEncMask (
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before setting the bit
- (mostly TRUE except MMIO addresses)
@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -73,8 +68,7 @@ EFIAPI
MemEncryptSevSetPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
)
{
//
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
index a2bf698bcde7..a57e8fd37fa7 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
@@ -27,8 +27,6 @@
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before clearing the bit
- (mostly TRUE except MMIO addresses)
@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -41,15 +39,13 @@ EFIAPI
MemEncryptSevClearPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
)
{
return InternalMemEncryptSevSetMemoryDecrypted (
Cr3BaseAddress,
BaseAddress,
- EFI_PAGES_TO_SIZE (NumPages),
- Flush
+ EFI_PAGES_TO_SIZE (NumPages)
);
}
@@ -63,8 +59,6 @@ MemEncryptSevClearPageEncMask (
address of a memory region.
@param[in] NumPages The number of pages from start memory
region.
- @param[in] Flush Flush the caches before setting the bit
- (mostly TRUE except MMIO addresses)
@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -77,15 +71,13 @@ EFIAPI
MemEncryptSevSetPageEncMask (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS BaseAddress,
- IN UINTN NumPages,
- IN BOOLEAN Flush
+ IN UINTN NumPages
)
{
return InternalMemEncryptSevSetMemoryEncrypted (
Cr3BaseAddress,
BaseAddress,
- EFI_PAGES_TO_SIZE (NumPages),
- Flush
+ EFI_PAGES_TO_SIZE (NumPages)
);
}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
index a18d336a8789..c696745f9d26 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -828,8 +828,6 @@ SetMemoryEncDec (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask
@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -842,8 +840,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryDecrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
@@ -852,7 +849,7 @@ InternalMemEncryptSevSetMemoryDecrypted (
PhysicalAddress,
Length,
ClearCBit,
- Flush
+ TRUE
);
}
@@ -865,8 +862,6 @@ InternalMemEncryptSevSetMemoryDecrypted (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask
@retval RETURN_SUCCESS The attributes were set for the memory
region.
@@ -879,8 +874,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryEncrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
return SetMemoryEncDec (
@@ -888,7 +882,7 @@ InternalMemEncryptSevSetMemoryEncrypted (
PhysicalAddress,
Length,
SetCBit,
- Flush
+ TRUE
);
}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
index e0d3a15e8503..a155c8729bfb 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
@@ -42,8 +42,6 @@ InternalGetMemEncryptionAddressMask (
@param[in] PhysicalAddress The physical address that is the start
address of a memory region.
@param[in] Length The length of memory region
- @param[in] Flush Flush the caches before applying the
- encryption mask
@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -56,8 +54,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryDecrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
//
@@ -89,8 +86,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryEncrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
//
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index fdf2380974fa..c7cc5b0389c8 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -283,8 +283,7 @@ SmmCpuFeaturesSmmRelocationComplete (
Status = MemEncryptSevSetPageEncMask (
0, // Cr3BaseAddress -- use current CR3
MapPagesBase, // BaseAddress
- MapPagesCount, // NumPages
- TRUE // Flush
+ MapPagesCount // NumPages
);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevSetPageEncMask(): %r\n",
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda4b..a8bf610022ba 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -72,8 +72,7 @@ AmdSevEsInitialize (
DecryptStatus = MemEncryptSevClearPageEncMask (
0,
GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
- 1,
- TRUE
+ 1
);
ASSERT_RETURN_ERROR (DecryptStatus);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
2021-05-12 23:46 ` [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation Brijesh Singh
@ 2021-05-17 3:08 ` Laszlo Ersek
2021-05-17 7:39 ` 回复: " gaoliming
2021-05-17 14:17 ` Lendacky, Thomas
0 siblings, 2 replies; 27+ messages in thread
From: Laszlo Ersek @ 2021-05-17 3:08 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: Tom Lendacky, James Bottomley, Min Xu, Jiewen Yao, Jordan Justen,
Ard Biesheuvel, Erdem Aktas, Michael D Kinney, Liming Gao,
Zhiguang Liu
Patches v2 01-05 look good to me, thanks for the updates. Now on to this
one:
On 05/13/21 01:46, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3275
(1) The "3D" seems like a typo in the bug ticket URL. (This crept into
v2 somehow; v1 didn't have it.)
>
> Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is enabled
> in the guest VM. See the GHCB specification, Table 5 "List of Supported
> Non-Automatic Events" and sections 4.1.9 and 4.3.2, for further details.
>
> While at it, define the VMSA state save area that is required for creating
> the AP. The save area format is defined in AMD APM volume 2, Table B-4
> (there is a mistake in the table that defines the size of the reserved
> area at offset 0xc8 as a dword, when it is actually a word). The format of
> the save area segment registers is further defined in AMD APM volume 2,
> sections 10 and 15.5.
>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> MdePkg/Include/Register/Amd/Ghcb.h | 76 ++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
> index 029904b1c63a..4d1ee29e0a5e 100644
> --- a/MdePkg/Include/Register/Amd/Ghcb.h
> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> @@ -55,6 +55,7 @@
> #define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
> #define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
> #define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
> +#define SVM_EXIT_SNP_AP_CREATION 0x80000013ULL
> #define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
> #define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
>
> @@ -83,6 +84,12 @@
> #define IOIO_SEG_ES 0
> #define IOIO_SEG_DS (BIT11 | BIT10)
>
> +//
> +// AP Creation Information
> +//
> +#define SVM_VMGEXIT_SNP_AP_CREATE_ON_INIT 0
> +#define SVM_VMGEXIT_SNP_AP_CREATE 1
> +#define SVM_VMGEXIT_SNP_AP_DESTROY 2
>
> typedef PACKED struct {
> UINT8 Reserved1[203];
> @@ -195,4 +202,73 @@ typedef struct {
> SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
> } SNP_PAGE_STATE_CHANGE_INFO;
>
> +//
> +// SEV-ES save area mapping structures used for SEV-SNP AP Creation.
> +// Only the fields required to be set to a non-zero value are defined.
> +//
> +#define SEV_ES_RESET_CODE_SEGMENT_TYPE 0xA
> +#define SEV_ES_RESET_DATA_SEGMENT_TYPE 0x2
> +
> +#define SEV_ES_RESET_LDT_TYPE 0x2
> +#define SEV_ES_RESET_TSS_TYPE 0x3
> +
> +#pragma pack (1)
> +typedef union {
> + struct {
> + UINT16 Type:4;
> + UINT16 Sbit:1;
> + UINT16 Dpl:2;
> + UINT16 Present:1;
> + UINT16 Avl:1;
> + UINT16 Reserved1:1;
> + UINT16 Db:1;
> + UINT16 Granularity:1;
> + } Bits;
> + UINT16 Uint16;
> +} SEV_ES_SEGMENT_REGISTER_ATTRIBUTES;
> +
> +typedef struct {
> + UINT16 Selector;
> + SEV_ES_SEGMENT_REGISTER_ATTRIBUTES Attributes;
> + UINT32 Limit;
> + UINT64 Base;
> +} SEV_ES_SEGMENT_REGISTER;
> +
I'm not saying anything is incorrect about this, but I *am* going to
rant about the APM.
It's simply impenetrable. I've been staring at it for ~50 minutes now,
and I still cannot fully connect it to your code.
[1] In sections "4.8.1 Code-Segment Descriptors" and "4.8.2 Data-Segment
Descriptors", the reader is introduced to the "normal" (not SEV-ES, not
virtualized, not SMM) segment descriptors. Why *these* are relevant
*here* is nothing short of mind-boggling, but please bear with me.
[2] In section "10.2.3 SMRAM State-Save Area", "Table 10-1. AMD64
Architecture SMM State-Save Area", the reader is introduced to the
2+2+4+8 segment register representation. The table only lists "Selector,
Attributes, Limit, Base" as fields, and nothing about the actual
contents. Way too little information. I guess this is covered by the
commit message reference "section 10".
[3] In section "15.5 VMRUN Instruction", "15.5.1 Basic Operation",
paragraph "Segment State in the VMCB", we're given a long-winded,
*textual* only -- no diagram! -- and *differential* (relative)
explanation, on top of the two, above-quoted, locations of the spec. I'm
sorry but this is just impossible to follow. Would it have been a
unaffordable to insert a self-contained diagram here, with
self-contained, absolute explanation?
So let me quote:
The segment registers are stored in the VMCB in a format similar to
that for SMM: both base and limit are fully expanded; segment
attributes are stored as 12-bit values formed by the concatenation
of bits 55:52 and 47:40 from the original 64-bit (in-memory) segment
descriptors; the descriptor “P” bit is used to signal NULL segments
(P=0) where permissible and/or relevant.
So, if we apply this to [1] and [2], the "Selector", "Limit" and "Base"
fields of the SMM and VMCB segment register representation are
explained. Further, we get the following for the Attributes field, by
manual reconstruction:
55 54 53 52 47 46 45 44 43 42 41 40
G D L AVL P [DPL] 1 1 C R A Code Segment Descriptor
* * * * * * ignored bits in 64-bit mode
G D/B - AVL P [DPL] 1 0 E W A Data Segment Descriptor
* * * * * * * * * * ignored bits in 64-bit mode
In other words, in the code segment descriptor, D, L, P, DPL, and C
matter. In the data segment descriptor, only P matters.
In particular, what [3] says, quoted above, about the "P" bit, seems
sensible -- "P" is indeed *not* ignored for either segment descriptor
type (code or data).
But then we continure reading [3], and we find (quote again):
The loading of segment attributes from the VMCB (which may have been
overwritten by software) may result in attribute bit values that are
otherwise not allowed. However, only some of the attribute bits are
actually observed by hardware, depending on the segment register in
question:
* CS—D, L, P, and R.
* SS—B, P, E, W, and Code/Data
* DS, ES, FS, GS —D, P, DPL, E, W, and Code/Data.
* LDTR—P, S, and Type (LDT)
* TR—P, S, and Type (32- or 16-bit TSS)
I'm going to ignore SS, LDTR, and TR, but let's check CS and DS.
For CS, the spec says, "D, L, P, and R" are observed by the hardware.
But we've just shown that R is ignored *in general* for code segments in
64-bit mode! In other words, { D, L, P, R } is *not a subset* of { D, L,
P, DPL, C }.
For DS, the spec says here, "D, P, DPL, E, W, and Code/Data" are
observed. I'm going to give "Code/Data" a pass (bit 43 in the original
representation), but the rest is {D, P, DPL, E, W}, which is *not a
subset* of { P }.
Again, from [1], section 4.8.2 specifically, E (expand-down) and W
(writeable) are ignored, DPL is ignored, and D isn't even called D but
"D/B", and it is ignored. So how can the spec say in [3] that the
hardware observes { D, DPL, E, W } when all those are ignored per prior
definition [1]?
- And then I see no reference that SEV-ES uses the same
(circumstantially-defined) segment register format.
So anyway, I'll just accept that I don't understand the spec -- I think
it has inconsistencies. Let's look at the code:
- Bits 43:40 are packed into the "Type" bit-field. That means [1,C,R,A]
for the code segment descriptor, and [0,E,W,A] for data segment descriptors
SEV_ES_RESET_CODE_SEGMENT_TYPE has value 0xA (binary 1010), which maps
to: 1=1, C=0, R=1, A=0. Problem: per [1], the R bit is ignored, so I
have no clue why we set it.
(2) Can you please explain that? Just in this discussion thread, no need
ot change the code or the commit message.
The C ("conforming") bit actually matters. It is documented in section
"4.7.2 Code-Segment Descriptors" (Code-Segment Descriptor—Legacy Mode).
It seems like "non-conforming" is not a problem here, as long as we
don't use multiple privilege levels, which I think we don't, in
firmware-land. OK.
Then, on to SEV_ES_RESET_DATA_SEGMENT_TYPE. Value 0x2 (binary 0010).
Maps to [0,E,W,A], meaning 0=0, E=0, W=1, A=0.
(3) Why is W (writeable) set to 1, when, according to [1], it is ignored
in 64-bit mode?
- "Sbit" seems to stand for bit 44 from the original representation --
constant 1. OK I think.
- "Dpl", "Present" and "Avl" look OK to me.
- Should "Reserved1" really be called that? It seems to map to bit 53 in
the original representation, and the L (long mode) bit actually matters
for the code segment. (It indeed appears undefined / reserved for data
segments.)
Am I *totally* mistaken here and we're not even talking long mode?
- "Db" and "Granularity" look OK.
- SEV_ES_RESET_LDT_TYPE (value 2) matches "64-bit LDT" in "4.8.3 System
Descriptors", "Table 4-6. System-Segment Descriptor Types—Long Mode". OK.
- SEV_ES_RESET_TSS_TYPE (value 3) seems wrong. In Table 4-6, value 3 is
associated with "Reserved (Illegal)". And, according to "12.2.2 TSS
Descriptor", "The TSS descriptor is a system-segment descriptor", which
makes me think that I'm looking at value 3 in the proper table (Table 4-6).
(4) Can you please explain why SEV_ES_RESET_TSS_TYPE is 3, and not (say)
0x9 ("Available 64-bit TSS") or 0xB ("Busy 64-bit TSS")?
(Please note that this is only superficial pattern matching from my side
("TSS"); I don't know the first thing about "hardware task management".)
> +typedef struct {
> + SEV_ES_SEGMENT_REGISTER Es;
> + SEV_ES_SEGMENT_REGISTER Cs;
> + SEV_ES_SEGMENT_REGISTER Ss;
> + SEV_ES_SEGMENT_REGISTER Ds;
> + SEV_ES_SEGMENT_REGISTER Fs;
> + SEV_ES_SEGMENT_REGISTER Gs;
> + SEV_ES_SEGMENT_REGISTER Gdtr;
> + SEV_ES_SEGMENT_REGISTER Ldtr;
> + SEV_ES_SEGMENT_REGISTER Idtr;
> + SEV_ES_SEGMENT_REGISTER Tr;
> + UINT8 Reserved1[42];
> + UINT8 Vmpl;
> + UINT8 Reserved2[5];
> + UINT64 Efer;
> + UINT8 Reserved3[112];
> + UINT64 Cr4;
> + UINT8 Reserved4[8];
> + UINT64 Cr0;
> + UINT64 Dr7;
> + UINT64 Dr6;
> + UINT64 Rflags;
> + UINT64 Rip;
> + UINT8 Reserved5[232];
> + UINT64 GPat;
> + UINT8 Reserved6[320];
> + UINT64 SevFeatures;
> + UINT8 Reserved7[48];
> + UINT64 XCr0;
> + UINT8 Reserved8[24];
> + UINT32 Mxcsr;
> + UINT16 X87Ftw;
> + UINT8 Reserved9[2];
> + UINT16 X87Fcw;
> +} SEV_ES_SAVE_AREA;
> +#pragma pack ()
> +
> #endif
>
This part looks good to me.
(I spent almost two hours reviewing this patch.)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v2 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction
2021-05-12 23:46 ` [PATCH v2 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction Brijesh Singh
@ 2021-05-17 3:16 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2021-05-17 3:16 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas, Michael D Kinney, Liming Gao,
Zhiguang Liu
On 05/13/21 01:46, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> The PVALIDATE instruction validates or rescinds validation of a guest
> page RMP entry. Upon completion, a return code is stored in EAX, rFLAGS
> bits OF, ZF, AF, PF and SF are set based on this return code. If the
> instruction completed succesfully, the rFLAGS bit CF indicates if the
> contents of the RMP entry were changed or not.
>
> For more information about the instruction see AMD APM volume 3.
>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> MdePkg/Library/BaseLib/BaseLib.inf | 1 +
> MdePkg/Include/Library/BaseLib.h | 50 +++++++++++++++++++++++
> MdePkg/Include/X64/Nasm.inc | 8 ++++
> MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 42 +++++++++++++++++++
> 4 files changed, 101 insertions(+)
> create mode 100644 MdePkg/Library/BaseLib/X64/Pvalidate.nasm
Many thanks for the updates!
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
>
> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
> index b76f3af380ea..89a52f72c08a 100644
> --- a/MdePkg/Library/BaseLib/BaseLib.inf
> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> @@ -317,6 +317,7 @@ [Sources.X64]
> X64/GccInlinePriv.c | GCC
> X64/EnableDisableInterrupts.nasm
> X64/DisablePaging64.nasm
> + X64/Pvalidate.nasm
> X64/RdRand.nasm
> X64/XGetBv.nasm
> X64/XSetBv.nasm
> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
> index 7253997a6f8c..a2cd134bea9a 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4813,6 +4813,56 @@ SpeculationBarrier (
> VOID
> );
>
> +#if defined (MDE_CPU_X64)
> +//
> +// The page size for the PVALIDATE instruction
> +//
> +typedef enum {
> + PvalidatePageSize4K = 0,
> + PvalidatePageSize2MB,
> +} PVALIDATE_PAGE_SIZE;
> +
> +//
> +// PVALIDATE Return Code.
> +//
> +#define PVALIDATE_RET_SUCCESS 0
> +#define PVALIDATE_RET_FAIL_INPUT 1
> +#define PVALIDATE_RET_SIZE_MISMATCH 6
> +
> +//
> +// The PVALIDATE instruction did not make any changes to the RMP entry.
> +//
> +#define PVALIDATE_RET_NO_RMPUPDATE 255
> +
> +/**
> + Execute a PVALIDATE instruction to validate or to rescinds validation of a guest
> + page's RMP entry.
> +
> + The instruction is available only when CPUID Fn8000_001F_EAX[SNP]=1.
> +
> + The function is available on X64.
> +
> + @param[in] PageSize The page size to use.
> + @param[in] Validate If TRUE, validate the guest virtual address
> + otherwise invalidate the guest virtual address.
> + @param[in] Address The guest virtual address.
> +
> + @retval PVALIDATE_RET_SUCCESS The PVALIDATE instruction succeeded, and
> + updated the RMP entry.
> + @retval PVALIDATE_RET_NO_RMPUPDATE The PVALIDATE instruction succeeded, but
> + did not update the RMP entry.
> + @return Failure code from the PVALIDATE
> + instruction.
> +**/
> +UINT32
> +EFIAPI
> +AsmPvalidate (
> + IN PVALIDATE_PAGE_SIZE PageSize,
> + IN BOOLEAN Validate,
> + IN PHYSICAL_ADDRESS Address
> + );
> +#endif
> +
>
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
> ///
> diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc
> index 527f71e9eb4d..528bb3385609 100644
> --- a/MdePkg/Include/X64/Nasm.inc
> +++ b/MdePkg/Include/X64/Nasm.inc
> @@ -33,6 +33,14 @@
> DB 0xF3, 0x48, 0x0F, 0xAE, 0xE8
> %endmacro
>
> +;
> +; Macro for the PVALIDATE instruction, defined in AMD APM volume 3.
> +; NASM feature request URL: https://bugzilla.nasm.us/show_bug.cgi?id=3392753
> +;
> +%macro PVALIDATE 0
> + DB 0xF2, 0x0F, 0x01, 0xFF
> +%endmacro
> +
> ; NASM provides built-in macros STRUC and ENDSTRUC for structure definition.
> ; For example, to define a structure called mytype containing a longword,
> ; a word, a byte and a string of bytes, you might code
> diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
> new file mode 100644
> index 000000000000..a7d177913405
> --- /dev/null
> +++ b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
> @@ -0,0 +1,42 @@
> +;-----------------------------------------------------------------------------
> +;
> +; Copyright (c) 2021, AMD. All rights reserved.<BR>
> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> +;
> +;-----------------------------------------------------------------------------
> +
> +%include "Nasm.inc"
> +
> + SECTION .text
> +
> +;-----------------------------------------------------------------------------
> +; UINT32
> +; EFIAPI
> +; AsmPvalidate (
> +; IN UINT32 PageSize
> +; IN UINT32 Validate,
> +; IN UINT64 Address
> +; )
> +;-----------------------------------------------------------------------------
> +global ASM_PFX(AsmPvalidate)
> +ASM_PFX(AsmPvalidate):
> + mov rax, r8
> +
> + PVALIDATE
> +
> + ; Save the carry flag.
> + setc dl
> +
> + ; The PVALIDATE instruction returns the status in rax register.
> + cmp rax, 0
> + jne PvalidateExit
> +
> + ; Check the carry flag to determine if RMP entry was updated.
> + cmp dl, 0
> + je PvalidateExit
> +
> + ; Return the PVALIDATE_RET_NO_RMPUPDATE.
> + mov rax, 255
> +
> +PvalidateExit:
> + ret
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v2 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction
2021-05-12 23:46 ` [PATCH v2 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction Brijesh Singh
@ 2021-05-17 3:25 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2021-05-17 3:25 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: Tom Lendacky, James Bottomley, Min Xu, Jiewen Yao, Jordan Justen,
Ard Biesheuvel, Erdem Aktas, Michael D Kinney, Liming Gao,
Zhiguang Liu
On 05/13/21 01:46, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> The RMPADJUST instruction will be used by the SEV-SNP guest to modify the
> RMP permissions for a guest page. See AMD APM volume 3 for further
> details.
>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> MdePkg/Library/BaseLib/BaseLib.inf | 1 +
> MdePkg/Include/Library/BaseLib.h | 35 ++++++++++++++++++++
> MdePkg/Include/X64/Nasm.inc | 8 +++++
> MdePkg/Library/BaseLib/X64/RmpAdjust.nasm | 40 +++++++++++++++++++++++
> 4 files changed, 84 insertions(+)
> create mode 100644 MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
>
> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
> index 89a52f72c08a..6ccb8997b7e8 100644
> --- a/MdePkg/Library/BaseLib/BaseLib.inf
> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> @@ -319,6 +319,7 @@ [Sources.X64]
> X64/DisablePaging64.nasm
> X64/Pvalidate.nasm
> X64/RdRand.nasm
> + X64/RmpAdjust.nasm
> X64/XGetBv.nasm
> X64/XSetBv.nasm
> X64/VmgExit.nasm
> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
> index a2cd134bea9a..e8eff32716b8 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4861,6 +4861,41 @@ AsmPvalidate (
> IN BOOLEAN Validate,
> IN PHYSICAL_ADDRESS Address
> );
> +
> +//
> +// RDX settings for RMPADJUST
> +//
> +#define RMPADJUST_VMPL_MAX 3
> +#define RMPADJUST_VMPL_MASK 0xFF
> +#define RMPADJUST_VMPL_SHIFT 0
> +#define RMPADJUST_PERMISSION_MASK_MASK 0xFF
> +#define RMPADJUST_PERMISSION_MASK_SHIFT 8
> +#define RMPADJUST_VMSA_PAGE_BIT BIT16
> +
> +/**
> + Adjusts the permissions of an SEV-SNP guest page.
> +
> + Executes a RMPADJUST instruction with the register state specified by Rax,
> + Rcx, and Rdx. Returns Rax. This function is only available on X64.
> +
> + The instruction is available only when CPUID Fn8000_001F_EAX[SNP]=1.
> +
> + @param[in] Rax The value to load into RAX before executing the RMPADJUST
> + instruction.
> + @param[in] Rcx The value to load into RCX before executing the RMPADJUST
> + instruction.
> + @param[in] Rdx The value to load into RDX before executing the RMPADJUST
> + instruction.
> +
> + @return Rax
> +**/
> +UINT32
> +EFIAPI
> +AsmRmpAdjust (
> + IN UINT64 Rax,
> + IN UINT64 Rcx,
> + IN UINT64 Rdx
> + );
> #endif
(1) Relative to v1, the updates are all good (thanks!), except for one:
"@return Rax" is actually a regression -- "@return Eax" was correct, in
my opinion.
This also affects the top of the leading comment, where Eax was also
replaced with Rax.
My R-b stands, but please fix the docs on this function again, because
now the documentation ("Rax", twice) is actually inconsistent with the
return type UINT32.
Thanks
Laszlo
>
>
> diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc
> index 528bb3385609..cfb14edc9449 100644
> --- a/MdePkg/Include/X64/Nasm.inc
> +++ b/MdePkg/Include/X64/Nasm.inc
> @@ -41,6 +41,14 @@
> DB 0xF2, 0x0F, 0x01, 0xFF
> %endmacro
>
> +;
> +; Macro for the RMPADJUST instruction, defined in AMD APM volume 3.
> +; NASM feature request URL: https://bugzilla.nasm.us/show_bug.cgi?id=3392754
> +;
> +%macro RMPADJUST 0
> + DB 0xF3, 0x0F, 0x01, 0xFE
> +%endmacro
> +
> ; NASM provides built-in macros STRUC and ENDSTRUC for structure definition.
> ; For example, to define a structure called mytype containing a longword,
> ; a word, a byte and a string of bytes, you might code
> diff --git a/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm b/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
> new file mode 100644
> index 000000000000..c307f64b518a
> --- /dev/null
> +++ b/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
> @@ -0,0 +1,40 @@
> +;-----------------------------------------------------------------------------
> +;
> +; Copyright (c) 2021, Advanced Micro Devices, Inc. All rights reserved.<BR>
> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> +;
> +; Module Name:
> +;
> +; RmpAdjust.Asm
> +;
> +; Abstract:
> +;
> +; AsmRmpAdjust function
> +;
> +; Notes:
> +;
> +;-----------------------------------------------------------------------------
> +
> +%include "Nasm.inc"
> +
> + SECTION .text
> +
> +;-----------------------------------------------------------------------------
> +; UINT32
> +; EFIAPI
> +; AsmRmpAdjust (
> +; IN UINT64 Rax,
> +; IN UINT64 Rcx,
> +; IN UINT64 Rdx
> +; )
> +;-----------------------------------------------------------------------------
> +global ASM_PFX(AsmRmpAdjust)
> +ASM_PFX(AsmRmpAdjust):
> + mov rax, rcx ; Input Rax is in RCX by calling convention
> + mov rcx, rdx ; Input Rcx is in RDX by calling convention
> + mov rdx, r8 ; Input Rdx is in R8 by calling convention
> +
> + RMPADJUST
> +
> + ; RMPADJUST returns the status in the EAX register.
> + ret
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v2 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask()
2021-05-12 23:46 ` [PATCH v2 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask() Brijesh Singh
@ 2021-05-17 3:32 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2021-05-17 3:32 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas
On 05/13/21 01:46, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> The MemEncryptSevClearMmioPageEncMask() helper can be used for clearing
> the memory encryption mask for the Mmio region.
>
> The MemEncryptSevClearMmioPageEncMask() is a simplified version of
> MemEncryptSevClearPageEncMask() -- it does not flush the caches after
> clearing the page encryption mask.
>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> OvmfPkg/Include/Library/MemEncryptSevLib.h | 25 ++++++++++++++
> .../BaseMemEncryptSevLib/X64/VirtualMemory.h | 23 +++++++++++++
> .../Ia32/MemEncryptSevLib.c | 31 +++++++++++++++++
> .../X64/MemEncryptSevLib.c | 33 +++++++++++++++++++
> .../X64/PeiDxeVirtualMemory.c | 33 +++++++++++++++++++
> .../X64/SecVirtualMemory.c | 30 +++++++++++++++++
> 6 files changed, 175 insertions(+)
Sorry that I missed last time that the "SecMemEncryptSevLib.inf"
instance was not extended with a definition for the new function.
This update looks OK; my R-b stands.
Thanks
Laszlo
>
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 99f15a7d1271..b91490d5d44d 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -203,4 +203,29 @@ MemEncryptSevGetAddressRangeState (
> IN UINTN Length
> );
>
> +/**
> + This function clears memory encryption bit for the MMIO region specified by
> + BaseAddress and NumPages.
> +
> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
> + current CR3)
> + @param[in] BaseAddress The physical address that is the start
> + address of a MMIO region.
> + @param[in] NumPages The number of pages from start memory
> + region.
> +
> + @retval RETURN_SUCCESS The attributes were cleared for the
> + memory region.
> + @retval RETURN_INVALID_PARAMETER Number of pages is zero.
> + @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute
> + is not supported
> +**/
> +RETURN_STATUS
> +EFIAPI
> +MemEncryptSevClearMmioPageEncMask (
> + IN PHYSICAL_ADDRESS Cr3BaseAddress,
> + IN PHYSICAL_ADDRESS BaseAddress,
> + IN UINTN NumPages
> + );
> +
> #endif // _MEM_ENCRYPT_SEV_LIB_H_
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> index fe2a0b2826cd..8dc39e647b90 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> @@ -126,4 +126,27 @@ InternalMemEncryptSevGetAddressRangeState (
> IN UINTN Length
> );
>
> +/**
> + This function clears memory encryption bit for the MMIO region specified by
> + PhysicalAddress and Length.
> +
> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
> + current CR3)
> + @param[in] PhysicalAddress The physical address that is the start
> + address of a MMIO region.
> + @param[in] Length The length of memory region
> +
> + @retval RETURN_SUCCESS The attributes were cleared for the
> + memory region.
> + @retval RETURN_INVALID_PARAMETER Length is zero.
> + @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute
> + is not supported
> +**/
> +RETURN_STATUS
> +EFIAPI
> +InternalMemEncryptSevClearMmioPageEncMask (
> + IN PHYSICAL_ADDRESS Cr3BaseAddress,
> + IN PHYSICAL_ADDRESS PhysicalAddress,
> + IN UINTN Length
> + );
> #endif
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> index 12a5bf495bd7..169d3118e44f 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> @@ -111,3 +111,34 @@ MemEncryptSevGetAddressRangeState (
> //
> return MemEncryptSevAddressRangeEncrypted;
> }
> +
> +/**
> + This function clears memory encryption bit for the MMIO region specified by
> + BaseAddress and NumPages.
> +
> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
> + current CR3)
> + @param[in] BaseAddress The physical address that is the start
> + address of a MMIO region.
> + @param[in] NumPages The number of pages from start memory
> + region.
> +
> + @retval RETURN_SUCCESS The attributes were cleared for the
> + memory region.
> + @retval RETURN_INVALID_PARAMETER Number of pages is zero.
> + @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute
> + is not supported
> +**/
> +RETURN_STATUS
> +EFIAPI
> +MemEncryptSevClearMmioPageEncMask (
> + IN PHYSICAL_ADDRESS Cr3BaseAddress,
> + IN PHYSICAL_ADDRESS BaseAddress,
> + IN UINTN NumPages
> + )
> +{
> + //
> + // Memory encryption bit is not accessible in 32-bit mode
> + //
> + return RETURN_UNSUPPORTED;
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> index 4fea6a6be0ac..a2bf698bcde7 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> @@ -118,3 +118,36 @@ MemEncryptSevGetAddressRangeState (
> Length
> );
> }
> +
> +/**
> + This function clears memory encryption bit for the mmio region specified by
> + BaseAddress and NumPages.
> +
> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
> + current CR3)
> + @param[in] BaseAddress The physical address that is the start
> + address of a mmio region.
> + @param[in] NumPages The number of pages from start memory
> + region.
> +
> + @retval RETURN_SUCCESS The attributes were cleared for the
> + memory region.
> + @retval RETURN_INVALID_PARAMETER Number of pages is zero.
> + @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute
> + is not supported
> +**/
> +RETURN_STATUS
> +EFIAPI
> +MemEncryptSevClearMmioPageEncMask (
> + IN PHYSICAL_ADDRESS Cr3BaseAddress,
> + IN PHYSICAL_ADDRESS BaseAddress,
> + IN UINTN NumPages
> + )
> +{
> + return InternalMemEncryptSevClearMmioPageEncMask (
> + Cr3BaseAddress,
> + BaseAddress,
> + EFI_PAGES_TO_SIZE (NumPages)
> + );
> +
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> index d3455e812bd1..a18d336a8789 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> @@ -891,3 +891,36 @@ InternalMemEncryptSevSetMemoryEncrypted (
> Flush
> );
> }
> +
> +/**
> + This function clears memory encryption bit for the MMIO region specified by
> + PhysicalAddress and Length.
> +
> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
> + current CR3)
> + @param[in] PhysicalAddress The physical address that is the start
> + address of a MMIO region.
> + @param[in] Length The length of memory region
> +
> + @retval RETURN_SUCCESS The attributes were cleared for the
> + memory region.
> + @retval RETURN_INVALID_PARAMETER Length is zero.
> + @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute
> + is not supported
> +**/
> +RETURN_STATUS
> +EFIAPI
> +InternalMemEncryptSevClearMmioPageEncMask (
> + IN PHYSICAL_ADDRESS Cr3BaseAddress,
> + IN PHYSICAL_ADDRESS PhysicalAddress,
> + IN UINTN Length
> + )
> +{
> + return SetMemoryEncDec (
> + Cr3BaseAddress,
> + PhysicalAddress,
> + Length,
> + ClearCBit,
> + FALSE
> + );
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> index bca5e3febb1b..e0d3a15e8503 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> @@ -98,3 +98,33 @@ InternalMemEncryptSevSetMemoryEncrypted (
> //
> return RETURN_UNSUPPORTED;
> }
> +
> +/**
> + This function clears memory encryption bit for the MMIO region specified by
> + PhysicalAddress and Length.
> +
> + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use
> + current CR3)
> + @param[in] PhysicalAddress The physical address that is the start
> + address of a MMIO region.
> + @param[in] Length The length of memory region
> +
> + @retval RETURN_SUCCESS The attributes were cleared for the
> + memory region.
> + @retval RETURN_INVALID_PARAMETER Length is zero.
> + @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute
> + is not supported
> +**/
> +RETURN_STATUS
> +EFIAPI
> +InternalMemEncryptSevClearMmioPageEncMask (
> + IN PHYSICAL_ADDRESS Cr3BaseAddress,
> + IN PHYSICAL_ADDRESS PhysicalAddress,
> + IN UINTN Length
> + )
> +{
> + //
> + // This function is not available during SEC.
> + //
> + return RETURN_UNSUPPORTED;
> +}
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v2 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
2021-05-12 23:46 ` [PATCH v2 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter Brijesh Singh
@ 2021-05-17 3:46 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2021-05-17 3:46 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas
On 05/13/21 01:46, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> The Flush parameter is used to provide a hint whether the specified range
> is Mmio address. Now that we have a dedicated helper to clear the
> memory encryption mask for the Mmio address range, its safe to remove the
> Flush parameter from MemEncryptSev{Set,Clear}PageEncMask().
>
> Since the address specified in the MemEncryptSev{Set,Clear}PageEncMask()
> points to a system RAM, thus a cache flush is required during the
> encryption mask update.
>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> OvmfPkg/Include/Library/MemEncryptSevLib.h | 10 ++--------
> .../BaseMemEncryptSevLib/X64/VirtualMemory.h | 10 ++--------
> OvmfPkg/AmdSevDxe/AmdSevDxe.c | 3 +--
> OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 6 ++----
> .../BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 10 ++--------
> .../BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 16 ++++------------
> .../X64/PeiDxeVirtualMemory.c | 14 ++++----------
> .../BaseMemEncryptSevLib/X64/SecVirtualMemory.c | 8 ++------
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +--
> OvmfPkg/PlatformPei/AmdSev.c | 3 +--
> 10 files changed, 21 insertions(+), 62 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(v2 patches 10 through 12 are OK as well)
Thanks
Laszlo
>
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index b91490d5d44d..76d06c206c8b 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -100,8 +100,6 @@ MemEncryptSevIsEnabled (
> address of a memory region.
> @param[in] NumPages The number of pages from start memory
> region.
> - @param[in] Flush Flush the caches before clearing the bit
> - (mostly TRUE except MMIO addresses)
>
> @retval RETURN_SUCCESS The attributes were cleared for the
> memory region.
> @@ -114,8 +112,7 @@ EFIAPI
> MemEncryptSevClearPageEncMask (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS BaseAddress,
> - IN UINTN NumPages,
> - IN BOOLEAN Flush
> + IN UINTN NumPages
> );
>
> /**
> @@ -128,8 +125,6 @@ MemEncryptSevClearPageEncMask (
> address of a memory region.
> @param[in] NumPages The number of pages from start memory
> region.
> - @param[in] Flush Flush the caches before setting the bit
> - (mostly TRUE except MMIO addresses)
>
> @retval RETURN_SUCCESS The attributes were set for the memory
> region.
> @@ -142,8 +137,7 @@ EFIAPI
> MemEncryptSevSetPageEncMask (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS BaseAddress,
> - IN UINTN NumPages,
> - IN BOOLEAN Flush
> + IN UINTN NumPages
> );
>
>
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> index 8dc39e647b90..21bbbd1c4f9c 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> @@ -58,8 +58,6 @@ InternalGetMemEncryptionAddressMask (
> @param[in] PhysicalAddress The physical address that is the start
> address of a memory region.
> @param[in] Length The length of memory region
> - @param[in] Flush Flush the caches before applying the
> - encryption mask
>
> @retval RETURN_SUCCESS The attributes were cleared for the
> memory region.
> @@ -72,8 +70,7 @@ EFIAPI
> InternalMemEncryptSevSetMemoryDecrypted (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS PhysicalAddress,
> - IN UINTN Length,
> - IN BOOLEAN Flush
> + IN UINTN Length
> );
>
> /**
> @@ -85,8 +82,6 @@ InternalMemEncryptSevSetMemoryDecrypted (
> @param[in] PhysicalAddress The physical address that is the start
> address of a memory region.
> @param[in] Length The length of memory region
> - @param[in] Flush Flush the caches before applying the
> - encryption mask
>
> @retval RETURN_SUCCESS The attributes were set for the memory
> region.
> @@ -99,8 +94,7 @@ EFIAPI
> InternalMemEncryptSevSetMemoryEncrypted (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS PhysicalAddress,
> - IN UINTN Length,
> - IN BOOLEAN Flush
> + IN UINTN Length
> );
>
> /**
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> index 80831b81facf..c66c4e9b9272 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -120,8 +120,7 @@ AmdSevDxeEntryPoint (
> Status = MemEncryptSevClearPageEncMask (
> 0, // Cr3BaseAddress -- use current CR3
> MapPagesBase, // BaseAddress
> - MapPagesCount, // NumPages
> - TRUE // Flush
> + MapPagesCount // NumPages
> );
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevClearPageEncMask(): %r\n",
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index 49ffa2448811..b30628078f73 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -252,8 +252,7 @@ IoMmuMap (
> Status = MemEncryptSevClearPageEncMask (
> 0,
> MapInfo->PlainTextAddress,
> - MapInfo->NumberOfPages,
> - TRUE
> + MapInfo->NumberOfPages
> );
> ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR (Status)) {
> @@ -407,8 +406,7 @@ IoMmuUnmapWorker (
> Status = MemEncryptSevSetPageEncMask (
> 0,
> MapInfo->PlainTextAddress,
> - MapInfo->NumberOfPages,
> - TRUE
> + MapInfo->NumberOfPages
> );
> ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR (Status)) {
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> index 169d3118e44f..be260e0d1014 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> @@ -25,8 +25,6 @@
> address of a memory region.
> @param[in] NumPages The number of pages from start memory
> region.
> - @param[in] Flush Flush the caches before clearing the bit
> - (mostly TRUE except MMIO addresses)
>
> @retval RETURN_SUCCESS The attributes were cleared for the
> memory region.
> @@ -39,8 +37,7 @@ EFIAPI
> MemEncryptSevClearPageEncMask (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS BaseAddress,
> - IN UINTN NumPages,
> - IN BOOLEAN Flush
> + IN UINTN NumPages
> )
> {
> //
> @@ -59,8 +56,6 @@ MemEncryptSevClearPageEncMask (
> address of a memory region.
> @param[in] NumPages The number of pages from start memory
> region.
> - @param[in] Flush Flush the caches before setting the bit
> - (mostly TRUE except MMIO addresses)
>
> @retval RETURN_SUCCESS The attributes were set for the memory
> region.
> @@ -73,8 +68,7 @@ EFIAPI
> MemEncryptSevSetPageEncMask (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS BaseAddress,
> - IN UINTN NumPages,
> - IN BOOLEAN Flush
> + IN UINTN NumPages
> )
> {
> //
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> index a2bf698bcde7..a57e8fd37fa7 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> @@ -27,8 +27,6 @@
> address of a memory region.
> @param[in] NumPages The number of pages from start memory
> region.
> - @param[in] Flush Flush the caches before clearing the bit
> - (mostly TRUE except MMIO addresses)
>
> @retval RETURN_SUCCESS The attributes were cleared for the
> memory region.
> @@ -41,15 +39,13 @@ EFIAPI
> MemEncryptSevClearPageEncMask (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS BaseAddress,
> - IN UINTN NumPages,
> - IN BOOLEAN Flush
> + IN UINTN NumPages
> )
> {
> return InternalMemEncryptSevSetMemoryDecrypted (
> Cr3BaseAddress,
> BaseAddress,
> - EFI_PAGES_TO_SIZE (NumPages),
> - Flush
> + EFI_PAGES_TO_SIZE (NumPages)
> );
> }
>
> @@ -63,8 +59,6 @@ MemEncryptSevClearPageEncMask (
> address of a memory region.
> @param[in] NumPages The number of pages from start memory
> region.
> - @param[in] Flush Flush the caches before setting the bit
> - (mostly TRUE except MMIO addresses)
>
> @retval RETURN_SUCCESS The attributes were set for the memory
> region.
> @@ -77,15 +71,13 @@ EFIAPI
> MemEncryptSevSetPageEncMask (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS BaseAddress,
> - IN UINTN NumPages,
> - IN BOOLEAN Flush
> + IN UINTN NumPages
> )
> {
> return InternalMemEncryptSevSetMemoryEncrypted (
> Cr3BaseAddress,
> BaseAddress,
> - EFI_PAGES_TO_SIZE (NumPages),
> - Flush
> + EFI_PAGES_TO_SIZE (NumPages)
> );
> }
>
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> index a18d336a8789..c696745f9d26 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> @@ -828,8 +828,6 @@ SetMemoryEncDec (
> @param[in] PhysicalAddress The physical address that is the start
> address of a memory region.
> @param[in] Length The length of memory region
> - @param[in] Flush Flush the caches before applying the
> - encryption mask
>
> @retval RETURN_SUCCESS The attributes were cleared for the
> memory region.
> @@ -842,8 +840,7 @@ EFIAPI
> InternalMemEncryptSevSetMemoryDecrypted (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS PhysicalAddress,
> - IN UINTN Length,
> - IN BOOLEAN Flush
> + IN UINTN Length
> )
> {
>
> @@ -852,7 +849,7 @@ InternalMemEncryptSevSetMemoryDecrypted (
> PhysicalAddress,
> Length,
> ClearCBit,
> - Flush
> + TRUE
> );
> }
>
> @@ -865,8 +862,6 @@ InternalMemEncryptSevSetMemoryDecrypted (
> @param[in] PhysicalAddress The physical address that is the start
> address of a memory region.
> @param[in] Length The length of memory region
> - @param[in] Flush Flush the caches before applying the
> - encryption mask
>
> @retval RETURN_SUCCESS The attributes were set for the memory
> region.
> @@ -879,8 +874,7 @@ EFIAPI
> InternalMemEncryptSevSetMemoryEncrypted (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS PhysicalAddress,
> - IN UINTN Length,
> - IN BOOLEAN Flush
> + IN UINTN Length
> )
> {
> return SetMemoryEncDec (
> @@ -888,7 +882,7 @@ InternalMemEncryptSevSetMemoryEncrypted (
> PhysicalAddress,
> Length,
> SetCBit,
> - Flush
> + TRUE
> );
> }
>
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> index e0d3a15e8503..a155c8729bfb 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> @@ -42,8 +42,6 @@ InternalGetMemEncryptionAddressMask (
> @param[in] PhysicalAddress The physical address that is the start
> address of a memory region.
> @param[in] Length The length of memory region
> - @param[in] Flush Flush the caches before applying the
> - encryption mask
>
> @retval RETURN_SUCCESS The attributes were cleared for the
> memory region.
> @@ -56,8 +54,7 @@ EFIAPI
> InternalMemEncryptSevSetMemoryDecrypted (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS PhysicalAddress,
> - IN UINTN Length,
> - IN BOOLEAN Flush
> + IN UINTN Length
> )
> {
> //
> @@ -89,8 +86,7 @@ EFIAPI
> InternalMemEncryptSevSetMemoryEncrypted (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS PhysicalAddress,
> - IN UINTN Length,
> - IN BOOLEAN Flush
> + IN UINTN Length
> )
> {
> //
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index fdf2380974fa..c7cc5b0389c8 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -283,8 +283,7 @@ SmmCpuFeaturesSmmRelocationComplete (
> Status = MemEncryptSevSetPageEncMask (
> 0, // Cr3BaseAddress -- use current CR3
> MapPagesBase, // BaseAddress
> - MapPagesCount, // NumPages
> - TRUE // Flush
> + MapPagesCount // NumPages
> );
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevSetPageEncMask(): %r\n",
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index dddffdebda4b..a8bf610022ba 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -72,8 +72,7 @@ AmdSevEsInitialize (
> DecryptStatus = MemEncryptSevClearPageEncMask (
> 0,
> GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
> - 1,
> - TRUE
> + 1
> );
> ASSERT_RETURN_ERROR (DecryptStatus);
> }
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* 回复: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
2021-05-17 3:08 ` [edk2-devel] " Laszlo Ersek
@ 2021-05-17 7:39 ` gaoliming
2021-05-17 14:17 ` Lendacky, Thomas
1 sibling, 0 replies; 27+ messages in thread
From: gaoliming @ 2021-05-17 7:39 UTC (permalink / raw)
To: devel, lersek, brijesh.singh
Cc: 'Tom Lendacky', 'James Bottomley',
'Min Xu', 'Jiewen Yao', 'Jordan Justen',
'Ard Biesheuvel', 'Erdem Aktas',
'Michael D Kinney', 'Zhiguang Liu'
Laszlo:
Thanks for your detail review. I have no comments for the changes in this patch set. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek
> 发送时间: 2021年5月17日 11:09
> 收件人: devel@edk2.groups.io; brijesh.singh@amd.com
> 抄送: Tom Lendacky <thomas.lendacky@amd.com>; James Bottomley
> <jejb@linux.ibm.com>; Min Xu <min.m.xu@intel.com>; Jiewen Yao
> <jiewen.yao@intel.com>; Jordan Justen <jordan.l.justen@intel.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
> <erdemaktas@google.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Zhiguang Liu <zhiguang.liu@intel.com>
> 主题: Re: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB
> macros for SNP AP creation
>
> Patches v2 01-05 look good to me, thanks for the updates. Now on to this
> one:
>
> On 05/13/21 01:46, Brijesh Singh wrote:
> > From: Tom Lendacky <thomas.lendacky@amd.com>
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3275
>
> (1) The "3D" seems like a typo in the bug ticket URL. (This crept into
> v2 somehow; v1 didn't have it.)
>
> >
> > Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is enabled
> > in the guest VM. See the GHCB specification, Table 5 "List of Supported
> > Non-Automatic Events" and sections 4.1.9 and 4.3.2, for further details.
> >
> > While at it, define the VMSA state save area that is required for creating
> > the AP. The save area format is defined in AMD APM volume 2, Table B-4
> > (there is a mistake in the table that defines the size of the reserved
> > area at offset 0xc8 as a dword, when it is actually a word). The format of
> > the save area segment registers is further defined in AMD APM volume 2,
> > sections 10 and 15.5.
> >
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Min Xu <min.m.xu@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Erdem Aktas <erdemaktas@google.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > ---
> > MdePkg/Include/Register/Amd/Ghcb.h | 76
> ++++++++++++++++++++++++++++++
> > 1 file changed, 76 insertions(+)
> >
> > diff --git a/MdePkg/Include/Register/Amd/Ghcb.h
> b/MdePkg/Include/Register/Amd/Ghcb.h
> > index 029904b1c63a..4d1ee29e0a5e 100644
> > --- a/MdePkg/Include/Register/Amd/Ghcb.h
> > +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> > @@ -55,6 +55,7 @@
> > #define SVM_EXIT_AP_RESET_HOLD
> 0x80000004ULL
> > #define SVM_EXIT_AP_JUMP_TABLE
> 0x80000005ULL
> > #define SVM_EXIT_SNP_PAGE_STATE_CHANGE
> 0x80000010ULL
> > +#define SVM_EXIT_SNP_AP_CREATION
> 0x80000013ULL
> > #define SVM_EXIT_HYPERVISOR_FEATURES
> 0x8000FFFDULL
> > #define SVM_EXIT_UNSUPPORTED
> 0x8000FFFFULL
> >
> > @@ -83,6 +84,12 @@
> > #define IOIO_SEG_ES 0
> > #define IOIO_SEG_DS (BIT11 | BIT10)
> >
> > +//
> > +// AP Creation Information
> > +//
> > +#define SVM_VMGEXIT_SNP_AP_CREATE_ON_INIT 0
> > +#define SVM_VMGEXIT_SNP_AP_CREATE 1
> > +#define SVM_VMGEXIT_SNP_AP_DESTROY 2
> >
> > typedef PACKED struct {
> > UINT8 Reserved1[203];
> > @@ -195,4 +202,73 @@ typedef struct {
> > SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
> > } SNP_PAGE_STATE_CHANGE_INFO;
> >
> > +//
> > +// SEV-ES save area mapping structures used for SEV-SNP AP Creation.
> > +// Only the fields required to be set to a non-zero value are defined.
> > +//
> > +#define SEV_ES_RESET_CODE_SEGMENT_TYPE 0xA
> > +#define SEV_ES_RESET_DATA_SEGMENT_TYPE 0x2
> > +
> > +#define SEV_ES_RESET_LDT_TYPE 0x2
> > +#define SEV_ES_RESET_TSS_TYPE 0x3
> > +
> > +#pragma pack (1)
> > +typedef union {
> > + struct {
> > + UINT16 Type:4;
> > + UINT16 Sbit:1;
> > + UINT16 Dpl:2;
> > + UINT16 Present:1;
> > + UINT16 Avl:1;
> > + UINT16 Reserved1:1;
> > + UINT16 Db:1;
> > + UINT16 Granularity:1;
> > + } Bits;
> > + UINT16 Uint16;
> > +} SEV_ES_SEGMENT_REGISTER_ATTRIBUTES;
> > +
> > +typedef struct {
> > + UINT16 Selector;
> > + SEV_ES_SEGMENT_REGISTER_ATTRIBUTES Attributes;
> > + UINT32 Limit;
> > + UINT64 Base;
> > +} SEV_ES_SEGMENT_REGISTER;
> > +
>
> I'm not saying anything is incorrect about this, but I *am* going to
> rant about the APM.
>
> It's simply impenetrable. I've been staring at it for ~50 minutes now,
> and I still cannot fully connect it to your code.
>
> [1] In sections "4.8.1 Code-Segment Descriptors" and "4.8.2 Data-Segment
> Descriptors", the reader is introduced to the "normal" (not SEV-ES, not
> virtualized, not SMM) segment descriptors. Why *these* are relevant
> *here* is nothing short of mind-boggling, but please bear with me.
>
> [2] In section "10.2.3 SMRAM State-Save Area", "Table 10-1. AMD64
> Architecture SMM State-Save Area", the reader is introduced to the
> 2+2+4+8 segment register representation. The table only lists "Selector,
> Attributes, Limit, Base" as fields, and nothing about the actual
> contents. Way too little information. I guess this is covered by the
> commit message reference "section 10".
>
> [3] In section "15.5 VMRUN Instruction", "15.5.1 Basic Operation",
> paragraph "Segment State in the VMCB", we're given a long-winded,
> *textual* only -- no diagram! -- and *differential* (relative)
> explanation, on top of the two, above-quoted, locations of the spec. I'm
> sorry but this is just impossible to follow. Would it have been a
> unaffordable to insert a self-contained diagram here, with
> self-contained, absolute explanation?
>
> So let me quote:
>
> The segment registers are stored in the VMCB in a format similar to
> that for SMM: both base and limit are fully expanded; segment
> attributes are stored as 12-bit values formed by the concatenation
> of bits 55:52 and 47:40 from the original 64-bit (in-memory) segment
> descriptors; the descriptor “P” bit is used to signal NULL segments
> (P=0) where permissible and/or relevant.
>
> So, if we apply this to [1] and [2], the "Selector", "Limit" and "Base"
> fields of the SMM and VMCB segment register representation are
> explained. Further, we get the following for the Attributes field, by
> manual reconstruction:
>
> 55 54 53 52 47 46 45 44 43 42 41 40
>
> G D L AVL P [DPL] 1 1 C R A Code Segment Descriptor
> * * * * * * ignored bits in 64-bit
> mode
>
> G D/B - AVL P [DPL] 1 0 E W A Data Segment Descriptor
> * * * * * * * * * * ignored bits in 64-bit
> mode
>
> In other words, in the code segment descriptor, D, L, P, DPL, and C
> matter. In the data segment descriptor, only P matters.
>
> In particular, what [3] says, quoted above, about the "P" bit, seems
> sensible -- "P" is indeed *not* ignored for either segment descriptor
> type (code or data).
>
> But then we continure reading [3], and we find (quote again):
>
> The loading of segment attributes from the VMCB (which may have been
> overwritten by software) may result in attribute bit values that are
> otherwise not allowed. However, only some of the attribute bits are
> actually observed by hardware, depending on the segment register in
> question:
> * CS—D, L, P, and R.
> * SS—B, P, E, W, and Code/Data
> * DS, ES, FS, GS —D, P, DPL, E, W, and Code/Data.
> * LDTR—P, S, and Type (LDT)
> * TR—P, S, and Type (32- or 16-bit TSS)
>
> I'm going to ignore SS, LDTR, and TR, but let's check CS and DS.
>
> For CS, the spec says, "D, L, P, and R" are observed by the hardware.
> But we've just shown that R is ignored *in general* for code segments in
> 64-bit mode! In other words, { D, L, P, R } is *not a subset* of { D, L,
> P, DPL, C }.
>
> For DS, the spec says here, "D, P, DPL, E, W, and Code/Data" are
> observed. I'm going to give "Code/Data" a pass (bit 43 in the original
> representation), but the rest is {D, P, DPL, E, W}, which is *not a
> subset* of { P }.
>
> Again, from [1], section 4.8.2 specifically, E (expand-down) and W
> (writeable) are ignored, DPL is ignored, and D isn't even called D but
> "D/B", and it is ignored. So how can the spec say in [3] that the
> hardware observes { D, DPL, E, W } when all those are ignored per prior
> definition [1]?
>
> - And then I see no reference that SEV-ES uses the same
> (circumstantially-defined) segment register format.
>
>
> So anyway, I'll just accept that I don't understand the spec -- I think
> it has inconsistencies. Let's look at the code:
>
> - Bits 43:40 are packed into the "Type" bit-field. That means [1,C,R,A]
> for the code segment descriptor, and [0,E,W,A] for data segment descriptors
>
> SEV_ES_RESET_CODE_SEGMENT_TYPE has value 0xA (binary 1010), which
> maps
> to: 1=1, C=0, R=1, A=0. Problem: per [1], the R bit is ignored, so I
> have no clue why we set it.
>
> (2) Can you please explain that? Just in this discussion thread, no need
> ot change the code or the commit message.
>
> The C ("conforming") bit actually matters. It is documented in section
> "4.7.2 Code-Segment Descriptors" (Code-Segment Descriptor—Legacy Mode).
> It seems like "non-conforming" is not a problem here, as long as we
> don't use multiple privilege levels, which I think we don't, in
> firmware-land. OK.
>
> Then, on to SEV_ES_RESET_DATA_SEGMENT_TYPE. Value 0x2 (binary 0010).
> Maps to [0,E,W,A], meaning 0=0, E=0, W=1, A=0.
>
> (3) Why is W (writeable) set to 1, when, according to [1], it is ignored
> in 64-bit mode?
>
>
> - "Sbit" seems to stand for bit 44 from the original representation --
> constant 1. OK I think.
>
> - "Dpl", "Present" and "Avl" look OK to me.
>
> - Should "Reserved1" really be called that? It seems to map to bit 53 in
> the original representation, and the L (long mode) bit actually matters
> for the code segment. (It indeed appears undefined / reserved for data
> segments.)
>
> Am I *totally* mistaken here and we're not even talking long mode?
>
> - "Db" and "Granularity" look OK.
>
> - SEV_ES_RESET_LDT_TYPE (value 2) matches "64-bit LDT" in "4.8.3 System
> Descriptors", "Table 4-6. System-Segment Descriptor Types—Long Mode". OK.
>
> - SEV_ES_RESET_TSS_TYPE (value 3) seems wrong. In Table 4-6, value 3 is
> associated with "Reserved (Illegal)". And, according to "12.2.2 TSS
> Descriptor", "The TSS descriptor is a system-segment descriptor", which
> makes me think that I'm looking at value 3 in the proper table (Table 4-6).
>
>
> (4) Can you please explain why SEV_ES_RESET_TSS_TYPE is 3, and not (say)
> 0x9 ("Available 64-bit TSS") or 0xB ("Busy 64-bit TSS")?
>
> (Please note that this is only superficial pattern matching from my side
> ("TSS"); I don't know the first thing about "hardware task management".)
>
>
> > +typedef struct {
> > + SEV_ES_SEGMENT_REGISTER Es;
> > + SEV_ES_SEGMENT_REGISTER Cs;
> > + SEV_ES_SEGMENT_REGISTER Ss;
> > + SEV_ES_SEGMENT_REGISTER Ds;
> > + SEV_ES_SEGMENT_REGISTER Fs;
> > + SEV_ES_SEGMENT_REGISTER Gs;
> > + SEV_ES_SEGMENT_REGISTER Gdtr;
> > + SEV_ES_SEGMENT_REGISTER Ldtr;
> > + SEV_ES_SEGMENT_REGISTER Idtr;
> > + SEV_ES_SEGMENT_REGISTER Tr;
> > + UINT8 Reserved1[42];
> > + UINT8 Vmpl;
> > + UINT8 Reserved2[5];
> > + UINT64 Efer;
> > + UINT8 Reserved3[112];
> > + UINT64 Cr4;
> > + UINT8 Reserved4[8];
> > + UINT64 Cr0;
> > + UINT64 Dr7;
> > + UINT64 Dr6;
> > + UINT64 Rflags;
> > + UINT64 Rip;
> > + UINT8 Reserved5[232];
> > + UINT64 GPat;
> > + UINT8 Reserved6[320];
> > + UINT64 SevFeatures;
> > + UINT8 Reserved7[48];
> > + UINT64 XCr0;
> > + UINT8 Reserved8[24];
> > + UINT32 Mxcsr;
> > + UINT16 X87Ftw;
> > + UINT8 Reserved9[2];
> > + UINT16 X87Fcw;
> > +} SEV_ES_SAVE_AREA;
> > +#pragma pack ()
> > +
> > #endif
> >
>
> This part looks good to me.
>
> (I spent almost two hours reviewing this patch.)
>
> Thanks!
> Laszlo
>
>
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
2021-05-17 3:08 ` [edk2-devel] " Laszlo Ersek
2021-05-17 7:39 ` 回复: " gaoliming
@ 2021-05-17 14:17 ` Lendacky, Thomas
2021-05-18 16:00 ` Laszlo Ersek
1 sibling, 1 reply; 27+ messages in thread
From: Lendacky, Thomas @ 2021-05-17 14:17 UTC (permalink / raw)
To: Laszlo Ersek, devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Jordan Justen,
Ard Biesheuvel, Erdem Aktas, Michael D Kinney, Liming Gao,
Zhiguang Liu
On 5/16/21 10:08 PM, Laszlo Ersek wrote:
> Patches v2 01-05 look good to me, thanks for the updates. Now on to this
> one:
>
> On 05/13/21 01:46, Brijesh Singh wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3D3275&data=04%7C01%7Cthomas.lendacky%40amd.com%7C55d74e19a5554988e0b608d918e1215c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568177488739589%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ZIy6xEXWmxgVxZm6mrdlroM7OPQajEjUSD8W5z2ohu0%3D&reserved=0
>
> (1) The "3D" seems like a typo in the bug ticket URL. (This crept into
> v2 somehow; v1 didn't have it.)
>
>>
>> Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is enabled
>> in the guest VM. See the GHCB specification, Table 5 "List of Supported
>> Non-Automatic Events" and sections 4.1.9 and 4.3.2, for further details.
>>
>> While at it, define the VMSA state save area that is required for creating
>> the AP. The save area format is defined in AMD APM volume 2, Table B-4
>> (there is a mistake in the table that defines the size of the reserved
>> area at offset 0xc8 as a dword, when it is actually a word). The format of
>> the save area segment registers is further defined in AMD APM volume 2,
>> sections 10 and 15.5.
>>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> MdePkg/Include/Register/Amd/Ghcb.h | 76 ++++++++++++++++++++++++++++++
>> 1 file changed, 76 insertions(+)
>>
>> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
>> index 029904b1c63a..4d1ee29e0a5e 100644
>> --- a/MdePkg/Include/Register/Amd/Ghcb.h
>> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
>> @@ -55,6 +55,7 @@
>> #define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
>> #define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
>> #define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
>> +#define SVM_EXIT_SNP_AP_CREATION 0x80000013ULL
>> #define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
>> #define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
>>
>> @@ -83,6 +84,12 @@
>> #define IOIO_SEG_ES 0
>> #define IOIO_SEG_DS (BIT11 | BIT10)
>>
>> +//
>> +// AP Creation Information
>> +//
>> +#define SVM_VMGEXIT_SNP_AP_CREATE_ON_INIT 0
>> +#define SVM_VMGEXIT_SNP_AP_CREATE 1
>> +#define SVM_VMGEXIT_SNP_AP_DESTROY 2
>>
>> typedef PACKED struct {
>> UINT8 Reserved1[203];
>> @@ -195,4 +202,73 @@ typedef struct {
>> SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
>> } SNP_PAGE_STATE_CHANGE_INFO;
>>
>> +//
>> +// SEV-ES save area mapping structures used for SEV-SNP AP Creation.
>> +// Only the fields required to be set to a non-zero value are defined.
>> +//
>> +#define SEV_ES_RESET_CODE_SEGMENT_TYPE 0xA
>> +#define SEV_ES_RESET_DATA_SEGMENT_TYPE 0x2
>> +
>> +#define SEV_ES_RESET_LDT_TYPE 0x2
>> +#define SEV_ES_RESET_TSS_TYPE 0x3
>> +
>> +#pragma pack (1)
>> +typedef union {
>> + struct {
>> + UINT16 Type:4;
>> + UINT16 Sbit:1;
>> + UINT16 Dpl:2;
>> + UINT16 Present:1;
>> + UINT16 Avl:1;
>> + UINT16 Reserved1:1;
>> + UINT16 Db:1;
>> + UINT16 Granularity:1;
>> + } Bits;
>> + UINT16 Uint16;
>> +} SEV_ES_SEGMENT_REGISTER_ATTRIBUTES;
>> +
>> +typedef struct {
>> + UINT16 Selector;
>> + SEV_ES_SEGMENT_REGISTER_ATTRIBUTES Attributes;
>> + UINT32 Limit;
>> + UINT64 Base;
>> +} SEV_ES_SEGMENT_REGISTER;
>> +
>
> I'm not saying anything is incorrect about this, but I *am* going to
> rant about the APM.
Yes, the APM is definitely lacking in this area.
>
> It's simply impenetrable. I've been staring at it for ~50 minutes now,
> and I still cannot fully connect it to your code.
>
> [1] In sections "4.8.1 Code-Segment Descriptors" and "4.8.2 Data-Segment
> Descriptors", the reader is introduced to the "normal" (not SEV-ES, not
> virtualized, not SMM) segment descriptors. Why *these* are relevant
> *here* is nothing short of mind-boggling, but please bear with me.
>
> [2] In section "10.2.3 SMRAM State-Save Area", "Table 10-1. AMD64
> Architecture SMM State-Save Area", the reader is introduced to the
> 2+2+4+8 segment register representation. The table only lists "Selector,
> Attributes, Limit, Base" as fields, and nothing about the actual
> contents. Way too little information. I guess this is covered by the
> commit message reference "section 10".
>
> [3] In section "15.5 VMRUN Instruction", "15.5.1 Basic Operation",
> paragraph "Segment State in the VMCB", we're given a long-winded,
> *textual* only -- no diagram! -- and *differential* (relative)
> explanation, on top of the two, above-quoted, locations of the spec. I'm
> sorry but this is just impossible to follow. Would it have been a
> unaffordable to insert a self-contained diagram here, with
> self-contained, absolute explanation?
>
> So let me quote:
>
> The segment registers are stored in the VMCB in a format similar to
> that for SMM: both base and limit are fully expanded; segment
> attributes are stored as 12-bit values formed by the concatenation
> of bits 55:52 and 47:40 from the original 64-bit (in-memory) segment
> descriptors; the descriptor “P” bit is used to signal NULL segments
> (P=0) where permissible and/or relevant.
>
> So, if we apply this to [1] and [2], the "Selector", "Limit" and "Base"
> fields of the SMM and VMCB segment register representation are
> explained. Further, we get the following for the Attributes field, by
> manual reconstruction:
>
> 55 54 53 52 47 46 45 44 43 42 41 40
>
> G D L AVL P [DPL] 1 1 C R A Code Segment Descriptor
> * * * * * * ignored bits in 64-bit mode
>
> G D/B - AVL P [DPL] 1 0 E W A Data Segment Descriptor
> * * * * * * * * * * ignored bits in 64-bit mode
>
> In other words, in the code segment descriptor, D, L, P, DPL, and C
> matter. In the data segment descriptor, only P matters.
The APs won't be in 64-bit mode when being started. In reset, they will be
in real mode, so the attributes will be set up for that. Please see Table
4-3 and Table 4-4 for how these will matter.
>
> In particular, what [3] says, quoted above, about the "P" bit, seems
> sensible -- "P" is indeed *not* ignored for either segment descriptor
> type (code or data).
>
> But then we continure reading [3], and we find (quote again):
>
> The loading of segment attributes from the VMCB (which may have been
> overwritten by software) may result in attribute bit values that are
> otherwise not allowed. However, only some of the attribute bits are
> actually observed by hardware, depending on the segment register in
> question:
> * CS—D, L, P, and R.
> * SS—B, P, E, W, and Code/Data
> * DS, ES, FS, GS —D, P, DPL, E, W, and Code/Data.
> * LDTR—P, S, and Type (LDT)
> * TR—P, S, and Type (32- or 16-bit TSS)
>
> I'm going to ignore SS, LDTR, and TR, but let's check CS and DS.
>
> For CS, the spec says, "D, L, P, and R" are observed by the hardware.
> But we've just shown that R is ignored *in general* for code segments in
> 64-bit mode! In other words, { D, L, P, R } is *not a subset* of { D, L,
> P, DPL, C }.
>
> For DS, the spec says here, "D, P, DPL, E, W, and Code/Data" are
> observed. I'm going to give "Code/Data" a pass (bit 43 in the original
> representation), but the rest is {D, P, DPL, E, W}, which is *not a
> subset* of { P }.
>
> Again, from [1], section 4.8.2 specifically, E (expand-down) and W
> (writeable) are ignored, DPL is ignored, and D isn't even called D but
> "D/B", and it is ignored. So how can the spec say in [3] that the
> hardware observes { D, DPL, E, W } when all those are ignored per prior
> definition [1]?
>
> - And then I see no reference that SEV-ES uses the same
> (circumstantially-defined) segment register format.
>
>
> So anyway, I'll just accept that I don't understand the spec -- I think
> it has inconsistencies. Let's look at the code:
>
> - Bits 43:40 are packed into the "Type" bit-field. That means [1,C,R,A]
> for the code segment descriptor, and [0,E,W,A] for data segment descriptors
>
> SEV_ES_RESET_CODE_SEGMENT_TYPE has value 0xA (binary 1010), which maps
> to: 1=1, C=0, R=1, A=0. Problem: per [1], the R bit is ignored, so I
> have no clue why we set it.
For reset, when we are in real mode, that would correspond to executable /
readable type.
>
> (2) Can you please explain that? Just in this discussion thread, no need
> ot change the code or the commit message.
The idea is that we need to support real mode for the AP creation support,
but actually AP creation isn't limited to that. I didn't want to
overly-define the segment register for all the different modes, since it
would only be real mode that would be used by OVMF, but maybe that would
make it eaiser / clearer to understand...
>
> The C ("conforming") bit actually matters. It is documented in section
> "4.7.2 Code-Segment Descriptors" (Code-Segment Descriptor—Legacy Mode).
> It seems like "non-conforming" is not a problem here, as long as we
> don't use multiple privilege levels, which I think we don't, in
> firmware-land. OK.
>
> Then, on to SEV_ES_RESET_DATA_SEGMENT_TYPE. Value 0x2 (binary 0010).
> Maps to [0,E,W,A], meaning 0=0, E=0, W=1, A=0.
>
> (3) Why is W (writeable) set to 1, when, according to [1], it is ignored
> in 64-bit mode?
Same as previous comment, the AP will be starting in real mode.
>
>
> - "Sbit" seems to stand for bit 44 from the original representation --
> constant 1. OK I think.
>
> - "Dpl", "Present" and "Avl" look OK to me.
>
> - Should "Reserved1" really be called that? It seems to map to bit 53 in
> the original representation, and the L (long mode) bit actually matters
> for the code segment. (It indeed appears undefined / reserved for data
> segments.)
>
> Am I *totally* mistaken here and we're not even talking long mode?
:)
>
> - "Db" and "Granularity" look OK.
>
> - SEV_ES_RESET_LDT_TYPE (value 2) matches "64-bit LDT" in "4.8.3 System
> Descriptors", "Table 4-6. System-Segment Descriptor Types—Long Mode". OK.
>
> - SEV_ES_RESET_TSS_TYPE (value 3) seems wrong. In Table 4-6, value 3 is
> associated with "Reserved (Illegal)". And, according to "12.2.2 TSS
> Descriptor", "The TSS descriptor is a system-segment descriptor", which
> makes me think that I'm looking at value 3 in the proper table (Table 4-6).
I'll add a reference to table 14-2 under the "Processor Initialization"
section.
>
>
> (4) Can you please explain why SEV_ES_RESET_TSS_TYPE is 3, and not (say)
> 0x9 ("Available 64-bit TSS") or 0xB ("Busy 64-bit TSS")?
For processor reset, type 3 represents a busy 16-bit TSS.
So I can work on clarifying the comments around all of the definitions and
indicate that values are defined for processor reset support and that more
definitions would be needed if the segment registers want to be examined /
set in different modes. Thoughts?
Thanks,
Tom
>
> (Please note that this is only superficial pattern matching from my side
> ("TSS"); I don't know the first thing about "hardware task management".)
>
>
>> +typedef struct {
>> + SEV_ES_SEGMENT_REGISTER Es;
>> + SEV_ES_SEGMENT_REGISTER Cs;
>> + SEV_ES_SEGMENT_REGISTER Ss;
>> + SEV_ES_SEGMENT_REGISTER Ds;
>> + SEV_ES_SEGMENT_REGISTER Fs;
>> + SEV_ES_SEGMENT_REGISTER Gs;
>> + SEV_ES_SEGMENT_REGISTER Gdtr;
>> + SEV_ES_SEGMENT_REGISTER Ldtr;
>> + SEV_ES_SEGMENT_REGISTER Idtr;
>> + SEV_ES_SEGMENT_REGISTER Tr;
>> + UINT8 Reserved1[42];
>> + UINT8 Vmpl;
>> + UINT8 Reserved2[5];
>> + UINT64 Efer;
>> + UINT8 Reserved3[112];
>> + UINT64 Cr4;
>> + UINT8 Reserved4[8];
>> + UINT64 Cr0;
>> + UINT64 Dr7;
>> + UINT64 Dr6;
>> + UINT64 Rflags;
>> + UINT64 Rip;
>> + UINT8 Reserved5[232];
>> + UINT64 GPat;
>> + UINT8 Reserved6[320];
>> + UINT64 SevFeatures;
>> + UINT8 Reserved7[48];
>> + UINT64 XCr0;
>> + UINT8 Reserved8[24];
>> + UINT32 Mxcsr;
>> + UINT16 X87Ftw;
>> + UINT8 Reserved9[2];
>> + UINT16 X87Fcw;
>> +} SEV_ES_SAVE_AREA;
>> +#pragma pack ()
>> +
>> #endif
>>
>
> This part looks good to me.
>
> (I spent almost two hours reviewing this patch.)
>
> Thanks!
> Laszlo
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection
2021-05-12 23:46 ` [PATCH v2 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection Brijesh Singh
@ 2021-05-17 18:19 ` Erdem Aktas
0 siblings, 0 replies; 27+ messages in thread
From: Erdem Aktas @ 2021-05-17 18:19 UTC (permalink / raw)
To: Brijesh Singh
Cc: devel, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Michael D Kinney,
Liming Gao, Zhiguang Liu
I verified that the values align with the GHCB spec publication:
#56421 Revision: 2.00
Reviewed-by: Erdem Aktas <erdemaktas@google.com>
-Erdem
On Wed, May 12, 2021 at 4:46 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> Version 2 of GHCB introduces advertisement of features that are supported
> by the hypervisor. See the GHCB spec section 2.2 for an additional details.
>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++++++
> MdePkg/Include/Register/Amd/Ghcb.h | 8 ++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
> index 7368ce7af02a..cdb8f588ccf8 100644
> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
> @@ -48,6 +48,11 @@ typedef union {
> UINT32 Reserved2:32;
> } GhcbTerminate;
>
> + struct {
> + UINT64 Function:12;
> + UINT64 Features:52;
> + } GhcbHypervisorFeatures;
> +
> VOID *Ghcb;
>
> UINT64 GhcbPhysicalAddress;
> @@ -57,6 +62,8 @@ typedef union {
> #define GHCB_INFO_SEV_INFO_GET 2
> #define GHCB_INFO_CPUID_REQUEST 4
> #define GHCB_INFO_CPUID_RESPONSE 5
> +#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
> +#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
> #define GHCB_INFO_TERMINATE_REQUEST 256
>
> #define GHCB_TERMINATE_GHCB 0
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
> index 712dc8e769c0..ec232ebd3807 100644
> --- a/MdePkg/Include/Register/Amd/Ghcb.h
> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> @@ -54,6 +54,7 @@
> #define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
> #define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
> #define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
> +#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
> #define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
>
> //
> @@ -154,4 +155,11 @@ typedef union {
> #define GHCB_EVENT_INJECTION_TYPE_EXCEPTION 3
> #define GHCB_EVENT_INJECTION_TYPE_SOFT_INT 4
>
> +//
> +// Hypervisor features
> +//
> +#define GHCB_HV_FEATURES_SNP BIT0
> +#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
> +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
> +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
> #endif
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure
2021-05-12 23:46 ` [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure Brijesh Singh
@ 2021-05-17 18:25 ` Erdem Aktas
2021-05-18 16:02 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Erdem Aktas @ 2021-05-17 18:25 UTC (permalink / raw)
To: Brijesh Singh
Cc: devel, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Michael D Kinney,
Liming Gao, Zhiguang Liu
I verified that the values align with the GHCB spec publication:
#56421 Revision: 2.00
Just one question: is there any reason why GHCB_* defines are decimal
while the SVM_EXIT_* are all in hexadecimal? Does EDK2 have any
preference?
Reviewed-by: Erdem Aktas <erdemaktas@google.com>
-Erdem
On Wed, May 12, 2021 at 4:46 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> An SEV-SNP guest is required to perform the GHCB GPA registration. See
> the GHCB specification for further details.
>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
> index cdb8f588ccf8..542e4cdf4782 100644
> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
> @@ -53,6 +53,11 @@ typedef union {
> UINT64 Features:52;
> } GhcbHypervisorFeatures;
>
> + struct {
> + UINT64 Function:12;
> + UINT64 GuestFrameNumber:52;
> + } GhcbGpaRegister;
> +
> VOID *Ghcb;
>
> UINT64 GhcbPhysicalAddress;
> @@ -62,6 +67,8 @@ typedef union {
> #define GHCB_INFO_SEV_INFO_GET 2
> #define GHCB_INFO_CPUID_REQUEST 4
> #define GHCB_INFO_CPUID_RESPONSE 5
> +#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
> +#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
> #define GHCB_HYPERVISOR_FEATURES_REQUEST 128
> #define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
> #define GHCB_INFO_TERMINATE_REQUEST 256
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change
2021-05-12 23:46 ` [PATCH v2 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change Brijesh Singh
@ 2021-05-17 19:20 ` Erdem Aktas
0 siblings, 0 replies; 27+ messages in thread
From: Erdem Aktas @ 2021-05-17 19:20 UTC (permalink / raw)
To: Brijesh Singh
Cc: devel, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Laszlo Ersek, Michael D Kinney,
Liming Gao, Zhiguang Liu
I verified that the values align with the GHCB spec publication:
#56421 Revision: 2.00
Reviewed-by: Erdem Aktas <erdemaktas@google.com>
On Wed, May 12, 2021 at 4:46 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> The Page State Change NAE exit will be used by the SEV-SNP guest to
> request a page state change using the GHCB protocol. See the GHCB
> spec section 4.1.6 and 2.3.1 for more detail on the structure
> definitions.
>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> MdePkg/Include/Register/Amd/Fam17Msr.h | 15 ++++++++++++
> MdePkg/Include/Register/Amd/Ghcb.h | 33 ++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
> index 542e4cdf4782..62014854d9b7 100644
> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
> @@ -58,6 +58,19 @@ typedef union {
> UINT64 GuestFrameNumber:52;
> } GhcbGpaRegister;
>
> + struct {
> + UINT64 Function:12;
> + UINT64 GuestFrameNumber:40;
> + UINT64 Operation:4;
> + UINT64 Reserved:8;
> + } SnpPageStateChangeRequest;
> +
> + struct {
> + UINT32 Function:12;
> + UINT32 Reserved:20;
> + UINT32 ErrorCode;
> + } SnpPageStateChangeResponse;
> +
> VOID *Ghcb;
>
> UINT64 GhcbPhysicalAddress;
> @@ -69,6 +82,8 @@ typedef union {
> #define GHCB_INFO_CPUID_RESPONSE 5
> #define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
> #define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
> +#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20
> +#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE 21
> #define GHCB_HYPERVISOR_FEATURES_REQUEST 128
> #define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
> #define GHCB_INFO_TERMINATE_REQUEST 256
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
> index ec232ebd3807..029904b1c63a 100644
> --- a/MdePkg/Include/Register/Amd/Ghcb.h
> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> @@ -54,6 +54,7 @@
> #define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
> #define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
> #define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
> +#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
> #define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
> #define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
>
> @@ -162,4 +163,36 @@ typedef union {
> #define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
> #define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
> #define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
> +
> +//
> +// SNP Page State Change.
> +//
> +// Note that the PSMASH and UNSMASH operations are not supported when using the MSR protocol.
> +//
> +#define SNP_PAGE_STATE_PRIVATE 1
> +#define SNP_PAGE_STATE_SHARED 2
> +#define SNP_PAGE_STATE_PSMASH 3
> +#define SNP_PAGE_STATE_UNSMASH 4
> +
> +typedef struct {
> + UINT64 CurrentPage:12;
> + UINT64 GuestFrameNumber:40;
> + UINT64 Operation:4;
> + UINT64 PageSize:1;
> + UINT64 Reserved:7;
> +} SNP_PAGE_STATE_ENTRY;
> +
> +typedef struct {
> + UINT16 CurrentEntry;
> + UINT16 EndEntry;
> + UINT32 Reserved;
> +} SNP_PAGE_STATE_HEADER;
> +
> +#define SNP_PAGE_STATE_MAX_ENTRY 253
> +
> +typedef struct {
> + SNP_PAGE_STATE_HEADER Header;
> + SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
> +} SNP_PAGE_STATE_CHANGE_INFO;
> +
> #endif
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
2021-05-17 14:17 ` Lendacky, Thomas
@ 2021-05-18 16:00 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2021-05-18 16:00 UTC (permalink / raw)
To: Tom Lendacky, devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Jordan Justen,
Ard Biesheuvel, Erdem Aktas, Michael D Kinney, Liming Gao,
Zhiguang Liu
On 05/17/21 16:17, Tom Lendacky wrote:
> On 5/16/21 10:08 PM, Laszlo Ersek wrote:
>> Patches v2 01-05 look good to me, thanks for the updates. Now on to this
>> one:
>>
>> On 05/13/21 01:46, Brijesh Singh wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3D3275&data=04%7C01%7Cthomas.lendacky%40amd.com%7C55d74e19a5554988e0b608d918e1215c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568177488739589%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ZIy6xEXWmxgVxZm6mrdlroM7OPQajEjUSD8W5z2ohu0%3D&reserved=0
>>
>> (1) The "3D" seems like a typo in the bug ticket URL. (This crept into
>> v2 somehow; v1 didn't have it.)
>>
>>>
>>> Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is enabled
>>> in the guest VM. See the GHCB specification, Table 5 "List of Supported
>>> Non-Automatic Events" and sections 4.1.9 and 4.3.2, for further details.
>>>
>>> While at it, define the VMSA state save area that is required for creating
>>> the AP. The save area format is defined in AMD APM volume 2, Table B-4
>>> (there is a mistake in the table that defines the size of the reserved
>>> area at offset 0xc8 as a dword, when it is actually a word). The format of
>>> the save area segment registers is further defined in AMD APM volume 2,
>>> sections 10 and 15.5.
>>>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>>> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>> ---
>>> MdePkg/Include/Register/Amd/Ghcb.h | 76 ++++++++++++++++++++++++++++++
>>> 1 file changed, 76 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
>>> index 029904b1c63a..4d1ee29e0a5e 100644
>>> --- a/MdePkg/Include/Register/Amd/Ghcb.h
>>> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
>>> @@ -55,6 +55,7 @@
>>> #define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
>>> #define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
>>> #define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
>>> +#define SVM_EXIT_SNP_AP_CREATION 0x80000013ULL
>>> #define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
>>> #define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
>>>
>>> @@ -83,6 +84,12 @@
>>> #define IOIO_SEG_ES 0
>>> #define IOIO_SEG_DS (BIT11 | BIT10)
>>>
>>> +//
>>> +// AP Creation Information
>>> +//
>>> +#define SVM_VMGEXIT_SNP_AP_CREATE_ON_INIT 0
>>> +#define SVM_VMGEXIT_SNP_AP_CREATE 1
>>> +#define SVM_VMGEXIT_SNP_AP_DESTROY 2
>>>
>>> typedef PACKED struct {
>>> UINT8 Reserved1[203];
>>> @@ -195,4 +202,73 @@ typedef struct {
>>> SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
>>> } SNP_PAGE_STATE_CHANGE_INFO;
>>>
>>> +//
>>> +// SEV-ES save area mapping structures used for SEV-SNP AP Creation.
>>> +// Only the fields required to be set to a non-zero value are defined.
>>> +//
>>> +#define SEV_ES_RESET_CODE_SEGMENT_TYPE 0xA
>>> +#define SEV_ES_RESET_DATA_SEGMENT_TYPE 0x2
>>> +
>>> +#define SEV_ES_RESET_LDT_TYPE 0x2
>>> +#define SEV_ES_RESET_TSS_TYPE 0x3
>>> +
>>> +#pragma pack (1)
>>> +typedef union {
>>> + struct {
>>> + UINT16 Type:4;
>>> + UINT16 Sbit:1;
>>> + UINT16 Dpl:2;
>>> + UINT16 Present:1;
>>> + UINT16 Avl:1;
>>> + UINT16 Reserved1:1;
>>> + UINT16 Db:1;
>>> + UINT16 Granularity:1;
>>> + } Bits;
>>> + UINT16 Uint16;
>>> +} SEV_ES_SEGMENT_REGISTER_ATTRIBUTES;
>>> +
>>> +typedef struct {
>>> + UINT16 Selector;
>>> + SEV_ES_SEGMENT_REGISTER_ATTRIBUTES Attributes;
>>> + UINT32 Limit;
>>> + UINT64 Base;
>>> +} SEV_ES_SEGMENT_REGISTER;
>>> +
>>
>> I'm not saying anything is incorrect about this, but I *am* going to
>> rant about the APM.
>
> Yes, the APM is definitely lacking in this area.
>
>>
>> It's simply impenetrable. I've been staring at it for ~50 minutes now,
>> and I still cannot fully connect it to your code.
>>
>> [1] In sections "4.8.1 Code-Segment Descriptors" and "4.8.2 Data-Segment
>> Descriptors", the reader is introduced to the "normal" (not SEV-ES, not
>> virtualized, not SMM) segment descriptors. Why *these* are relevant
>> *here* is nothing short of mind-boggling, but please bear with me.
>>
>> [2] In section "10.2.3 SMRAM State-Save Area", "Table 10-1. AMD64
>> Architecture SMM State-Save Area", the reader is introduced to the
>> 2+2+4+8 segment register representation. The table only lists "Selector,
>> Attributes, Limit, Base" as fields, and nothing about the actual
>> contents. Way too little information. I guess this is covered by the
>> commit message reference "section 10".
>>
>> [3] In section "15.5 VMRUN Instruction", "15.5.1 Basic Operation",
>> paragraph "Segment State in the VMCB", we're given a long-winded,
>> *textual* only -- no diagram! -- and *differential* (relative)
>> explanation, on top of the two, above-quoted, locations of the spec. I'm
>> sorry but this is just impossible to follow. Would it have been a
>> unaffordable to insert a self-contained diagram here, with
>> self-contained, absolute explanation?
>>
>> So let me quote:
>>
>> The segment registers are stored in the VMCB in a format similar to
>> that for SMM: both base and limit are fully expanded; segment
>> attributes are stored as 12-bit values formed by the concatenation
>> of bits 55:52 and 47:40 from the original 64-bit (in-memory) segment
>> descriptors; the descriptor “P” bit is used to signal NULL segments
>> (P=0) where permissible and/or relevant.
>>
>> So, if we apply this to [1] and [2], the "Selector", "Limit" and "Base"
>> fields of the SMM and VMCB segment register representation are
>> explained. Further, we get the following for the Attributes field, by
>> manual reconstruction:
>>
>> 55 54 53 52 47 46 45 44 43 42 41 40
>>
>> G D L AVL P [DPL] 1 1 C R A Code Segment Descriptor
>> * * * * * * ignored bits in 64-bit mode
>>
>> G D/B - AVL P [DPL] 1 0 E W A Data Segment Descriptor
>> * * * * * * * * * * ignored bits in 64-bit mode
>>
>> In other words, in the code segment descriptor, D, L, P, DPL, and C
>> matter. In the data segment descriptor, only P matters.
>
> The APs won't be in 64-bit mode when being started. In reset, they will be
> in real mode, so the attributes will be set up for that. Please see Table
> 4-3 and Table 4-4 for how these will matter.
>
>>
>> In particular, what [3] says, quoted above, about the "P" bit, seems
>> sensible -- "P" is indeed *not* ignored for either segment descriptor
>> type (code or data).
>>
>> But then we continure reading [3], and we find (quote again):
>>
>> The loading of segment attributes from the VMCB (which may have been
>> overwritten by software) may result in attribute bit values that are
>> otherwise not allowed. However, only some of the attribute bits are
>> actually observed by hardware, depending on the segment register in
>> question:
>> * CS—D, L, P, and R.
>> * SS—B, P, E, W, and Code/Data
>> * DS, ES, FS, GS —D, P, DPL, E, W, and Code/Data.
>> * LDTR—P, S, and Type (LDT)
>> * TR—P, S, and Type (32- or 16-bit TSS)
>>
>> I'm going to ignore SS, LDTR, and TR, but let's check CS and DS.
>>
>> For CS, the spec says, "D, L, P, and R" are observed by the hardware.
>> But we've just shown that R is ignored *in general* for code segments in
>> 64-bit mode! In other words, { D, L, P, R } is *not a subset* of { D, L,
>> P, DPL, C }.
>>
>> For DS, the spec says here, "D, P, DPL, E, W, and Code/Data" are
>> observed. I'm going to give "Code/Data" a pass (bit 43 in the original
>> representation), but the rest is {D, P, DPL, E, W}, which is *not a
>> subset* of { P }.
>>
>> Again, from [1], section 4.8.2 specifically, E (expand-down) and W
>> (writeable) are ignored, DPL is ignored, and D isn't even called D but
>> "D/B", and it is ignored. So how can the spec say in [3] that the
>> hardware observes { D, DPL, E, W } when all those are ignored per prior
>> definition [1]?
>>
>> - And then I see no reference that SEV-ES uses the same
>> (circumstantially-defined) segment register format.
>>
>>
>> So anyway, I'll just accept that I don't understand the spec -- I think
>> it has inconsistencies. Let's look at the code:
>>
>> - Bits 43:40 are packed into the "Type" bit-field. That means [1,C,R,A]
>> for the code segment descriptor, and [0,E,W,A] for data segment descriptors
>>
>> SEV_ES_RESET_CODE_SEGMENT_TYPE has value 0xA (binary 1010), which maps
>> to: 1=1, C=0, R=1, A=0. Problem: per [1], the R bit is ignored, so I
>> have no clue why we set it.
>
> For reset, when we are in real mode, that would correspond to executable /
> readable type.
>
>>
>> (2) Can you please explain that? Just in this discussion thread, no need
>> ot change the code or the commit message.
>
> The idea is that we need to support real mode for the AP creation support,
> but actually AP creation isn't limited to that. I didn't want to
> overly-define the segment register for all the different modes, since it
> would only be real mode that would be used by OVMF, but maybe that would
> make it eaiser / clearer to understand...
>
>>
>> The C ("conforming") bit actually matters. It is documented in section
>> "4.7.2 Code-Segment Descriptors" (Code-Segment Descriptor—Legacy Mode).
>> It seems like "non-conforming" is not a problem here, as long as we
>> don't use multiple privilege levels, which I think we don't, in
>> firmware-land. OK.
>>
>> Then, on to SEV_ES_RESET_DATA_SEGMENT_TYPE. Value 0x2 (binary 0010).
>> Maps to [0,E,W,A], meaning 0=0, E=0, W=1, A=0.
>>
>> (3) Why is W (writeable) set to 1, when, according to [1], it is ignored
>> in 64-bit mode?
>
> Same as previous comment, the AP will be starting in real mode.
>
>>
>>
>> - "Sbit" seems to stand for bit 44 from the original representation --
>> constant 1. OK I think.
>>
>> - "Dpl", "Present" and "Avl" look OK to me.
>>
>> - Should "Reserved1" really be called that? It seems to map to bit 53 in
>> the original representation, and the L (long mode) bit actually matters
>> for the code segment. (It indeed appears undefined / reserved for data
>> segments.)
>>
>> Am I *totally* mistaken here and we're not even talking long mode?
>
> :)
>
>>
>> - "Db" and "Granularity" look OK.
>>
>> - SEV_ES_RESET_LDT_TYPE (value 2) matches "64-bit LDT" in "4.8.3 System
>> Descriptors", "Table 4-6. System-Segment Descriptor Types—Long Mode". OK.
>>
>> - SEV_ES_RESET_TSS_TYPE (value 3) seems wrong. In Table 4-6, value 3 is
>> associated with "Reserved (Illegal)". And, according to "12.2.2 TSS
>> Descriptor", "The TSS descriptor is a system-segment descriptor", which
>> makes me think that I'm looking at value 3 in the proper table (Table 4-6).
>
> I'll add a reference to table 14-2 under the "Processor Initialization"
> section.
>
>>
>>
>> (4) Can you please explain why SEV_ES_RESET_TSS_TYPE is 3, and not (say)
>> 0x9 ("Available 64-bit TSS") or 0xB ("Busy 64-bit TSS")?
>
> For processor reset, type 3 represents a busy 16-bit TSS.
>
> So I can work on clarifying the comments around all of the definitions and
> indicate that values are defined for processor reset support and that more
> definitions would be needed if the segment registers want to be examined /
> set in different modes. Thoughts?
My bad -- I feel really dumb now.
The four important macros which I got hung upon are all named
SEV_ES_RESET_*. Keyword being "reset". :/
With the typo (1) fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks & sorry!
Laszlo
>
> Thanks,
> Tom
>
>>
>> (Please note that this is only superficial pattern matching from my side
>> ("TSS"); I don't know the first thing about "hardware task management".)
>>
>>
>>> +typedef struct {
>>> + SEV_ES_SEGMENT_REGISTER Es;
>>> + SEV_ES_SEGMENT_REGISTER Cs;
>>> + SEV_ES_SEGMENT_REGISTER Ss;
>>> + SEV_ES_SEGMENT_REGISTER Ds;
>>> + SEV_ES_SEGMENT_REGISTER Fs;
>>> + SEV_ES_SEGMENT_REGISTER Gs;
>>> + SEV_ES_SEGMENT_REGISTER Gdtr;
>>> + SEV_ES_SEGMENT_REGISTER Ldtr;
>>> + SEV_ES_SEGMENT_REGISTER Idtr;
>>> + SEV_ES_SEGMENT_REGISTER Tr;
>>> + UINT8 Reserved1[42];
>>> + UINT8 Vmpl;
>>> + UINT8 Reserved2[5];
>>> + UINT64 Efer;
>>> + UINT8 Reserved3[112];
>>> + UINT64 Cr4;
>>> + UINT8 Reserved4[8];
>>> + UINT64 Cr0;
>>> + UINT64 Dr7;
>>> + UINT64 Dr6;
>>> + UINT64 Rflags;
>>> + UINT64 Rip;
>>> + UINT8 Reserved5[232];
>>> + UINT64 GPat;
>>> + UINT8 Reserved6[320];
>>> + UINT64 SevFeatures;
>>> + UINT8 Reserved7[48];
>>> + UINT64 XCr0;
>>> + UINT8 Reserved8[24];
>>> + UINT32 Mxcsr;
>>> + UINT16 X87Ftw;
>>> + UINT8 Reserved9[2];
>>> + UINT16 X87Fcw;
>>> +} SEV_ES_SAVE_AREA;
>>> +#pragma pack ()
>>> +
>>> #endif
>>>
>>
>> This part looks good to me.
>>
>> (I spent almost two hours reviewing this patch.)
>>
>> Thanks!
>> Laszlo
>>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure
2021-05-17 18:25 ` Erdem Aktas
@ 2021-05-18 16:02 ` Laszlo Ersek
2021-05-19 1:09 ` 回复: " gaoliming
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2021-05-18 16:02 UTC (permalink / raw)
To: Erdem Aktas, Brijesh Singh
Cc: devel, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Michael D Kinney, Liming Gao,
Zhiguang Liu
On 05/17/21 20:25, Erdem Aktas wrote:
> I verified that the values align with the GHCB spec publication:
> #56421 Revision: 2.00
>
> Just one question: is there any reason why GHCB_* defines are decimal
> while the SVM_EXIT_* are all in hexadecimal? Does EDK2 have any
> preference?
(I'm unaware of any preference in edk2 -- it's probably best to stick
with the base that the spec itself uses, but even using a different base
is not a huge deal, if the numbers in question are not large.)
Thanks!
Laszlo
>
> Reviewed-by: Erdem Aktas <erdemaktas@google.com>
>
> -Erdem
>
> On Wed, May 12, 2021 at 4:46 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>>
>> An SEV-SNP guest is required to perform the GHCB GPA registration. See
>> the GHCB specification for further details.
>>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
>> index cdb8f588ccf8..542e4cdf4782 100644
>> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
>> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
>> @@ -53,6 +53,11 @@ typedef union {
>> UINT64 Features:52;
>> } GhcbHypervisorFeatures;
>>
>> + struct {
>> + UINT64 Function:12;
>> + UINT64 GuestFrameNumber:52;
>> + } GhcbGpaRegister;
>> +
>> VOID *Ghcb;
>>
>> UINT64 GhcbPhysicalAddress;
>> @@ -62,6 +67,8 @@ typedef union {
>> #define GHCB_INFO_SEV_INFO_GET 2
>> #define GHCB_INFO_CPUID_REQUEST 4
>> #define GHCB_INFO_CPUID_RESPONSE 5
>> +#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
>> +#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
>> #define GHCB_HYPERVISOR_FEATURES_REQUEST 128
>> #define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
>> #define GHCB_INFO_TERMINATE_REQUEST 256
>> --
>> 2.17.1
>>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* 回复: [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure
2021-05-18 16:02 ` Laszlo Ersek
@ 2021-05-19 1:09 ` gaoliming
0 siblings, 0 replies; 27+ messages in thread
From: gaoliming @ 2021-05-19 1:09 UTC (permalink / raw)
To: 'Laszlo Ersek', 'Erdem Aktas',
'Brijesh Singh'
Cc: devel, 'James Bottomley', 'Min Xu',
'Jiewen Yao', 'Tom Lendacky',
'Jordan Justen', 'Ard Biesheuvel',
'Michael D Kinney', 'Zhiguang Liu'
Erdem:
Laszlo is right. The rule is to align spec definition.
Thanks
Liming
> -----邮件原件-----
> 发件人: Laszlo Ersek <lersek@redhat.com>
> 发送时间: 2021年5月19日 0:02
> 收件人: Erdem Aktas <erdemaktas@google.com>; Brijesh Singh
> <brijesh.singh@amd.com>
> 抄送: devel@edk2.groups.io; James Bottomley <jejb@linux.ibm.com>; Min
> Xu <min.m.xu@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; Jordan Justen
> <jordan.l.justen@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Zhiguang Liu <zhiguang.liu@intel.com>
> 主题: Re: [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for
> Register GPA structure
>
> On 05/17/21 20:25, Erdem Aktas wrote:
> > I verified that the values align with the GHCB spec publication:
> > #56421 Revision: 2.00
> >
> > Just one question: is there any reason why GHCB_* defines are decimal
> > while the SVM_EXIT_* are all in hexadecimal? Does EDK2 have any
> > preference?
>
> (I'm unaware of any preference in edk2 -- it's probably best to stick
> with the base that the spec itself uses, but even using a different base
> is not a huge deal, if the numbers in question are not large.)
>
> Thanks!
> Laszlo
>
> >
> > Reviewed-by: Erdem Aktas <erdemaktas@google.com>
> >
> > -Erdem
> >
> > On Wed, May 12, 2021 at 4:46 PM Brijesh Singh <brijesh.singh@amd.com>
> wrote:
> >>
> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> >>
> >> An SEV-SNP guest is required to perform the GHCB GPA registration. See
> >> the GHCB specification for further details.
> >>
> >> Cc: James Bottomley <jejb@linux.ibm.com>
> >> Cc: Min Xu <min.m.xu@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> >> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> Cc: Laszlo Ersek <lersek@redhat.com>
> >> Cc: Erdem Aktas <erdemaktas@google.com>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> >> ---
> >> MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++++++
> >> 1 file changed, 7 insertions(+)
> >>
> >> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h
> b/MdePkg/Include/Register/Amd/Fam17Msr.h
> >> index cdb8f588ccf8..542e4cdf4782 100644
> >> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
> >> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
> >> @@ -53,6 +53,11 @@ typedef union {
> >> UINT64 Features:52;
> >> } GhcbHypervisorFeatures;
> >>
> >> + struct {
> >> + UINT64 Function:12;
> >> + UINT64 GuestFrameNumber:52;
> >> + } GhcbGpaRegister;
> >> +
> >> VOID *Ghcb;
> >>
> >> UINT64 GhcbPhysicalAddress;
> >> @@ -62,6 +67,8 @@ typedef union {
> >> #define GHCB_INFO_SEV_INFO_GET 2
> >> #define GHCB_INFO_CPUID_REQUEST 4
> >> #define GHCB_INFO_CPUID_RESPONSE 5
> >> +#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
> >> +#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
> >> #define GHCB_HYPERVISOR_FEATURES_REQUEST 128
> >> #define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
> >> #define GHCB_INFO_TERMINATE_REQUEST 256
> >> --
> >> 2.17.1
> >>
> >
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2021-05-19 1:09 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-05-12 23:46 [PATCH v2 00/13] Add GHCBv2 macro and helpers Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 02/13] MdePkg/Register/Amd: realign macros with more space for future expansion Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection Brijesh Singh
2021-05-17 18:19 ` Erdem Aktas
2021-05-12 23:46 ` [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure Brijesh Singh
2021-05-17 18:25 ` Erdem Aktas
2021-05-18 16:02 ` Laszlo Ersek
2021-05-19 1:09 ` 回复: " gaoliming
2021-05-12 23:46 ` [PATCH v2 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change Brijesh Singh
2021-05-17 19:20 ` Erdem Aktas
2021-05-12 23:46 ` [PATCH v2 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation Brijesh Singh
2021-05-17 3:08 ` [edk2-devel] " Laszlo Ersek
2021-05-17 7:39 ` 回复: " gaoliming
2021-05-17 14:17 ` Lendacky, Thomas
2021-05-18 16:00 ` Laszlo Ersek
2021-05-12 23:46 ` [PATCH v2 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction Brijesh Singh
2021-05-17 3:16 ` [edk2-devel] " Laszlo Ersek
2021-05-12 23:46 ` [PATCH v2 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction Brijesh Singh
2021-05-17 3:25 ` [edk2-devel] " Laszlo Ersek
2021-05-12 23:46 ` [PATCH v2 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask() Brijesh Singh
2021-05-17 3:32 ` [edk2-devel] " Laszlo Ersek
2021-05-12 23:46 ` [PATCH v2 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask() Brijesh Singh
2021-05-12 23:46 ` [PATCH v2 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter Brijesh Singh
2021-05-17 3:46 ` [edk2-devel] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox