Thread (62 messages) 62 messages, 2 authors, 2021-05-27

Re: [PATCH Part1 RFC v2 03/20] x86/sev: Add support for hypervisor feature VMGEXIT

From: Borislav Petkov <bp@alien8.de>
Date: 2021-05-11 10:01:53
Also in: lkml

On Fri, Apr 30, 2021 at 07:15:59AM -0500, Brijesh Singh wrote:
quoted hunk ↗ jump to hunk
Version 2 of GHCB specification introduced advertisement of a features
that are supported by the hypervisor. Define the GHCB MSR protocol and NAE
for the hypervisor feature request and query the feature during the GHCB
protocol negotitation. See the GHCB specification for more details.

Signed-off-by: Brijesh Singh <redacted>
---
 arch/x86/include/asm/sev-common.h | 17 +++++++++++++++++
 arch/x86/include/uapi/asm/svm.h   |  2 ++
 arch/x86/kernel/sev-shared.c      | 24 ++++++++++++++++++++++++
 3 files changed, 43 insertions(+)
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index 9f1b66090a4c..8142e247d8da 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -51,6 +51,22 @@
 #define GHCB_MSR_AP_RESET_HOLD_RESULT_POS	12
 #define GHCB_MSR_AP_RESET_HOLD_RESULT_MASK	0xfffffffffffff
 
+/* GHCB Hypervisor Feature Request */
+#define GHCB_MSR_HV_FEATURES_REQ	0x080
+#define GHCB_MSR_HV_FEATURES_RESP	0x081
+#define GHCB_MSR_HV_FEATURES_POS	12
+#define GHCB_MSR_HV_FEATURES_MASK	0xfffffffffffffUL
+#define GHCB_MSR_HV_FEATURES_RESP_VAL(v)	\
+	(((v) >> GHCB_MSR_HV_FEATURES_POS) & GHCB_MSR_HV_FEATURES_MASK)
+
+#define GHCB_HV_FEATURES_SNP		BIT_ULL(0)
+#define GHCB_HV_FEATURES_SNP_AP_CREATION			\
+		(BIT_ULL(1) | GHCB_HV_FEATURES_SNP)
+#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION		\
+		(BIT_ULL(2) | GHCB_HV_FEATURES_SNP_AP_CREATION)
+#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER		\
+		(BIT_ULL(3) | GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION)
Please add those in the patches which use them - not in bulk here.

And GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER is a mouthfull and
looks like BIOS code to me. But this is still the kernel, remember? :-)

So let's do

GHCB_MSR_HV_FT_*

GHCB_SNP_AP_CREATION
GHCB_SNP_RESTRICTED_INJ
GHCB_SNP_RESTRICTED_INJ_TMR

and so on so that we can all keep our sanity when reading that code.
quoted hunk ↗ jump to hunk
+
 #define GHCB_MSR_TERM_REQ		0x100
 #define GHCB_MSR_TERM_REASON_SET_POS	12
 #define GHCB_MSR_TERM_REASON_SET_MASK	0xf
@@ -62,6 +78,7 @@
 
 #define GHCB_SEV_ES_REASON_GENERAL_REQUEST	0
 #define GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED	1
+#define GHCB_SEV_ES_REASON_SNP_UNSUPPORTED	2
I remember asking for those to get shortened too

GHCB_SEV_ES_GEN_REQ
GHCB_SEV_ES_PROT_UNSUPPORTED
GHCB_SEV_ES_SNP_UNSUPPORTED

Perhaps in a prepatch?
quoted hunk ↗ jump to hunk
 #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
 
diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
index 554f75fe013c..7fbc311e2de1 100644
--- a/arch/x86/include/uapi/asm/svm.h
+++ b/arch/x86/include/uapi/asm/svm.h
@@ -108,6 +108,7 @@
 #define SVM_VMGEXIT_AP_JUMP_TABLE		0x80000005
 #define SVM_VMGEXIT_SET_AP_JUMP_TABLE		0
 #define SVM_VMGEXIT_GET_AP_JUMP_TABLE		1
+#define SVM_VMGEXIT_HYPERVISOR_FEATURES		0x8000fffd
SVM_VMGEXIT_HV_FT

you get the idea.
quoted hunk ↗ jump to hunk
 #define SVM_VMGEXIT_UNSUPPORTED_EVENT		0x8000ffff
 
 #define SVM_EXIT_ERR           -1
@@ -215,6 +216,7 @@
 	{ SVM_VMGEXIT_NMI_COMPLETE,	"vmgexit_nmi_complete" }, \
 	{ SVM_VMGEXIT_AP_HLT_LOOP,	"vmgexit_ap_hlt_loop" }, \
 	{ SVM_VMGEXIT_AP_JUMP_TABLE,	"vmgexit_ap_jump_table" }, \
+	{ SVM_VMGEXIT_HYPERVISOR_FEATURES,	"vmgexit_hypervisor_feature" }, \
 	{ SVM_EXIT_ERR,         "invalid_guest_state" }
 
 
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 48a47540b85f..874f911837db 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -20,6 +20,7 @@
  * out when the .bss section is later cleared.
  */
 static u16 ghcb_version __section(".data") = 0;
+static u64 hv_features __section(".data") = 0;
ERROR: do not initialise statics to 0
#181: FILE: arch/x86/kernel/sev-shared.c:23:
+static u64 hv_features __section(".data") = 0;
quoted hunk ↗ jump to hunk
 static bool __init sev_es_check_cpu_features(void)
 {
@@ -49,6 +50,26 @@ static void __noreturn sev_es_terminate(unsigned int reason)
 		asm volatile("hlt\n" : : : "memory");
 }
 
+static bool ghcb_get_hv_features(void)
Used only once here - no need for the ghcb_ prefix.
+{
+	u64 val;
+
+	/* The hypervisor features are available from version 2 onward. */
+	if (ghcb_version < 2)
+		return true;
		return false;
no?

Also, this should be done differently:

	if (ghcb_version >= 2)
		get_hv_features();

at the call site.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help