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@xxxxxxx>
>> ---
>> 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@xxxxxxxxx>
>
>
>> +
>> +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
>
