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?
