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

[David Hildenbrand <david@redhat.com>]

>>> + * 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).
> 

LGTM

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

Yes, that would be my assumption. I mean, we also msut make sure that if 
the user does something stupid like that, that we won't trigger other 
undesired code paths (like, suddenly the guest_memfd being !shared).

> 
>> As an alternative ... could we simple get/put when managing the memslot?
> 
> What does a simple get/put mean here?

s/simple/simply/

So when we create the memslot, we'd perform the get, and when we destroy 
the memslot, we'd do the put.

Just an idea.

-- 
Cheers,

David / dhildenb