Re: [PATCH v2.1 3/4] KVM: SVM: Add support for Hypervisor Feature support MSR protocol

From: Sean Christopherson
Date: Wed Sep 01 2021 - 18:42:09 EST


On Thu, Jul 22, 2021, Joerg Roedel wrote:
> From: Brijesh Singh <brijesh.singh@xxxxxxx>
>
> 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 <brijesh.singh@xxxxxxx>
> Signed-off-by: Joerg Roedel <jroedel@xxxxxxx>
> ---
> 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(+)
>
> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
> index efa969325ede..fbb6f8d27a80 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_HV_FT 0x8000fffd

For this KVM-only (for all intents and purposes) name, please use the verbose
SVM_VMGEXIT_HYPERVISOR_FEATURES.

https://lkml.kernel.org/r/b73ad44e-7719-cde7-d543-df34e5acf9a5@xxxxxxx

> #define SVM_VMGEXIT_UNSUPPORTED_EVENT 0x8000ffff
>
> /* Exit code reserved for hypervisor/software use */
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index a32ef011025f..4565c360d87d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -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;
> @@ -2425,6 +2436,10 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>
> break;
> }
> + case GHCB_MSR_HV_FT_REQ: {
> + set_ghcb_msr_hv_feat_resp(svm, GHCB_HV_FT_SUPPORTED);

I definitely think there are too many small wrappers that bury the write to
svm->vmcb->control.ghcb_gpa. E.g. with a rename, this

control->ghcb_msr = GHCB_MSR_HV_FT_RESP |
(GHCB_HV_FT_SUPPORTED << GHCB_DATA_LOW);

or maybe add a generic helper for simple data responses? E.g. GHCB_MSR_AP_RESET_HOLD_REQ
can share a macro.

control->ghcb_msr = GHCB_MSR_RESP_WITH_DATA(GHCB_MSR_HV_FT_RESP,
GHCB_HV_FT_SUPPORTED);

> + break;
> + }

Unnecessary braces.

> case GHCB_MSR_TERM_REQ: {
> u64 reason_set, reason_code;
>
> @@ -2537,6 +2552,12 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
> ret = 1;
> break;
> }
> + case SVM_VMGEXIT_HV_FT: {
> + ghcb_set_sw_exit_info_2(ghcb, GHCB_HV_FT_SUPPORTED);
> +
> + ret = 1;
> + break;
> + }

Unnecessary braces.

> case SVM_VMGEXIT_UNSUPPORTED_EVENT:
> vcpu_unimpl(vcpu,
> "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n",
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 7e2090752d8f..9cafeba3340e 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -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;
>
> --
> 2.31.1
>