Re: [PATCH v4 09/16] KVM: Introduce KVM_CAP_NOWAIT_ON_FAULT without implementation

[Sean Christopherson <seanjc@google.com>]

On Wed, Jun 14, 2023, 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;
> +       }

Gah, got turned around and forgot to account for @atomic.  So this?

	if (!atomic && memslot_is_nowait_on_fault(slot)) {
		atomic = true;
		if (async) {
			*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?