[PATCH 0/2] KVM: SVM: Make VMGEXIT GHCB exit codes more readable

[PATCH 0/2] KVM: SVM: Make VMGEXIT GHCB exit codes more readable

[Melody Wang]

Hi all,

Here are two patches to make VMGEXIT GHCB exit codes more readable. All
feedback is appreciated.

Thanks,
Melody

Melody Wang (2):
  KVM: SVM: Convert plain error code numbers to defines
  KVM: SVM: Provide helpers to set the error code

 arch/x86/include/asm/sev-common.h |  8 +++++++
 arch/x86/kvm/svm/sev.c            | 39 +++++++++++++++++--------------
 arch/x86/kvm/svm/svm.c            |  6 +----
 arch/x86/kvm/svm/svm.h            | 24 +++++++++++++++++++
 4 files changed, 54 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH 1/2] KVM: SVM: Convert plain error code numbers to defines

[Melody Wang]

Convert VMGEXIT SW_EXITINFO1 codes from plain numbers to proper defines.

No functionality changed.

Signed-off-by: Melody Wang <huibo.wang@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Reviewed-by: Pavan Kumar Paluri <papaluri@amd.com>
---
 arch/x86/include/asm/sev-common.h |  8 ++++++++
 arch/x86/kvm/svm/sev.c            | 12 ++++++------
 arch/x86/kvm/svm/svm.c            |  2 +-
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index 98726c2b04f8..01d4744e880a 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -209,6 +209,14 @@ struct snp_psc_desc {
 
 #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
 
+/*
+ * Error codes of the GHCB SW_EXITINFO1 related to GHCB input that can be
+ * communicated back to the guest
+ */
+#define GHCB_HV_RESP_SUCCESS		0
+#define GHCB_HV_RESP_ISSUE_EXCEPTION	1
+#define GHCB_HV_RESP_MALFORMED_INPUT	2
+
 /*
  * Error codes related to GHCB input that can be communicated back to the guest
  * by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 72674b8825c4..e7db7a5703b7 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3433,7 +3433,7 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 		dump_ghcb(svm);
 	}
 
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, 2);
+	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
 	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, reason);
 
 	/* Resume the guest to "return" the error code. */
@@ -3577,7 +3577,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 	return 0;
 
 e_scratch:
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, 2);
+	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
 	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_SCRATCH_AREA);
 
 	return 1;
@@ -4124,7 +4124,7 @@ static int snp_handle_ext_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t r
 	return snp_handle_guest_req(svm, req_gpa, resp_gpa);
 
 request_invalid:
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, 2);
+	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
 	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_INPUT);
 	return 1; /* resume guest */
 }
@@ -4317,7 +4317,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 	if (ret)
 		return ret;
 
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, 0);
+	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_SUCCESS);
 	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, 0);
 
 	exit_code = kvm_ghcb_get_sw_exit_code(control);
@@ -4367,7 +4367,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 		default:
 			pr_err("svm: vmgexit: unsupported AP jump table request - exit_info_1=%#llx\n",
 			       control->exit_info_1);
-			ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, 2);
+			ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
 			ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_INPUT);
 		}
 
@@ -4397,7 +4397,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 	case SVM_VMGEXIT_AP_CREATION:
 		ret = sev_snp_ap_creation(svm);
 		if (ret) {
-			ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, 2);
+			ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
 			ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_INPUT);
 		}
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index dd15cc635655..58bce5f1ab0c 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2977,7 +2977,7 @@ static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
 	if (!err || !sev_es_guest(vcpu->kvm) || WARN_ON_ONCE(!svm->sev_es.ghcb))
 		return kvm_complete_insn_gp(vcpu, err);
 
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, 1);
+	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_ISSUE_EXCEPTION);
 	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb,
 				X86_TRAP_GP |
 				SVM_EVTINJ_TYPE_EXEPT |
-- 
2.34.1

Re: [PATCH 1/2] KVM: SVM: Convert plain error code numbers to defines

[Sean Christopherson]

