From: Michael Roth <michael.roth@amd.com>
To: Paolo Bonzini <pbonzini@redhat.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>,
<seanjc@google.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>,
<papaluri@amd.com>
Subject: Re: [PATCH v15 23/23] KVM: SEV: Fix PSC handling for SMASH/UNSMASH and partial update ops
Date: Fri, 10 May 2024 14:08:43 -0500 [thread overview]
Message-ID: <20240510190843.yivmiwaj5isvqyph@amd.com> (raw)
In-Reply-To: <fe5cc86b-43e0-4685-99e7-998e61db333f@redhat.com>
On Fri, May 10, 2024 at 07:09:07PM +0200, Paolo Bonzini wrote:
> On 5/10/24 03:58, Michael Roth wrote:
> > There are a few edge-cases that the current processing for GHCB PSC
> > requests doesn't handle properly:
> >
> > - KVM properly ignores SMASH/UNSMASH ops when they are embedded in a
> > PSC request buffer which contains other PSC operations, but
> > inadvertantly forwards them to userspace as private->shared PSC
> > requests if they appear at the end of the buffer. Make sure these are
> > ignored instead, just like cases where they are not at the end of the
> > request buffer.
> >
> > - Current code handles non-zero 'cur_page' fields when they are at the
> > beginning of a new GPA range, but it is not handling properly when
> > iterating through subsequent entries which are otherwise part of a
> > contiguous range. Fix up the handling so that these entries are not
> > combined into a larger contiguous range that include unintended GPA
> > ranges and are instead processed later as the start of a new
> > contiguous range.
> >
> > - The page size variable used to track 2M entries in KVM for inflight PSCs
> > might be artifically set to a different value, which can lead to
> > unexpected values in the entry's final 'cur_page' update. Use the
> > entry's 'pagesize' field instead to determine what the value of
> > 'cur_page' should be upon completion of processing.
> >
> > While here, also add a small helper for clearing in-flight PSCs
> > variables and fix up comments for better readability.
> >
> > Fixes: 266205d810d2 ("KVM: SEV: Add support to handle Page State Change VMGEXIT")
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
>
> There are some more improvements that can be made to the readability of
> the code... this one is already better than the patch is fixing up, but I
> don't like the code that is in the loop even though it is unconditionally
> followed by "break".
>
> Here's my attempt at replacing this patch, which is really more of a
> rewrite of the whole function... Untested beyond compilation.
Thanks for the suggested rework. I tested with/without 2MB pages and
everything worked as-written. This is the full/squashed patch I plan to
include in the pull request:
https://github.com/mdroth/linux/commit/91f6d31c4dfc88dd1ac378e2db6117b0c982e63c
-Mike
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 35f0bd91f92e..6e612789c35f 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3555,23 +3555,25 @@ struct psc_buffer {
> static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc);
> -static int snp_complete_psc(struct kvm_vcpu *vcpu)
> +static void snp_complete_psc(struct vcpu_svm *svm, u64 psc_ret)
> +{
> + svm->sev_es.psc_inflight = 0;
> + svm->sev_es.psc_idx = 0;
> + svm->sev_es.psc_2m = 0;
> + ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
> +}
> +
> +static void __snp_complete_one_psc(struct vcpu_svm *svm)
> {
> - struct vcpu_svm *svm = to_svm(vcpu);
> struct psc_buffer *psc = svm->sev_es.ghcb_sa;
> struct psc_entry *entries = psc->entries;
> struct psc_hdr *hdr = &psc->hdr;
> - __u64 psc_ret;
> __u16 idx;
> - if (vcpu->run->hypercall.ret) {
> - psc_ret = VMGEXIT_PSC_ERROR_GENERIC;
> - goto out_resume;
> - }
> -
> /*
> * Everything in-flight has been processed successfully. Update the
> - * corresponding entries in the guest's PSC buffer.
> + * corresponding entries in the guest's PSC buffer and zero out the
> + * count of in-flight PSC entries.
> */
> for (idx = svm->sev_es.psc_idx; svm->sev_es.psc_inflight;
> svm->sev_es.psc_inflight--, idx++) {
> @@ -3581,17 +3583,22 @@ static int snp_complete_psc(struct kvm_vcpu *vcpu)
> }
> hdr->cur_entry = idx;
> +}
> +
> +static int snp_complete_one_psc(struct kvm_vcpu *vcpu)
> +{
> + struct vcpu_svm *svm = to_svm(vcpu);
> + struct psc_buffer *psc = svm->sev_es.ghcb_sa;
> +
> + if (vcpu->run->hypercall.ret) {
> + snp_complete_psc(svm, VMGEXIT_PSC_ERROR_GENERIC);
> + return 1; /* resume guest */
> + }
> +
> + __snp_complete_one_psc(svm);
> /* Handle the next range (if any). */
> return snp_begin_psc(svm, psc);
> -
> -out_resume:
> - svm->sev_es.psc_idx = 0;
> - svm->sev_es.psc_inflight = 0;
> - svm->sev_es.psc_2m = false;
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
> -
> - return 1; /* resume guest */
> }
> static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
> @@ -3601,18 +3608,20 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
> struct psc_hdr *hdr = &psc->hdr;
> struct psc_entry entry_start;
> u16 idx, idx_start, idx_end;
> - __u64 psc_ret, gpa;
> + u64 gfn;
> int npages;
> -
> - /* There should be no other PSCs in-flight at this point. */
> - if (WARN_ON_ONCE(svm->sev_es.psc_inflight)) {
> - psc_ret = VMGEXIT_PSC_ERROR_GENERIC;
> - goto out_resume;
> - }
> + bool huge;
> if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE))) {
> - psc_ret = VMGEXIT_PSC_ERROR_GENERIC;
> - goto out_resume;
> + snp_complete_psc(svm, VMGEXIT_PSC_ERROR_GENERIC);
> + return 1;
> + }
> +
> +next_range:
> + /* There should be no other PSCs in-flight at this point. */
> + if (WARN_ON_ONCE(svm->sev_es.psc_inflight)) {
> + snp_complete_psc(svm, VMGEXIT_PSC_ERROR_GENERIC);
> + return 1;
> }
> /*
> @@ -3624,97 +3633,99 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
> idx_end = hdr->end_entry;
> if (idx_end >= VMGEXIT_PSC_MAX_COUNT) {
> - psc_ret = VMGEXIT_PSC_ERROR_INVALID_HDR;
> - goto out_resume;
> - }
> -
> - /* Nothing more to process. */
> - if (idx_start > idx_end) {
> - psc_ret = 0;
> - goto out_resume;
> + snp_complete_psc(svm, VMGEXIT_PSC_ERROR_INVALID_HDR);
> + return 1;
> }
> /* Find the start of the next range which needs processing. */
> for (idx = idx_start; idx <= idx_end; idx++, hdr->cur_entry++) {
> - __u16 cur_page;
> - gfn_t gfn;
> - bool huge;
> -
> entry_start = entries[idx];
> -
> - /* Only private/shared conversions are currently supported. */
> - if (entry_start.operation != VMGEXIT_PSC_OP_PRIVATE &&
> - entry_start.operation != VMGEXIT_PSC_OP_SHARED)
> - continue;
> -
> gfn = entry_start.gfn;
> - cur_page = entry_start.cur_page;
> huge = entry_start.pagesize;
> + npages = huge ? 512 : 1;
> - if ((huge && (cur_page > 512 || !IS_ALIGNED(gfn, 512))) ||
> - (!huge && cur_page > 1)) {
> - psc_ret = VMGEXIT_PSC_ERROR_INVALID_ENTRY;
> - goto out_resume;
> + if (entry_start.cur_page > npages || !IS_ALIGNED(gfn, npages)) {
> + snp_complete_psc(svm, VMGEXIT_PSC_ERROR_INVALID_ENTRY);
> + return 1;
> }
> + if (entry_start.cur_page) {
> + /*
> + * If this is a partially-completed 2M range, force 4K
> + * handling for the remaining pages since they're effectively
> + * split at this point. Subsequent code should ensure this
> + * doesn't get combined with adjacent PSC entries where 2M
> + * handling is still possible.
> + */
> + npages -= entry_start.cur_page;
> + gfn += entry_start.cur_page;
> + huge = false;
> + }
> + if (npages)
> + break;
> +
> /* All sub-pages already processed. */
> - if ((huge && cur_page == 512) || (!huge && cur_page == 1))
> - continue;
> -
> - /*
> - * If this is a partially-completed 2M range, force 4K handling
> - * for the remaining pages since they're effectively split at
> - * this point. Subsequent code should ensure this doesn't get
> - * combined with adjacent PSC entries where 2M handling is still
> - * possible.
> - */
> - svm->sev_es.psc_2m = cur_page ? false : huge;
> - svm->sev_es.psc_idx = idx;
> - svm->sev_es.psc_inflight = 1;
> -
> - gpa = gfn_to_gpa(gfn + cur_page);
> - npages = huge ? 512 - cur_page : 1;
> - break;
> }
> + if (idx > idx_end) {
> + /* Nothing more to process. */
> + snp_complete_psc(svm, 0);
> + return 1;
> + }
> +
> + svm->sev_es.psc_2m = huge;
> + svm->sev_es.psc_idx = idx;
> + svm->sev_es.psc_inflight = 1;
> +
> /*
> * Find all subsequent PSC entries that contain adjacent GPA
> * ranges/operations and can be combined into a single
> * KVM_HC_MAP_GPA_RANGE exit.
> */
> - for (idx = svm->sev_es.psc_idx + 1; idx <= idx_end; idx++) {
> + while (++idx <= idx_end) {
> struct psc_entry entry = entries[idx];
> if (entry.operation != entry_start.operation ||
> - entry.gfn != entry_start.gfn + npages ||
> - !!entry.pagesize != svm->sev_es.psc_2m)
> + entry.gfn != gfn + npages ||
> + entry.cur_page ||
> + !!entry.pagesize != huge)
> break;
> svm->sev_es.psc_inflight++;
> - npages += entry_start.pagesize ? 512 : 1;
> + npages += huge ? 512 : 1;
> }
> - vcpu->run->exit_reason = KVM_EXIT_HYPERCALL;
> - vcpu->run->hypercall.nr = KVM_HC_MAP_GPA_RANGE;
> - vcpu->run->hypercall.args[0] = gpa;
> - vcpu->run->hypercall.args[1] = npages;
> - vcpu->run->hypercall.args[2] = entry_start.operation == VMGEXIT_PSC_OP_PRIVATE
> - ? KVM_MAP_GPA_RANGE_ENCRYPTED
> - : KVM_MAP_GPA_RANGE_DECRYPTED;
> - vcpu->run->hypercall.args[2] |= entry_start.pagesize
> - ? KVM_MAP_GPA_RANGE_PAGE_SZ_2M
> - : KVM_MAP_GPA_RANGE_PAGE_SZ_4K;
> - vcpu->arch.complete_userspace_io = snp_complete_psc;
> + switch (entry_start.operation) {
> + case VMGEXIT_PSC_OP_PRIVATE:
> + case VMGEXIT_PSC_OP_SHARED:
> + vcpu->run->exit_reason = KVM_EXIT_HYPERCALL;
> + vcpu->run->hypercall.nr = KVM_HC_MAP_GPA_RANGE;
> + vcpu->run->hypercall.args[0] = gfn_to_gpa(gfn);
> + vcpu->run->hypercall.args[1] = npages;
> + vcpu->run->hypercall.args[2] = entry_start.operation == VMGEXIT_PSC_OP_PRIVATE
> + ? KVM_MAP_GPA_RANGE_ENCRYPTED
> + : KVM_MAP_GPA_RANGE_DECRYPTED;
> + vcpu->run->hypercall.args[2] |= huge
> + ? KVM_MAP_GPA_RANGE_PAGE_SZ_2M
> + : KVM_MAP_GPA_RANGE_PAGE_SZ_4K;
> + vcpu->arch.complete_userspace_io = snp_complete_one_psc;
> - return 0; /* forward request to userspace */
> + return 0; /* forward request to userspace */
> -out_resume:
> - svm->sev_es.psc_idx = 0;
> - svm->sev_es.psc_inflight = 0;
> - svm->sev_es.psc_2m = false;
> - ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
> + default:
> + /*
> + * Only shared/private PSC operations are currently supported, so if the
> + * entire range consists of unsupported operations (e.g. SMASH/UNSMASH),
> + * then consider the entire range completed and avoid exiting to
> + * userspace. In theory snp_complete_psc() can be called directly
> + * at this point to complete the current range and start the next one,
> + * but that could lead to unexpected levels of recursion.
> + */
> + __snp_complete_one_psc(svm);
> + goto next_range;
> + }
> - return 1; /* resume guest */
> + unreachable();
> }
> static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
>
>
next prev parent reply other threads:[~2024-05-10 19:09 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-01 8:51 [PATCH v15 00/20] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Michael Roth
2024-05-01 8:51 ` [PATCH v15 01/20] Revert "KVM: x86: Add gmem hook for determining max NPT mapping level" Michael Roth
2024-05-01 8:51 ` [PATCH v15 02/20] KVM: x86: Add hook for determining max NPT mapping level Michael Roth
2024-05-02 23:11 ` Isaku Yamahata
2024-05-07 17:48 ` Paolo Bonzini
2024-05-01 8:51 ` [PATCH v15 03/20] KVM: SEV: Select KVM_GENERIC_PRIVATE_MEM when CONFIG_KVM_AMD_SEV=y Michael Roth
2024-05-01 8:51 ` [PATCH v15 04/20] KVM: SEV: Add initial SEV-SNP support Michael Roth
2024-05-01 8:51 ` [PATCH v15 05/20] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command Michael Roth
2024-05-01 8:51 ` [PATCH v15 06/20] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command Michael Roth
2024-05-01 8:51 ` [PATCH v15 07/20] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_FINISH command Michael Roth
2024-05-01 8:51 ` [PATCH v15 08/20] KVM: SEV: Add support to handle GHCB GPA register VMGEXIT Michael Roth
2024-05-01 8:51 ` [PATCH v15 09/20] KVM: SEV: Add support to handle MSR based Page State Change VMGEXIT Michael Roth
2024-05-16 8:28 ` Binbin Wu
2024-05-16 17:23 ` Paolo Bonzini
2024-05-21 0:49 ` Binbin Wu
2024-05-21 21:49 ` Michael Roth
2024-05-01 8:52 ` [PATCH v15 10/20] KVM: SEV: Add support to handle " Michael Roth
2024-05-01 8:52 ` [PATCH v15 11/20] KVM: SEV: Add support to handle RMP nested page faults Michael Roth
2024-05-01 8:52 ` [PATCH v15 12/20] KVM: SEV: Support SEV-SNP AP Creation NAE event Michael Roth
2024-05-01 8:52 ` [PATCH v15 13/20] KVM: SEV: Implement gmem hook for initializing private pages Michael Roth
2024-05-20 10:16 ` Huang, Kai
2024-05-20 17:35 ` Sean Christopherson
2024-05-20 21:57 ` Huang, Kai
2024-05-20 23:15 ` Sean Christopherson
2024-05-20 23:41 ` Huang, Kai
2024-05-21 0:30 ` Sean Christopherson
2024-05-20 19:14 ` Isaku Yamahata
2024-05-01 8:52 ` [PATCH v15 14/20] KVM: SEV: Implement gmem hook for invalidating " Michael Roth
2024-05-01 8:52 ` [PATCH v15 15/20] KVM: x86: Implement hook for determining max NPT mapping level Michael Roth
2024-05-01 8:52 ` [PATCH v15 16/20] KVM: SEV: Avoid WBINVD for HVA-based MMU notifications for SNP Michael Roth
2024-05-01 8:52 ` [PATCH v15 17/20] KVM: SVM: Add module parameter to enable SEV-SNP Michael Roth
2024-05-01 8:52 ` [PATCH v15 18/20] KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event Michael Roth
2024-05-01 8:52 ` [PATCH v15 19/20] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST " Michael Roth
2024-05-13 23:48 ` Sean Christopherson
2024-05-14 2:51 ` Michael Roth
2024-05-14 14:36 ` Sean Christopherson
2024-05-15 1:25 ` [PATCH] KVM: SEV: Replace KVM_EXIT_VMGEXIT with KVM_EXIT_SNP_REQ_CERTS Michael Roth
2024-05-01 8:52 ` [PATCH v15 20/20] crypto: ccp: Add the SNP_VLEK_LOAD command Michael Roth
2024-05-07 18:04 ` [PATCH v15 00/20] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Paolo Bonzini
2024-05-07 18:14 ` Michael Roth
2024-05-10 2:34 ` Michael Roth
2024-05-10 1:58 ` [PATCH v15 21/23] KVM: MMU: Disable fast path for private memslots Michael Roth
2024-05-10 1:58 ` [PATCH v15 22/23] KVM: SEV: Fix return code interpretation for RMP nested page faults Michael Roth
2024-05-10 13:58 ` Sean Christopherson
2024-05-10 15:36 ` Michael Roth
2024-05-10 16:01 ` Paolo Bonzini
2024-05-10 16:37 ` Michael Roth
2024-05-10 16:59 ` Paolo Bonzini
2024-05-10 17:25 ` Paolo Bonzini
2024-05-14 8:10 ` Borislav Petkov
2024-05-10 1:58 ` [PATCH v15 23/23] KVM: SEV: Fix PSC handling for SMASH/UNSMASH and partial update ops Michael Roth
2024-05-10 17:09 ` Paolo Bonzini
2024-05-10 19:08 ` Michael Roth [this message]
2024-05-10 13:47 ` [PATCH v15 21/23] KVM: MMU: Disable fast path for private memslots Sean Christopherson
2024-05-10 13:50 ` Paolo Bonzini
2024-05-10 15:27 ` Michael Roth
2024-05-10 15:59 ` Sean Christopherson
2024-05-10 17:47 ` Isaku Yamahata
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=20240510190843.yivmiwaj5isvqyph@amd.com \
--to=michael.roth@amd.com \
--cc=ak@linux.intel.com \
--cc=alpergun@google.com \
--cc=ardb@kernel.org \
--cc=ashish.kalra@amd.com \
--cc=bp@alien8.de \
--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=mingo@redhat.com \
--cc=nikunj.dadhania@amd.com \
--cc=pankaj.gupta@amd.com \
--cc=papaluri@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=seanjc@google.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).