On Wed, Jan 31, 2024 at 2:00 PM Anish Moorthy <amoorthy@xxxxxxxxxx> wrote:
>
> On Tue, Jan 30, 2024 at 4:26 PM James Houghton <jthoughton@xxxxxxxxxx> wrote:
> > > @@ -3070,6 +3074,15 @@ kvm_pfn_t __gfn_to_pfn_memslot(const struct kvm_memory_slot *slot, gfn_t gfn,
> > > writable = NULL;
> > > }
> > >
> > > + if (!atomic && can_exit_on_missing
> > > + && kvm_is_slot_exit_on_missing(slot)) {
> > > + atomic = true;
> > > + if (async) {
> > > + *async = false;
> > > + async = NULL;
> > > + }
> > > + }
> > > +
> >
> > Perhaps we should have a comment for this? Maybe something like: "If
> > we want to exit-on-missing, we want to bail out if fast GUP fails, and
> > we do not want to go into slow GUP. Setting atomic=true does exactly
> > this."
>
> I was going to push back on the use of "we" but I see that it's all
> over kvm_main.c :).
>
> I agree that a comment would be good, but isn't the "fast GUP only iff
> atomic=true" statement a tautology? That's an actual question, my
> memory's fuzzy.
>
> What about
>
> > When the slot is exit-on-missing (and when we should respect that)
> > set atomic=true to prevent GUP from faulting in the userspace
> > mappings.
>
> instead?
This is much better than what I wrote, thanks! We merely want GUP not
to fault the page in; we don't actually care about fast GUP vs. slow
GUP.
On that note, I think we need to drop the patch that causes
read-faults in RO memslots to go through fast GUP. KVM didn't do that
for a good reason[1].
That would break KVM_EXIT_ON_MISSING for RO memslots, so I think that
the right way to implement KVM_EXIT_ON_MISSING is to have
hva_to_pfn_slow pass FOLL_NOFAULT, at least for the RO memslot case.
We still get the win we're looking for: don't grab the userfaultfd
locks.
[1]: Commit 17839856fd5 ("gup: document and work around "COW can break
either way" issue")