On Fri, Dec 06, 2024, Melody Wang wrote:
> Convert VMGEXIT SW_EXITINFO1 codes from plain numbers to proper defines.
> 
> No functionality changed.
> 
> Signed-off-by: Melody Wang <huibo.wang@amd.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> Reviewed-by: Pavan Kumar Paluri <papaluri@amd.com>
> ---
>  arch/x86/include/asm/sev-common.h |  8 ++++++++
>  arch/x86/kvm/svm/sev.c            | 12 ++++++------
>  arch/x86/kvm/svm/svm.c            |  2 +-
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index 98726c2b04f8..01d4744e880a 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -209,6 +209,14 @@ struct snp_psc_desc {
>  
>  #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
>  
> +/*
> + * Error codes of the GHCB SW_EXITINFO1 related to GHCB input that can be
> + * communicated back to the guest
> + */
> +#define GHCB_HV_RESP_SUCCESS		0

Somewhat of a nit, but I don't think "SUCCESS" is appropriate due to the bizarre
return codes for Page State Change (PSC) requests.  For unknown reasons (really,
why!?!?), PSC requests apparently always get back '0', but then put a bunch of
errors into SW_EXITINFO2, including cases that are clearly not "success".

FWIW, "no action" isn't much better, because it too directly conflicts with
the documentation for PSC:

  The page state change request was interrupted, retry the request.
                                                 ^^^^^^^^^^^^^^^^^
I'm all for having svm_vmgexit_success(), but I think the macro needs to be
NO_ACTION (even though it too is flawed), because I strongly suspect that patch 2
deliberately avoided SUCCESS in snp_handle_guest_req() and snp_complete_psc()
specifically because you knew SUCCESS would be misleading.

> +#define GHCB_HV_RESP_ISSUE_EXCEPTION	1
> +#define GHCB_HV_RESP_MALFORMED_INPUT	2

Where is '2' actually documented?  I looked all over the GHCB and the only ones
I can find are '0' and '1'.

  0x0000
    o No action requested by the hypervisor.
  0x0001
    o The hypervisor has requested an exception be issued

And again, somewhat of a nit, but PSC ruins all the fun once more, because it
quite clearly has multiple "malformed input" responses.  So if PSC can get rejected
with "bad input", why on earth would it not use GHCB_HV_RESP_MALFORMED_INPUT?

  o SW_EXITINFO2[31:0] == 0x00000001
    The page_state_change_header structure is not valid

  o SW_EXITINFO2[31:0] == 0x00000002
    The page_state_change_entry structure, identified by
    page_state_change_header.cur_entry, is not valid.

Re: [PATCH 1/2] KVM: SVM: Convert plain error code numbers to defines

[Paluri, PavanKumar]

Hi Sean,

On 12/16/2024 6:41 PM, Sean Christopherson wrote:
> On Fri, Dec 06, 2024, Melody Wang wrote:
>> Convert VMGEXIT SW_EXITINFO1 codes from plain numbers to proper defines.
>>
>> No functionality changed.
>>
>> Signed-off-by: Melody Wang <huibo.wang@amd.com>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>> Reviewed-by: Pavan Kumar Paluri <papaluri@amd.com>
>> ---
>>  arch/x86/include/asm/sev-common.h |  8 ++++++++
>>  arch/x86/kvm/svm/sev.c            | 12 ++++++------
>>  arch/x86/kvm/svm/svm.c            |  2 +-
>>  3 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
>> index 98726c2b04f8..01d4744e880a 100644
>> --- a/arch/x86/include/asm/sev-common.h
>> +++ b/arch/x86/include/asm/sev-common.h
>> @@ -209,6 +209,14 @@ struct snp_psc_desc {
>>  
>>  #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
>>  
>> +/*
>> + * Error codes of the GHCB SW_EXITINFO1 related to GHCB input that can be
>> + * communicated back to the guest
>> + */
>> +#define GHCB_HV_RESP_SUCCESS		0
> 
> Somewhat of a nit, but I don't think "SUCCESS" is appropriate due to the bizarre
> return codes for Page State Change (PSC) requests.  For unknown reasons (really,
> why!?!?), PSC requests apparently always get back '0', but then put a bunch of
> errors into SW_EXITINFO2, including cases that are clearly not "success".
> 
> FWIW, "no action" isn't much better, because it too directly conflicts with
> the documentation for PSC:
> 
>   The page state change request was interrupted, retry the request.
>                                                  ^^^^^^^^^^^^^^^^^
> I'm all for having svm_vmgexit_success(), but I think the macro needs to be
> NO_ACTION (even though it too is flawed), because I strongly suspect that patch 2
> deliberately avoided SUCCESS in snp_handle_guest_req() and snp_complete_psc()
> specifically because you knew SUCCESS would be misleading.
> 
>> +#define GHCB_HV_RESP_ISSUE_EXCEPTION	1
>> +#define GHCB_HV_RESP_MALFORMED_INPUT	2
> 
> Where is '2' actually documented?  I looked all over the GHCB and the only ones
> I can find are '0' and '1'.
> 
>   0x0000
>     o No action requested by the hypervisor.
>   0x0001
>     o The hypervisor has requested an exception be issued
> 

GHCB spec (Pub# 56421, Rev:2.03), section 4.1 Invoking VMGEXIT documents
0x0002 as well.

0x0002
 o The hypervisor encountered malformed input for the VMGEXIT.

Thanks,
Pavan

> And again, somewhat of a nit, but PSC ruins all the fun once more, because it
> quite clearly has multiple "malformed input" responses.  So if PSC can get rejected
> with "bad input", why on earth would it not use GHCB_HV_RESP_MALFORMED_INPUT?
> 
>   o SW_EXITINFO2[31:0] == 0x00000001
>     The page_state_change_header structure is not valid
> 
>   o SW_EXITINFO2[31:0] == 0x00000002
>     The page_state_change_entry structure, identified by
>     page_state_change_header.cur_entry, is not valid.

Re: [PATCH 1/2] KVM: SVM: Convert plain error code numbers to defines

[Sean Christopherson]

On Tue, Dec 17, 2024, PavanKumar Paluri wrote:
> >> +#define GHCB_HV_RESP_ISSUE_EXCEPTION	1
> >> +#define GHCB_HV_RESP_MALFORMED_INPUT	2
> > 
> > Where is '2' actually documented?  I looked all over the GHCB and the only ones
> > I can find are '0' and '1'.
> > 
> >   0x0000
> >     o No action requested by the hypervisor.
> >   0x0001
> >     o The hypervisor has requested an exception be issued
> > 
> 
> GHCB spec (Pub# 56421, Rev:2.03), section 4.1 Invoking VMGEXIT documents

Ah, I only had Rev 2.00.  Found it in 2.03.

Thanks!

> 0x0002 as well.
> 
> 0x0002
>  o The hypervisor encountered malformed input for the VMGEXIT.

[PATCH 2/2] KVM: SVM: Provide helpers to set the error code

[Melody Wang]

Provide helpers to set the error code when converting VMGEXIT SW_EXITINFO1 and
SW_EXITINFO2 codes from plain numbers to proper defines. Add comments for
better code readability.

No functionality changed.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Melody Wang <huibo.wang@amd.com>
---
 arch/x86/kvm/svm/sev.c | 39 +++++++++++++++++++++------------------
 arch/x86/kvm/svm/svm.c |  6 +-----
 arch/x86/kvm/svm/svm.h | 24 ++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index e7db7a5703b7..409f77a6151e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3433,8 +3433,7 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 		dump_ghcb(svm);
 	}
 
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
-	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, reason);
+	svm_vmgexit_bad_input(svm, reason);
 
 	/* Resume the guest to "return" the error code. */
 	return 1;
@@ -3577,8 +3576,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 	return 0;
 
 e_scratch:
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
-	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_SCRATCH_AREA);
+	svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_SCRATCH_AREA);
 
 	return 1;
 }
