On Thursday, June 15, 2023 5:21 AM, Sean Christopherson wrote:
> On Fri, Jun 02, 2023, Anish Moorthy wrote:
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index
> > 69a221f71914..abbc5dd72292 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -2297,4 +2297,10 @@ static inline void
> kvm_account_pgtable_pages(void *virt, int nr)
> > */
> > inline void kvm_populate_efault_info(struct kvm_vcpu *vcpu,
> > uint64_t gpa, uint64_t len, uint64_t flags);
> > +
> > +static inline bool kvm_slot_nowait_on_fault(
> > + const struct kvm_memory_slot *slot)
>
> Just when I was starting to think that we had beat all of the Google3 out of
> you :-)
>
> And predicate helpers in KVM typically have "is" or "has" in the name, so that it's
> clear the helper queries, versus e.g. sets the flag or something.
>
> > +{
> > + return slot->flags & KVM_MEM_NOWAIT_ON_FAULT;
>
> KVM is anything but consistent, but if the helper is likely to be called without a
> known good memslot, it probably makes sense to have the helper check for
> NULL, e.g.
>
> static inline bool kvm_is_slot_nowait_on_fault(const struct kvm_memory_slot
> *slot) {
> return slot && slot->flags & KVM_MEM_NOWAIT_ON_FAULT; }
>
> However, do we actually need to force vendor code to query nowait? At a
> glance, the only external (relative to kvm_main.c) users of
> __gfn_to_pfn_memslot() are in flows that play nice with nowait or that don't
> support it at all. So I *think* we can do this?
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index
> be06b09e9104..78024318286d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2403,6 +2403,11 @@ static bool memslot_is_readonly(const struct
> kvm_memory_slot *slot)
> return slot->flags & KVM_MEM_READONLY; }
>
> +static bool memslot_is_nowait_on_fault(const struct kvm_memory_slot
> +*slot) {
> + return slot->flags & KVM_MEM_NOWAIT_ON_FAULT; }
> +
> static unsigned long __gfn_to_hva_many(const struct kvm_memory_slot *slot,
> gfn_t gfn,
> gfn_t *nr_pages, bool write) { @@ -2730,6 +2735,11 @@
> kvm_pfn_t __gfn_to_pfn_memslot(const struct kvm_memory_slot *slot, gfn_t
> gfn,
> writable = NULL;
> }
>
> + if (async && memslot_is_nowait_on_fault(slot)) {
> + *async = false;
> + async = NULL;
> + }
> +
> return hva_to_pfn(addr, atomic, interruptible, async, write_fault,
> writable);
> }
>
> Ah, crud. The above highlights something I missed in v3. The memslot NOWAIT
> flag isn't tied to FOLL_NOWAIT, it's really truly a "fast-only" flag. And even
> more confusingly, KVM does set FOLL_NOWAIT, but for the async #PF case,
> which will get even more confusing if/when KVM uses FOLL_NOWAIT internally.
>
> Drat. I really like the NOWAIT name, but unfortunately it doesn't do what as the
> name says.
>
> I still don't love "fast-only" as that bleeds kernel internals to userspace.
> Anyone have ideas? Maybe something about not installing new mappings?
Yes, "NOWAIT" sounds a bit confusing here. If this is a patch applied to userfaultfd
to solve the "wait" issue on queuing/handling faults, then it would make sense.
But this is a KVM specific solution, which is not directly related to userfaultfd, and
it's not related to FOLL_NOWAIT. There seems nothing to wait in the KVM context
here.
Why not just name the cap as what it does (i.e. something to indicate the cap of
having the fault exited to userspace to handle), e.g. KVM_CAP_EXIT_ON_FAULT
or KVM_CAP_USERSPACE_FAULT.
