Re: [PATCH v12 08/18] KVM: guest_memfd: Allow host to map guest_memfd pages

[David Hildenbrand <david@redhat.com>]

>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index d00b85cb168c..cb19150fd595 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1570,6 +1570,7 @@ struct kvm_memory_attributes {
>>   #define KVM_MEMORY_ATTRIBUTE_PRIVATE           (1ULL << 3)
>>   
>>   #define KVM_CREATE_GUEST_MEMFD	_IOWR(KVMIO,  0xd4, struct kvm_create_guest_memfd)
>> +#define GUEST_MEMFD_FLAG_SUPPORT_SHARED	(1ULL << 0)
> 

Coming back to this part of the initial mail:

> I find the SUPPORT_SHARED terminology to be super confusing.  I had to dig quite
> deep to undesrtand that "support shared" actually mean "userspace explicitly
> enable sharing on _this_ guest_memfd instance".  E.g. I was surprised to see
> 
> IMO, GUEST_MEMFD_FLAG_SHAREABLE would be more appropriate.  But even that is
> weird to me.  For non-CoCo VMs, there is no concept of shared vs. private.  What's
> novel and notable is that the memory is _mappable_.  Yeah, yeah, pKVM's use case
> is to share memory, but that's a _use case_, not the property of guest_memfd that
> is being controlled by userspace.

Looking back, it would all have made more sense if one would have to 
explicitly request support for "private" memory, and the non-private 
(ordinary?) would have been the default ... :)


> 
> And kvm_gmem_memslot_supports_shared() is even worse.  It's simply that the
> memslot is bound to a mappable guest_memfd instance, it's that the guest_memfd
> instance is the _only_ entry point to the memslot.
> 
> So my vote would be "GUEST_MEMFD_FLAG_MAPPABLE", and then something like
> KVM_MEMSLOT_GUEST_MEMFD_ONLY.  That will make code like this:

As raised, GUEST_MEMFD_FLAG_MMAPPABLE / GUEST_MEMFD_FLAG_MMAP or sth. 
like that that spells out "mmap" would be better IMHO. Better than 
talking about faultability or mappability. (fault into what? map into 
what? mmap() cleanly implies user space)

That means that we have "mmap() support implies support for non-private 
memory", I can live with that, although some code might end up being a 
bit confusing (e.g., that's why you proposed kvm_is_memslot_gmem_only() 
below).

> 
> 	if (kvm_slot_has_gmem(slot) &&
> 	    (kvm_gmem_memslot_supports_shared(slot) ||
> 	     kvm_get_memory_attributes(kvm, gfn) & KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
> 		return kvm_gmem_max_mapping_level(slot, gfn, max_level);
> 	}
> 
> much more intutive:
> 
> 	if (kvm_is_memslot_gmem_only(slot) ||
> 	    kvm_get_memory_attributes(kvm, gfn) & KVM_MEMORY_ATTRIBUTE_PRIVATE))
> 		return kvm_gmem_max_mapping_level(slot, gfn, max_level);

Hiding the details in such a helper makes it easier to digest.

> 
> And then have kvm_gmem_mapping_order() do:
> 
> 	WARN_ON_ONCE(!kvm_slot_has_gmem(slot));
> 	return 0;
> 
>>   struct kvm_create_guest_memfd {
>>   	__u64 size;
>> diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
>> index 559c93ad90be..e90884f74404 100644
>> --- a/virt/kvm/Kconfig
>> +++ b/virt/kvm/Kconfig
>> @@ -128,3 +128,7 @@ config HAVE_KVM_ARCH_GMEM_PREPARE
>>   config HAVE_KVM_ARCH_GMEM_INVALIDATE
>>          bool
>>          depends on KVM_GMEM
>> +
>> +config KVM_GMEM_SHARED_MEM
>> +       select KVM_GMEM
>> +       bool
>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>> index 6db515833f61..06616b6b493b 100644
>> --- a/virt/kvm/guest_memfd.c
>> +++ b/virt/kvm/guest_memfd.c
>> @@ -312,7 +312,77 @@ static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
>>   	return gfn - slot->base_gfn + slot->gmem.pgoff;
>>   }
>>   
>> +static bool kvm_gmem_supports_shared(struct inode *inode)
>> +{
>> +	const u64 flags = (u64)inode->i_private;
>> +
>> +	if (!IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM))
>> +		return false;
>> +
>> +	return flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED;
>> +}
>> +
>> +static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
> 
> And to my point about "shared", this is also very confusing, because there are
> zero checks in here about shared vs. private.

I assume it's simply a "kvm_gmem_fault" right now.

> 
>> +{
>> +	struct inode *inode = file_inode(vmf->vma->vm_file);
>> +	struct folio *folio;
>> +	vm_fault_t ret = VM_FAULT_LOCKED;
>> +
>> +	if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode))
>> +		return VM_FAULT_SIGBUS;
>> +
>> +	folio = kvm_gmem_get_folio(inode, vmf->pgoff);
>> +	if (IS_ERR(folio)) {
>> +		int err = PTR_ERR(folio);
>> +
>> +		if (err == -EAGAIN)
>> +			return VM_FAULT_RETRY;
>> +
>> +		return vmf_error(err);
>> +	}
>> +
>> +	if (WARN_ON_ONCE(folio_test_large(folio))) {
>> +		ret = VM_FAULT_SIGBUS;
>> +		goto out_folio;
>> +	}
>> +
>> +	if (!folio_test_uptodate(folio)) {
>> +		clear_highpage(folio_page(folio, 0));
>> +		kvm_gmem_mark_prepared(folio);
>> +	}
>> +
>> +	vmf->page = folio_file_page(folio, vmf->pgoff);
>> +
>> +out_folio:
>> +	if (ret != VM_FAULT_LOCKED) {
>> +		folio_unlock(folio);
>> +		folio_put(folio);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct vm_operations_struct kvm_gmem_vm_ops = {
>> +	.fault = kvm_gmem_fault_shared,
>> +};
>> +
>> +static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> +	if (!kvm_gmem_supports_shared(file_inode(file)))
>> +		return -ENODEV;
>> +
>> +	if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) !=
>> +	    (VM_SHARED | VM_MAYSHARE)) {
> 
> And the SHARED terminology gets really confusing here, due to colliding with the
> existing notion of SHARED file mappings.
> 

kvm_gmem_supports_mmap() would be as clear as it gets for this case here.

-- 
Cheers,

David / dhildenb