On Wed, Jun 14, 2023 at 2:20 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> > +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 :-)
I was trying to stay under the line limit here :( But you've commented
on that elsewhere. Fixed (hopefully :)
> 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.
Done
> 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.
Done: I was doing the null checks in other ways in the arch-specific
code, but yeah it's easier to centralize that here.
> 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);
> }
Hmm, well not having to modify the vendor code would be nice... but
I'll have to look more at __gfn_to_pfn_memslot()'s callers (and
probably send more questions your way :). Hopefully it works out more
like what you suggest.
