Re: [PATCH v7 4/9] KVM: guest_memfd: Handle in-place shared memory as guest_memfd backed memory

[David Hildenbrand <david@redhat.com>]

On 18.03.25 17:18, Fuad Tabba wrote:
> For VMs that allow sharing guest_memfd backed memory in-place,
> handle that memory the same as "private" guest_memfd memory. This
> means that faulting that memory in the host or in the guest will
> go through the guest_memfd subsystem.
> 
> 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.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>   include/linux/kvm_host.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 601bbcaa5e41..3d5595a71a2a 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -2521,7 +2521,8 @@ static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
>   #else
>   static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
>   {
> -	return false;
> +	return kvm_arch_gmem_supports_shared_mem(kvm) &&
> +	       kvm_slot_can_be_private(gfn_to_memslot(kvm, gfn));
>   }
>   #endif /* CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES */
>   

I've been thinking long about this, and was wondering if we should instead
clean up the code to decouple the "private" from gmem handling first.

I know, this was already discussed a couple of times, but faking that
shared memory is private looks odd.

I played with the code to star cleaning this up. I ended up with the following
gmem-terminology  cleanup patches (not even compile tested)

KVM: rename CONFIG_KVM_GENERIC_PRIVATE_MEM to CONFIG_KVM_GENERIC_GMEM_POPULATE
KVM: rename CONFIG_KVM_PRIVATE_MEM to CONFIG_KVM_GMEM
KVM: rename kvm_arch_has_private_mem() to kvm_arch_supports_gmem()
KVM: x86: rename kvm->arch.has_private_mem to kvm->arch.supports_gmem
KVM: rename kvm_slot_can_be_private() to kvm_slot_has_gmem()
KVM: x86: generalize private fault lookups to "gmem" fault lookups

https://github.com/davidhildenbrand/linux/tree/gmem_shared_prep

On top of that, I was wondering if we could look into doing something like
the following. It would also allow for pulling pages out of gmem for
existing SW-protected VMs once they enable shared memory for GMEM IIUC.


diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 08eebd24a0e18..6f878cab0f466 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4495,11 +4495,6 @@ static int kvm_mmu_faultin_pfn_gmem(struct kvm_vcpu *vcpu,
  {
         int max_order, r;
  
-       if (!kvm_slot_has_gmem(fault->slot)) {
-               kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
-               return -EFAULT;
-       }
-
         r = kvm_gmem_get_pfn(vcpu->kvm, fault->slot, fault->gfn, &fault->pfn,
                              &fault->refcounted_page, &max_order);
         if (r) {
@@ -4518,8 +4513,19 @@ static int __kvm_mmu_faultin_pfn(struct kvm_vcpu *vcpu,
                                  struct kvm_page_fault *fault)
  {
         unsigned int foll = fault->write ? FOLL_WRITE : 0;
+       bool use_gmem = false;
+
+       if (fault->is_private) {
+               if (!kvm_slot_has_gmem(fault->slot)) {
+                       kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
+                       return -EFAULT;
+               }
+               use_gmem = true;
+       } else if (kvm_slot_has_gmem_with_shared(fault->slot)) {
+               use_gmem = true;
+       }
  
-       if (fault->is_private)
+       if (use_gmem)
                 return kvm_mmu_faultin_pfn_gmem(vcpu, fault);
  
         foll |= FOLL_NOWAIT;


That is, we'd not claim that things are private when they are not, but instead
teach the code about shared memory coming from gmem.

There might be some more missing, just throwing it out there if I am completely off.

-- 
Cheers,

David / dhildenb