Re: [PATCH v9 07/41] KVM: guest_memfd: Wire up core private/shared attribute interfaces

On 8/1/2026 12:26 AM, Sean Christopherson wrote:
> On Fri, Jul 31, 2026, Xiaoyao Li wrote:
>> On 7/31/2026 4:42 AM, Ackerley Tng wrote:
>>> Xiaoyao Li <[email protected]> writes:
>>>
>>>> On 7/29/2026 8:35 AM, Ackerley Tng via B4 Relay wrote:
>>>>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>>>>> index 33c9830190e2e..89cf922232920 100644
>>>>> --- a/virt/kvm/guest_memfd.c
>>>>> +++ b/virt/kvm/guest_memfd.c
>>>>> @@ -893,6 +893,27 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>>>>>     EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
>>>>>
>>>>>     #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_POPULATE
>>>>> +static bool kvm_range_is_private(struct file *file, pgoff_t index,
>>>>> +				 size_t nr_pages, struct kvm *kvm, gfn_t gfn)
>>>>> +{
>>>>> +	struct inode *inode = file_inode(file);
>>>>> +	pgoff_t last = index + nr_pages - 1;
>>>>> +	struct maple_tree *mt;
>>>>> +	void *entry;
>>>>> +
>>>>> +	if (!gmem_in_place_conversion)
>>>>> +		return kvm_range_has_vm_memory_attributes(kvm, gfn, gfn + nr_pages,
>>>>> +							  KVM_MEMORY_ATTRIBUTE_PRIVATE,
>>>>> +							  KVM_MEMORY_ATTRIBUTE_PRIVATE);
>>>>> +
>>>>> +	mt = &GMEM_I(inode)->attributes;
>>>>> +	mt_for_each(mt, entry, index, last) {
>>>>> +		if (kvm_gmem_interpret_entry(inode, entry) !=
>>>>> +		    KVM_MEMORY_ATTRIBUTE_PRIVATE)
>>>>> +			return false;
>>>>> +	}
>>>>> +	return true;
>>>>> +}
>>>>>
>>>>>     static long __kvm_gmem_populate(struct kvm *kvm, struct kvm_memory_slot *slot,
>>>>>     				struct file *file, gfn_t gfn, struct page *src_page,
>>>>> @@ -913,9 +934,7 @@ static long __kvm_gmem_populate(struct kvm *kvm, struct kvm_memory_slot *slot,
>>>>>
>>>>>     	folio_unlock(folio);
>>>>>
>>>>> -	if (!kvm_range_has_vm_memory_attributes(kvm, gfn, gfn + 1,
>>>>> -						KVM_MEMORY_ATTRIBUTE_PRIVATE,
>>>>> -						KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
>>>>> +	if (!kvm_range_is_private(file, index, 1, kvm, gfn)) {
>>>>
>>>> It's checking if a single gfn is private.
>>>>
>>>
>>> But with huge page support we'd need to check a range of gfns... I guess
>>> there's the argument of not keeping complexity for the future, but in
>>> this case it's undoing functionality for this series and probably adding
>>> it back later.
>>
>> let's leave the work for huge page support in the future.
> 
> It's not just for hugepage support, the core functionality is used in this series
> by __kvm_gmem_set_attributes() in:
> 
>    KVM: guest_memfd: Return early if range already has requested attributes
> 
>>>> We can just use kvm_mem_is_private()? And it seems can be a separate patch.
>>>>
>>>
>>> Do you mean that this refactoring could be a separate patch? I could
>>> refactor out kvm_range_is_private() in a separate patch, then put in the
>>> mt_for_each() check in this patch together as part of all the rest of
>>> the "wiring".
>>>
>>> I considered that but it seemed like the refactoring was too small to
>>> separate out into another patch when the mt_for_each() part is going to
>>> be in this patch anyway.
>>>
>>> Or, I could add an earlier patch to first replace the call to
>>> kvm_range_has_vm_memory_attributes() with a call to check just 1 gfn,
>>> then this wiring patch would be simpler.
>>>
>>
>> This is what I meant. An earlier patch to just replace
>> kvm_range_has_vm_memory_attributes() with kvm_mem_is_private(). Then we can
>> drop this 'big' diff in this patch.
>>
>> If kvm_range_is_private() is useful/required by huge page support, then let
>> the huge page series to introduce it. It has nothing to do with the in-place
>> conversion series.
> 
> If it weren't for the fact that the range-based search is used later in this
> series, I would 100% agree with Xiaoyao.  
 > But since the core logic is used and needed elsewhere,

I don't see it a problem. Without introducing kvm_range_is_private() in 
this patch, the core logic of range-based search on gmem can still be 
introduced as kvm_gmem_range_has_attributes() directly in patch 10 or in 
a separate patch.

 > and because kvm_range_has_vm_memory_attributes() takes a range,> my 
vote is to provide the plumbing now, even though a small portion of it isn't
> strictly necessary.

So my initial feedback was "we can just use kvm_mem_is_private(kvm, 
gfn)". It makes code simpler. And as a bonus, the logic to choose 
between gmem-based attribute query and vm-based attribute query is 
hidden from the static call.

The second bonus of dropping kvm_range_is_private() is that we can 
eliminate one more chunk in patch 20.