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

[Sean Christopherson]

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
>