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

[Ackerley Tng <ackerleytng@google.com>]

David Hildenbrand <david@redhat.com> writes:

> 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.
>

Thank you for making this suggestion more concrete, I like the renaming!

> 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()

Perhaps zooming into this [1] can clarify a lot. In
kvm_mmu_max_mapping_level(), it was

  bool is_private = kvm_slot_has_gmem(slot) && kvm_mem_is_private(kvm, gfn);

and now it is

  bool is_gmem = kvm_slot_has_gmem(slot) && kvm_mem_from_gmem(kvm, gfn);

Is this actually saying that the mapping level is to be fully determined
from lpage_info as long as this memslot has gmem and

A. this specific gfn is backed by gmem, or
B. if the specific gfn is private?

I noticed some other places where kvm_mem_is_private() is left as-is
[2], is that intentional? Are you not just renaming but splitting out
the case two cases A and B?

[1] https://github.com/davidhildenbrand/linux/blob/ac8ae8eb494c3d5a943a4a44c0e9e34b4976895a/arch/x86/kvm/mmu/mmu.c#L3286
[2] https://github.com/davidhildenbrand/linux/blob/ac8ae8eb494c3d5a943a4a44c0e9e34b4976895a/arch/x86/kvm/mmu/mmu.c#L4585

> 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