Re: [PATCH v3 08/11] KVM: arm64: Handle guest_memfd()-backed guest page faults

[Quentin Perret <qperret@google.com>]

On Tuesday 11 Feb 2025 at 17:04:05 (+0000), Fuad Tabba wrote:
> Hi Quentin,
> 
> On Tue, 11 Feb 2025 at 16:57, Quentin Perret <qperret@google.com> wrote:
> >
> > On Tuesday 11 Feb 2025 at 16:34:02 (+0000), Fuad Tabba wrote:
> > > > Sorry, yes, that wasn't clear. I meant that kvm_mem_is_private() calls
> > > > kvm_get_memory_attributes() which indexes kvm->mem_attr_array. The
> > > > comment in struct kvm indicates that this xarray is protected by RCU for
> > > > readers, so I was just checking if we were relying on
> > > > kvm_handle_guest_abort() to take srcu_read_lock(&kvm->srcu) for us, or
> > > > if there was something else more subtle here.
> > >
> > > I was kind of afraid that people would be confused by this, and I
> > > commented on it in the commit message of the earlier patch:
> > > https://lore.kernel.org/all/20250211121128.703390-6-tabba@google.com/
> > >
> > > > Note that the word "private" in the name of the function
> > > > kvm_mem_is_private() doesn't necessarily indicate that the memory
> > > > isn't shared, but is due to the history and evolution of
> > > > guest_memfd and the various names it has received. In effect,
> > > > this function is used to multiplex between the path of a normal
> > > > page fault and the path of a guest_memfd backed page fault.
> > >
> > > kvm_mem_is_private() is property of the memslot itself. No xarrays
> > > harmed in the process :)
> >
> > Ah, I see, but could someone enable CONFIG_GENERIC_PRIVATE_MEM and
> > related and get confused? Should KVM_GENERIC_MEMORY_ATTRIBUTES=n
> > depend on !ARM64? Or is it KVM_GMEM_SHARED_MEM that needs to depend on
> > the generic implementation being off?
> 
> VMs that have sharing in place don't need
> KVM_GENERIC_MEMORY_ATTRIBUTES, since that presents the userspace
> view/desire of the state of the folio. It's not necessarily an arm64
> thing, for example, CCA would need it, since it behaves like TDX.
> 
> I guess that KVM_GMEM_SHARED_MEM and KVM_GENERIC_MEMORY_ATTRIBUTES are
> mutually exclusive. I cannot think how the two could be used or useful
> together. We could have a check to ensure that both are not enabled at
> the same time.

Right, that should be a matter of adding

	depend on !KVM_GENERIC_MEMORY_ATTRIBUTES

to the KVM_GMEM_SHARED_MEM Kconfig I think then.

> The behavior in this patch series is that
> KVM_GMEM_SHARED_MEM selects GENERIC_PRIVATE_MEM.

You meant s/GENERIC_PRIVATE_MEM/KVM_PRIVATE_MEM right?

> Also, to help reduce the confusion above, I could rename the variable
> is_private in user_mem_abort() to is_guestmem. WDYT?

I actually don't mind the variable name in that it is consistent with the
rest of the code, but I do positively hate how the definition of
'private' in this code doesn't match my intuition :-)