Re: [PATCH v1 3/5] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST NAE event

[Carlos Bilbao <carlos.bilbao.osdev@gmail.com>]

Hello folks,

On 6/21/24 08:40, Michael Roth wrote:
> Version 2 of GHCB specification added support for the SNP Extended Guest
> Request Message NAE event. This event serves a nearly identical purpose
> to the previously-added SNP_GUEST_REQUEST event, but for certain message
> types it allows the guest to supply a buffer to be used for additional
> information in some cases.
>
> Currently the GHCB spec only defines extended handling of this sort in
> the case of attestation requests, where the additional buffer is used to
> supply a table of certificate data corresponding to the attestion
> report's signing key. Support for this extended handling will require
> additional KVM APIs to handle coordinating with userspace.
>
> Whether or not the hypervisor opts to provide this certificate data is
> optional. However, support for processing SNP_EXTENDED_GUEST_REQUEST
> GHCB requests is required by the GHCB 2.0 specification for SNP guests,
> so for now implement a stub implementation that provides an empty
> certificate table to the guest if it supplies an additional buffer, but
> otherwise behaves identically to SNP_GUEST_REQUEST.
>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  arch/x86/kvm/svm/sev.c | 60 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7338b987cadd..b5dcf36b50f5 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3323,6 +3323,7 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
>  			goto vmgexit_err;
>  		break;
>  	case SVM_VMGEXIT_GUEST_REQUEST:
> +	case SVM_VMGEXIT_EXT_GUEST_REQUEST:
>  		if (!sev_snp_guest(vcpu->kvm))
>  			goto vmgexit_err;
>  		break;
> @@ -4005,6 +4006,62 @@ static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_
>  	return ret;
>  }
>  
> +/*
> + * As per GHCB spec (see "SNP Extended Guest Request"), the certificate table
> + * is terminated by 24-bytes of zeroes.
> + */
> +static const u8 empty_certs_table[24];


Should this be:
staticconstu8 empty_certs_table[24] = { 0};
Besides that,
Reviewed-by: Carlos Bilbao <carlos.bilbao.osdev@gmail.com>


> +
> +static int snp_handle_ext_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
> +{
> +	struct kvm *kvm = svm->vcpu.kvm;
> +	u8 msg_type;
> +
> +	if (!sev_snp_guest(kvm) || !PAGE_ALIGNED(req_gpa) || !PAGE_ALIGNED(resp_gpa))
> +		return -EINVAL;
> +
> +	if (kvm_read_guest(kvm, req_gpa + offsetof(struct snp_guest_msg_hdr, msg_type),
> +			   &msg_type, 1))
> +		goto abort_request;
> +
> +	/*
> +	 * As per GHCB spec, requests of type MSG_REPORT_REQ also allow for
> +	 * additional certificate data to be provided alongside the attestation
> +	 * report via the guest-provided data pages indicated by RAX/RBX. The
> +	 * certificate data is optional and requires additional KVM enablement
> +	 * to provide an interface for userspace to provide it, but KVM still
> +	 * needs to be able to handle extended guest requests either way. So
> +	 * provide a stub implementation that will always return an empty
> +	 * certificate table in the guest-provided data pages.
> +	 */
> +	if (msg_type == SNP_MSG_REPORT_REQ) {
> +		struct kvm_vcpu *vcpu = &svm->vcpu;
> +		u64 data_npages;
> +		gpa_t data_gpa;
> +
> +		if (!kvm_ghcb_rax_is_valid(svm) || !kvm_ghcb_rbx_is_valid(svm))
> +			goto abort_request;
> +
> +		data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
> +		data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
> +
> +		if (!PAGE_ALIGNED(data_gpa))
> +			goto abort_request;
> +
> +		if (data_npages &&
> +		    kvm_write_guest(kvm, data_gpa, empty_certs_table,
> +				    sizeof(empty_certs_table)))
> +			goto abort_request;
> +	}
> +
> +	return snp_handle_guest_req(svm, req_gpa, resp_gpa);
> +
> +abort_request:
> +	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb,
> +				SNP_GUEST_ERR(SNP_GUEST_VMM_ERR_GENERIC, 0));
> +	return 1; /* resume guest */
> +}
> +
>  static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>  {
>  	struct vmcb_control_area *control = &svm->vmcb->control;
> @@ -4282,6 +4339,9 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>  	case SVM_VMGEXIT_GUEST_REQUEST:
>  		ret = snp_handle_guest_req(svm, control->exit_info_1, control->exit_info_2);
>  		break;
> +	case SVM_VMGEXIT_EXT_GUEST_REQUEST:
> +		ret = snp_handle_ext_guest_req(svm, control->exit_info_1, control->exit_info_2);
> +		break;
>  	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>  		vcpu_unimpl(vcpu,
>  			    "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n",


Thanks,
Carlos