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

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* 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?


> 
> Main change since v6 [2]:
> Protected the shared_offsets array with a rwlock instead of hopping on
> the invalidate_lock. The main reason for this is that the final put
> callback (kvm_gmem_handle_folio_put()) could be called from an atomic
> context, and the invalidate_lock is an rw_semaphore (Vishal).
> 
> The other reason is, in hindsight, 

Can you please try to be more brief and to the point?

>it didn't make sense to use the
> invalidate_lock since they're not quite protecting the same thing.
> 
> I did consider using the folio lock to implicitly protect the array, and
> even have another series that does that. The result was more
> complicated, and not obviously race free. One of the difficulties with
> relying on the folio lock is that there are cases (e.g., on
> initilization and teardown) when we want to toggle the state of an
> offset that doesn't have a folio in the cache. We could special case
> these, but the result was more complicated (and to me not obviously
> better) than adding a separate lock for the shared_offsets array.

This must be captured elsewhere and doesn't need to be here?

> 
> Based on the `KVM: Mapping guest_memfd backed memory at the host for
> software protected VMs` series [3], which in turn is based on Linux
> 6.14-rc7.

Wait, what?  You have another in-flight series that I need for this
series?

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?

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.

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.

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.

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.  Otherwise you will be
forced to rebase on changed patch series every time something is
accepted upstream.  That's all to say, you don't have a solid base of
development for the newer patches and I don't know what interactions
will happen when all the in-flight things meet.

I'm sorry, but you have made the barrier of entry to review this too
high.

Thanks,
Liam

> 
> Cheers,
> /fuad
> 
> [1] https://lore.kernel.org/all/20241010085930.1546800-1-tabba@google.com/
> [2] https://lore.kernel.org/all/20250318162046.4016367-1-tabba@google.com/
> [3] https://lore.kernel.org/all/20250318161823.4005529-1-tabba@google.com/
> [4] https://lpc.events/event/18/contributions/1758/attachments/1457/3699/Guestmemfd%20folio%20state%20page_type.pdf