On Fri, Mar 17, 2023, Oliver Upton wrote: > On Wed, Mar 15, 2023 at 02:17:24AM +0000, Anish Moorthy wrote: > > Hi Sean, here's what I'm planing to send up as v2 of the scalable > > userfaultfd series. > > I don't see a ton of value in sending a targeted posting of a series to the > list. IOW, just CC all of the appropriate reviewers+maintainers. I promise, > we won't bite. +1. And though I discourage off-list review, if something is really truly not ready for public review, e.g. will do more harm than good by causing confusing, then just send the patches off-list. Half measures like this will just make folks grumpy. > > Don't worry, I'm not asking you to review this all :) I just have a few > > remaining questions regarding KVM_CAP_MEMORY_FAULT_EXIT which seem important > > enough to mention before I ask for more attention from others, and they'll be > > clearer with the patches in hand. Anything else I'm happy to find out about when > > I send the actual v2. > > > > I want your opinion on > > > > 1. The general API I've set up for KVM_CAP_MEMORY_FAULT_EXIT > > (described in the api.rst file) > > 2. Whether the UNKNOWN exit reason cases (everywhere but > > handle_error_pfn atm) would need to be given "real" reasons > > before this could be merged. > > 3. If you think I've missed sites that currently -EFAULT to userspace > > > > About (3): after we agreed to only tackle cases where -EFAULT currently makes it > > to userspace, I went though our list and tried to trace which EFAULTS actually > > bubble up to KVM_RUN. That set ended being suspiciously small, so I wanted to > > sanity-check my findings with you. Lmk if you see obvious errors in my list > > below. > > > > --- EFAULTs under KVM_RUN --- > > > > Confident that needs conversion (already converted) > > --------------------------------------------------- > > * direct_map > > * handle_error_pfn > > * setup_vmgexit_scratch > > * kvm_handle_page_fault > > * FNAME(fetch) > > > > EFAULT does not propagate to userspace (do not convert) > > ------------------------------------------------------- > > * record_steal_time (arch/x86/kvm/x86.c:3463) > > * hva_to_pfn_retry > > * kvm_vcpu_map > > * FNAME(update_accessed_dirty_bits) > > * __kvm_gfn_to_hva_cache_init > > Might actually make it to userspace, but only through > > kvm_read|write_guest_offset_cached- would be covered by those conversions > > * kvm_gfn_to_hva_cache_init > > * __kvm_read_guest_page > > * hva_to_pfn_remapped > > handle_error_pfn will handle this for the scalable uffd case. Don't think > > other callers -EFAULT to userspace. > > > > Still unsure if needs conversion > > -------------------------------- > > * __kvm_read_guest_atomic > > The EFAULT might be propagated though FNAME(sync_page)? > > * kvm_write_guest_offset_cached (virt/kvm/kvm_main.c:3226) > > * __kvm_write_guest_page > > Called from kvm_write_guest_offset_cached: if that needs change, this does too > > The low-level accessors are common across architectures and can be called from > other contexts besides a vCPU. Is it possible for the caller to catch -EFAULT > and convert that into an exit? Ya, as things stand today, the conversions _must_ be performed at the caller, as there are (sadly) far too many flows where KVM squashes the error. E.g. almost all of x86's paravirt code just suppresses user memory faults :-( Anish, when we discussed this off-list, what I meant by limiting the intial support to existing -EFAULT cases was limiting support to existing cases where KVM directly returns -EFAULT to userspace, not to all existing cases where -EFAULT is ever returned _within KVM_ while handling KVM_RUN. My apologies if I didn't make that clear.
