Messages in this thread | | | Date | Wed, 11 Jan 2023 11:48:56 +1100 | Subject | Re: [PATCH RFC v7 52/64] KVM: SVM: Provide support for SNP_GUEST_REQUEST NAE event | From | Alexey Kardashevskiy <> |
| |
On 10/1/23 19:33, Kalra, Ashish wrote: > > On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote: >> >> >> On 10/1/23 10:41, Kalra, Ashish wrote: >>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote: >>>> On 15/12/22 06:40, Michael Roth wrote: >>>>> From: Brijesh Singh <brijesh.singh@amd.com> >>>>> >>>>> Version 2 of GHCB specification added the support for two SNP Guest >>>>> Request Message NAE events. The events allows for an SEV-SNP guest to >>>>> make request to the SEV-SNP firmware through hypervisor using the >>>>> SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification. >>>>> >>>>> The SNP_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the >>>>> difference of an additional certificate blob that can be passed >>>>> through >>>>> the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver >>>>> provides snp_guest_ext_guest_request() that is used by the KVM to get >>>>> both the report and certificate data at once. >>>>> >>>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >>>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >>>>> Signed-off-by: Michael Roth <michael.roth@amd.com> >>>>> --- >>>>> arch/x86/kvm/svm/sev.c | 185 >>>>> +++++++++++++++++++++++++++++++++++++++-- >>>>> arch/x86/kvm/svm/svm.h | 2 + >>>>> 2 files changed, 181 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >>>>> index 5f2b2092cdae..18efa70553c2 100644 >>>>> --- a/arch/x86/kvm/svm/sev.c >>>>> +++ b/arch/x86/kvm/svm/sev.c >>>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, >>>>> struct kvm_sev_cmd *argp) >>>>> if (ret) >>>>> goto e_free; >>>>> + mutex_init(&sev->guest_req_lock); >>>>> ret = sev_snp_init(&argp->error, false); >>>>> } else { >>>>> ret = sev_platform_init(&argp->error); >>>>> @@ -2051,23 +2052,34 @@ int sev_vm_move_enc_context_from(struct kvm >>>>> *kvm, unsigned int source_fd) >>>>> */ >>>>> static void *snp_context_create(struct kvm *kvm, struct >>>>> kvm_sev_cmd *argp) >>>>> { >>>>> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; >>>>> struct sev_data_snp_addr data = {}; >>>>> - void *context; >>>>> + void *context, *certs_data; >>>>> int rc; >>>>> + /* Allocate memory used for the certs data in SNP guest >>>>> request */ >>>>> + certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT); >>>>> + if (!certs_data) >>>>> + return NULL; >>>>> + >>>>> /* Allocate memory for context page */ >>>>> context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT); >>>>> if (!context) >>>>> - return NULL; >>>>> + goto e_free; >>>>> data.gctx_paddr = __psp_pa(context); >>>>> rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, >>>>> &data, &argp->error); >>>>> - if (rc) { >>>>> - snp_free_firmware_page(context); >>>>> - return NULL; >>>>> - } >>>>> + if (rc) >>>>> + goto e_free; >>>>> + >>>>> + sev->snp_certs_data = certs_data; >>>>> return context; >>>>> + >>>>> +e_free: >>>>> + snp_free_firmware_page(context); >>>>> + kfree(certs_data); >>>>> + return NULL; >>>>> } >>>>> static int snp_bind_asid(struct kvm *kvm, int *error) >>>>> @@ -2653,6 +2665,8 @@ static int snp_decommission_context(struct >>>>> kvm *kvm) >>>>> snp_free_firmware_page(sev->snp_context); >>>>> sev->snp_context = NULL; >>>>> + kfree(sev->snp_certs_data); >>>>> + >>>>> return 0; >>>>> } >>>>> @@ -3174,6 +3188,8 @@ static int sev_es_validate_vmgexit(struct >>>>> vcpu_svm *svm, u64 *exit_code) >>>>> case SVM_VMGEXIT_UNSUPPORTED_EVENT: >>>>> case SVM_VMGEXIT_HV_FEATURES: >>>>> case SVM_VMGEXIT_PSC: >>>>> + case SVM_VMGEXIT_GUEST_REQUEST: >>>>> + case SVM_VMGEXIT_EXT_GUEST_REQUEST: >>>>> break; >>>>> default: >>>>> reason = GHCB_ERR_INVALID_EVENT; >>>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct kvm_vcpu >>>>> *vcpu) >>>>> return 1; >>>>> } >>>>> +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm, >>>>> + struct sev_data_snp_guest_request *data, >>>>> + gpa_t req_gpa, gpa_t resp_gpa) >>>>> +{ >>>>> + struct kvm_vcpu *vcpu = &svm->vcpu; >>>>> + struct kvm *kvm = vcpu->kvm; >>>>> + kvm_pfn_t req_pfn, resp_pfn; >>>>> + struct kvm_sev_info *sev; >>>>> + >>>>> + sev = &to_kvm_svm(kvm)->sev_info; >>>>> + >>>>> + if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, >>>>> PAGE_SIZE)) >>>>> + return SEV_RET_INVALID_PARAM; >>>>> + >>>>> + req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa)); >>>>> + if (is_error_noslot_pfn(req_pfn)) >>>>> + return SEV_RET_INVALID_ADDRESS; >>>>> + >>>>> + resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa)); >>>>> + if (is_error_noslot_pfn(resp_pfn)) >>>>> + return SEV_RET_INVALID_ADDRESS; >>>>> + >>>>> + if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true)) >>>>> + return SEV_RET_INVALID_ADDRESS; >>>>> + >>>>> + data->gctx_paddr = __psp_pa(sev->snp_context); >>>>> + data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT); >>>>> + data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static void snp_cleanup_guest_buf(struct >>>>> sev_data_snp_guest_request *data, unsigned long *rc) >>>>> +{ >>>>> + u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT; >>>>> + int ret; >>>>> + >>>>> + ret = snp_page_reclaim(pfn); >>>>> + if (ret) >>>>> + *rc = SEV_RET_INVALID_ADDRESS; >>>>> + >>>>> + ret = rmp_make_shared(pfn, PG_LEVEL_4K); >>>>> + if (ret) >>>>> + *rc = SEV_RET_INVALID_ADDRESS; >>>>> +} >>>>> + >>>>> +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t >>>>> req_gpa, gpa_t resp_gpa) >>>>> +{ >>>>> + struct sev_data_snp_guest_request data = {0}; >>>>> + struct kvm_vcpu *vcpu = &svm->vcpu; >>>>> + struct kvm *kvm = vcpu->kvm; >>>>> + struct kvm_sev_info *sev; >>>>> + unsigned long rc; >>>>> + int err; >>>>> + >>>>> + if (!sev_snp_guest(vcpu->kvm)) { >>>>> + rc = SEV_RET_INVALID_GUEST; >>>>> + goto e_fail; >>>>> + } >>>>> + >>>>> + sev = &to_kvm_svm(kvm)->sev_info; >>>>> + >>>>> + mutex_lock(&sev->guest_req_lock); >>>>> + >>>>> + rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa); >>>>> + if (rc) >>>>> + goto unlock; >>>>> + >>>>> + rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err); >>>> >>>> >>>> This one goes via sev_issue_cmd_external_user() and uses sev-fd... >>>> >>>>> + if (rc) >>>>> + /* use the firmware error code */ >>>>> + rc = err; >>>>> + >>>>> + snp_cleanup_guest_buf(&data, &rc); >>>>> + >>>>> +unlock: >>>>> + mutex_unlock(&sev->guest_req_lock); >>>>> + >>>>> +e_fail: >>>>> + svm_set_ghcb_sw_exit_info_2(vcpu, rc); >>>>> +} >>>>> + >>>>> +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; >>>>> + } >>>>> + >>>>> + 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); >>>> >>>> but this one does not and jump straight to >>>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? Can >>>> these two be unified? sev_issue_cmd_external_user() only checks if >>>> fd is /dev/sev which is hardly useful. >>>> >>>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query extended >>>> attestation report" added this one. >>> >>> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and >>> that's why it goes through the CCP driver interface >>> snp_guest_ext_guest_request() that is used to get both the report and >>> certificate data/blob at the same time. >> >> True. I thought though that this calls for extending sev_issue_cmd() >> to take care of these extra parameters rather than just skipping the >> sev->fd. >> >> >>> All the FW API calls on the KVM side go through sev_issue_cmd() and >>> sev_issue_cmd_external_user() interfaces and that i believe uses >>> sev->fd more of as a sanity check. >> >> Does not look like it: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290 >> >> === >> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd, >> void *data, int *error) >> { >> if (!filep || filep->f_op != &sev_fops) >> return -EBADF; >> >> return sev_do_cmd(cmd, data, error); >> } >> EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user); >> === >> >> The only "more" is that it requires sev->fd to be a valid open fd, >> what is the value in that? I may easily miss the bigger picture here. >> Thanks, >> >> > > Have a look at following functions in drivers/crypto/ccp/sev-dev.c: > sev_dev_init() and sev_misc_init(). > > static int sev_misc_init(struct sev_device *sev) > { > struct device *dev = sev->dev; > int ret; > > /* > * SEV feature support can be detected on multiple devices but > * the SEV FW commands must be issued on the master. During > * probe, we do not know the master hence we create /dev/sev on > * the first device probe. > * sev_do_cmd() finds the right master device to which to issue > * the command to the firmware. > */
It is still a single /dev/sev node and the userspace cannot get it wrong, it does not have to choose between (for instance) /dev/sev0 and /dev/sev1 on a 2 SOC system.
> ... > ... > > Hence, sev_issue_cmd_external_user() needs to ensure that the correct > device (master device) is being operated upon and that's why there is > the check for file operations matching sev_fops as below : > > int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd, > void *data, int *error) > { > if (!filep || filep->f_op != &sev_fops) > return -EBADF; > .. > .. > > Essentially, sev->fd is the misc. device created for the master PSP > device on which the SEV/SNP firmware commands are issued, hence, > sev_issue_cmd() uses sev->fd.
There is always just one fd which always uses psp_master, nothing from that fd is used.
More to the point, if sev->fd is still important, why is it ok to skip it for snp_handle_ext_guest_request()? Thanks,
-- Alexey
| |