KVM: SVM: Add initial GHCB protocol version 2 support

PATCH Superseded Joerg Roedel 3 participants 9 messages Sep 29, 2021 – Oct 20, 2021

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Joerg Roedel

author

2021-09-29 15:53 UTC #0 lore raw

v4 cover KVM: SVM: Add initial GHCB protocol version 2 support

From: Joerg Roedel <[email protected]>

Hi,

here is a small set of patches which I took from the pending SEV-SNP patch-sets to enable basic support for GHCB protocol version 2.

When SEV-SNP is not supported, only two new MSR protocol VMGEXIT calls need to be supported:

- MSR-based AP-reset-hold
- MSR-based HV-feature-request

These calls are implemented here and then the protocol is lifted to version 2.

This is submitted separately because the MSR-based AP-reset-hold call is required to support kexec/kdump in SEV-ES guests.

The previous version can be found here:

	https://lore.kernel.org/kvm/[email protected]/

Regards,

Joerg

Changes v3->v4:

- Rebased to kvm/queue
- Addressed Sean's review comments on v3

Brijesh Singh (2):
KVM: SVM: Add support for Hypervisor Feature support MSR protocol
KVM: SVM: Increase supported GHCB protocol version

Joerg Roedel (1):
KVM: SVM: Get rid of set_ghcb_msr() and *ghcb_msr_bits() functions

Sean Christopherson (1):
KVM: SVM: Add helper to generate GHCB MSR version info, and drop macro

Tom Lendacky (1):
KVM: SVM: Add support to handle AP reset MSR protocol

 arch/x86/include/asm/kvm_host.h   |  10 ++-
 arch/x86/include/asm/sev-common.h |  14 ++--
 arch/x86/include/uapi/asm/svm.h   |   1 +
 arch/x86/kvm/svm/sev.c            | 131 ++++++++++++++++++++----------
 arch/x86/kvm/svm/svm.h            |   5 --
 arch/x86/kvm/x86.c                |   5 +-
 6 files changed, 112 insertions(+), 54 deletions(-)

base-commit: 8960bc57dfb78f5de088af800576d9096282dca2

Signature · 1 line

2.33.0

Joerg Roedel

author

2021-09-29 15:53 UTC #1 lore raw

PATCH v4 1/5 KVM: SVM: Get rid of set_ghcb_msr() and *ghcb_msr_bits() functions

From: Joerg Roedel <[email protected]>

Replace the get_ghcb_msr_bits() function with macros and open code the GHCB MSR setters with hypercall specific helper macros and functions. This will avoid preserving any previous bits in the GHCB-MSR and improves code readability.

Also get rid of the set_ghcb_msr() function and open-code it at its call-sites for better code readability.

Signed-off-by Joerg Roedel <[email protected]>

Reviewed-by Sean Christopherson <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/include/asm/sev-common.h |  9 +++++
 arch/x86/kvm/svm/sev.c            | 56 +++++++++++--------------------
 2 files changed, 29 insertions(+), 36 deletions(-)

arch/x86/include/asm/sev-common.h +9 -0

index 2cef6c5a52c2..8540972cad04 100644
@@ -50,6 +50,10 @@
 		(GHCB_MSR_CPUID_REQ | \
 		(((unsigned long)reg & GHCB_MSR_CPUID_REG_MASK) << GHCB_MSR_CPUID_REG_POS) | \
 		(((unsigned long)fn) << GHCB_MSR_CPUID_FUNC_POS))
+#define GHCB_MSR_CPUID_FN(msr)		\
+	(((msr) >> GHCB_MSR_CPUID_FUNC_POS) & GHCB_MSR_CPUID_FUNC_MASK)
+#define GHCB_MSR_CPUID_REG(msr)		\
+	(((msr) >> GHCB_MSR_CPUID_REG_POS) & GHCB_MSR_CPUID_REG_MASK)
 
 /* AP Reset Hold */
 #define GHCB_MSR_AP_RESET_HOLD_REQ		0x006

@@ -67,6 +71,11 @@
 #define GHCB_SEV_TERM_REASON(reason_set, reason_val)						  \
 	(((((u64)reason_set) &  GHCB_MSR_TERM_REASON_SET_MASK) << GHCB_MSR_TERM_REASON_SET_POS) | \
 	((((u64)reason_val) & GHCB_MSR_TERM_REASON_MASK) << GHCB_MSR_TERM_REASON_POS))
