KVM: SVM: Add initial GHCB protocol version 2 support

From: Joerg Roedel <[email protected]>

Replace the get function with macros and the set function with hypercall specific setters. This will avoid preserving any previous bits in the GHCB-MSR and improved code readability.

Suggested-by Sean Christopherson <[email protected]>

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

---
 arch/x86/include/asm/sev-common.h |  9 +++++++
 arch/x86/kvm/svm/sev.c            | 41 +++++++++++--------------------
 2 files changed, 24 insertions(+), 26 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 +1 -5

index 6710d9ee2e4b..d7b3557b8dbb 100644
@@ -2342,16 +2342,15 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 	return true;
 }
 
-static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask,
-			      unsigned int pos)
+static void set_ghcb_msr_cpuid_resp(struct vcpu_svm *svm, u64 reg, u64 value)
 {
-	svm->vmcb->control.ghcb_gpa &= ~(mask << pos);
-	svm->vmcb->control.ghcb_gpa |= (value & mask) << pos;
-}

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

 	case GHCB_MSR_TERM_REQ: {
 		u64 reason_set, reason_code;
 
-		reason_set = get_ghcb_msr_bits(svm,
-					       GHCB_MSR_TERM_REASON_SET_MASK,
-					       GHCB_MSR_TERM_REASON_SET_POS);

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

 	}
 	default:

Signature · 1 line

2.31.1

arch/x86/kvm/svm/sev.c static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) · line 2353

Quoted 8 lines

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

-static u64 get_ghcb_msr_bits(struct vcpu_svm *svm, u64 mask, unsigned int pos)
-{
-	return (svm->vmcb->control.ghcb_gpa >> pos) & mask;
+	msr  = GHCB_MSR_CPUID_RESP;
+	msr |= (reg & GHCB_MSR_CPUID_REG_MASK) << GHCB_MSR_CPUID_REG_POS;
+	msr |= (value & GHCB_MSR_CPUID_VALUE_MASK) << GHCB_MSR_CPUID_VALUE_POS;
+
+	svm->vmcb->control.ghcb_gpa = msr;

I would rather have the get/set pairs be roughly symmetric, i.e. both functions or both macros, and both work on svm->vmcb->control.ghcb_gpa or both be purely functional (that may not be the correct word).

I don't have a strong preference on function vs. macro. But for the second one, my preference would be to have the helper generate the value as opposed to taken and filling a pointer, e.g. to yield something like:

		cpuid_reg = GHCB_MSR_CPUID_REG(control->ghcb_gpa);

		if (cpuid_reg == 0)
			cpuid_value = vcpu->arch.regs[VCPU_REGS_RAX];
		else if (cpuid_reg == 1)
			cpuid_value = vcpu->arch.regs[VCPU_REGS_RBX];
		else if (cpuid_reg == 2)
			cpuid_value = vcpu->arch.regs[VCPU_REGS_RCX];
		else
			cpuid_value = vcpu->arch.regs[VCPU_REGS_RDX];

		control->ghcb_gpa = MAKE_GHCB_MSR_RESP(cpuid_reg, cpuid_value);

The advantage is that it's obvious from the code that control->ghcb_gpa is being read _and_ written.

Quoted 1 line · Sean Christopherson

		control->ghcb_gpa = MAKE_GHCB_MSR_RESP(cpuid_reg, cpuid_value);

	case GHCB_MSR_SEV_INFO_REQ:
		control->ghcb_gpa = GHCB_MSR_SEV_INFO(GHCB_VERSION_MAX,
						      GHCB_VERSION_MIN,
						      sev_enc_bit));
		break;

and drop set_ghcb_msr() altogether.

Side topic, what about renaming control->ghcb_gpa => control->ghcb_msr so that the code for the MSR protocol is a bit more self-documenting? The APM defines the field as "Guest physical address of GHCB", so it's not exactly prescribing a specific name.

