Re: [PATCH v2] KVM: SVM: Convert plain error code numbers to defines

[Sean Christopherson <seanjc@google.com>]

On Mon, Dec 02, 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
> +#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);

Hmm, IMO, open coding literals in multiple locations is a symptom of not providing
helpers.  From a certain (slightly warped) perspective, setting exit_info_1 vs.
exit_info_2 is just weird version of an open coded literal.

Rather than provide macros (or probably in addition to), what if we provide helpers
to set the error code and the payload?  That would also ensure KVM sets both '1'
and '2' fields.  E.g. in sev_handle_vmgexit()'s SVM_VMGEXIT_AP_JUMP_TABLE path,
setting '2' but not '1' is mildly confusing at first glance, because it takes some
staring to understand that's it's NOT a failure path.  Ditto for
sev_vcpu_deliver_sipi_vector().

And IMO, not having to parse input parameters to understand success vs. failure
usually makes code easier to read.

E.g. something like this?  Definitely feel free to suggest better names.

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);
}