+#define GHCB_MSR_TERM_REASON_SET(msr)	\
+	(((msr) >> GHCB_MSR_TERM_REASON_SET_POS) & GHCB_MSR_TERM_REASON_SET_MASK)
+#define GHCB_MSR_TERM_REASON(msr)	\
+	(((msr) >> GHCB_MSR_TERM_REASON_POS) & GHCB_MSR_TERM_REASON_MASK)
+
 
 #define GHCB_SEV_ES_REASON_GENERAL_REQUEST	0
 #define GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED	1

arch/x86/kvm/svm/sev.c +20 ~~-36~~

101 lines not on this page · raw message

Signature · 1 line

2.33.0

Sean Christopherson

maintainer

2021-10-13 22:08 UTC #2 lore raw

Re: [PATCH v4 1/5] KVM: SVM: Get rid of set_ghcb_msr() and *ghcb_msr_bits() functions

Quoted 7 lines · Joerg Roedel

From: Joerg Roedel <[email protected]>
Replace the get_ghcb_msr_bits() function with macros and open code the GHCB MSR setters with hypercall specific helper macros and functions. This will avoid preserving any previous bits in the GHCB-MSR and improves code readability.

Quoted 7 lines

Also get rid of the set_ghcb_msr() function and open-code it at its call-sites for better code readability.
Suggested-by Sean Christopherson <[email protected]>
Signed-off-by Joerg Roedel <[email protected]>
Reviewed-by Sean Christopherson <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>

I suspect my SoB got picked up accidentally. If the intent was to attribute some code snippets via Co-developed-by, feel free to just drop my SoB.

Joerg Roedel

author

2021-09-29 15:53 UTC #3 lore raw

PATCH v4 2/5 KVM: SVM: Add helper to generate GHCB MSR version info, and drop macro

In reply to Joerg Roedel

From: Sean Christopherson <[email protected]>

Convert the GHCB_MSR_SEV_INFO macro into a helper function, and have the helper hardcode the min/max versions instead of relying on the caller to do the same. Under no circumstance should different pieces of KVM define different min/max versions.

No functional change intended.

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/include/asm/sev-common.h |  5 -----
 arch/x86/kvm/svm/sev.c            | 24 ++++++++++++++++++------
 arch/x86/kvm/svm/svm.h            |  5 -----
 3 files changed, 18 insertions(+), 16 deletions(-)

arch/x86/include/asm/sev-common.h +0 -5

index 8540972cad04..886c36f0cb16 100644
@@ -24,11 +24,6 @@
 #define GHCB_MSR_VER_MIN_MASK		0xffff
 #define GHCB_MSR_CBIT_POS		24
 #define GHCB_MSR_CBIT_MASK		0xff
-#define GHCB_MSR_SEV_INFO(_max, _min, _cbit)				\
-	((((_max) & GHCB_MSR_VER_MAX_MASK) << GHCB_MSR_VER_MAX_POS) |	\
-	 (((_min) & GHCB_MSR_VER_MIN_MASK) << GHCB_MSR_VER_MIN_POS) |	\
-	 (((_cbit) & GHCB_MSR_CBIT_MASK) << GHCB_MSR_CBIT_POS) |	\
-	 GHCB_MSR_SEV_INFO_RESP)
 #define GHCB_MSR_INFO(v)		((v) & 0xfffUL)
 #define GHCB_MSR_PROTO_MAX(v)		(((v) >> GHCB_MSR_VER_MAX_POS) & GHCB_MSR_VER_MAX_MASK)
 #define GHCB_MSR_PROTO_MIN(v)		(((v) >> GHCB_MSR_VER_MIN_POS) & GHCB_MSR_VER_MIN_MASK)

arch/x86/kvm/svm/sev.c +18 -6

index 7aa6ad4c3118..159b22bb74e4 100644
@@ -2389,6 +2389,22 @@ static u64 ghcb_msr_cpuid_resp(u64 reg, u64 value)
 	return msr;
 }
 
