From: Paolo Bonzini <pbonzini@redhat.com>
To: Sean Christopherson <seanjc@google.com>,
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, 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 22/23] KVM: SEV: Fix return code interpretation for RMP nested page faults
Date: Fri, 10 May 2024 18:01:52 +0200 [thread overview]
Message-ID: <566b57c0-27cd-4591-bded-9a397a1d44d5@redhat.com> (raw)
In-Reply-To: <Zj4oFffc7OQivyV-@google.com>
On 5/10/24 15:58, Sean Christopherson wrote:
> On Thu, May 09, 2024, Michael Roth wrote:
>> The intended logic when handling #NPFs with the RMP bit set (31) is to
>> first check to see if the #NPF requires a shared<->private transition
>> and, if so, to go ahead and let the corresponding KVM_EXIT_MEMORY_FAULT
>> get forwarded on to userspace before proceeding with any handling of
>> other potential RMP fault conditions like needing to PSMASH the RMP
>> entry/etc (which will be done later if the guest still re-faults after
>> the KVM_EXIT_MEMORY_FAULT is processed by userspace).
>>
>> The determination of whether any userspace handling of
>> KVM_EXIT_MEMORY_FAULT is needed is done by interpreting the return code
>> of kvm_mmu_page_fault(). However, the current code misinterprets the
>> return code, expecting 0 to indicate a userspace exit rather than less
>> than 0 (-EFAULT). This leads to the following unexpected behavior:
>>
>> - for KVM_EXIT_MEMORY_FAULTs resulting for implicit shared->private
>> conversions, warnings get printed from sev_handle_rmp_fault()
>> because it does not expect to be called for GPAs where
>> KVM_MEMORY_ATTRIBUTE_PRIVATE is not set. Standard linux guests don't
>> generally do this, but it is allowed and should be handled
>> similarly to private->shared conversions rather than triggering any
>> sort of warnings
>>
>> - if gmem support for 2MB folios is enabled (via currently out-of-tree
>> code), implicit shared<->private conversions will always result in
>> a PSMASH being attempted, even if it's not actually needed to
>> resolve the RMP fault. This doesn't cause any harm, but results in a
>> needless PSMASH and zapping of the sPTE
>>
>> Resolve these issues by calling sev_handle_rmp_fault() only when
>> kvm_mmu_page_fault()'s return code is greater than or equal to 0,
>> indicating a KVM_MEMORY_EXIT_FAULT/-EFAULT isn't needed. While here,
>> simplify the code slightly and fix up the associated comments for better
>> clarity.
>>
>> Fixes: ccc9d836c5c3 ("KVM: SEV: Add support to handle RMP nested page faults")
>>
>> Signed-off-by: Michael Roth <michael.roth@amd.com>
>> ---
>> arch/x86/kvm/svm/svm.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 426ad49325d7..9431ce74c7d4 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -2070,14 +2070,12 @@ static int npf_interception(struct kvm_vcpu *vcpu)
>> svm->vmcb->control.insn_len);
>>
>> /*
>> - * rc == 0 indicates a userspace exit is needed to handle page
>> - * transitions, so do that first before updating the RMP table.
>> + * rc < 0 indicates a userspace exit may be needed to handle page
>> + * attribute updates, so deal with that first before handling other
>> + * potential RMP fault conditions.
>> */
>> - if (error_code & PFERR_GUEST_RMP_MASK) {
>> - if (rc == 0)
>> - return rc;
>> + if (rc >= 0 && error_code & PFERR_GUEST_RMP_MASK)
>
> This isn't correct either. A return of '0' also indiciates "exit to userspace",
> it just doesn't happen with SNP because '0' is returned only when KVM attempts
> emulation, and that too gets short-circuited by svm_check_emulate_instruction().
>
> And I would honestly drop the comment, KVM's less-than-pleasant 1/0/-errno return
> values overload is ubiquitous enough that it should be relatively self-explanatory.
>
> Or if you prefer to keep a comment, drop the part that specifically calls out
> attributes updates, because that incorrectly implies that's the _only_ reason
> why KVM checks the return. But my vote is to drop the comment, because it
> essentially becomes "don't proceed to step 2 if step 1 failed", which kind of
> makes the reader go "well, yeah".
So IIUC you're suggesting
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 426ad49325d7..c39eaeb21981 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2068,16 +2068,11 @@ static int npf_interception(struct kvm_vcpu *vcpu)
static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
svm->vmcb->control.insn_bytes : NULL,
svm->vmcb->control.insn_len);
+ if (rc <= 0)
+ return rc;
- /*
- * rc == 0 indicates a userspace exit is needed to handle page
- * transitions, so do that first before updating the RMP table.
- */
- if (error_code & PFERR_GUEST_RMP_MASK) {
- if (rc == 0)
- return rc;
+ if (error_code & PFERR_GUEST_RMP_MASK)
sev_handle_rmp_fault(vcpu, fault_address, error_code);
- }
return rc;
}
?
So, we're... a bit tight for 6.10 to include SNP and that is an
understatement. My plan is to merge it for 6.11, but do so
immediately after the merge window ends. In other words, it
is a delay in terms of release but not in terms of time. I
don't want QEMU and kvm-unit-tests work to be delayed any
further, in particular.
Once we sort out the loose ends of patches 21-23, you could send
it as a pull request.
Paolo
next prev parent reply other threads:[~2024-05-10 16:02 UTC|newest]
Thread overview: 64+ 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-27 12:25 ` Binbin Wu
2024-05-28 10:39 ` Paolo Bonzini
2024-05-29 20:02 ` Sean Christopherson
2024-05-31 1:22 ` Binbin Wu
2024-05-31 13:10 ` Paolo Bonzini
2024-05-30 16:47 ` Zhi Wang
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 [this message]
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
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=566b57c0-27cd-4591-bded-9a397a1d44d5@redhat.com \
--to=pbonzini@redhat.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=michael.roth@amd.com \
--cc=mingo@redhat.com \
--cc=nikunj.dadhania@amd.com \
--cc=pankaj.gupta@amd.com \
--cc=papaluri@amd.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).