Re: [PATCH v7 0/7] KVM: Restricted mapping of guest_memfd at the host and arm64 support

[David Hildenbrand <david@redhat.com>]

On 04.04.25 20:05, Liam R. Howlett wrote:
> * Fuad Tabba <tabba@google.com> [250328 11:32]:
>> This series adds restricted mmap() support to guest_memfd, as well as
>> support for guest_memfd on arm64. Please see v3 for the context [1].
> 
> As I'm sure you are aware, we have issues getting people to review
> patches.  The lower the barrier to entry, the better for everyone.
> 
> Sorry if I go too much into detail about the process, I am not sure
> where you are starting from.  The linux-mm maintainer (Andrew, aka
> akpm), usually takes this cover letter and puts it on patch 1 so that
> the git history captures all the details necessary for the entire patch
> set to make sense.  What you have done here is made a lot of work for
> the maintainer.  I'm not sure what information below is or is not
> captured in the v3 context.
> 
> But then again, maybe are are not going through linux-mm for upstream?

[replying to some bits]

As all patches and this subject is "KVM:" I would assume this goes 
through the kvm tree once ready.

[...]

> 
> So this means I need 6.14-rc7 + patches from march 18th to review your
> series?
> 
> Oh my, and I just responded to another patch set built off this patch
> set?  So we have 3 in-flight patches that I need for the last patch set?
> What is going on with guest_memfd that everything needs to be in-flight
> at once?

Yeah, we're still all trying to connect the pieces to make it all fly 
together. Looks like we're getting there.

> 
> I was actually thinking that maybe it would be better to split *this*
> set into 2 logical parts: 1. mmap() support and 2. guest_memfd arm64
> support.  But now I have so many lore emails opened in my browser trying
> to figure this out that I don't want any more.
> 
> There's also "mm: Consolidate freeing of typed folios on final
> folio_put()" and I don't know where it fits.

That will likely be moved into a different patch set after some 
discussions we had yesterday.

> 
> Is this because the upstream path differs?  It's very difficult to
> follow what's going on.
> 
>>
>> The state diagram that uses the new states in this patch series,
>> and how they would interact with sharing/unsharing in pKVM [4].
> 
> This cover letter is very difficult to follow.  Where do the main
> changes since v6 end?  I was going to suggest bullets, but v3 has
> bullets and is more clear already.  I'd much prefer if it remained line
> v3, any reason you changed?
> 
> I am not sure what upstream repository you are working with, but
> if you are sending this for the mm stream, please base your work on
> mm-new and AT LEAST wait until the patches are in mm-new, but ideally
> wait for mm-unstable and mm-stable for wider test coverage.  This might
> vary based on your upstream path though.
> 
> I did respond to one of the other patch set that's based off this one
> that the lockdep issue in patch 3 makes testing a concern.  Considering
> there are patches on patches, I don't think you are going to get a whole
> lot of people reviewing or testing it until it falls over once it hits
> linux-next.

I think for now it's mostly KVM + guest_memfd people reviewing this. 
linux-mm is only CCed for awareness (clearly MM related stuff).

> 
> The number of patches in-flight, the ordering, and the base is so
> confusing.  Are you sure none of these should be RFC?  The flood of
> changes pretty much guarantees things will be missed, more work will be
> created, and people (like me) will get frustrated.

Yeah, I think everything basing the work on this series should be RFC or 
clearly tagged as based on this work differently.

> It looks like a small team across companies are collaborating on this,
> and that's awesome.  I think you need to change how you are doing things
> and let the rest of us in on the code earlier. 

I think the approach taken to share the different pieces early makes 
sense, it just has to be clearer what the dependencies are and what is 
actually the first thing that should go in so people can focus review on 
that.

> Otherwise you will be
> forced to rebase on changed patch series every time something is
> accepted upstream. 

I don't think that's a problem, the work built on top of this are mostly 
shared for early review/feedback -- whereby I agree that tagging them 
differently makes a lot of sense.

-- 
Cheers,

David / dhildenb