+/* The min/max GHCB version supported by KVM. */
+#define GHCB_VERSION_MAX	1ULL
+#define GHCB_VERSION_MIN	1ULL
+
+static u64 ghcb_msr_version_info(void)
+{
+	u64 msr;
+
+	msr  = GHCB_MSR_SEV_INFO_RESP;
+	msr |= GHCB_VERSION_MAX << GHCB_MSR_VER_MAX_POS;
+	msr |= GHCB_VERSION_MIN << GHCB_MSR_VER_MIN_POS;
+	msr |= sev_enc_bit << GHCB_MSR_CBIT_POS;
+
+	return msr;
+}
+
 static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 {
 	struct vmcb_control_area *control = &svm->vmcb->control;

@@ -2403,9 +2419,7 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 
 	switch (ghcb_info) {
 	case GHCB_MSR_SEV_INFO_REQ:
-		svm->vmcb->control.ghcb_gpa = GHCB_MSR_SEV_INFO(GHCB_VERSION_MAX,
-								GHCB_VERSION_MIN,
-								sev_enc_bit);
+		control->ghcb_gpa = ghcb_msr_version_info();
 		break;
 	case GHCB_MSR_CPUID_REQ: {
 		u64 cpuid_fn, cpuid_reg, cpuid_value;

@@ -2621,9 +2635,7 @@ void sev_es_vcpu_reset(struct vcpu_svm *svm)
 	 * Set the GHCB MSR value as per the GHCB specification when emulating
 	 * vCPU RESET for an SEV-ES guest.
 	 */
-	svm->vmcb->control.ghcb_gpa = GHCB_MSR_SEV_INFO(GHCB_VERSION_MAX,
-							GHCB_VERSION_MIN,
-							sev_enc_bit);
+	svm->vmcb->control.ghcb_gpa = ghcb_msr_version_info();
 }
 
 void sev_es_prepare_guest_switch(struct vcpu_svm *svm, unsigned int cpu)

arch/x86/kvm/svm/svm.h +0 -5

index 0d7bbe548ac3..68e5f16a0554 100644
@@ -544,11 +544,6 @@ void svm_vcpu_blocking(struct kvm_vcpu *vcpu);
 void svm_vcpu_unblocking(struct kvm_vcpu *vcpu);
 
 /* sev.c */
-
-#define GHCB_VERSION_MAX	1ULL
-#define GHCB_VERSION_MIN	1ULL
-
-
 extern unsigned int max_sev_asid;
 
 void sev_vm_destroy(struct kvm *kvm);

Signature · 1 line

2.33.0

Joerg Roedel

author

2021-09-29 15:53 UTC #4 lore raw

PATCH v4 3/5 KVM: SVM: Add support to handle AP reset MSR protocol

In reply to Joerg Roedel

From: Tom Lendacky <[email protected]>

Add support for AP Reset Hold being invoked using the GHCB MSR protocol, available in version 2 of the GHCB specification.

Signed-off-by Tom Lendacky <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Joerg Roedel <[email protected]>

---
 arch/x86/include/asm/kvm_host.h | 10 ++++++-
 arch/x86/kvm/svm/sev.c          | 48 ++++++++++++++++++++++++++-------
 arch/x86/kvm/x86.c              |  5 +++-
 3 files changed, 51 insertions(+), 12 deletions(-)

arch/x86/include/asm/kvm_host.h +9 -1

index 88f0326c184a..501a55cecd73 100644
@@ -237,6 +237,11 @@ enum x86_intercept_stage;
 	KVM_GUESTDBG_INJECT_DB | \
 	KVM_GUESTDBG_BLOCKIRQ)
 
+enum ap_reset_hold_type {
+	AP_RESET_HOLD_NONE,
+	AP_RESET_HOLD_NAE_EVENT,
+	AP_RESET_HOLD_MSR_PROTO,
+};
 
 #define PFERR_PRESENT_BIT 0
 #define PFERR_WRITE_BIT 1

@@ -908,6 +913,8 @@ struct kvm_vcpu_arch {
 #if IS_ENABLED(CONFIG_HYPERV)
 	hpa_t hv_root_tdp;
 #endif
+
+	enum ap_reset_hold_type reset_hold_type;
 };
 
 struct kvm_lpage_info {

@@ -1690,7 +1697,8 @@ int kvm_fast_pio(struct kvm_vcpu *vcpu, int size, unsigned short port, int in);
 int kvm_emulate_cpuid(struct kvm_vcpu *vcpu);
 int kvm_emulate_halt(struct kvm_vcpu *vcpu);
 int kvm_vcpu_halt(struct kvm_vcpu *vcpu);
-int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu);
+int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu,
+			      enum ap_reset_hold_type type);
 int kvm_emulate_wbinvd(struct kvm_vcpu *vcpu);
 
 void kvm_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);

arch/x86/kvm/svm/sev.c +38 ~~-10~~

76 lines not on this page · raw message

arch/x86/kvm/x86.c +4 -1

index 46ee9bf61df4..1756511b873e 100644
@@ -8672,10 +8672,13 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_emulate_halt);
 
