Re: [PATCH v9 09/17] KVM: x86/mmu: Handle guest page faults for guest_memfd with shared memory

[Ackerley Tng <ackerleytng@google.com>]

David Hildenbrand <david@redhat.com> writes:

> On 13.05.25 18:34, Fuad Tabba wrote:
>> From: Ackerley Tng <ackerleytng@google.com>
>>
>> For memslots backed by guest_memfd with shared mem support, the KVM MMU
>> always faults-in pages from guest_memfd, and not from the userspace_addr.
>> Towards this end, this patch also introduces a new guest_memfd flag,
>> GUEST_MEMFD_FLAG_SUPPORT_SHARED, which indicates that the guest_memfd
>> instance supports in-place shared memory.
>>
>> This flag is only supported if the VM creating the guest_memfd instance
>> belongs to certain types determined by architecture. Only non-CoCo VMs
>> are permitted to use guest_memfd with shared mem, for now.
>>
>> Function names have also been updated for accuracy -
>> kvm_mem_is_private() returns true only when the current private/shared
>> state (in the CoCo sense) of the memory is private, and returns false if
>> the current state is shared explicitly or impicitly, e.g., belongs to a
>> non-CoCo VM.
>>
>> kvm_mmu_faultin_pfn_gmem() is updated to indicate that it can be used
>> to fault in not just private memory, but more generally, from
>> guest_memfd.
>>
>> Co-developed-by: Fuad Tabba <tabba@google.com>
>> Signed-off-by: Fuad Tabba <tabba@google.com>
>> Co-developed-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
>> ---
>
>
> [...]
>
>> +
>>   #ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES
>>   static inline unsigned long kvm_get_memory_attributes(struct kvm *kvm, gfn_t gfn)
>>   {
>> @@ -2515,10 +2524,30 @@ bool kvm_arch_pre_set_memory_attributes(struct kvm *kvm,
>>   bool kvm_arch_post_set_memory_attributes(struct kvm *kvm,
>>   					 struct kvm_gfn_range *range);
>>
>> +/*
>> + * Returns true if the given gfn's private/shared status (in the CoCo sense) is
>> + * private.
>> + *
>> + * A return value of false indicates that the gfn is explicitly or implicity
>
> s/implicity/implicitly/
>

Thanks!

>> + * shared (i.e., non-CoCo VMs).
>> + */
>>   static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
>>   {
>> -	return IS_ENABLED(CONFIG_KVM_GMEM) &&
>> -	       kvm_get_memory_attributes(kvm, gfn) & KVM_MEMORY_ATTRIBUTE_PRIVATE;
>> +	struct kvm_memory_slot *slot;
>> +
>> +	if (!IS_ENABLED(CONFIG_KVM_GMEM))
>> +		return false;
>> +
>> +	slot = gfn_to_memslot(kvm, gfn);
>> +	if (kvm_slot_has_gmem(slot) && kvm_gmem_memslot_supports_shared(slot)) {
>> +		/*
>> +		 * For now, memslots only support in-place shared memory if the
>> +		 * host is allowed to mmap memory (i.e., non-Coco VMs).
>> +		 */
>
> Not accurate: there is no in-place conversion support in this series,
> because there is no such itnerface. So the reason is that all memory is
> shared for there VM types?
>

True that there's no in-place conversion yet.

In this patch series, guest_memfd memslots support shared memory only
for specific VM types (on x86, that would be KVM_X86_DEFAULT_VM and
KVM_X86_SW_PROTECTED_VMs).

How about this wording:

Without conversion support, if the guest_memfd memslot supports shared
memory, all memory must be used as not private (implicitly shared).

>> +		return false;
>> +	}
>> +
>> +	return kvm_get_memory_attributes(kvm, gfn) & KVM_MEMORY_ATTRIBUTE_PRIVATE;
>>   }
>>   #else
>>   static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>> index 2f499021df66..fe0245335c96 100644
>> --- a/virt/kvm/guest_memfd.c
>> +++ b/virt/kvm/guest_memfd.c
>> @@ -388,6 +388,23 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
>>
>>   	return 0;
>>   }
>> +
>> +bool kvm_gmem_memslot_supports_shared(const struct kvm_memory_slot *slot)
>> +{
>> +	struct file *file;
>> +	bool ret;
>> +
>> +	file = kvm_gmem_get_file((struct kvm_memory_slot *)slot);
>> +	if (!file)
>> +		return false;
>> +
>> +	ret = kvm_gmem_supports_shared(file_inode(file));
>> +
>> +	fput(file);
>> +	return ret;
>
> Would it make sense to cache that information in the memslot, to avoid
> the get/put?
>
> We could simply cache when creating the memslot I guess.
>

When I wrote it I was assuming that to ensure correctness we should
check with guest memfd, like what if someone closed the gmem file in the
middle of the fault path?

But I guess after the discussion at the last call, since the faulting
process is long and racy, if this check passed and we go to guest memfd
and the file was closed, it would just fail so I guess caching is fine.

> As an alternative ... could we simple get/put when managing the memslot?

What does a simple get/put mean here?