@@ -3671,7 +3669,11 @@ static void snp_complete_psc(struct vcpu_svm *svm, u64 psc_ret)
 	svm->sev_es.psc_inflight = 0;
 	svm->sev_es.psc_idx = 0;
 	svm->sev_es.psc_2m = false;
-	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
+	/*
+	 * psc_ret contains 0 if all entries have been processed successfully
+	 * or a reason code identifying why the request has not completed.
+	 */
+	svm_vmgexit_set_return_code(svm, 0, psc_ret);
 }
 
 static void __snp_complete_one_psc(struct vcpu_svm *svm)
@@ -4067,8 +4069,12 @@ static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_
 		ret = -EIO;
 		goto out_unlock;
 	}
-
-	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, SNP_GUEST_ERR(0, fw_err));
+	/*
+	 * SNP_GUEST_ERR(0, fw_err): Upper 32-bits (63:32) will contain the
+	 * return code from the hypervisor. Lower 32-bits (31:0) will contain
+	 * the return code from the firmware call (0 = success)
+	 */
+	svm_vmgexit_set_return_code(svm, 0, SNP_GUEST_ERR(0, fw_err));
 
 	ret = 1; /* resume guest */
 
@@ -4124,8 +4130,7 @@ static int snp_handle_ext_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t r
 	return snp_handle_guest_req(svm, req_gpa, resp_gpa);
 
 request_invalid:
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
-	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_INPUT);
+	svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_INPUT);
 	return 1; /* resume guest */
 }
 