-int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu)
+int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu,
+			      enum ap_reset_hold_type type)
 {
 	int ret = kvm_skip_emulated_instruction(vcpu);
 
+	vcpu->arch.reset_hold_type = type;
+
 	return __kvm_vcpu_halt(vcpu, KVM_MP_STATE_AP_RESET_HOLD, KVM_EXIT_AP_RESET_HOLD) && ret;
 }
 EXPORT_SYMBOL_GPL(kvm_emulate_ap_reset_hold);

Signature · 1 line

2.33.0

Sean Christopherson

maintainer

2021-10-13 22:04 UTC #5 lore raw

Re: [PATCH v4 3/5] KVM: SVM: Add support to handle AP reset MSR protocol

arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 917

Quoted 8 lines · Joerg Roedel

#define PFERR_PRESENT_BIT 0
#define PFERR_WRITE_BIT 1
arch/x86/include/asm/kvm_host.h +2 -0
@@ -908,6 +913,8 @@ struct kvm_vcpu_arch {
 #if IS_ENABLED(CONFIG_HYPERV)
 	hpa_t hv_root_tdp;
 #endif
+
+	enum ap_reset_hold_type reset_hold_type;

Apologies for very belated feedback...

This living in kvm_vcpu_arch came about from feedback (see bottom) that _if_ kvm_emulate_ap_reset_hold() is in x86.c, so should the hold type information.

But clearing the hold in SEV here...

arch/x86/kvm/svm/sev.c static int sev_es_validate_vmgexit(struct vcpu_svm *svm) · line 2253

Quoted 7 lines

arch/x86/kvm/svm/sev.c +3 -0

 void sev_es_unmap_ghcb(struct vcpu_svm *svm)
 {
+	/* Clear any indication that the vCPU is in a type of AP Reset Hold */
+	svm->vcpu.arch.reset_hold_type = AP_RESET_HOLD_NONE;
+
 	if (!svm->ghcb)
 		return;

makes this completely unbalanced, i.e. common x86 doesn't clear the reset_hold_type when the vCPU is awakened, despite it being set in common x86. More at the end.

Quoted 11 lines

arch/x86/kvm/x86.c +4 -1

-int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu)
+int kvm_emulate_ap_reset_hold(struct kvm_vcpu *vcpu,
+			      enum ap_reset_hold_type type)
 {
 	int ret = kvm_skip_emulated_instruction(vcpu);
 
+	vcpu->arch.reset_hold_type = type;
+
 	return __kvm_vcpu_halt(vcpu, KVM_MP_STATE_AP_RESET_HOLD, KVM_EXIT_AP_RESET_HOLD) && ret;
 }
 EXPORT_SYMBOL_GPL(kvm_emulate_ap_reset_hold);

...

Quoted 15 lines · Tom Lendacky

Quoted 14 lines · Sean Christopherson
Quoted 12 lines · Tom Lendacky
Quoted 5 lines · Sean Christopherson
Review
Quoted 3 lines
(quoted diff) +3 -0
+        case GHCB_MSR_AP_RESET_HOLD_REQ:
+                svm->ap_reset_hold_type = AP_RESET_HOLD_MSR_PROTO;
+                ret = kvm_emulate_ap_reset_hold(&svm->vcpu);
The hold type feels like it should be a param to kvm_emulate_ap_reset_hold().
I suppose it could be, but then the type would have to be tracked in the kvm_vcpu_arch struct instead of the vcpu_svm struct, so I opted for the latter. Maybe a helper function, sev_ap_reset_hold(), that sets the type and then calls kvm_emulate_ap_reset_hold(), but I'm not seeing a big need for it.

Quoted 6 lines

Quoted 3 lines
Huh. Why is kvm_emulate_ap_reset_hold() in x86.c? That entire concept is very much SEV specific. And if anyone argues its not SEV specific, then the hold type should also be considered generic, i.e. put in kvm_vcpu_arch.
That was based on review comments where it was desired that the halt be identified as specifically from the AP reset hold vs a normal halt.

The reason I emphasized "if", is that IMO this patch goes in the wrong direction. My feedback here was that kvm_emulate_ap_reset_hold() and reset_hold_type should tied together. I completely agree with the review comments Tom mentioned, but IMO adding a common kvm_emulate_ap_reset_hold() was the wrong solution. That's very much an SEV specific concept, as demonstrated by this patch.

Rather than put more stuff into x86 that really belongs to SEV, what about moving kvm_emulate_ap_reset_hold() into sev.c and instead exporting __kvm_vcpu_halt()?

Note, there's a conflict there with a proposed function rename[*], but it's minor and should be trivial to resolve depending how which series wins the race.

[*] https://lkml.kernel.org/r/[email protected]

Joerg Roedel

2021-10-20 12:32 UTC #6 lore raw

Re: [PATCH v4 3/5] KVM: SVM: Add support to handle AP reset MSR protocol

Quoted 2 lines · Sean Christopherson

Rather than put more stuff into x86 that really belongs to SEV, what about moving kvm_emulate_ap_reset_hold() into sev.c and instead exporting __kvm_vcpu_halt()?

Did that in a separate patch and fixed things up. Also replaced the kvm_ with and sev_ prefix and the function now takes an vcpu_svm parameter.

New version coming soon.

Thanks,

Joerg

Joerg Roedel

author

2021-09-29 15:53 UTC #7 lore raw

PATCH v4 4/5 KVM: SVM: Add support for Hypervisor Feature support MSR protocol

In reply to Joerg Roedel

From: Brijesh Singh <[email protected]>

Version 2 of the GHCB specification introduced advertisement of supported Hypervisor SEV features. This request is required to support a the GHCB version 2 protocol.

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Joerg Roedel <[email protected]>

---
 arch/x86/include/uapi/asm/svm.h |  1 +
 arch/x86/kvm/svm/sev.c          | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

arch/x86/include/uapi/asm/svm.h +1 -0

index efa969325ede..67cf153fe580 100644
@@ -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
 #define SVM_VMGEXIT_UNSUPPORTED_EVENT		0x8000ffff
 
 /* Exit code reserved for hypervisor/software use */

arch/x86/kvm/svm/sev.c +23 -0

index 69653d7838e3..22523ec08a7b 100644
@@ -2216,6 +2216,7 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 	case SVM_VMGEXIT_AP_HLT_LOOP:
 	case SVM_VMGEXIT_AP_JUMP_TABLE:
 	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
+	case SVM_VMGEXIT_HYPERVISOR_FEATURES:
 		break;
 	default:
 		goto vmgexit_err;

@@ -2413,6 +2414,20 @@ static u64 ghcb_msr_ap_rst_resp(u64 value)
 	return (u64)GHCB_MSR_AP_RESET_HOLD_RESP | (value << GHCB_DATA_LOW);
 }
 
