From: Sean Christopherson <seanjc@google.com>
To: Michael Roth <michael.roth@amd.com>
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-mm@kvack.org, linux-crypto@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org,
tglx@linutronix.de, mingo@redhat.com, jroedel@suse.de,
thomas.lendacky@amd.com, hpa@zytor.com, ardb@kernel.org,
pbonzini@redhat.com, vkuznets@redhat.com, jmattson@google.com,
luto@kernel.org, dave.hansen@linux.intel.com, slp@redhat.com,
pgonda@google.com, peterz@infradead.org,
srinivas.pandruvada@linux.intel.com, rientjes@google.com,
dovmurik@linux.ibm.com, tobin@ibm.com, bp@alien8.de,
vbabka@suse.cz, kirill@shutemov.name, ak@linux.intel.com,
tony.luck@intel.com, sathyanarayanan.kuppuswamy@linux.intel.com,
alpergun@google.com, jarkko@kernel.org, ashish.kalra@amd.com,
nikunj.dadhania@amd.com, pankaj.gupta@amd.com,
liam.merwick@oracle.com, Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [PATCH v14 06/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command
Date: Wed, 24 Apr 2024 16:58:13 -0700 [thread overview]
Message-ID: <ZimclfBdk1Y9iKVY@google.com> (raw)
In-Reply-To: <20240421180122.1650812-7-michael.roth@amd.com>
On Sun, Apr 21, 2024, Michael Roth wrote:
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 9d08d1202544..d3ae4ded91df 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -258,6 +258,35 @@ static void sev_decommission(unsigned int handle)
> sev_guest_decommission(&decommission, NULL);
> }
>
> +static int snp_page_reclaim(u64 pfn)
> +{
> + struct sev_data_snp_page_reclaim data = {0};
> + int err, rc;
> +
> + data.paddr = __sme_set(pfn << PAGE_SHIFT);
> + rc = sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, &data, &err);
> + if (WARN_ON_ONCE(rc)) {
This now WARNs here *and* in the caller, which is overkill. Pick one.
> + /*
> + * This shouldn't happen under normal circumstances, but if the
> + * reclaim failed, then the page is no longer safe to use.
Explain _why_ it's unsafe. The code makes it quite clear that reclaim shouldn't
fail. E.g. I assume it's bound to the ASID still? Something else?
This is all messy, too. snp_launch_update_vmsa() does RECLAIM but doesn't
convert the page back to shared, which seems wrong. sev_gmem_post_populate()
open codes the calls separately, and then snp_cleanup_guest_buf() bundles them
together.
Yeesh, and the return types are all mixed. snp_cleanup_guest_buf() returns a
bool on success, but the helpers it uses return 0/-errno. Please convert
everything to return 0/-errno, boolean "error codes" are hard to follow and make
the code unnecessarily brittle.
> + if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src)) {
> + ret = -EINVAL;
Just return -EINVAL, no?
> + goto out;
> + }
> +
> + for (gfn = gfn_start, i = 0; gfn < gfn_start + npages; gfn++, i++) {
> + struct sev_data_snp_launch_update fw_args = {0};
> + bool assigned;
> + void *vaddr;
> + int level;
> +
> + if (!kvm_mem_is_private(kvm, gfn)) {
> + pr_debug("%s: Failed to ensure GFN 0x%llx has private memory attribute set\n",
> + __func__, gfn);
> + ret = -EINVAL;
> + break;
> + }
> +
> + ret = snp_lookup_rmpentry((u64)pfn + i, &assigned, &level);
> + if (ret || assigned) {
> + pr_debug("%s: Failed to ensure GFN 0x%llx RMP entry is initial shared state, ret: %d assigned: %d\n",
> + __func__, gfn, ret, assigned);
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (src) {
> + vaddr = kmap_local_pfn(pfn + i);
> + ret = copy_from_user(vaddr, src + i * PAGE_SIZE, PAGE_SIZE);
> + if (ret) {
> + pr_debug("Failed to copy source page into GFN 0x%llx\n", gfn);
> + goto out_unmap;
> + }
> + }
> +
> + ret = rmp_make_private(pfn + i, gfn << PAGE_SHIFT, PG_LEVEL_4K,
> + sev_get_asid(kvm), true);
> + if (ret) {
> + pr_debug("%s: Failed to mark RMP entry for GFN 0x%llx as private, ret: %d\n",
> + __func__, gfn, ret);
> + goto out_unmap;
> + }
> +
> + n_private++;
> +
> + fw_args.gctx_paddr = __psp_pa(sev->snp_context);
> + fw_args.address = __sme_set(pfn_to_hpa(pfn + i));
> + fw_args.page_size = PG_LEVEL_TO_RMP(PG_LEVEL_4K);
> + fw_args.page_type = sev_populate_args->type;
> + ret = __sev_issue_cmd(sev_populate_args->sev_fd, SEV_CMD_SNP_LAUNCH_UPDATE,
> + &fw_args, &sev_populate_args->fw_error);
> + if (ret) {
> + pr_debug("%s: SEV-SNP launch update failed, ret: 0x%x, fw_error: 0x%x\n",
> + __func__, ret, sev_populate_args->fw_error);
> +
> + if (WARN_ON_ONCE(snp_page_reclaim(pfn + i)))
> + goto out_unmap;
> +
> + /*
> + * When invalid CPUID function entries are detected,
> + * firmware writes the expected values into the page and
> + * leaves it unencrypted so it can be used for debugging
> + * and error-reporting.
> + *
> + * Copy this page back into the source buffer so
> + * userspace can use this information to provide
> + * information on which CPUID leaves/fields failed CPUID
> + * validation.
> + */
> + if (sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_CPUID &&
> + sev_populate_args->fw_error == SEV_RET_INVALID_PARAM) {
> + if (WARN_ON_ONCE(host_rmp_make_shared(pfn + i, PG_LEVEL_4K)))
> + goto out_unmap;
> +
> + if (copy_to_user(src + i * PAGE_SIZE, vaddr,
> + PAGE_SIZE))
> + pr_debug("Failed to write CPUID page back to userspace\n");
> +
> + /* PFN is hypervisor-owned at this point, skip cleanup for it. */
> + n_private--;
If reclaim or make_shared fails, KVM will attempt make_shared again. And I am
not a fan of keeping vaddr mapped after making the pfn private. Functionally
it probably changes nothing, but conceptually it's odd. And "mapping" is free
(AFAIK). Oh, and vaddr is subtly guaranteed to be valid based on the type, which
is fun.
So why not unmap immediately after the first copy_from_user(), and then this
can be:
if (ret)
goto err;
}
return 0;
err:
if (!<reclaim and make shared>() &&
sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_CPUID &&
sev_populate_args->fw_error == SEV_RET_INVALID_PARAM) {
vaddr = kmap_local(...);
copy_to_user(...);
kunmap_local(vaddr);
}
for (i = 0; i < n_private - 1; i++)
<reclaim? and make shared()>
return ret;
> + sev_populate_args->fw_error == SEV_RET_INVALID_PARAM)
> + }
> + }
> +
> +out_unmap:
> + kunmap_local(vaddr);
> + if (ret)
> + break;
> + }
> +
> +out:
> + if (ret) {
> + pr_debug("%s: exiting with error ret %d, restoring %d gmem PFNs to shared.\n",
> + __func__, ret, n_private);
> + for (i = 0; i < n_private; i++)
> + WARN_ON_ONCE(host_rmp_make_shared(pfn + i, PG_LEVEL_4K));
> + }
> +
> + return ret;
> +}
...
> + src = params.type == KVM_SEV_SNP_PAGE_TYPE_ZERO ? NULL : u64_to_user_ptr(params.uaddr);
> +
> + count = kvm_gmem_populate(kvm, params.gfn_start, src, npages,
> + sev_gmem_post_populate, &sev_populate_args);
> + if (count < 0) {
> + argp->error = sev_populate_args.fw_error;
> + pr_debug("%s: kvm_gmem_populate failed, ret %ld (fw_error %d)\n",
> + __func__, count, argp->error);
> + ret = -EIO;
> + } else if (count <= npages) {
This seems like excessive paranoia.
> + params.gfn_start += count;
> + params.len -= count * PAGE_SIZE;
> + if (params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)
> + params.uaddr += count * PAGE_SIZE;
> +
> + ret = copy_to_user(u64_to_user_ptr(argp->data), ¶ms, sizeof(params))
> + ? -EIO : 0;
copy_to_user() failures typically return -EFAULT. The style is not the most
readable. Maybe this?
ret = 0;
if (copy_to_user(...))
ret = -EFAULT;
> + } else {
> + WARN_ONCE(1, "Completed page count %ld exceeds requested amount %ld",
> + count, npages);
> + ret = -EINVAL;
> + }
> +
> +out:
> + mutex_unlock(&kvm->slots_lock);
> +
> + return ret;
> +}
> +
> int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> {
> struct kvm_sev_cmd sev_cmd;
> @@ -2217,6 +2451,9 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> case KVM_SEV_SNP_LAUNCH_START:
> r = snp_launch_start(kvm, &sev_cmd);
> break;
> + case KVM_SEV_SNP_LAUNCH_UPDATE:
> + r = snp_launch_update(kvm, &sev_cmd);
> + break;
> default:
> r = -EINVAL;
> goto out;
> --
> 2.25.1
>
next prev parent reply other threads:[~2024-04-24 23:58 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-21 18:01 [PATCH v14 00/22] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Michael Roth
2024-04-21 18:01 ` [PATCH v14 01/22] KVM: SEV: Select KVM_GENERIC_PRIVATE_MEM when CONFIG_KVM_AMD_SEV=y Michael Roth
2024-04-21 18:01 ` [PATCH v14 02/22] KVM: SEV: Add support to handle AP reset MSR protocol Michael Roth
2024-04-21 18:01 ` [PATCH v14 03/22] KVM: SEV: Add GHCB handling for Hypervisor Feature Support requests Michael Roth
2024-04-24 20:21 ` Sean Christopherson
2024-04-25 20:52 ` Michael Roth
2024-04-25 21:55 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 04/22] KVM: SEV: Add initial SEV-SNP support Michael Roth
2024-04-21 18:01 ` [PATCH v14 05/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command Michael Roth
2024-04-24 23:26 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 06/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command Michael Roth
2024-04-24 23:58 ` Sean Christopherson [this message]
2024-04-21 18:01 ` [PATCH v14 07/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_FINISH command Michael Roth
2024-04-21 18:01 ` [PATCH v14 08/22] KVM: SEV: Add support to handle GHCB GPA register VMGEXIT Michael Roth
2024-04-21 18:01 ` [PATCH v14 09/22] KVM: SEV: Add support to handle MSR based Page State Change VMGEXIT Michael Roth
2024-04-24 20:59 ` Sean Christopherson
2024-04-25 22:00 ` Michael Roth
2024-04-25 22:13 ` Sean Christopherson
2024-04-26 17:16 ` Michael Roth
2024-04-26 20:14 ` Sean Christopherson
2024-04-26 22:24 ` Michael Roth
2024-04-26 22:48 ` Michael Roth
2024-04-21 18:01 ` [PATCH v14 10/22] KVM: SEV: Add support to handle " Michael Roth
2024-04-21 18:01 ` [PATCH v14 11/22] KVM: SEV: Add support to handle RMP nested page faults Michael Roth
2024-04-21 18:01 ` [PATCH v14 12/22] KVM: SEV: Support SEV-SNP AP Creation NAE event Michael Roth
2024-04-21 18:01 ` [PATCH v14 13/22] KVM: SEV: Add support for GHCB-based termination requests Michael Roth
2024-04-21 18:01 ` [PATCH v14 14/22] KVM: SEV: Implement gmem hook for initializing private pages Michael Roth
2024-04-21 18:01 ` [PATCH v14 15/22] KVM: SEV: Implement gmem hook for invalidating " Michael Roth
2024-04-21 18:01 ` [PATCH v14 16/22] KVM: x86: Implement gmem hook for determining max NPT mapping level Michael Roth
2024-04-25 0:45 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 17/22] KVM: SEV: Avoid WBINVD for HVA-based MMU notifications for SNP Michael Roth
2024-04-25 0:17 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 18/22] KVM: SVM: Add module parameter to enable SEV-SNP Michael Roth
2024-04-21 18:01 ` [PATCH v14 19/22] KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event Michael Roth
2024-04-21 18:01 ` [PATCH v14 20/22] crypto: ccp: Add the SNP_VLEK_LOAD command Michael Roth
2024-04-21 18:01 ` [PATCH v14 21/22] crypto: ccp: Add the SNP_{PAUSE,RESUME}_ATTESTATION commands Michael Roth
2024-04-25 0:15 ` Sean Christopherson
2024-04-26 17:35 ` Michael Roth
2024-04-26 19:57 ` Sean Christopherson
2024-04-26 21:46 ` Michael Roth
2024-04-27 0:10 ` Sean Christopherson
2024-04-27 1:32 ` Michael Roth
2024-04-29 14:27 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 22/22] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST NAE event Michael Roth
2024-04-25 0:10 ` Sean Christopherson
2024-04-26 17:57 ` Michael Roth
2024-04-23 16:21 ` [PATCH v14 23/22] [SQUASH] KVM: SEV: Add support to handle GHCB GPA register VMGEXIT Michael Roth
2024-04-23 16:21 ` [PATCH v14 24/22] [SQUASH] KVM: SEV: Add support to handle MSR based Page State Change VMGEXIT Michael Roth
2024-04-23 16:21 ` [PATCH v14 25/22] [SQUASH] KVM: SEV: Add support to handle " Michael Roth
2024-04-23 16:21 ` [PATCH v14 26/22] [SQUASH] KVM: SEV: Add support for GHCB-based termination requests Michael Roth
2024-04-23 16:21 ` [PATCH v14 27/22] [SQUASH] KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event Michael Roth
2024-04-23 16:21 ` [PATCH v14 28/22] [SQUASH] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST " Michael Roth
2024-04-23 21:36 ` Jarkko Sakkinen
2024-04-23 16:21 ` [PATCH v14 29/22] [SQUASH] KVM: SEV: Support SEV-SNP AP Creation " Michael Roth
2024-04-23 16:31 ` [PATCH v14 00/22] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Michael Roth
2024-04-24 16:51 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZimclfBdk1Y9iKVY@google.com \
--to=seanjc@google.com \
--cc=ak@linux.intel.com \
--cc=alpergun@google.com \
--cc=ardb@kernel.org \
--cc=ashish.kalra@amd.com \
--cc=bp@alien8.de \
--cc=brijesh.singh@amd.com \
--cc=dave.hansen@linux.intel.com \
--cc=dovmurik@linux.ibm.com \
--cc=hpa@zytor.com \
--cc=jarkko@kernel.org \
--cc=jmattson@google.com \
--cc=jroedel@suse.de \
--cc=kirill@shutemov.name \
--cc=kvm@vger.kernel.org \
--cc=liam.merwick@oracle.com \
--cc=linux-coco@lists.linux.dev \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@kernel.org \
--cc=michael.roth@amd.com \
--cc=mingo@redhat.com \
--cc=nikunj.dadhania@amd.com \
--cc=pankaj.gupta@amd.com \
--cc=pbonzini@redhat.com \
--cc=peterz@infradead.org \
--cc=pgonda@google.com \
--cc=rientjes@google.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=slp@redhat.com \
--cc=srinivas.pandruvada@linux.intel.com \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=tobin@ibm.com \
--cc=tony.luck@intel.com \
--cc=vbabka@suse.cz \
--cc=vkuznets@redhat.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).