@@ -4317,8 +4322,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 	if (ret)
 		return ret;
 
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_SUCCESS);
-	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, 0);
+	svm_vmgexit_success(svm, 0);
 
 	exit_code = kvm_ghcb_get_sw_exit_code(control);
 	switch (exit_code) {
@@ -4362,20 +4366,20 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 			break;
 		case 1:
 			/* Get AP jump table address */
-			ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, sev->ap_jump_table);
+			svm_vmgexit_set_return_code(svm, 0, sev->ap_jump_table);
 			break;
 		default:
 			pr_err("svm: vmgexit: unsupported AP jump table request - exit_info_1=%#llx\n",
 			       control->exit_info_1);
-			ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
-			ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_INPUT);
+			svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_INPUT);
 		}
 
 		ret = 1;
 		break;
 	}
 	case SVM_VMGEXIT_HV_FEATURES:
-		ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_HV_FT_SUPPORTED);
+		/* Get hypervisor supported features */
+		svm_vmgexit_success(svm, GHCB_HV_FT_SUPPORTED);
 
 		ret = 1;
 		break;
@@ -4397,8 +4401,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 	case SVM_VMGEXIT_AP_CREATION:
 		ret = sev_snp_ap_creation(svm);
 		if (ret) {
-			ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
-			ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_INPUT);
+			svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_INPUT);
 		}
 
 		ret = 1;
@@ -4635,7 +4638,7 @@ void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)
 		 * Return from an AP Reset Hold VMGEXIT, where the guest will
 		 * set the CS and RIP. Set SW_EXIT_INFO_2 to a non-zero value.
 		 */
-		ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, 1);
+		svm_vmgexit_success(svm, 1);
 		break;
 	case AP_RESET_HOLD_MSR_PROTO:
 		/*
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 58bce5f1ab0c..104e13d59c8a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2977,11 +2977,7 @@ static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
 	if (!err || !sev_es_guest(vcpu->kvm) || WARN_ON_ONCE(!svm->sev_es.ghcb))
 		return kvm_complete_insn_gp(vcpu, err);
 
-	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_ISSUE_EXCEPTION);
-	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb,
-				X86_TRAP_GP |
-				SVM_EVTINJ_TYPE_EXEPT |
-				SVM_EVTINJ_VALID);
+	svm_vmgexit_inject_exception(svm, X86_TRAP_GP);
 	return 1;
 }
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 43fa6a16eb19..baff8237c5c9 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -588,6 +588,30 @@ static inline bool is_vnmi_enabled(struct vcpu_svm *svm)
 		return false;
 }
 
+static inline void svm_vmgexit_set_return_code(struct vcpu_svm *svm,
+						u64 response, u64 data)
+{
+	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, response);
+	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, data);
+}
+
+static inline void svm_vmgexit_inject_exception(struct vcpu_svm *svm, u8 vector)
+{
+	u64 data = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_EXEPT | vector;
+
+	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_ISSUE_EXCEPTION, data);
+}
+
+static inline void svm_vmgexit_bad_input(struct vcpu_svm *svm, u64 suberror)
+{
+	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_MALFORMED_INPUT, suberror);
+}
+
+static inline void svm_vmgexit_success(struct vcpu_svm *svm, u64 data)
+{
+	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_SUCCESS, data);
+}
+
 /* svm.c */
 #define MSR_INVALID				0xffffffffU
 
-- 
2.34.1

Re: [PATCH 2/2] KVM: SVM: Provide helpers to set the error code

[Sean Christopherson]

On Fri, Dec 06, 2024, Melody Wang wrote:
> @@ -3577,8 +3576,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>  	return 0;
>  
>  e_scratch:
> -	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, GHCB_HV_RESP_MALFORMED_INPUT);
> -	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_ERR_INVALID_SCRATCH_AREA);
> +	svm_vmgexit_bad_input(svm, GHCB_ERR_INVALID_SCRATCH_AREA);
>  
>  	return 1;
>  }
> @@ -3671,7 +3669,11 @@ static void snp_complete_psc(struct vcpu_svm *svm, u64 psc_ret)
>  	svm->sev_es.psc_inflight = 0;
>  	svm->sev_es.psc_idx = 0;
>  	svm->sev_es.psc_2m = false;
> -	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
> +	/*
> +	 * psc_ret contains 0 if all entries have been processed successfully

No, it doesn't.

  • SW_EXITINFO2 == 0x00000000
    The page state change request was interrupted, retry the request.

> +	 * or a reason code identifying why the request has not completed.

Honestly, even if it were correct, this comment does far more harm than good.
The literal '0' below has nothing to do with the '0' referenced in the comment,
and so all the comment does is add more confusion.  Furthermore, this is the
wrong place to document SW_EXITINFO2 return codes.  That's the responsibility of
the call sites and/or individual PSC-specific macros.

This code should instead document the weirdness with PSC's SW_EXITINFO1:

	/*
	 * Because the GHCB says so, PSC requests always get a "no action"
	 * response, with a PSC-specific return code in SW_EXITINFO2.
	 *
	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_NO_ACTION, psc_ret);

The readers of _this_ code don't care about the individual return codes, they
care about knowing the semantics of the return itself.

Alternatively, it might even make sense to add a svm_vmgexit_no_action() helper
(along with svm_vmgexit_success()).  There are at least two VMGEXIT types that
roll their own error codes with NO_ACTION, and it might be better to have the
initial zeroing in sev_handle_vmgexit() use NO_ACTION, e.g.

	/*
	 * Assume no action is required as the vast majority of VMGEXITs don't
	 * require the guest to take action upon success, and initializing the
	 * GHCB for the happy case avoids the need to do so for exits that are
	 * handled via SVM's common exit handlers.
	 */
	svm_vmgexit_no_action(svm, 0);

