* [PATCH 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 8:32 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 02/13] MdePkg/Amd: add white spaces to retain alignment for future expansion Brijesh Singh
` (12 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
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] 35+ messages in thread* Re: [edk2-devel] [PATCH 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition
2021-05-07 20:38 ` [PATCH 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition Brijesh Singh
@ 2021-05-11 8:32 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 8:32 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/07/21 22:38, Brijesh Singh wrote:
> 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>
> 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
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 02/13] MdePkg/Amd: add white spaces to retain alignment for future expansion
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
2021-05-07 20:38 ` [PATCH 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 8:36 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection Brijesh Singh
` (11 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
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] 35+ messages in thread* Re: [edk2-devel] [PATCH 02/13] MdePkg/Amd: add white spaces to retain alignment for future expansion
2021-05-07 20:38 ` [PATCH 02/13] MdePkg/Amd: add white spaces to retain alignment for future expansion Brijesh Singh
@ 2021-05-11 8:36 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 8:36 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/07/21 22:38, Brijesh Singh wrote:
> 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>
> 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
>
Based on Liming's feedback at
<https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00157.html>,
I'll change the subject of this patch to:
MdePkg/Register/Amd: realign macros with more space for future expansion
(72 chars)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
2021-05-07 20:38 ` [PATCH 01/13] MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition Brijesh Singh
2021-05-07 20:38 ` [PATCH 02/13] MdePkg/Amd: add white spaces to retain alignment for future expansion Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 8:47 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure Brijesh Singh
` (10 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
Reviewd-by: Laszlo Ersek <lersek@redhat.com>
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..326b11479779 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 dections
+//
+#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] 35+ messages in thread* Re: [edk2-devel] [PATCH 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection
2021-05-07 20:38 ` [PATCH 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection Brijesh Singh
@ 2021-05-11 8:47 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 8:47 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/07/21 22:38, Brijesh Singh 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>
> Reviewd-by: Laszlo Ersek <lersek@redhat.com>
(1) It's best to use the clipboard for picking up feedback tags -- I
didn't mistype my R-b. (I've checked, plus I use keyboard shortcuts to
insert them; I don't type them out individually).
Anyway I can fix this up upon merge.
> 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..326b11479779 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 dections
> +//
(2) The comment style is OK now, but the comment itself has some kind of
typo. I can't really guess what "dections" was supposed to be. I guess I
can replace the comment with "Hypervisor feature sets".
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> +#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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (2 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 8:50 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change Brijesh Singh
` (9 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
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] 35+ messages in thread* Re: [edk2-devel] [PATCH 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure
2021-05-07 20:38 ` [PATCH 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure Brijesh Singh
@ 2021-05-11 8:50 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 8:50 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/07/21 22:38, Brijesh Singh 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>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++++++
> 1 file changed, 7 insertions(+)
Looks OK, thanks!
Laszlo
>
> 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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (3 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 8:59 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation Brijesh Singh
` (8 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
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 326b11479779..a15b4b7e2760 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] 35+ messages in thread* Re: [edk2-devel] [PATCH 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change
2021-05-07 20:38 ` [PATCH 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change Brijesh Singh
@ 2021-05-11 8:59 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 8:59 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/07/21 22:38, Brijesh Singh 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>
> 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 326b11479779..a15b4b7e2760 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;
(1) You didn't remove the stray space: my point (6) in
<https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00044.html>.
I'll fix it up.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Laszlo
> +} 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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (4 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 9:59 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction Brijesh Singh
` (7 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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
Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is
enabled in the guest VM. See the GHCB spec section for additional
details.
While at it, define the VMSA state save area that are required for
creating the AP. The save area format is defined in AMD APM volume
2 (Table B-4).
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>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Register/Amd/Ghcb.h | 70 ++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index a15b4b7e2760..956cefbc003c 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,67 @@ 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.
+//
+#pragma pack(1)
+typedef struct {
+ UINT16 Selector;
+ UINT16 Attributes;
+ UINT32 Limit;
+ UINT64 Base;
+} SEV_ES_SEGMENT_REGISTER;
+#pragma pack()
+
+#define SEV_ES_RESET_CS_ATTRIBUTES (BIT7 | BIT4 | BIT3 | BIT1)
+#define SEV_ES_RESET_DS_ATTRIBUTES (BIT7 | BIT4 | BIT1)
+#define SEV_ES_RESET_ES_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
+#define SEV_ES_RESET_FS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
+#define SEV_ES_RESET_GS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
+#define SEV_ES_RESET_SS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
+
+#define SEV_ES_RESET_GDTR_ATTRIBUTES 0
+#define SEV_ES_RESET_LDTR_ATTRIBUTES (BIT7 | 2)
+#define SEV_ES_RESET_IDTR_ATTRIBUTES 0
+#define SEV_ES_RESET_TR_ATTRIBUTES (BIT7 | 3)
+
+#pragma pack(1)
+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;
+ UINT64 X87Ftw;
+ UINT64 Reserved9[8];
+ UINT64 X87Fcw;
+} SEV_ES_SAVE_AREA;
+#pragma pack()
+
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
2021-05-07 20:38 ` [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation Brijesh Singh
@ 2021-05-11 9:59 ` Laszlo Ersek
2021-05-11 15:43 ` Lendacky, Thomas
0 siblings, 1 reply; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 9:59 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/07/21 22:38, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is
> enabled in the guest VM. See the GHCB spec section for additional
> details.
(1) The actual section reference is missing. I'll fix it up: from where
the spec introduces exit code 0x8000_0013, the sections appear to be
4.1.9 and 4.3.2. Also, Table 5. "List of Supported Non-Automatic Events"
is relevant for the SVM_VMGEXIT_SNP_AP_* macros.
>
> While at it, define the VMSA state save area that are required for
(2) I think "area" (singular) is correct here, so we should say "is".
I'll update it.
> creating the AP. The save area format is defined in AMD APM volume
> 2 (Table B-4).
>
> 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>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> MdePkg/Include/Register/Amd/Ghcb.h | 70 ++++++++++++++++++++++++++++++
> 1 file changed, 70 insertions(+)
>
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
> index a15b4b7e2760..956cefbc003c 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,67 @@ 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.
> +//
> +#pragma pack(1)
> +typedef struct {
> + UINT16 Selector;
> + UINT16 Attributes;
> + UINT32 Limit;
> + UINT64 Base;
> +} SEV_ES_SEGMENT_REGISTER;
> +#pragma pack()
(3) there should be a space character between "pack" and "(" -- two
instances.
> +
> +#define SEV_ES_RESET_CS_ATTRIBUTES (BIT7 | BIT4 | BIT3 | BIT1)
> +#define SEV_ES_RESET_DS_ATTRIBUTES (BIT7 | BIT4 | BIT1)
> +#define SEV_ES_RESET_ES_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
> +#define SEV_ES_RESET_FS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
> +#define SEV_ES_RESET_GS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
> +#define SEV_ES_RESET_SS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
> +
> +#define SEV_ES_RESET_GDTR_ATTRIBUTES 0
> +#define SEV_ES_RESET_LDTR_ATTRIBUTES (BIT7 | 2)
> +#define SEV_ES_RESET_IDTR_ATTRIBUTES 0
> +#define SEV_ES_RESET_TR_ATTRIBUTES (BIT7 | 3)
(4) ... I guess I can't go ahead merging this myself, after all (Liming
may of course still merge the MdePkg patches, if he wants to).
My problem here is that the bit positions are cryptic.
I've found the *normal* (not SEV-ES) segment descriptor attributes in
the AMD APM (publication #24593, revision 3.37, date March 2021, volume
2, sections sections 4.7 and 4.8).
However, the bit positions SEV-ES descriptors are surely different. For
the "normal" segment descriptors, we already have the
IA32_SEGMENT_DESCRIPTOR type in edk2, with the nicely broken-out
attribute bits. The bit meanings within
"SEV_ES_SEGMENT_REGISTER.Attributes" remain unclear to me.
Please at least provide a *specific* documentation reference in the
commit message where I can verify (or at least "decode") the attribute bits.
> +
> +#pragma pack(1)
> +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];
(5) This doesn't seem right. The comment higher up says that "Only the
fields required to be set to a non-zero value are defined", which is
fine. But in Table B-4, between fields "Tr" and "Vmpl", we have 5 qword
fields (PL0_SSP through PL3_SSP, plus U_CET), and a reserved dword
field. That makes for 5*8+4 = 44 bytes, not 42.
Hmmm. If I look at the table differently, I get a different result.
Namely, the first offset right after Tr is 0A0h, and the start offset of
Vmpl is 0CAh. The difference is indeed 42 (decimal).
Ah, I've found it. The bug is in the spec. The Reserved field at offset
0C8h is listed with size "dword", but the field right after it, namely
VMPL, starts at offset 0CAh -- that is, only two bytes later. Which
means that the "dword" size for Reserved is wrong; it should be "word" only.
I couldn't find a more recent release of the APM than "revision 3.37".
Please add a remark to the commit message on this patch that highlights
this typo in the spec.
> + UINT8 Vmpl;
> + UINT8 Reserved2[5];
> + UINT64 Efer;
> + UINT8 Reserved3[112];
OK
> + UINT64 Cr4;
> + UINT8 Reserved4[8];
OK (although I'm not sure much is saved over spelling out "Cr3")
> + UINT64 Cr0;
> + UINT64 Dr7;
> + UINT64 Dr6;
> + UINT64 Rflags;
> + UINT64 Rip;
> + UINT8 Reserved5[232];
OK
> + UINT64 GPat;
> + UINT8 Reserved6[320];
OK
> + UINT64 SevFeatures;
> + UINT8 Reserved7[48];
OK
> + UINT64 XCr0;
> + UINT8 Reserved8[24];
OK
> + UINT32 Mxcsr;
> + UINT64 X87Ftw;
(6) If you are packing all four words (X87_FTW, X87_FSW, X87_FCW,
X87_FOP) into a qword, then please give the latter a different name. The
spec associates X87_FTW with just the word at offset 40Ch. I propose the
name "FpState" for the UINT64 field in the edk2 structure.
> + UINT64 Reserved9[8];
> + UINT64 X87Fcw;
(7) Ugh, wait, something's wrong -- you *are* apparently adding a field
for X87_FCW separately! But then the type of X87Ftw is wrong -- it
should be UINT16. Same for X87Fcw.
The distance between them is also wrong, it should be only 2 bytes
(basically the X87_FSW field). I have no idea at all where the 8*8=64
bytes padding comes from!
> +} SEV_ES_SAVE_AREA;
> +#pragma pack()
(8) same as (3); please insert a space character between
> +
> #endif
>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
2021-05-11 9:59 ` [edk2-devel] " Laszlo Ersek
@ 2021-05-11 15:43 ` Lendacky, Thomas
2021-05-13 11:29 ` Laszlo Ersek
0 siblings, 1 reply; 35+ messages in thread
From: Lendacky, Thomas @ 2021-05-11 15:43 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/11/21 4:59 AM, Laszlo Ersek wrote:
> On 05/07/21 22:38, 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%3D3275&data=04%7C01%7Cthomas.lendacky%40amd.com%7C92c1323bd1e84a2a38e208d914636ddf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563239563579592%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=DMDhcseilROSsL6EISUoT9p0pI%2BmXtEC3rLHIQS4NmI%3D&reserved=0
>>
>> Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is
>> enabled in the guest VM. See the GHCB spec section for additional
>> details.
>
> (1) The actual section reference is missing. I'll fix it up: from where
> the spec introduces exit code 0x8000_0013, the sections appear to be
> 4.1.9 and 4.3.2. Also, Table 5. "List of Supported Non-Automatic Events"
> is relevant for the SVM_VMGEXIT_SNP_AP_* macros.
There are some needed changes to this patch, so I can fix that up. I just
avoided putting actual section numbers in there because it is possible
that they can change in future versions.
>
>>
>> While at it, define the VMSA state save area that are required for
>
> (2) I think "area" (singular) is correct here, so we should say "is".
> I'll update it.
I can fix that up.
>
>> creating the AP. The save area format is defined in AMD APM volume
>> 2 (Table B-4).
>>
>> 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>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> MdePkg/Include/Register/Amd/Ghcb.h | 70 ++++++++++++++++++++++++++++++
>> 1 file changed, 70 insertions(+)
>>
>> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
>> index a15b4b7e2760..956cefbc003c 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,67 @@ 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.
>> +//
>> +#pragma pack(1)
>> +typedef struct {
>> + UINT16 Selector;
>> + UINT16 Attributes;
>> + UINT32 Limit;
>> + UINT64 Base;
>> +} SEV_ES_SEGMENT_REGISTER;
>> +#pragma pack()
>
> (3) there should be a space character between "pack" and "(" -- two
> instances.
Will do.
>
>> +
>> +#define SEV_ES_RESET_CS_ATTRIBUTES (BIT7 | BIT4 | BIT3 | BIT1)
>> +#define SEV_ES_RESET_DS_ATTRIBUTES (BIT7 | BIT4 | BIT1)
>> +#define SEV_ES_RESET_ES_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>> +#define SEV_ES_RESET_FS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>> +#define SEV_ES_RESET_GS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>> +#define SEV_ES_RESET_SS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>> +
>> +#define SEV_ES_RESET_GDTR_ATTRIBUTES 0
>> +#define SEV_ES_RESET_LDTR_ATTRIBUTES (BIT7 | 2)
>> +#define SEV_ES_RESET_IDTR_ATTRIBUTES 0
>> +#define SEV_ES_RESET_TR_ATTRIBUTES (BIT7 | 3)
>
> (4) ... I guess I can't go ahead merging this myself, after all (Liming
> may of course still merge the MdePkg patches, if he wants to).
>
> My problem here is that the bit positions are cryptic.
>
> I've found the *normal* (not SEV-ES) segment descriptor attributes in
> the AMD APM (publication #24593, revision 3.37, date March 2021, volume
> 2, sections sections 4.7 and 4.8).
>
> However, the bit positions SEV-ES descriptors are surely different. For
> the "normal" segment descriptors, we already have the
> IA32_SEGMENT_DESCRIPTOR type in edk2, with the nicely broken-out
> attribute bits. The bit meanings within
> "SEV_ES_SEGMENT_REGISTER.Attributes" remain unclear to me.
>
> Please at least provide a *specific* documentation reference in the
> commit message where I can verify (or at least "decode") the attribute bits.
Yeah, it is a strange format. The format is documented in sections 15.5
(VMRUN Instruction) and 10 (System-Management Mode).
I can try to further document the bit assignments, e.g.
#define SEV_ES_SEGMENT_ATTRIBUTE_PRESENT BIT7
#define SEV_ES_SEGMENT_ATTRIBUTE_USER BIT4
etc.
>
>> +
>> +#pragma pack(1)
>> +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];
>
> (5) This doesn't seem right. The comment higher up says that "Only the
> fields required to be set to a non-zero value are defined", which is
> fine. But in Table B-4, between fields "Tr" and "Vmpl", we have 5 qword
> fields (PL0_SSP through PL3_SSP, plus U_CET), and a reserved dword
> field. That makes for 5*8+4 = 44 bytes, not 42.
>
> Hmmm. If I look at the table differently, I get a different result.
> Namely, the first offset right after Tr is 0A0h, and the start offset of
> Vmpl is 0CAh. The difference is indeed 42 (decimal).
>
> Ah, I've found it. The bug is in the spec. The Reserved field at offset
> 0C8h is listed with size "dword", but the field right after it, namely
> VMPL, starts at offset 0CAh -- that is, only two bytes later. Which
> means that the "dword" size for Reserved is wrong; it should be "word" only.
Right, the offsets are correct, the use of "dword" is incorrect.
>
> I couldn't find a more recent release of the APM than "revision 3.37".
> Please add a remark to the commit message on this patch that highlights
> this typo in the spec.
Will do.
>
>> + UINT8 Vmpl;
>> + UINT8 Reserved2[5];
>> + UINT64 Efer;
>> + UINT8 Reserved3[112];
>
> OK
>
>> + UINT64 Cr4;
>> + UINT8 Reserved4[8];
>
> OK (although I'm not sure much is saved over spelling out "Cr3")
>
>> + UINT64 Cr0;
>> + UINT64 Dr7;
>> + UINT64 Dr6;
>> + UINT64 Rflags;
>> + UINT64 Rip;
>> + UINT8 Reserved5[232];
>
> OK
>
>> + UINT64 GPat;
>> + UINT8 Reserved6[320];
>
> OK
>
>> + UINT64 SevFeatures;
>> + UINT8 Reserved7[48];
>
> OK
>
>> + UINT64 XCr0;
>> + UINT8 Reserved8[24];
>
> OK
>
>> + UINT32 Mxcsr;
>> + UINT64 X87Ftw;
>
> (6) If you are packing all four words (X87_FTW, X87_FSW, X87_FCW,
> X87_FOP) into a qword, then please give the latter a different name. The
> spec associates X87_FTW with just the word at offset 40Ch. I propose the
> name "FpState" for the UINT64 field in the edk2 structure.
Yes, that is incorrect. They should be UINT16 entries as you state below.
>
>> + UINT64 Reserved9[8];
>> + UINT64 X87Fcw;
>
> (7) Ugh, wait, something's wrong -- you *are* apparently adding a field
> for X87_FCW separately! But then the type of X87Ftw is wrong -- it
> should be UINT16. Same for X87Fcw.
>
> The distance between them is also wrong, it should be only 2 bytes
> (basically the X87_FSW field). I have no idea at all where the 8*8=64
> bytes padding comes from!
>
>> +} SEV_ES_SAVE_AREA;
>> +#pragma pack()
>
> (8) same as (3); please insert a space character between
Will do.
Thanks,
Tom
>
>> +
>> #endif
>>
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation
2021-05-11 15:43 ` Lendacky, Thomas
@ 2021-05-13 11:29 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-13 11:29 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/11/21 17:43, Tom Lendacky wrote:
> On 5/11/21 4:59 AM, Laszlo Ersek wrote:
>> On 05/07/21 22:38, 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%3D3275&data=04%7C01%7Cthomas.lendacky%40amd.com%7C92c1323bd1e84a2a38e208d914636ddf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563239563579592%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=DMDhcseilROSsL6EISUoT9p0pI%2BmXtEC3rLHIQS4NmI%3D&reserved=0
>>>
>>> Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is
>>> enabled in the guest VM. See the GHCB spec section for additional
>>> details.
>>
>> (1) The actual section reference is missing. I'll fix it up: from where
>> the spec introduces exit code 0x8000_0013, the sections appear to be
>> 4.1.9 and 4.3.2. Also, Table 5. "List of Supported Non-Automatic Events"
>> is relevant for the SVM_VMGEXIT_SNP_AP_* macros.
>
> There are some needed changes to this patch, so I can fix that up. I just
> avoided putting actual section numbers in there because it is possible
> that they can change in future versions.
As long as AMD keeps older revisions of the spec available for download,
I think it's fine to include precise references (covering the spec
revision number too).
>>> +#define SEV_ES_RESET_CS_ATTRIBUTES (BIT7 | BIT4 | BIT3 | BIT1)
>>> +#define SEV_ES_RESET_DS_ATTRIBUTES (BIT7 | BIT4 | BIT1)
>>> +#define SEV_ES_RESET_ES_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>>> +#define SEV_ES_RESET_FS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>>> +#define SEV_ES_RESET_GS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>>> +#define SEV_ES_RESET_SS_ATTRIBUTES SEV_ES_RESET_DS_ATTRIBUTES
>>> +
>>> +#define SEV_ES_RESET_GDTR_ATTRIBUTES 0
>>> +#define SEV_ES_RESET_LDTR_ATTRIBUTES (BIT7 | 2)
>>> +#define SEV_ES_RESET_IDTR_ATTRIBUTES 0
>>> +#define SEV_ES_RESET_TR_ATTRIBUTES (BIT7 | 3)
>>
>> (4) ... I guess I can't go ahead merging this myself, after all (Liming
>> may of course still merge the MdePkg patches, if he wants to).
>>
>> My problem here is that the bit positions are cryptic.
>>
>> I've found the *normal* (not SEV-ES) segment descriptor attributes in
>> the AMD APM (publication #24593, revision 3.37, date March 2021, volume
>> 2, sections sections 4.7 and 4.8).
>>
>> However, the bit positions SEV-ES descriptors are surely different. For
>> the "normal" segment descriptors, we already have the
>> IA32_SEGMENT_DESCRIPTOR type in edk2, with the nicely broken-out
>> attribute bits. The bit meanings within
>> "SEV_ES_SEGMENT_REGISTER.Attributes" remain unclear to me.
>>
>> Please at least provide a *specific* documentation reference in the
>> commit message where I can verify (or at least "decode") the attribute bits.
>
> Yeah, it is a strange format. The format is documented in sections 15.5
> (VMRUN Instruction) and 10 (System-Management Mode).
>
> I can try to further document the bit assignments, e.g.
>
> #define SEV_ES_SEGMENT_ATTRIBUTE_PRESENT BIT7
> #define SEV_ES_SEGMENT_ATTRIBUTE_USER BIT4
> etc.
If it's not a big burden, can you please do both? I.e., (a) include the
spec reference(s) in the commit message, and (b) introduce either
bit-fields, or symbolic names (macros), for the relevant bits?
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (5 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 10:29 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction Brijesh Singh
` (6 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Include/Library/BaseLib.h | 46 +++++++++++++++++++++++
MdePkg/Include/X64/Nasm.inc | 8 ++++
MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 42 +++++++++++++++++++++
4 files changed, 97 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..f177034af6a1 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4813,6 +4813,52 @@ 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 made any changes to the RMP entry.
+//
+#define PVALIDATE_RET_NO_RMPUPDATE 255
+
+/**
+ Execute a PVALIDATE instruction to validate or 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 Validate or rescinds.
+ @param[in] Address The guest virtual address to validate.
+
+ @retval The return value from the PVALIDATE instruction, and
+ PVALIDATE_RET_NO_RMPUPDATE when there was no change in
+ the RMP entry.
+**/
+UINTN
+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..b20dac7e6831
--- /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
+
+;-----------------------------------------------------------------------------
+; UINTN
+; EFIAPI
+; AsmPvalidate (
+; IN UINT32 RmpPageSize
+; IN UINT32 Validate,
+; IN PHYSICAL_ADDRESS Address
+; )
+;-----------------------------------------------------------------------------
+global ASM_PFX(AsmPvalidate)
+ASM_PFX(AsmPvalidate):
+ mov rax, r8
+
+ PVALIDATE
+
+ ; Save the carry flag.
+ setb 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
+ jz PvalidateExit
+
+ ; Return the PVALIDATE_RET_NO_RMPUPDATE.
+ mov rax, 255
+
+PvalidateExit:
+ ret
--
2.17.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction
2021-05-07 20:38 ` [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction Brijesh Singh
@ 2021-05-11 10:29 ` Laszlo Ersek
2021-05-11 17:18 ` Brijesh Singh
0 siblings, 1 reply; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 10:29 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/07/21 22:38, 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>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> MdePkg/Library/BaseLib/BaseLib.inf | 1 +
> MdePkg/Include/Library/BaseLib.h | 46 +++++++++++++++++++++++
> MdePkg/Include/X64/Nasm.inc | 8 ++++
> MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 42 +++++++++++++++++++++
> 4 files changed, 97 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..f177034af6a1 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4813,6 +4813,52 @@ 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 made any changes to the RMP entry.
(1) Typo: should be "did not make".
> +//
> +#define PVALIDATE_RET_NO_RMPUPDATE 255
> +
> +/**
> + Execute a PVALIDATE instruction to validate or rescinds validation of a guest
(2) should be "to validate or to rescind validation" (infinitive form).
> + 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 Validate or rescinds.
(3) If you use the imperative for "validate", then "rescinds"
(indicative) reads strangely.
> + @param[in] Address The guest virtual address to validate.
> +
> + @retval The return value from the PVALIDATE instruction, and
> + PVALIDATE_RET_NO_RMPUPDATE when there was no change in
> + the RMP entry.
(4) @retval is only usable with actual return values (constants). If you
provide a natural language explanation, then @return is the proper
doxygen directive.
You can combine these BTW, for example:
@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 codes from the PVALIDATE
instruction.
> +**/
> +UINTN
> +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
Thanks for filing the NASM BZ!
> diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
> new file mode 100644
> index 000000000000..b20dac7e6831
> --- /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
> +
> +;-----------------------------------------------------------------------------
> +; UINTN
> +; EFIAPI
> +; AsmPvalidate (
> +; IN UINT32 RmpPageSize
> +; IN UINT32 Validate,
> +; IN PHYSICAL_ADDRESS Address
> +; )
(5) This prototype does not match the one from the header file.
I guess it's reasonable to replace the enum type and the BOOLEAN type
with UINT32, in the assembly source code. But then I don't understand
why PHYSICAL_ADDRESS is not replaced with UINT64 -- that would only be
consistent with the other replacements.
Furthermore, the parameter *names* PageSize and RmpPageSize do not match.
> +;-----------------------------------------------------------------------------
> +global ASM_PFX(AsmPvalidate)
> +ASM_PFX(AsmPvalidate):
> + mov rax, r8
> +
> + PVALIDATE
> +
> + ; Save the carry flag.
> + setb 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
> + jz PvalidateExit
> +
> + ; Return the PVALIDATE_RET_NO_RMPUPDATE.
> + mov rax, 255
> +
> +PvalidateExit:
> + ret
>
This looks OK. I'm not very used to reading assembly, so "setc" (set if
carry) instead of the equivalent "setb" (set if below) would have been
easier to parse.
Similarly, "je" (jump if equal) would be easier to read than the
equivalent "jz" (jump if zero), especially after using "jne" (and not
"jnz") with the previous "cmp".
But, the assembly does look correct to me, so there's no need to change it.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction
2021-05-11 10:29 ` [edk2-devel] " Laszlo Ersek
@ 2021-05-11 17:18 ` Brijesh Singh
0 siblings, 0 replies; 35+ messages in thread
From: Brijesh Singh @ 2021-05-11 17:18 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: brijesh.singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Erdem Aktas, Michael D Kinney,
Liming Gao, Zhiguang Liu
On 5/11/21 5:29 AM, Laszlo Ersek wrote:
> On 05/07/21 22:38, Brijesh Singh wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5e03d8f093c430d01b608d91467bc91%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563258060234952%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QFATwHj5Jtoughp2bLTtFChUuKCWzQBSOGcZb2I%2B534%3D&reserved=0
>>
>> 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>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> MdePkg/Library/BaseLib/BaseLib.inf | 1 +
>> MdePkg/Include/Library/BaseLib.h | 46 +++++++++++++++++++++++
>> MdePkg/Include/X64/Nasm.inc | 8 ++++
>> MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 42 +++++++++++++++++++++
>> 4 files changed, 97 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..f177034af6a1 100644
>> --- a/MdePkg/Include/Library/BaseLib.h
>> +++ b/MdePkg/Include/Library/BaseLib.h
>> @@ -4813,6 +4813,52 @@ 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 made any changes to the RMP entry.
> (1) Typo: should be "did not make".
Noted.
>
>> +//
>> +#define PVALIDATE_RET_NO_RMPUPDATE 255
>> +
>> +/**
>> + Execute a PVALIDATE instruction to validate or rescinds validation of a guest
> (2) should be "to validate or to rescind validation" (infinitive form).
Noted.
>
>> + 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 Validate or rescinds.
> (3) If you use the imperative for "validate", then "rescinds"
> (indicative) reads strangely.
I will use validate or invalidate in future patches.
>
>> + @param[in] Address The guest virtual address to validate.
>> +
>> + @retval The return value from the PVALIDATE instruction, and
>> + PVALIDATE_RET_NO_RMPUPDATE when there was no change in
>> + the RMP entry.
> (4) @retval is only usable with actual return values (constants). If you
> provide a natural language explanation, then @return is the proper
> doxygen directive.
>
> You can combine these BTW, for example:
>
> @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 codes from the PVALIDATE
> instruction.
Will do.
>> +**/
>> +UINTN
>> +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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.nasm.us%2Fshow_bug.cgi%3Fid%3D3392753&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5e03d8f093c430d01b608d91467bc91%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563258060234952%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Sk1vgmvOHjp2VVlONWIqDMZnb5nfV5%2Fl9pegv9AQgmg%3D&reserved=0
>> +;
>> +%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
> Thanks for filing the NASM BZ!
>
>> diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
>> new file mode 100644
>> index 000000000000..b20dac7e6831
>> --- /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
>> +
>> +;-----------------------------------------------------------------------------
>> +; UINTN
>> +; EFIAPI
>> +; AsmPvalidate (
>> +; IN UINT32 RmpPageSize
>> +; IN UINT32 Validate,
>> +; IN PHYSICAL_ADDRESS Address
>> +; )
> (5) This prototype does not match the one from the header file.
>
> I guess it's reasonable to replace the enum type and the BOOLEAN type
> with UINT32, in the assembly source code. But then I don't understand
> why PHYSICAL_ADDRESS is not replaced with UINT64 -- that would only be
> consistent with the other replacements.
>
> Furthermore, the parameter *names* PageSize and RmpPageSize do not match.
>
Will fix in next rev.
>> +;-----------------------------------------------------------------------------
>> +global ASM_PFX(AsmPvalidate)
>> +ASM_PFX(AsmPvalidate):
>> + mov rax, r8
>> +
>> + PVALIDATE
>> +
>> + ; Save the carry flag.
>> + setb 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
>> + jz PvalidateExit
>> +
>> + ; Return the PVALIDATE_RET_NO_RMPUPDATE.
>> + mov rax, 255
>> +
>> +PvalidateExit:
>> + ret
>>
> This looks OK. I'm not very used to reading assembly, so "setc" (set if
> carry) instead of the equivalent "setb" (set if below) would have been
> easier to parse.
>
> Similarly, "je" (jump if equal) would be easier to read than the
> equivalent "jz" (jump if zero), especially after using "jne" (and not
> "jnz") with the previous "cmp".
>
> But, the assembly does look correct to me, so there's no need to change it.
>
> Thanks!
> Laszlo
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (6 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 11:01 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask() Brijesh Singh
` (5 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
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 | 36 +++++++++++++++++++-
MdePkg/Include/X64/Nasm.inc | 8 +++++
MdePkg/Library/BaseLib/X64/RmpAdjust.nasm | 40 +++++++++++++++++++++++
4 files changed, 84 insertions(+), 1 deletion(-)
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 f177034af6a1..04e58f995b9a 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4857,9 +4857,43 @@ 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 Eax. This function is only available 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 Eax
+**/
+UINTN
+EFIAPI
+AsmRmpAdjust (
+ IN UINTN Rax,
+ IN UINTN Rcx,
+ IN UINTN Rdx
+ );
#endif
-
#if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
///
/// IA32 and x64 Specific Functions.
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..f2c295b67c9c
--- /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
+
+;-----------------------------------------------------------------------------
+; UINTN
+; EFIAPI
+; AsmRmpAdjust (
+; IN UINTN Rax,
+; IN UINTN Rcx,
+; IN UINTN 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] 35+ messages in thread* Re: [edk2-devel] [PATCH 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction
2021-05-07 20:38 ` [PATCH 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction Brijesh Singh
@ 2021-05-11 11:01 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 11:01 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/07/21 22:38, 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>
> 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 | 36 +++++++++++++++++++-
> MdePkg/Include/X64/Nasm.inc | 8 +++++
> MdePkg/Library/BaseLib/X64/RmpAdjust.nasm | 40 +++++++++++++++++++++++
> 4 files changed, 84 insertions(+), 1 deletion(-)
> 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 f177034af6a1..04e58f995b9a 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4857,9 +4857,43 @@ 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 Eax. This function is only available x64.
(1) trivial typo: IMO it should be "on X64" (preposition missing, and
X64 should be upper case).
> +
> + 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 Eax
> +**/
> +UINTN
(2) Not a "hard requirement", just something I thought I'd raise: both
the spec, and the leading comment (twice), say that the return code is
in EAX (not RAX). Would it make sense to use UINT32 for the return type
of the function?
(3) Since we are talking return codes... For PVALIDATE, the previous
patch introduces macros for the return codes. I haven't looked at
RMPADJUST before, but now it seems like SEV-ES-related machine
instructions come with a "global" status code table: 0 for SUCCESS, 1
for FAIL_INPUT, 6 for FAIL_SIZEMISMATCH (<-- all of these are shared by
PVALIDATE and RMPADJUST), and now FAIL_PERMISSION (2) for RMPADJUST only.
So now I wonder if these macros belong in an AMD-specific header file...
Anyway, I definitely defer to Liming and Mike on this MdePkg content.
> +EFIAPI
> +AsmRmpAdjust (
> + IN UINTN Rax,
> + IN UINTN Rcx,
> + IN UINTN Rdx
> + );
(4) Given that we really call these R*x, shouldn't we make them
explicitly UINT64? I don't think there's an interpretation for RAX that
is *not* 64-bit.
> #endif
>
> -
(5) Indeed, this newline is superfluous, I just didn't want to obsess
about it under patch #7. If you agree it's unneeded, then please drop it
from patch#7.
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
> ///
> /// IA32 and x64 Specific Functions.
> 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..f2c295b67c9c
> --- /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
> +
> +;-----------------------------------------------------------------------------
> +; UINTN
> +; EFIAPI
> +; AsmRmpAdjust (
> +; IN UINTN Rax,
> +; IN UINTN Rcx,
> +; IN UINTN Rdx
> +; )
(6) If you agree with my points (2) and (4), then please don't forget to
sync this part.
> +;-----------------------------------------------------------------------------
> +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
>
I only made trivial comments on this patch; even if you disagreed with
everything I said, I'd be OK.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask()
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (7 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 11:16 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask Brijesh Singh
` (4 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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 simplifies 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>
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 +++++++++++++++++++
5 files changed, 145 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
+ );
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask()
2021-05-07 20:38 ` [PATCH 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask() Brijesh Singh
@ 2021-05-11 11:16 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 11:16 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas
On 05/07/21 22:38, 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 simplifies version of
(1) s/simplifies/simplified/
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> 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>
> 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 +++++++++++++++++++
> 5 files changed, 145 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
> + );
> +}
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (8 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask() Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 11:18 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask Brijesh Singh
` (3 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
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] 35+ messages in thread* Re: [edk2-devel] [PATCH 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask
2021-05-07 20:38 ` [PATCH 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask Brijesh Singh
@ 2021-05-11 11:18 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 11:18 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas
On 05/07/21 22:38, Brijesh Singh wrote:
> 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>
> 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);
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (9 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 10/13] OvmfPkg/AmdSevDxe: use MemEncryptSevClearMmioPageEncMask() to clear EncMask Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 11:19 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask() Brijesh Singh
` (2 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
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] 35+ messages in thread* Re: [edk2-devel] [PATCH 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask
2021-05-07 20:38 ` [PATCH 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask Brijesh Singh
@ 2021-05-11 11:19 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 11:19 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas
On 05/07/21 22:38, Brijesh Singh wrote:
> 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>
> 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);
> }
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask()
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (10 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 11/13] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: use Mmio helper to clear enc mask Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 11:20 ` [edk2-devel] " Laszlo Ersek
2021-05-07 20:38 ` [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter Brijesh Singh
2021-05-08 1:43 ` 回复: [edk2-devel] [PATCH 00/13] Add GHCBv2 macro and helpers gaoliming
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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>
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] 35+ messages in thread* Re: [edk2-devel] [PATCH 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask()
2021-05-07 20:38 ` [PATCH 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask() Brijesh Singh
@ 2021-05-11 11:20 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 11:20 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas
On 05/07/21 22:38, Brijesh Singh wrote:
> 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>
> 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)) {
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (11 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 12/13] OvmfPkg/TpmMmioSevDecryptPei: use MemEncryptSevClearMmioPageEncMask() Brijesh Singh
@ 2021-05-07 20:38 ` Brijesh Singh
2021-05-11 11:55 ` [edk2-devel] " Laszlo Ersek
2021-05-08 1:43 ` 回复: [edk2-devel] [PATCH 00/13] Add GHCBv2 macro and helpers gaoliming
13 siblings, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-07 20:38 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().
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 ++--
.../Ia32/MemEncryptSevLib.c | 10 ++----
.../X64/MemEncryptSevLib.c | 16 +++-------
.../X64/PeiDxeVirtualMemory.c | 32 +++++++++++--------
.../X64/SecVirtualMemory.c | 8 ++---
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +-
OvmfPkg/PlatformPei/AmdSev.c | 3 +-
10 files changed, 35 insertions(+), 66 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..41e4b291d070 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..ad1021bd3e43 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -555,8 +555,7 @@ EnableReadOnlyPageWriteProtect (
address of a memory region.
@param[in] Length The length of memory region
@param[in] Mode Set or Clear mode
- @param[in] CacheFlush Flush the caches before applying the
- encryption mask
+ @param[in] Mmio The physical address range is Mmio.
@retval RETURN_SUCCESS The attributes were cleared for the
memory region.
@@ -572,7 +571,7 @@ SetMemoryEncDec (
IN PHYSICAL_ADDRESS PhysicalAddress,
IN UINTN Length,
IN MAP_RANGE_MODE Mode,
- IN BOOLEAN CacheFlush
+ IN BOOLEAN Mmio
)
{
PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
@@ -585,12 +584,23 @@ SetMemoryEncDec (
UINT64 AddressEncMask;
BOOLEAN IsWpEnabled;
RETURN_STATUS Status;
+ BOOLEAN CacheFlush;
//
// Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings.
//
PageMapLevel4Entry = NULL;
+ //
+ // The cache need to flushed for the non-Mmio address range.
+ //
+ if (Mmio == TRUE) {
+ CacheFlush = FALSE;
+ } else {
+ CacheFlush = TRUE;
+ }
+
+ //
DEBUG ((
DEBUG_VERBOSE,
"%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u\n",
@@ -828,8 +838,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 +850,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryDecrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
@@ -852,7 +859,7 @@ InternalMemEncryptSevSetMemoryDecrypted (
PhysicalAddress,
Length,
ClearCBit,
- Flush
+ FALSE
);
}
@@ -865,8 +872,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 +884,7 @@ EFIAPI
InternalMemEncryptSevSetMemoryEncrypted (
IN PHYSICAL_ADDRESS Cr3BaseAddress,
IN PHYSICAL_ADDRESS PhysicalAddress,
- IN UINTN Length,
- IN BOOLEAN Flush
+ IN UINTN Length
)
{
return SetMemoryEncDec (
@@ -888,7 +892,7 @@ InternalMemEncryptSevSetMemoryEncrypted (
PhysicalAddress,
Length,
SetCBit,
- Flush
+ FALSE
);
}
@@ -921,6 +925,6 @@ InternalMemEncryptSevClearMmioPageEncMask (
PhysicalAddress,
Length,
ClearCBit,
- FALSE
+ TRUE
);
}
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
index bca5e3febb1b..24d19d3ca161 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] 35+ messages in thread* Re: [edk2-devel] [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
2021-05-07 20:38 ` [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter Brijesh Singh
@ 2021-05-11 11:55 ` Laszlo Ersek
2021-05-11 17:45 ` Brijesh Singh
0 siblings, 1 reply; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-11 11:55 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas
I don't fully understand the updates in this patch:
On 05/07/21 22:38, 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().
This looks good; it matches my request (1) from:
https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00109.html
>
> 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 ++--
> .../Ia32/MemEncryptSevLib.c | 10 ++----
> .../X64/MemEncryptSevLib.c | 16 +++-------
> .../X64/PeiDxeVirtualMemory.c | 32 +++++++++++--------
> .../X64/SecVirtualMemory.c | 8 ++---
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +-
> OvmfPkg/PlatformPei/AmdSev.c | 3 +-
> 10 files changed, 35 insertions(+), 66 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..41e4b291d070 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",
(1) You missed my comment (2) in
<https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00109.html>.
> 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..ad1021bd3e43 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> @@ -555,8 +555,7 @@ EnableReadOnlyPageWriteProtect (
> address of a memory region.
> @param[in] Length The length of memory region
> @param[in] Mode Set or Clear mode
> - @param[in] CacheFlush Flush the caches before applying the
> - encryption mask
> + @param[in] Mmio The physical address range is Mmio.
>
> @retval RETURN_SUCCESS The attributes were cleared for the
> memory region.
> @@ -572,7 +571,7 @@ SetMemoryEncDec (
> IN PHYSICAL_ADDRESS PhysicalAddress,
> IN UINTN Length,
> IN MAP_RANGE_MODE Mode,
> - IN BOOLEAN CacheFlush
> + IN BOOLEAN Mmio
> )
> {
> PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
> @@ -585,12 +584,23 @@ SetMemoryEncDec (
> UINT64 AddressEncMask;
> BOOLEAN IsWpEnabled;
> RETURN_STATUS Status;
> + BOOLEAN CacheFlush;
>
> //
> // Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings.
> //
> PageMapLevel4Entry = NULL;
>
> + //
> + // The cache need to flushed for the non-Mmio address range.
> + //
> + if (Mmio == TRUE) {
> + CacheFlush = FALSE;
> + } else {
> + CacheFlush = TRUE;
> + }
> +
> + //
> DEBUG ((
> DEBUG_VERBOSE,
> "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u\n",
(2) The calculation of "CacheFlush" from "Mmio" is awkward. First, we
don't compare BOOLEANs against TRUE or FALSE, BOOLEANs just stand alone
in controlling expression (or otherwise "logical") context. Second, why
not just write:
CacheFlush = !Mmio;
But even so...
(3) ... The introduction of the "Mmio" parameter is inexplicable to me.
It apparently replaces CacheFlush (with inverse meaning), but neither
the commit message, nor the (RFCv2 -> PATCH) changelog, explain why this
replacement makes sense.
The SetMemoryEncDec() function is an internal function (not a library
class API), so this change doesn't necessarily conflict with the commit
message -- but having this change in this particular patch (the last
patch in the series) seems unjustified.
In the previous version, we updated two SetMemoryEncDec() call sites: in
InternalMemEncryptSevSetMemoryDecrypted() and
InternalMemEncryptSevSetMemoryEncrypted(), we replaced the forwarding of
"Flush" with TRUE constants.
In this version, we update *three* SetMemoryEncDec() call sites:
- in InternalMemEncryptSevSetMemoryDecrypted() and
InternalMemEncryptSevSetMemoryEncrypted(), we replace the forwarding of
"Flush" with FALSE constants,
- and in InternalMemEncryptSevClearMmioPageEncMask(), we replace the
*constant* FALSE with TRUE.
I think this very last point -- regarding
InternalMemEncryptSevClearMmioPageEncMask() -- shows that the
replacement of CacheFlush with Mmio, at this point in the series, is
unwarranted.
Minimally, this replacement / negation should be a separate patch, but
even then, I think we'd need a defensible purpose (which is not clear to
me at this point); *plus*, the "re-calculation" of CacheFlush inside
SetMemoryEncDec() from Mmio feels like a cop-out. It's only being done
to save some additional replacements in the patch, but it leaves us with
a stricly worse -- harder to understand -- function. If you really need
this replacement / negation, then please do it in a separate patch, with
a good commit message; furthermore, please replace CacheFlush
*completely*, in SetMemoryEncDec() -- please don't reintroduce it.
Thanks,
Laszlo
> @@ -828,8 +838,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 +850,7 @@ EFIAPI
> InternalMemEncryptSevSetMemoryDecrypted (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS PhysicalAddress,
> - IN UINTN Length,
> - IN BOOLEAN Flush
> + IN UINTN Length
> )
> {
>
> @@ -852,7 +859,7 @@ InternalMemEncryptSevSetMemoryDecrypted (
> PhysicalAddress,
> Length,
> ClearCBit,
> - Flush
> + FALSE
> );
> }
>
> @@ -865,8 +872,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 +884,7 @@ EFIAPI
> InternalMemEncryptSevSetMemoryEncrypted (
> IN PHYSICAL_ADDRESS Cr3BaseAddress,
> IN PHYSICAL_ADDRESS PhysicalAddress,
> - IN UINTN Length,
> - IN BOOLEAN Flush
> + IN UINTN Length
> )
> {
> return SetMemoryEncDec (
> @@ -888,7 +892,7 @@ InternalMemEncryptSevSetMemoryEncrypted (
> PhysicalAddress,
> Length,
> SetCBit,
> - Flush
> + FALSE
> );
> }
>
> @@ -921,6 +925,6 @@ InternalMemEncryptSevClearMmioPageEncMask (
> PhysicalAddress,
> Length,
> ClearCBit,
> - FALSE
> + TRUE
> );
> }
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> index bca5e3febb1b..24d19d3ca161 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] 35+ messages in thread* Re: [edk2-devel] [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
2021-05-11 11:55 ` [edk2-devel] " Laszlo Ersek
@ 2021-05-11 17:45 ` Brijesh Singh
2021-05-12 16:35 ` Brijesh Singh
2021-05-13 11:24 ` Laszlo Ersek
0 siblings, 2 replies; 35+ messages in thread
From: Brijesh Singh @ 2021-05-11 17:45 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: brijesh.singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Erdem Aktas
On 5/11/21 6:55 AM, Laszlo Ersek wrote:
> I don't fully understand the updates in this patch:
>
> On 05/07/21 22:38, Brijesh Singh wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=M3xHM2yU0m3VtPn1xGe1k5Wq0d6Vbdf9gMqDX1NxpgA%3D&reserved=0
>>
>> 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().
> This looks good; it matches my request (1) from:
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00109.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3TQrliMABDaa%2FtCN%2FvKewTmLfVRKIou2La5yRGGyzJY%3D&reserved=0
>
>> 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 ++--
>> .../Ia32/MemEncryptSevLib.c | 10 ++----
>> .../X64/MemEncryptSevLib.c | 16 +++-------
>> .../X64/PeiDxeVirtualMemory.c | 32 +++++++++++--------
>> .../X64/SecVirtualMemory.c | 8 ++---
>> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +-
>> OvmfPkg/PlatformPei/AmdSev.c | 3 +-
>> 10 files changed, 35 insertions(+), 66 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..41e4b291d070 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",
> (1) You missed my comment (2) in
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00109.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3TQrliMABDaa%2FtCN%2FvKewTmLfVRKIou2La5yRGGyzJY%3D&reserved=0>.
Ah, I will fix in rev2.
>
>> 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..ad1021bd3e43 100644
>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
>> @@ -555,8 +555,7 @@ EnableReadOnlyPageWriteProtect (
>> address of a memory region.
>> @param[in] Length The length of memory region
>> @param[in] Mode Set or Clear mode
>> - @param[in] CacheFlush Flush the caches before applying the
>> - encryption mask
>> + @param[in] Mmio The physical address range is Mmio.
>>
>> @retval RETURN_SUCCESS The attributes were cleared for the
>> memory region.
>> @@ -572,7 +571,7 @@ SetMemoryEncDec (
>> IN PHYSICAL_ADDRESS PhysicalAddress,
>> IN UINTN Length,
>> IN MAP_RANGE_MODE Mode,
>> - IN BOOLEAN CacheFlush
>> + IN BOOLEAN Mmio
>> )
>> {
>> PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
>> @@ -585,12 +584,23 @@ SetMemoryEncDec (
>> UINT64 AddressEncMask;
>> BOOLEAN IsWpEnabled;
>> RETURN_STATUS Status;
>> + BOOLEAN CacheFlush;
>>
>> //
>> // Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings.
>> //
>> PageMapLevel4Entry = NULL;
>>
>> + //
>> + // The cache need to flushed for the non-Mmio address range.
>> + //
>> + if (Mmio == TRUE) {
>> + CacheFlush = FALSE;
>> + } else {
>> + CacheFlush = TRUE;
>> + }
>> +
>> + //
>> DEBUG ((
>> DEBUG_VERBOSE,
>> "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u\n",
> (2) The calculation of "CacheFlush" from "Mmio" is awkward. First, we
> don't compare BOOLEANs against TRUE or FALSE, BOOLEANs just stand alone
> in controlling expression (or otherwise "logical") context. Second, why
> not just write:
>
> CacheFlush = !Mmio;
>
> But even so...
>
> (3) ... The introduction of the "Mmio" parameter is inexplicable to me.
> It apparently replaces CacheFlush (with inverse meaning), but neither
> the commit message, nor the (RFCv2 -> PATCH) changelog, explain why this
> replacement makes sense.
The internal function is used for clearing the mask for both system RAM
as well as Mmio, so we need a way to tell the internal function that
call is for the Mmio range. I thought making all the changes in a single
file makes sense but I see it can get harder for the review. I guess I
could split the work in two patches
1) Drop the cache flush param from high level
MemEncryptSev{Set,Clear}PageEncMask and don't touch anything in the
SetMemoryEncDec()
2) Rename the Flush parameter to Mmio in the SetMemoryEncDec()
Does it makes sense to you ?
>
> The SetMemoryEncDec() function is an internal function (not a library
> class API), so this change doesn't necessarily conflict with the commit
> message -- but having this change in this particular patch (the last
> patch in the series) seems unjustified.
>
> In the previous version, we updated two SetMemoryEncDec() call sites: in
> InternalMemEncryptSevSetMemoryDecrypted() and
> InternalMemEncryptSevSetMemoryEncrypted(), we replaced the forwarding of
> "Flush" with TRUE constants.
>
> In this version, we update *three* SetMemoryEncDec() call sites:
>
> - in InternalMemEncryptSevSetMemoryDecrypted() and
> InternalMemEncryptSevSetMemoryEncrypted(), we replace the forwarding of
> "Flush" with FALSE constants,
>
> - and in InternalMemEncryptSevClearMmioPageEncMask(), we replace the
> *constant* FALSE with TRUE.
>
> I think this very last point -- regarding
> InternalMemEncryptSevClearMmioPageEncMask() -- shows that the
> replacement of CacheFlush with Mmio, at this point in the series, is
> unwarranted.
>
> Minimally, this replacement / negation should be a separate patch, but
> even then, I think we'd need a defensible purpose (which is not clear to
> me at this point); *plus*, the "re-calculation" of CacheFlush inside
> SetMemoryEncDec() from Mmio feels like a cop-out. It's only being done
> to save some additional replacements in the patch, but it leaves us with
> a stricly worse -- harder to understand -- function. If you really need
> this replacement / negation, then please do it in a separate patch, with
> a good commit message; furthermore, please replace CacheFlush
> *completely*, in SetMemoryEncDec() -- please don't reintroduce it.
>
> Thanks,
> Laszlo
>
>
>> @@ -828,8 +838,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 +850,7 @@ EFIAPI
>> InternalMemEncryptSevSetMemoryDecrypted (
>> IN PHYSICAL_ADDRESS Cr3BaseAddress,
>> IN PHYSICAL_ADDRESS PhysicalAddress,
>> - IN UINTN Length,
>> - IN BOOLEAN Flush
>> + IN UINTN Length
>> )
>> {
>>
>> @@ -852,7 +859,7 @@ InternalMemEncryptSevSetMemoryDecrypted (
>> PhysicalAddress,
>> Length,
>> ClearCBit,
>> - Flush
>> + FALSE
>> );
>> }
>>
>> @@ -865,8 +872,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 +884,7 @@ EFIAPI
>> InternalMemEncryptSevSetMemoryEncrypted (
>> IN PHYSICAL_ADDRESS Cr3BaseAddress,
>> IN PHYSICAL_ADDRESS PhysicalAddress,
>> - IN UINTN Length,
>> - IN BOOLEAN Flush
>> + IN UINTN Length
>> )
>> {
>> return SetMemoryEncDec (
>> @@ -888,7 +892,7 @@ InternalMemEncryptSevSetMemoryEncrypted (
>> PhysicalAddress,
>> Length,
>> SetCBit,
>> - Flush
>> + FALSE
>> );
>> }
>>
>> @@ -921,6 +925,6 @@ InternalMemEncryptSevClearMmioPageEncMask (
>> PhysicalAddress,
>> Length,
>> ClearCBit,
>> - FALSE
>> + TRUE
>> );
>> }
>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
>> index bca5e3febb1b..24d19d3ca161 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] 35+ messages in thread* Re: [edk2-devel] [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
2021-05-11 17:45 ` Brijesh Singh
@ 2021-05-12 16:35 ` Brijesh Singh
2021-05-13 11:25 ` Laszlo Ersek
2021-05-13 11:24 ` Laszlo Ersek
1 sibling, 1 reply; 35+ messages in thread
From: Brijesh Singh @ 2021-05-12 16:35 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: brijesh.singh, James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky,
Jordan Justen, Ard Biesheuvel, Erdem Aktas
On 5/11/21 12:45 PM, Brijesh Singh wrote:
> On 5/11/21 6:55 AM, Laszlo Ersek wrote:
>> I don't fully understand the updates in this patch:
>>
>> On 05/07/21 22:38, Brijesh Singh wrote:
>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=M3xHM2yU0m3VtPn1xGe1k5Wq0d6Vbdf9gMqDX1NxpgA%3D&reserved=0
>>>
>>> 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().
>> This looks good; it matches my request (1) from:
>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00109.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3TQrliMABDaa%2FtCN%2FvKewTmLfVRKIou2La5yRGGyzJY%3D&reserved=0
>>
>>> 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 ++--
>>> .../Ia32/MemEncryptSevLib.c | 10 ++----
>>> .../X64/MemEncryptSevLib.c | 16 +++-------
>>> .../X64/PeiDxeVirtualMemory.c | 32 +++++++++++--------
>>> .../X64/SecVirtualMemory.c | 8 ++---
>>> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +-
>>> OvmfPkg/PlatformPei/AmdSev.c | 3 +-
>>> 10 files changed, 35 insertions(+), 66 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..41e4b291d070 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",
>> (1) You missed my comment (2) in
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00109.html&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc383d8fdc1264644760508d91473b003%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563309382960811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3TQrliMABDaa%2FtCN%2FvKewTmLfVRKIou2La5yRGGyzJY%3D&reserved=0>.
> Ah, I will fix in rev2.
>
>
>>> 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..ad1021bd3e43 100644
>>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
>>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
>>> @@ -555,8 +555,7 @@ EnableReadOnlyPageWriteProtect (
>>> address of a memory region.
>>> @param[in] Length The length of memory region
>>> @param[in] Mode Set or Clear mode
>>> - @param[in] CacheFlush Flush the caches before applying the
>>> - encryption mask
>>> + @param[in] Mmio The physical address range is Mmio.
>>>
>>> @retval RETURN_SUCCESS The attributes were cleared for the
>>> memory region.
>>> @@ -572,7 +571,7 @@ SetMemoryEncDec (
>>> IN PHYSICAL_ADDRESS PhysicalAddress,
>>> IN UINTN Length,
>>> IN MAP_RANGE_MODE Mode,
>>> - IN BOOLEAN CacheFlush
>>> + IN BOOLEAN Mmio
>>> )
>>> {
>>> PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
>>> @@ -585,12 +584,23 @@ SetMemoryEncDec (
>>> UINT64 AddressEncMask;
>>> BOOLEAN IsWpEnabled;
>>> RETURN_STATUS Status;
>>> + BOOLEAN CacheFlush;
>>>
>>> //
>>> // Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings.
>>> //
>>> PageMapLevel4Entry = NULL;
>>>
>>> + //
>>> + // The cache need to flushed for the non-Mmio address range.
>>> + //
>>> + if (Mmio == TRUE) {
>>> + CacheFlush = FALSE;
>>> + } else {
>>> + CacheFlush = TRUE;
>>> + }
>>> +
>>> + //
>>> DEBUG ((
>>> DEBUG_VERBOSE,
>>> "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u\n",
>> (2) The calculation of "CacheFlush" from "Mmio" is awkward. First, we
>> don't compare BOOLEANs against TRUE or FALSE, BOOLEANs just stand alone
>> in controlling expression (or otherwise "logical") context. Second, why
>> not just write:
>>
>> CacheFlush = !Mmio;
>>
>> But even so...
>>
>> (3) ... The introduction of the "Mmio" parameter is inexplicable to me.
>> It apparently replaces CacheFlush (with inverse meaning), but neither
>> the commit message, nor the (RFCv2 -> PATCH) changelog, explain why this
>> replacement makes sense.
> The internal function is used for clearing the mask for both system RAM
> as well as Mmio, so we need a way to tell the internal function that
> call is for the Mmio range. I thought making all the changes in a single
> file makes sense but I see it can get harder for the review. I guess I
> could split the work in two patches
>
> 1) Drop the cache flush param from high level
> MemEncryptSev{Set,Clear}PageEncMask and don't touch anything in the
> SetMemoryEncDec()
>
> 2) Rename the Flush parameter to Mmio in the SetMemoryEncDec()
>
> Does it makes sense to you ?
I am going to drop the Mmio hunk from this patch to make it sane. I was
thinking ahead for the SEV-SNP support; For SEV and ES support, the
SetMemoryEncDec() does not need to know whether its called for Mmio or
RAM address. In SNP, the SetMemoryEncDec() will need to know this
information so that it can skip the page state change.
-Brijesh
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [edk2-devel] [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
2021-05-12 16:35 ` Brijesh Singh
@ 2021-05-13 11:25 ` Laszlo Ersek
0 siblings, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-13 11:25 UTC (permalink / raw)
To: Brijesh Singh, devel
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas
On 05/12/21 18:35, Brijesh Singh wrote:
>
> On 5/11/21 12:45 PM, Brijesh Singh wrote:
>> 1) Drop the cache flush param from high level
>> MemEncryptSev{Set,Clear}PageEncMask and don't touch anything in the
>> SetMemoryEncDec()
>>
>> 2) Rename the Flush parameter to Mmio in the SetMemoryEncDec()
>>
>> Does it makes sense to you ?
>
> I am going to drop the Mmio hunk from this patch to make it sane. I was
> thinking ahead for the SEV-SNP support; For SEV and ES support, the
> SetMemoryEncDec() does not need to know whether its called for Mmio or
> RAM address. In SNP, the SetMemoryEncDec() will need to know this
> information so that it can skip the page state change.
Yes, that's fine -- basically implement (2) as proposed above, just
delay it to a later series.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [edk2-devel] [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter
2021-05-11 17:45 ` Brijesh Singh
2021-05-12 16:35 ` Brijesh Singh
@ 2021-05-13 11:24 ` Laszlo Ersek
1 sibling, 0 replies; 35+ messages in thread
From: Laszlo Ersek @ 2021-05-13 11:24 UTC (permalink / raw)
To: Brijesh Singh, devel
Cc: James Bottomley, Min Xu, Jiewen Yao, Tom Lendacky, Jordan Justen,
Ard Biesheuvel, Erdem Aktas
On 05/11/21 19:45, Brijesh Singh wrote:
> The internal function is used for clearing the mask for both system RAM
> as well as Mmio, so we need a way to tell the internal function that
> call is for the Mmio range. I thought making all the changes in a single
> file makes sense but I see it can get harder for the review. I guess I
> could split the work in two patches
>
> 1) Drop the cache flush param from high level
> MemEncryptSev{Set,Clear}PageEncMask and don't touch anything in the
> SetMemoryEncDec()
>
> 2) Rename the Flush parameter to Mmio in the SetMemoryEncDec()
>
> Does it makes sense to you ?
Yes, sounds good. In (2), you can explain the motivation.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 35+ messages in thread
* 回复: [edk2-devel] [PATCH 00/13] Add GHCBv2 macro and helpers
2021-05-07 20:38 [PATCH 00/13] Add GHCBv2 macro and helpers Brijesh Singh
` (12 preceding siblings ...)
2021-05-07 20:38 ` [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter Brijesh Singh
@ 2021-05-08 1:43 ` gaoliming
13 siblings, 0 replies; 35+ messages in thread
From: gaoliming @ 2021-05-08 1:43 UTC (permalink / raw)
To: devel, brijesh.singh
Cc: 'James Bottomley', 'Min Xu', 'Jiewen Yao',
'Tom Lendacky', 'Jordan Justen',
'Ard Biesheuvel', 'Laszlo Ersek',
'Erdem Aktas', 'Michael D Kinney',
'Zhiguang Liu'
Brijesh:
The changes in MdePkg is good to me. Reviewed-by: Liming Gao
<gaoliming@byosoft.com.cn>
One minor comment is in Patch2. Its title should be MdePkg/Register/Amd:
xxxx to align other patches.
Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Brijesh Singh
> 发送时间: 2021年5月8日 4:38
> 收件人: devel@edk2.groups.io
> 抄送: Brijesh Singh <brijesh.singh@amd.com>; 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>; Laszlo Ersek <lersek@redhat.com>; 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>
> 主题: [edk2-devel] [PATCH 00/13] Add GHCBv2 macro and helpers
>
> 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:
> f297b7f20010 UnitTestFrameworkPkg: Sample unit test hangs when running
> in OVMF/QEMU
>
> 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>
>
> Brijesh Singh (11):
> MdePkg/Register/Amd: expand the SEV MSR to include the SNP definition
> MdePkg/Amd: add white spaces to retain alignment 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 | 80 ++++++++++++
> MdePkg/Include/Register/Amd/Fam17Msr.h | 46 ++++++-
> MdePkg/Include/Register/Amd/Ghcb.h | 123
> +++++++++++++++++-
> 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 | 63 +++++++--
> .../X64/SecVirtualMemory.c | 8 +-
> .../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, 526 insertions(+), 92 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] 35+ messages in thread