+/* Hypervisor GHCB Features supported by KVM */
+#define KVM_SUPPORTED_GHCB_HV_FEATURES		0UL
+
+static u64 ghcb_msr_hv_feat_resp(void)
+{
+	u64 msr;
+
+	msr  = GHCB_MSR_HV_FT_RESP;
+	msr |= (KVM_SUPPORTED_GHCB_HV_FEATURES << GHCB_DATA_LOW);
+
+	return msr;
+}
+
+
 static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 {
 	struct vmcb_control_area *control = &svm->vmcb->control;

@@ -2468,6 +2483,9 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 		 */
 		svm->vmcb->control.ghcb_gpa = ghcb_msr_ap_rst_resp(0);
 
+		break;
+	case GHCB_MSR_HV_FT_REQ:
+		svm->vmcb->control.ghcb_gpa = ghcb_msr_hv_feat_resp();
 		break;
 	case GHCB_MSR_TERM_REQ: {
 		u64 reason_set, reason_code;

@@ -2581,6 +2599,11 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 		ret = 1;
 		break;
 	}
+	case SVM_VMGEXIT_HYPERVISOR_FEATURES:
+		ghcb_set_sw_exit_info_2(ghcb, KVM_SUPPORTED_GHCB_HV_FEATURES);
+
+		ret = 1;
+		break;
 	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
 		vcpu_unimpl(vcpu,
 			    "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n",

Signature · 1 line

2.33.0

Joerg Roedel

author

2021-09-29 15:53 UTC #8 lore raw

PATCH v4 5/5 KVM: SVM: Increase supported GHCB protocol version

In reply to Joerg Roedel

From: Brijesh Singh <[email protected]>

Now that KVM has basic support for version 2 of the GHCB specification, bump the maximum supported protocol version. The SNP specific functions are still missing, but those are only required when the Hypervisor supports running SNP guests.

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Joerg Roedel <[email protected]>

---
 arch/x86/kvm/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/kvm/svm/sev.c +1 -1

index 22523ec08a7b..d2e03dc72558 100644
@@ -2394,7 +2394,7 @@ static u64 ghcb_msr_cpuid_resp(u64 reg, u64 value)
 }
 
 /* The min/max GHCB version supported by KVM. */
-#define GHCB_VERSION_MAX	1ULL
+#define GHCB_VERSION_MAX	2ULL
 #define GHCB_VERSION_MIN	1ULL
 
 static u64 ghcb_msr_version_info(void)

Signature · 1 line

2.33.0