> +	 */
> +	svm_vmgexit_set_return_code(svm, 0, psc_ret);
>  }
>  
>  static void __snp_complete_one_psc(struct vcpu_svm *svm)
> @@ -4067,8 +4069,12 @@ static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_
>  		ret = -EIO;
>  		goto out_unlock;
>  	}
> -
> -	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, SNP_GUEST_ERR(0, fw_err));
> +	/*
> +	 * SNP_GUEST_ERR(0, fw_err): Upper 32-bits (63:32) will contain the
> +	 * return code from the hypervisor. Lower 32-bits (31:0) will contain
> +	 * the return code from the firmware call (0 = success)

Same thing here.  A comment documenting SNP_GUEST_ERR() belongs above the
definition of SNP_GUEST_ERR.  How the "guest error code" is constructed isn't
relevant/unique to this code, what's relevant is that *KVM* isn't requesting an
action.

	/*
	 * No action is requested from KVM, even if the request failed due to a
	 * firmware error.
	 */
	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_NO_ACTION,
				    SNP_GUEST_ERR(0, fw_err));


> +	 */
> +	svm_vmgexit_set_return_code(svm, 0, SNP_GUEST_ERR(0, fw_err));
>  
>  	ret = 1; /* resume guest */
>  

...

> +static inline void svm_vmgexit_set_return_code(struct vcpu_svm *svm,
> +						u64 response, u64 data)
> +{
> +	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, response);
> +	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, data);
> +}
> +
> +static inline void svm_vmgexit_inject_exception(struct vcpu_svm *svm, u8 vector)
> +{
> +	u64 data = SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_EXEPT | vector;
> +
> +	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_ISSUE_EXCEPTION, data);
> +}
> +
> +static inline void svm_vmgexit_bad_input(struct vcpu_svm *svm, u64 suberror)
> +{
> +	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_MALFORMED_INPUT, suberror);
> +}
> +
> +static inline void svm_vmgexit_success(struct vcpu_svm *svm, u64 data)

As mentioned in patch 1, please keep svm_vmgexit_success() even if
GHCB_HV_RESP_SUCCESS is renamed to GHCB_HV_RESP_NO_ACTION.

> +{
> +	svm_vmgexit_set_return_code(svm, GHCB_HV_RESP_SUCCESS, data);
> +}
> +
>  /* svm.c */
>  #define MSR_INVALID				0xffffffffU
>  
> -- 
> 2.34.1
> 

Re: [PATCH 0/2] KVM: SVM: Make VMGEXIT GHCB exit codes more readable

[Sean Christopherson]

On Fri, Dec 06, 2024, Melody Wang wrote:
> Hi all,
> 
> Here are two patches to make VMGEXIT GHCB exit codes more readable. All
> feedback is appreciated.
> 
> Thanks,
> Melody
> 
> Melody Wang (2):
>   KVM: SVM: Convert plain error code numbers to defines

When adding patches to a series, please treat the "new" series as a continuation
of the previous series, and follow all of the normal "rules" for documenting the
delta between versions.  I.e. this should be v3, since patch 1 was posted as v2.

https://lore.kernel.org/all/20241202214032.350109-1-huibo.wang@amd.com

>   KVM: SVM: Provide helpers to set the error code
> 
>  arch/x86/include/asm/sev-common.h |  8 +++++++
>  arch/x86/kvm/svm/sev.c            | 39 +++++++++++++++++--------------
>  arch/x86/kvm/svm/svm.c            |  6 +----
>  arch/x86/kvm/svm/svm.h            | 24 +++++++++++++++++++
>  4 files changed, 54 insertions(+), 23 deletions(-)
> 
> -- 
> 2.34.1
>