Re: [WIP Patch v2 00/14] Avoiding slow get-user-pages via memory fault exit

[Oliver Upton <oliver.upton@linux.dev>]

Anish,

Generally the 'RFC PATCH' prefix is used for patches that are for feedback
only (i.e. not to be considered for inclusion).

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.

> 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?

> * kvm_write_guest_page
>   Two interesting paths:
>       - kvm_pv_clock_pairing returns a custom KVM_EFAULT error here
>         (arch/x86/kvm/x86.c:9578)

This is a hypercall handler, so the return code is ABI with the guest. So it
shouldn't be converted to an exit to userspace.

-- 
Thanks,
Oliver