On Mon, May 20, 2024 at 07:17:13AM -0700, Sean Christopherson wrote: > This needs a > > From: Michael Roth <michael.roth@xxxxxxx> > > otherwise Author will be assigned to your @utexas.edu email. Thanks, I hadn't considered that. My work email issue seems to be resolved now, but will keep that in mind if I ever need to use a fallback again. > > On Sat, May 18, 2024, Michael Roth wrote: > > Before forwarding guest requests to firmware, KVM takes a reference on > > the 2 pages the guest uses for its request/response buffers. Make sure > > to release these when cleaning up after the request is completed. > > > > Signed-off-by: Michael Roth <michael.roth@xxxxxxx> > > --- > > ... > > > @@ -3970,14 +3980,11 @@ static int __snp_handle_guest_req(struct kvm *kvm, gpa_t req_gpa, gpa_t resp_gpa > > return ret; > > > > ret = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, fw_err); > > - if (ret) > > - return ret; > > > > - ret = snp_cleanup_guest_buf(&data); > > - if (ret) > > - return ret; > > + if (snp_cleanup_guest_buf(&data)) > > + return -EINVAL; > > EINVAL feels wrong. The input was completely valid. Also, forwarding the error Yah, EIO seems more suitable here. > to the guest doesn't seem like the right thing to do if KVM can't reclaim the > response PFN. Shouldn't that be fatal to the VM? The thinking here is that pretty much all guest request failures will be fatal to the guest being able to continue. At least, that's definitely true for attestation. So reporting the error to the guest would allow that failure to be propagated along by handling in the guest where it would presumably be reported a little more clearly to the guest owner, at which point the guest would most likely terminate itself anyway. But there is a possibility that the guest will attempt access the response PFN before/during that reporting and spin on an #NPF instead though. So maybe the safer more repeatable approach is to handle the error directly from KVM and propagate it to userspace. But the GHCB spec does require that the firmware response code for SNP_GUEST_REQUEST be passed directly to the guest via lower 32-bits of SW_EXITINFO2, so we'd still want handling to pass that error on to the guest, so I made some changes to retain that behavior. > > > - return 0; > > + return ret; > > I find the setup/cleanup split makes this code harder to read, not easier. It > won't be pretty no matter waht due to the potential RMP failures, but IMO this > is easier to follow: It *might* make more sense to split things out into helpers when extended guest requests are implemented, but for the patch in question I agree what you have below is clearer. I also went a step further and moved __snp_handle_guest_req() back into snp_handle_guest_req() as well to simplify the logic for always passing firmware errors back to the guest. I'll post a v2 of the fixup with these changes added. But I've also pushed it here for reference: https://github.com/mdroth/linux/commit/8ceab17950dc5f1b94231037748104f7c31752f8 (from https://github.com/mdroth/linux/commits/kvm-next-snp-fixes2/) and here's the original PATCH 17/19 with all pending fixes squashed in: https://github.com/mdroth/linux/commit/b4f51e38da22a2b163c546cb2a3aefd04446b3c7 (from https://github.com/mdroth/linux/commits/kvm-next-snp-fixes2-squashed/) (also retested attestation with simulated failures and double-checked for clang warnings with W=1) Thanks! -Mike > > struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > struct sev_data_snp_guest_request data = {0}; > kvm_pfn_t req_pfn, resp_pfn; > int ret; > > if (!sev_snp_guest(kvm)) > return -EINVAL; > > if (!PAGE_ALIGNED(req_gpa) || !PAGE_ALIGNED(resp_gpa)) > return -EINVAL; > > req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa)); > if (is_error_noslot_pfn(req_pfn)) > return -EINVAL; > > ret = -EINVAL; > > resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa)); > if (is_error_noslot_pfn(resp_pfn)) > goto release_req; > > if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true)) { > kvm_release_pfn_clean(resp_pfn); > goto release_req; > } > > 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); > ret = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, fw_err); > > if (snp_page_reclaim(resp_pfn) || > rmp_make_shared(resp_pfn, PG_LEVEL_4K)) > ret = ret ?: -EIO; > else > kvm_release_pfn_dirty(resp_pfn); > release_req: > kvm_release_pfn_clean(req_pfn); > return ret; > >
