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

[Tom Lendacky <thomas.lendacky@amd.com>]

On 6/22/24 15:28, Carlos Bilbao wrote:
> Hello folks,

Hey Carlos,

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

Statics are always initialized to zero, so not necessary.

Thanks,
Tom

> 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
>