Re: [PATCH v3 0/6] KVM: guest_memfd: support for uffd minor

[David Hildenbrand]

On 07.04.25 17:14, Lorenzo Stoakes wrote:
> On Mon, Apr 07, 2025 at 04:46:48PM +0200, David Hildenbrand wrote:
> > On 07.04.25 16:24, Liam R. Howlett wrote:
> > > * Nikita Kalyazin <kalyazin@xxxxxxxxxx> [250407 10:05]:
> > > >
> > >
> > > ...
> > >
> > > > > All of this is extremely confusing because the onus of figuring out what
> > > > > the final code will look like is put on the reviewer.  As it is, we have
> > > > > issues with people not doing enough review of the code (due to limited
> > > > > time).  One way to get reviews is to make the barrier of entry as low as
> > > > > possible.
> > > > >
> > > > > I spent Friday going down a rabbit hole of patches referring to each
> > > > > other as dependencies and I gave up.  It looks like I mistook one set of
> > > > > patches as required vs them requiring the same in-flight ones as your
> > > > > patches.
> > > > >
> > > > > I am struggling to see how we can adequately support all of you given
> > > > > the way the patches are sent out in batches with dependencies - it is
> > > > > just too time consuming to sort out.
> > > >
> > > > I'm happy to do whatever I can to make the review easier.  I suppose the
> > > > extreme case is to wait for the dependencies to get accepted, effectively
> > > > serialising submissions, but that slows the process down significantly.  For
> > > > example, I received very good feedback on v1 and v2 of this series and was
> > > > able to address it instead of waiting for the dependency.  Would including
> > > > the required patches directly in the series help?  My only concern is in
> > > > that case the same patch will be submitted multiple times (as a part of
> > > > every depending series), but if it's better, I'll be doing that instead.
> > >
> > > Don't resend patches that someone else is upstreaming, that'll cause
> > > other problems.
> > >
> > > Three methods come to mind:
> > >
> > > 1. As you stated, wait for the dependencies to land.  This is will mean
> > > what you are working against is well tested and won't change (and you
> > > won't have to re-spin due to an unstable base).
> > >
> > > 2. Combine them into a bigger patch set.  I can then pull one patch set
> > > and look at the parts of interest to the mm side.
> > >
> > > 3. Provide a git repo with the necessary changes together.
> > >
> > > I think 2 and 3 together should be used for the guest_memfd patches.
> > > Someone needs to be managing these to send upstream.  See the discussion
> > > in another patch set on guest_memfd here [1].
> >
> > The issue is that most extensions are fairly independent from each other,
> > except that they built up on Fuad's mmap support,
> >
> > Sending all together as one thing might not be the best option.
> >
> > Once basic mmap support is upstream, some of the extensions (e.g., directmap
> > removal) can go in next.
> >
> > So until that is upstream, I agree that tagging the stuff that builds up on
> > that is the right thing to do, and providing git trees is another very good
> > idea.
> >
> > I'll prioritize getting Fuad's mmap stuff reviewed. (I keep saying that, I
> > know)
>
> Which series is this? Sorry maybe lost track of this one.

Heh, not your fault :)

The most important one for basic mmap support is "KVM: Mapping 
guest_memfd backed memory at the host for software protected VMs" [1]. 
Some stuff (e.g., direct map removal) should be able to make progress 
once that landed.

I do expect the MM-specific patch in there ("mm: Consolidate freeing of 
typed folios on final folio_put()") to not be included as part of that work.

[I shared the feedback from the LSF/MM session in the upstream 
guest_memfd call, and we decided to minimize the usage of the 
folio_put() callback to where absolutely required; that will simplify 
things and avoid issues as pointed out by Willy, which is great]

The next important one will be "[PATCH v7 0/7] KVM: Restricted mapping 
of guest_memfd at the host and arm64 support" [2], but I similarly 
expect a simplification as we try moving away from folio_put() for the 
"shared <-> private" page conversion case.


So I expect a v8 of [1] (and that also [2] needs to be updated).

@Fuad, please let me know if I am wrong.

[1] 
https://lore.kernel.org/all/20250318161823.4005529-1-tabba@xxxxxxxxxx/T/#u
[2] https://lore.kernel.org/all/20250328153133.3504118-1-tabba@xxxxxxxxxx/

--
Cheers,

David / dhildenb