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

[David Hildenbrand <david@redhat.com>]

On 16.04.25 14:30, Patrick Roy wrote:
> Hi!
> 
> On Mon, 2025-04-14 at 12:51 +0100, David Hildenbrand wrote:
>>
> 
> [...]
> 
>> 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.
> 
> I think I arrived at the need for this as well while experimenting with
> building a Firecracker version that works with my direct map removal
> patches.
> 
> With this patch series, on ARM, as soon as a memslot has a guest_memfd
> associated with it, all guest faults go through kvm_gmem_get_pfn, but on
> x86, they go through slot->userspace_addr by default, as
> CONFIG_KVM_SW_PROTECTED_VM selects CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES.
> There's no real difference between these if slot->userspace_addr can be
> GUP'd, but if its a VMA of a guest_memfd without direct map entries,
> faulting through slot->userspace_addr wont work. So on x86 Firecracker
> has to formally set the memory attributes to private, while on ARM it
> doesn't [1], which is a bit awkward.

Exactly. I proposed all that (especially the interactions/expectations 
regarding KVM memory slots) as a topic for tomorrow's meeting. See below 
on my current idea.

> 
> David, I couldn't find an implementation of
> kvm_slot_has_gmem_with_shared() in the branch you shared, but would it
> be something like "slot->userspace_addr points to a gmem VMA,
> particularly to a VMA of the gmem that's associated with this memslot,
> mapped at the same offset"?

That's open for discussion, and the implementation of that will have to 
be clarified in the concept of this series.

I would have made the KVM slot setup by user space a requirement:

If we enabled "gmem allows for shared memory / mmap" for a MM, then user 
space *must* provide a mmap() of that guest_memfd in slot->userspace_addr.

(we might also be able to use a new slot flag to specify that)

With that, we can just say
* In the MMU etc, avoid walking the page tables and just grab the page
   from guest_memfd.
* Where not possible (e.g., emulator access as discussed) we can just
   access the page in the user space mapping

Does that sound reasonable?

-- 
Cheers,

David / dhildenb