From: Sean Christopherson <seanjc@google.com>
To: Anish Moorthy <amoorthy@google.com>
Cc: Isaku Yamahata <isaku.yamahata@gmail.com>,
Marc Zyngier <maz@kernel.org>,
Oliver Upton <oliver.upton@linux.dev>,
jthoughton@google.com, kvm@vger.kernel.org
Subject: Re: [WIP Patch v2 04/14] KVM: x86: Add KVM_CAP_X86_MEMORY_FAULT_EXIT and associated kvm_run field
Date: Fri, 17 Mar 2023 14:50:22 -0700 [thread overview]
Message-ID: <ZBTgnjXJvR8jtc4i@google.com> (raw)
In-Reply-To: <CAF7b7mrTa735kDaEsJQSHTt7gpWy_QZEtsgsnKoe6c21s0jDVw@mail.gmail.com>
On Fri, Mar 17, 2023, Anish Moorthy wrote:
> On Thu, Mar 16, 2023 at 5:02 PM Isaku Yamahata <isaku.yamahata@gmail.com> wrote:
> > > +inline int kvm_memfault_exit_or_efault(
> > > + struct kvm_vcpu *vcpu, uint64_t gpa, uint64_t len, uint64_t exit_flags)
> > > +{
> > > + if (!(vcpu->kvm->memfault_exit_reasons & exit_flags))
> > > + return -EFAULT;
> > > + vcpu->run->exit_reason = KVM_EXIT_MEMORY_FAULT;
> > > + vcpu->run->memory_fault.gpa = gpa;
> > > + vcpu->run->memory_fault.len = len;
> > > + vcpu->run->memory_fault.flags = exit_flags;
> > > + return -1;
> >
> > Why -1? 0? Anyway enum exit_fastpath_completion is x86 kvm mmu internal
> > convention. As WIP, it's okay for now, though.
>
> The -1 isn't to indicate a failure in this function itself, but to
> allow callers to substitute this for "return -EFAULT." A return code
> of zero would mask errors and cause KVM to proceed in ways that it
> shouldn't. For instance, "setup_vmgexit_scratch" uses it like this
>
> if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
> ...
> - return -EFAULT;
> + return kvm_memfault_exit_or_efault(...);
> }
>
> and looking at one of its callers (sev_handle_vmgexit) shows how a
> return code of zero would cause a different control flow
>
> case SVM_VMGEXIT_MMIO_READ:
> ret = setup_vmgexit_scratch(svm, true, control->exit_info_2);
> if (ret)
> break;
>
> ret = ret = kvm_sev_es_mmio_read(vcpu,
Hmm, I generally agree with Isaku, the helper should really return 0. Returning
-1 might work, but it'll likely confuse userspace, and will definitely confuse
KVM developers.
The "0 means exit to userspace" behavior is definitely a pain though, and is likely
going to make this all extremely fragile.
I wonder if we can get away with returning -EFAULT, but still filling vcpu->run
with KVM_EXIT_MEMORY_FAULT and all the other metadata. That would likely simplify
the implementation greatly, and would let KVM fill vcpu->run unconditonally. KVM
would still need a capability to advertise support to userspace, but userspace
wouldn't need to opt in. I think this may have been my very original though, and
I just never actually wrote it down...
next prev parent reply other threads:[~2023-03-17 21:51 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-15 2:17 [WIP Patch v2 00/14] Avoiding slow get-user-pages via memory fault exit Anish Moorthy
2023-03-15 2:17 ` [WIP Patch v2 01/14] KVM: selftests: Allow many vCPUs and reader threads per UFFD in demand paging test Anish Moorthy
2023-03-15 2:17 ` [WIP Patch v2 02/14] KVM: selftests: Use EPOLL in userfaultfd_util reader threads and signal errors via TEST_ASSERT Anish Moorthy
2023-03-15 2:17 ` [WIP Patch v2 03/14] KVM: Allow hva_pfn_fast to resolve read-only faults Anish Moorthy
2023-03-15 2:17 ` [WIP Patch v2 04/14] KVM: x86: Add KVM_CAP_X86_MEMORY_FAULT_EXIT and associated kvm_run field Anish Moorthy
2023-03-17 0:02 ` Isaku Yamahata
2023-03-17 18:33 ` Anish Moorthy
2023-03-17 19:30 ` Oliver Upton
2023-03-17 21:50 ` Sean Christopherson [this message]
2023-03-17 22:44 ` Anish Moorthy
2023-03-20 15:53 ` Sean Christopherson
2023-03-20 18:19 ` Anish Moorthy
2023-03-20 22:11 ` Anish Moorthy
2023-03-21 15:21 ` Sean Christopherson
2023-03-21 18:01 ` Anish Moorthy
2023-03-21 19:43 ` Sean Christopherson
2023-03-22 21:06 ` Anish Moorthy
2023-03-22 23:17 ` Sean Christopherson
2023-03-28 22:19 ` Anish Moorthy
2023-04-04 19:34 ` Sean Christopherson
2023-04-04 20:40 ` Anish Moorthy
2023-04-04 22:07 ` Sean Christopherson
2023-04-05 20:21 ` Anish Moorthy
2023-03-17 18:35 ` Oliver Upton
2023-03-15 2:17 ` [WIP Patch v2 05/14] KVM: x86: Implement memory fault exit for direct_map Anish Moorthy
2023-03-15 2:17 ` [WIP Patch v2 06/14] KVM: x86: Implement memory fault exit for kvm_handle_page_fault Anish Moorthy
2023-03-15 2:17 ` [WIP Patch v2 07/14] KVM: x86: Implement memory fault exit for setup_vmgexit_scratch Anish Moorthy
2023-03-15 2:17 ` [WIP Patch v2 08/14] KVM: x86: Implement memory fault exit for FNAME(fetch) Anish Moorthy
2023-03-15 2:17 ` [WIP Patch v2 09/14] KVM: Introduce KVM_CAP_MEMORY_FAULT_NOWAIT without implementation Anish Moorthy
2023-03-17 18:59 ` Oliver Upton
2023-03-17 20:15 ` Anish Moorthy
2023-03-17 20:54 ` Sean Christopherson
2023-03-17 23:42 ` Anish Moorthy
2023-03-20 15:13 ` Sean Christopherson
2023-03-20 19:53 ` Anish Moorthy
2023-03-17 20:17 ` Sean Christopherson
2023-03-20 22:22 ` Oliver Upton
2023-03-21 14:50 ` Sean Christopherson
2023-03-21 20:23 ` Oliver Upton
2023-03-21 21:01 ` Sean Christopherson
2023-03-15 2:17 ` [WIP Patch v2 10/14] KVM: x86: Implement KVM_CAP_MEMORY_FAULT_NOWAIT Anish Moorthy
2023-03-17 0:32 ` Isaku Yamahata
2023-03-15 2:17 ` [WIP Patch v2 11/14] KVM: arm64: Allow user_mem_abort to return 0 to signal a 'normal' exit Anish Moorthy
2023-03-17 18:18 ` Oliver Upton
2023-03-15 2:17 ` [WIP Patch v2 12/14] KVM: arm64: Implement KVM_CAP_MEMORY_FAULT_NOWAIT Anish Moorthy
2023-03-17 18:27 ` Oliver Upton
2023-03-17 19:00 ` Anish Moorthy
2023-03-17 19:03 ` Oliver Upton
2023-03-17 19:24 ` Sean Christopherson
2023-03-15 2:17 ` [WIP Patch v2 13/14] KVM: selftests: Add memslot_flags parameter to memstress_create_vm Anish Moorthy
2023-03-15 2:17 ` [WIP Patch v2 14/14] KVM: selftests: Handle memory fault exits in demand_paging_test Anish Moorthy
2023-03-17 17:43 ` [WIP Patch v2 00/14] Avoiding slow get-user-pages via memory fault exit Oliver Upton
2023-03-17 18:13 ` Sean Christopherson
2023-03-17 18:46 ` David Matlack
2023-03-17 18:54 ` Oliver Upton
2023-03-17 18:59 ` David Matlack
2023-03-17 19:53 ` Anish Moorthy
2023-03-17 22:03 ` Sean Christopherson
2023-03-20 15:56 ` Sean Christopherson
2023-03-17 20:35 ` Sean Christopherson
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=ZBTgnjXJvR8jtc4i@google.com \
--to=seanjc@google.com \
--cc=amoorthy@google.com \
--cc=isaku.yamahata@gmail.com \
--cc=jthoughton@google.com \
--cc=kvm@vger.kernel.org \
--cc=maz@kernel.org \
--cc=oliver.upton@linux.dev \
/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 NNTP newsgroup(s).