RE: [PATCH Part2 v6 42/49] KVM: SVM: Provide support for SNP_GUEST_REQUEST NAE event

[Kalra, Ashish]

[Public]


>> +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t 
>> +req_gpa, gpa_t resp_gpa) {
>> +       struct sev_data_snp_guest_request req = {0};
>> +       struct kvm_vcpu *vcpu = &svm->vcpu;
>> +       struct kvm *kvm = vcpu->kvm;
>> +       unsigned long data_npages;
>> +       struct kvm_sev_info *sev;
>> +       unsigned long rc, err;
>> +       u64 data_gpa;
>> +
>> +       if (!sev_snp_guest(vcpu->kvm)) {
>> +               rc = SEV_RET_INVALID_GUEST;
>> +               goto e_fail;
>> +       }
>> +
>> +       sev = &to_kvm_svm(kvm)->sev_info;
>> +
>> +       data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
>> +       data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
>> +
>> +       if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) {
>> +               rc = SEV_RET_INVALID_ADDRESS;
>> +               goto e_fail;
>> +       }
>> +
>> +       /* Verify that requested blob will fit in certificate buffer */
>> +       if ((data_npages << PAGE_SHIFT) > SEV_FW_BLOB_MAX_SIZE) {
>> +               rc = SEV_RET_INVALID_PARAM;
>> +               goto e_fail;
>> +       }
>> +
>> +       mutex_lock(&sev->guest_req_lock);
>> +
>> +       rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa);
>> +       if (rc)
>> +               goto unlock;
>> +
>> +       rc = snp_guest_ext_guest_request(&req, (unsigned long)sev->snp_certs_data,
>> +                                        &data_npages, &err);
>> +       if (rc) {
>> +               /*
>> +                * If buffer length is small then return the expected
>> +                * length in rbx.
>> +                */
>> +               if (err == SNP_GUEST_REQ_INVALID_LEN)
>> +                       vcpu->arch.regs[VCPU_REGS_RBX] = data_npages;
>> +
>> +               /* pass the firmware error code */
>> +               rc = err;
>> +               goto cleanup;
>> +       }
>> +
>> +       /* Copy the certificate blob in the guest memory */
>> +       if (data_npages &&
>> +           kvm_write_guest(kvm, data_gpa, sev->snp_certs_data, data_npages << PAGE_SHIFT))
>> +               rc = SEV_RET_INVALID_ADDRESS;

>>Since at this point the PSP FW has correctly executed the command and incremented the VMPCK sequence number I think we need another error signal here since this will tell the guest the PSP had an error so it will not know if the VMPCK sequence >number should be incremented.

>Similarly as above, as this is an error path, so what's the guarantee that the next guest message request will succeed completely,  isn’t it better to let the 
>FW reject any subsequent guest messages once it has detected that the sequence numbers are out of sync ?

Alternately, we probably can return SEV_RET_INVALID_PAGE_STATE/SEV_RET_INVALID_PAGE_OWNER here, but that still does not indicate to the guest
that the FW has successfully executed the command and the error occurred during cleanup/result phase and it needs to increment the VMPCK sequence number. There is nothing as such defined in SNP FW API specs to indicate such kind of failures to guest. As I mentioned earlier, this is probably indicative of
a bigger system failure and it is better to let the FW reject subsequent guest messages/requests once it has detected that the sequence numbers are out of sync.

Thanks,
Ashish