Quoted 12 lines · Joerg Roedel

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

 	case GHCB_MSR_TERM_REQ: {
 		u64 reason_set, reason_code;
 
-		reason_set = get_ghcb_msr_bits(svm,
-					       GHCB_MSR_TERM_REASON_SET_MASK,
-					       GHCB_MSR_TERM_REASON_SET_POS);
-		reason_code = get_ghcb_msr_bits(svm,
-						GHCB_MSR_TERM_REASON_MASK,
-						GHCB_MSR_TERM_REASON_POS);
+		reason_set  = GHCB_MSR_TERM_REASON_SET(control->ghcb_gpa);
+		reason_code = GHCB_MSR_TERM_REASON(control->ghcb_gpa);
+

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

Quoted 4 lines

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

 		pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
 			reason_set, reason_code);
+
 		fallthrough;

Not related to this patch, but why use fallthrough and more importantly, why is this an -EINVAL return? Why wouldn't KVM forward the request to userspace instead of returning an opaque -EINVAL?

Quoted 13 lines · Sean Christopherson

Quoted 12 lines · Joerg Roedel

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

 	case GHCB_MSR_TERM_REQ: {
 		u64 reason_set, reason_code;
 
-		reason_set = get_ghcb_msr_bits(svm,
-					       GHCB_MSR_TERM_REASON_SET_MASK,
-					       GHCB_MSR_TERM_REASON_SET_POS);
-		reason_code = get_ghcb_msr_bits(svm,
-						GHCB_MSR_TERM_REASON_MASK,
-						GHCB_MSR_TERM_REASON_POS);
+		reason_set  = GHCB_MSR_TERM_REASON_SET(control->ghcb_gpa);
+		reason_code = GHCB_MSR_TERM_REASON(control->ghcb_gpa);
+

Quoted 8 lines

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

Quoted 4 lines

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

 		pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
 			reason_set, reason_code);
+
 		fallthrough;

Not related to this patch, but why use fallthrough and more importantly, why is this an -EINVAL return? Why wouldn't KVM forward the request to userspace instead of returning an opaque -EINVAL?

I guess it is to signal an error condition up the call-chain to get the guest terminated, like requested.

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

 #define TLB_CONTROL_DO_NOTHING 0
 #define TLB_CONTROL_FLUSH_ALL_ASID 1
 #define TLB_CONTROL_FLUSH_ASID 3

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          | 21 +++++++++++++++++++++
 arch/x86/kvm/svm/svm.h          |  1 +
 3 files changed, 23 insertions(+)

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

index efa969325ede..fbb6f8d27a80 100644

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

 #define SVM_VMGEXIT_UNSUPPORTED_EVENT		0x8000ffff
 
 /* Exit code reserved for hypervisor/software use */

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

index a32ef011025f..4565c360d87d 100644
@@ -2180,6 +2180,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_HV_FT:
 		break;
 	default:
 		goto vmgexit_err;

@@ -2361,6 +2362,16 @@ static void set_ghcb_msr_ap_rst_resp(struct vcpu_svm *svm, u64 value)
 	svm->vmcb->control.ghcb_gpa = GHCB_MSR_AP_RESET_HOLD_RESP | (value << GHCB_DATA_LOW);
 }
 
+static void set_ghcb_msr_hv_feat_resp(struct vcpu_svm *svm, u64 value)
+{
+	u64 msr;
+
+	msr  = GHCB_MSR_HV_FT_RESP;
+	msr |= (value << GHCB_DATA_LOW);
+
+	svm->vmcb->control.ghcb_gpa = msr;
+}
+
 static void set_ghcb_msr(struct vcpu_svm *svm, u64 value)
 {
 	svm->vmcb->control.ghcb_gpa = value;

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

 	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
 		vcpu_unimpl(vcpu,
 			    "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n",

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

index 7e2090752d8f..9cafeba3340e 100644
@@ -550,6 +550,7 @@ void svm_vcpu_unblocking(struct kvm_vcpu *vcpu);
 #define GHCB_VERSION_MAX	1ULL
 #define GHCB_VERSION_MIN	1ULL
 
+#define GHCB_HV_FT_SUPPORTED	0
 
 extern unsigned int max_sev_asid;
 

Signature · 1 line

2.31.1