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

From: Sean Christopherson <seanjc@google.com>
To: Anish Moorthy <amoorthy@google.com>
Cc: oliver.upton@linux.dev, kvm@vger.kernel.org,
	kvmarm@lists.linux.dev, pbonzini@redhat.com, maz@kernel.org,
	robert.hoo.linux@gmail.com, jthoughton@google.com,
	bgardon@google.com, dmatlack@google.com, ricarkol@google.com,
	axelrasmussen@google.com, peterx@redhat.com,
	nadav.amit@gmail.com, isaku.yamahata@gmail.com
Subject: Re: [PATCH v4 09/16] KVM: Introduce KVM_CAP_NOWAIT_ON_FAULT without implementation
Date: Wed, 14 Jun 2023 14:20:32 -0700	[thread overview]
Message-ID: <ZIovIBVLIM69E5Bo@google.com> (raw)
In-Reply-To: <20230602161921.208564-10-amoorthy@google.com>

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?
